Re: hidclass.sys: Properly quit hid_device_thread when both completion event and halt event are signaled (try 2)
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 <liuchanghui(a)linuxdeepin.com> 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.
-- Regards, Qian Hong - http://www.winehq.org
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 <liuchanghui(a)linuxdeepin.com> 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.
On Thu, Jan 7, 2016 at 1:23 PM, Sebastian Lackner <sebastian(a)fds-team.de> 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. -- Regards, Qian Hong - http://www.winehq.org
On 1/7/16 12:47 AM, Qian Hong wrote:
On Thu, Jan 7, 2016 at 1:23 PM, Sebastian Lackner <sebastian(a)fds-team.de> 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
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"<aric(a)codeweavers.com>; Date: Thu, Jan 7, 2016 10:49 PM To: "fracting"<fracting(a)gmail.com>; "Sebastian Lackner"<sebastian(a)fds-team.de>; Cc: "wine-devel"<wine-devel(a)winehq.org>; "刘昌辉"<liuchanghui(a)linuxdeepin.com>; 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 <sebastian(a)fds-team.de> 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
On Jan 7, 2016, at 8:14 PM, Changhui LIU <liuchanghui(a)linuxdeepin.com> 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"<aric(a)codeweavers.com>; Date: Thu, Jan 7, 2016 10:49 PM To: "fracting"<fracting(a)gmail.com>; "Sebastian Lackner"<sebastian(a)fds-team.de>; Cc: "wine-devel"<wine-devel(a)winehq.org>; "刘昌辉"<liuchanghui(a)linuxdeepin.com>; 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 <sebastian(a)fds-team.de> 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
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"<aric(a)codeweavers.com>; Date: Sat, Jan 9, 2016 05:25 AM To: "Changhui LIU"<liuchanghui(a)linuxdeepin.com>; Cc: "fracting"<fracting(a)gmail.com>; "Sebastian Lackner"<sebastian(a)fds-team.de>; "wine-devel"<wine-devel(a)winehq.org>; 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 <liuchanghui(a)linuxdeepin.com> 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"<aric(a)codeweavers.com>; Date: Thu, Jan 7, 2016 10:49 PM To: "fracting"<fracting(a)gmail.com>; "Sebastian Lackner"<sebastian(a)fds-team.de>; Cc: "wine-devel"<wine-devel(a)winehq.org>; "刘昌辉"<liuchanghui(a)linuxdeepin.com>; 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 <sebastian(a)fds-team.de> 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
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"<aric(a)codeweavers.com>; *Date: * Sat, Jan 9, 2016 05:25 AM *To: * "Changhui LIU"<liuchanghui(a)linuxdeepin.com>; *Cc: * "fracting"<fracting(a)gmail.com>; "Sebastian Lackner"<sebastian(a)fds-team.de>; "wine-devel"<wine-devel(a)winehq.org>; *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 <liuchanghui(a)linuxdeepin.com <mailto:liuchanghui(a)linuxdeepin.com>> 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"<aric(a)codeweavers.com <mailto:aric(a)codeweavers.com>>; *Date: * Thu, Jan 7, 2016 10:49 PM *To: * "fracting"<fracting(a)gmail.com <mailto:fracting(a)gmail.com>>; "Sebastian Lackner"<sebastian(a)fds-team.de <mailto:sebastian(a)fds-team.de>>; *Cc: * "wine-devel"<wine-devel(a)winehq.org <mailto:wine-devel(a)winehq.org>>; "刘昌辉"<liuchanghui(a)linuxdeepin.com <mailto:liuchanghui(a)linuxdeepin.com>>; *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 <sebastian(a)fds-team.de <mailto:sebastian(a)fds-team.de>> 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
participants (5)
-
Aric Stewart -
Changhui LIU -
Codeweavers
-
Qian Hong -
Sebastian Lackner