Re: hidclass.sys: Properly quit hid_device_thread when both completion event and halt event are signaled (try 2)

[Qian Hong]

The patch looks good to me, Aric, would you mind sign off the patch?

I'm glad to sign off, but I'm not qualified enough :)

On Tue, Dec 29, 2015 at 11:19 AM, Changhui LIU
 wrote:
> Superseded patch 117285.
>
> Try 2: Fixed  IRP completion event will not be signaled if  a driver is
> misbehaving and is blocked or crashes while processing an IRP.

hidclass.sys: Properly quit hid_device_thread when both completion event and halt event are signaled (try 2)

[Sebastian Lackner]

On 07.01.2016 06:07, Qian Hong wrote:
> The patch looks good to me, Aric, would you mind sign off the patch?
>
> I'm glad to sign off, but I'm not qualified enough :)
>
> On Tue, Dec 29, 2015 at 11:19 AM, Changhui LIU
>  wrote:
> > Superseded patch 117285.
> >
> > Try 2: Fixed  IRP completion event will not be signaled if  a driver is
> > misbehaving and is blocked or crashes while processing an IRP.

Wouldn't it be easier to just swap events[0] <-> events[1], to handle the
exit event with higher priority? I do not see any real need for two calls
to Wait*() functions.

Also, please note that your example above is not really a good one. If the
driver is blocked / crashed somehow, then events[0] will never be signaled,
so we don't have to care about both events signaled at the same time.
The only situation where this matters is when requests are processed too fast.

hidclass.sys: Properly quit hid_device_thread when both completion event and halt event are signaled (try 2)

[Qian Hong]

On Thu, Jan 7, 2016 at 1:23 PM, Sebastian Lackner  wrote:
> Wouldn't it be easier to just swap events[0] <-> events[1], to handle the
> exit event with higher priority? I do not see any real need for two calls
> to Wait*() functions.

After discussing on irc, we agree that swapping  events[0] <->
events[1] is not enough.
The original patch fix not only one bug, but two separate problems.
In the existent implementation, If ntrc is never set to
STATUS_PENDING, then there is no chance to turn exit_now to true,
Changhui's patch fix this bug, which is discovered by a real world
online bank driver.

Sebastian also pointed out another bug in the existent implementation:
halt_event is created as an auto-reset event, which means the
first wait*() call could reset it. The correct solution is creating
halt_event as manual-reset event instead.
It might make sense to create events[0] as manual-reset as well, which
makes the following ResetEvent() calls more reasonable.

hidclass.sys: Properly quit hid_device_thread when both completion event and halt event are signaled (try 2)

[Aric Stewart]

On 1/7/16 12:47 AM, Qian Hong wrote:
> On Thu, Jan 7, 2016 at 1:23 PM, Sebastian Lackner  wrote:
> > Wouldn't it be easier to just swap events[0] <-> events[1], to handle the
> > exit event with higher priority? I do not see any real need for two calls
> > to Wait*() functions.
>
> After discussing on irc, we agree that swapping  events[0] <->
> events[1] is not enough.
> The original patch fix not only one bug, but two separate problems.
> In the existent implementation, If ntrc is never set to
> STATUS_PENDING, then there is no chance to turn exit_now to true,
> Changhui's patch fix this bug, which is discovered by a real world
> online bank driver.

My ears perk up here.  Real world online bank driver?  Can I see and play with it. I am really looking for real world cases that use HID.

-aric

hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)

[Changhui LIU]

Hi Aric:

The driver is a winelib hidusb.sys work in progress, not a real world native driver.
China Merchant Bank's on-line bank program use HidD_SetFeature/HidD_GetFeature to
access hid token.
I write this dirver based on patches developed by Alexander Morozov .
(http://wiki.winehq.org/USB, ftp://ftp.etersoft.ru/pub/people/amorozov/usb )

Now the on-line bank program can detect hid token on wine, but has some little bug.
I will send this driver source code to wine upstream end of this month.

Thank you for your great work on hidclass.sys .

------------------
Regards,
Changhui Liu

------------------ Original ------------------
From:  "Aric Stewart";
Date:  Thu, Jan 7, 2016 10:49 PM
To:  "fracting"; "Sebastian Lackner"; 
Cc:  "wine-devel"; "刘昌辉"; 
Subject:  Re: hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)

On 1/7/16 12:47 AM, Qian Hong wrote:
> On Thu, Jan 7, 2016 at 1:23 PM, Sebastian Lackner  wrote:
> > Wouldn't it be easier to just swap events[0] <-> events[1], to handle the
> > exit event with higher priority? I do not see any real need for two calls
> > to Wait*() functions.
>
> After discussing on irc, we agree that swapping  events[0] <->
> events[1] is not enough.
> The original patch fix not only one bug, but two separate problems.
> In the existent implementation, If ntrc is never set to
> STATUS_PENDING, then there is no chance to turn exit_now to true,
> Changhui's patch fix this bug, which is discovered by a real world
> online bank driver.

My ears perk up here.  Real world online bank driver?  Can I see and play with it. I am really looking for real world cases that use HID.

-aric

hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)

[Codeweavers]

> On Jan 7, 2016, at 8:14 PM, Changhui LIU  wrote:
>
> Hi Aric:
>
> The driver is a winelib hidusb.sys work in progress, not a real world native driver.
> China Merchant Bank's on-line bank program use HidD_SetFeature/HidD_GetFeature to
> access hid token.
> I write this dirver based on patches developed by Alexander Morozov .
> (http://wiki.winehq.org/USB, ftp://ftp.etersoft.ru/pub/people/amorozov/usb )

Ok I understand. I believe the GetFeature and SetFeature parts of his are not implemented at all. They are not hard but where not my first priority.

I have read Alexander's code but it is not clear to me how you are hooking that into hid unless you have written a custom mini driver?

-aric
> Now the on-line bank program can detect hid token on wine, but has some little bug.
> I will send this driver source code to wine upstream end of this month.
>
> Thank you for your great work on hidclass.sys .
>
> Regards,
> Changhui Liu
>
> ------------------ Original ------------------
> From:  "Aric Stewart";
> Date:  Thu, Jan 7, 2016 10:49 PM
> To:  "fracting"; "Sebastian Lackner";
> Cc:  "wine-devel"; "刘昌辉";
> Subject:  Re: hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)
>
> On 1/7/16 12:47 AM, Qian Hong wrote:
> > On Thu, Jan 7, 2016 at 1:23 PM, Sebastian Lackner  wrote:
> > > Wouldn't it be easier to just swap events[0] <-> events[1], to handle the
> > > exit event with higher priority? I do not see any real need for two calls
> > > to Wait*() functions.
> >
> > After discussing on irc, we agree that swapping  events[0] <->
> > events[1] is not enough.
> > The original patch fix not only one bug, but two separate problems.
> > In the existent implementation, If ntrc is never set to
> > STATUS_PENDING, then there is no chance to turn exit_now to true,
> > Changhui's patch fix this bug, which is discovered by a real world
> > online bank driver.
>
> My ears perk up here.  Real world online bank driver?  Can I see and play with it. I am really looking for real world cases that use HID.
>
> -aric

hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)

[Changhui LIU]

Hi Aric:
Yes, it's a mini driver upon hidclass.sys.

The mini driver main work is handle  IRP_MJ_INTERNAL_DEVICE_CONTROL IRP.
In the IRP_MJ_INTERNAL_DEVICE_CONTROL  handle function, I use libusb to read the hid device's
attributes,  set feature report, get feature report, ..etc. 
(Refernece  https://github.com/signal11/hidapi/tree/master/libusb.)

And use udev to monitor device add or remove, once found new a hid device, create a PDO and then call 
hidclass.sys's  PNP_AddDevice to craete FDO.

Last associate the PDO, FDO with libusb's device handle.

------------------
Regards,
Changhui Liu

------------------ Original ------------------
From:  "Aric Stewart";
Date:  Sat, Jan 9, 2016 05:25 AM
To:  "Changhui LIU"; 
Cc:  "fracting"; "Sebastian Lackner"; "wine-devel"; 
Subject:  Re: hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)

On Jan 7, 2016, at 8:14 PM, Changhui LIU  wrote:

Hi Aric:

The driver is a winelib hidusb.sys work in progress, not a real world native driver.
China Merchant Bank's on-line bank program use HidD_SetFeature/HidD_GetFeature to
access hid token.
I write this dirver based on patches developed by Alexander Morozov .
(http://wiki.winehq.org/USB, ftp://ftp.etersoft.ru/pub/people/amorozov/usb )

Ok I understand. I believe the GetFeature and SetFeature parts of his are not implemented at all. They are not hard but where not my first priority.

I have read Alexander's code but it is not clear to me how you are hooking that into hid unless you have written a custom mini driver?

-aric

Now the on-line bank program can detect hid token on wine, but has some little bug.
I will send this driver source code to wine upstream end of this month.

Thank you for your great work on hidclass.sys .

------------------
Regards,
Changhui Liu

------------------ Original ------------------
From:  "Aric Stewart";
Date:  Thu, Jan 7, 2016 10:49 PM
To:  "fracting"; "Sebastian Lackner"; 
Cc:  "wine-devel"; "刘昌辉"; 
Subject:  Re: hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)

On 1/7/16 12:47 AM, Qian Hong wrote:
> On Thu, Jan 7, 2016 at 1:23 PM, Sebastian Lackner  wrote:
> > Wouldn't it be easier to just swap events[0] <-> events[1], to handle the
> > exit event with higher priority? I do not see any real need for two calls
> > to Wait*() functions.
>
> After discussing on irc, we agree that swapping  events[0] <->
> events[1] is not enough.
> The original patch fix not only one bug, but two separate problems.
> In the existent implementation, If ntrc is never set to
> STATUS_PENDING, then there is no chance to turn exit_now to true,
> Changhui's patch fix this bug, which is discovered by a real world
> online bank driver.

My ears perk up here.  Real world online bank driver?  Can I see and play with it. I am really looking for real world cases that use HID.

-aric

hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)

[Aric Stewart]

On 1/10/16 8:34 PM, Changhui LIU wrote:
> Hi Aric:
> Yes, it's a mini driver upon hidclass.sys.
>
> The mini driver main work is handle  IRP_MJ_INTERNAL_DEVICE_CONTROL IRP.
> In the IRP_MJ_INTERNAL_DEVICE_CONTROL  handle function, I use libusb to read the hid device's
> attributes,  set feature report, get feature report, ..etc.
> (Refernece  https://github.com/signal11/hidapi/tree/master/libusb.)
>
> And use udev to monitor device add or remove, once found new a hid device, create a PDO and then call
> hidclass.sys's  PNP_AddDevice to craete FDO.
>
> Last associate the PDO, FDO with libusb's device handle.

Cool, I think it is a few steps early. Ideally we would have Plug and Play in, then usb.sys support using lib usb then usbhid.sys that uses usb.sys.

But as we work in that direction we can update your minidriver.

-aric
> Regards,
> Changhui Liu
> ------------------ Original ------------------
> *From: * "Aric Stewart";
> *Date: * Sat, Jan 9, 2016 05:25 AM
> *To: * "Changhui LIU";
> *Cc: * "fracting"; "Sebastian Lackner"; "wine-devel";
> *Subject: * Re: hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)
>
> On Jan 7, 2016, at 8:14 PM, Changhui LIU <[e-mail] > wrote:
> > Hi Aric:
> >
> > The driver is a winelib hidusb.sys work in progress, not a real world native driver.
> > China Merchant Bank's on-line bank program use HidD_SetFeature/HidD_GetFeature to
> > access hid token.
> > I write this dirver based on patches developed by Alexander Morozov .
> > (http://wiki.winehq.org/USB, ftp://ftp.etersoft.ru/pub/people/amorozov/usb )
>
> Ok I understand. I believe the GetFeature and SetFeature parts of his are not implemented at all. They are not hard but where not my first priority.
>
> I have read Alexander's code but it is not clear to me how you are hooking that into hid unless you have written a custom mini driver?
>
> -aric
> > Now the on-line bank program can detect hid token on wine, but has some little bug.
> > I will send this driver source code to wine upstream end of this month.
> >
> > Thank you for your great work on hidclass.sys .
> >
> > Regards,
> > Changhui Liu
> > ------------------ Original ------------------
> > *From: * "Aric Stewart"<[e-mail] >;
> > *Date: * Thu, Jan 7, 2016 10:49 PM
> > *To: * "fracting"<[e-mail] >; "Sebastian Lackner"<[e-mail] >;
> > *Cc: * "wine-devel"<[e-mail] >; "刘昌辉"<[e-mail] >;
> > *Subject: * Re: hidclass.sys: Properly quit hid_device_thread when bothcompletion event and halt event are signaled (try 2)
> >
> > On 1/7/16 12:47 AM, Qian Hong wrote:
> > > On Thu, Jan 7, 2016 at 1:23 PM, Sebastian Lackner <[e-mail] > wrote:
> > > > Wouldn't it be easier to just swap events[0] <-> events[1], to handle the
> > > > exit event with higher priority? I do not see any real need for two calls
> > > > to Wait*() functions.
> > >
> > > After discussing on irc, we agree that swapping  events[0] <->
> > > events[1] is not enough.
> > > The original patch fix not only one bug, but two separate problems.
> > > In the existent implementation, If ntrc is never set to
> > > STATUS_PENDING, then there is no chance to turn exit_now to true,
> > > Changhui's patch fix this bug, which is discovered by a real world
> > > online bank driver.
> >
> > My ears perk up here.  Real world online bank driver?  Can I see and play with it. I am really looking for real world cases that use HID.
> >
> > -aric