On 6/18/21 11:06 PM, Zebediah Figura (she/her) wrote:
On 6/18/21 1:49 PM, Rémi Bernon wrote:
On 6/18/21 5:59 PM, Zebediah Figura (she/her) wrote:
On 6/18/21 7:06 AM, Rémi Bernon wrote:
So that mini driver gets notified first and has a chance to cancel pending IRPs.
Which minidriver?
Isn't it the responsibility of the child to terminate all pending IRPs (and disallow further ones) on removal? See also [0], [1], [2], which say that queued requests should be completed in (both, for some reason) IRP_MN_SURPRISE_REMOVAL and IRP_MN_REMOVE_DEVICE.
I think this deserves tests.
[0] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...
[2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
I really don't have the big picture yet, so I meant the one implemented in driver_hid.c. If it doesn't cancel the IRP it has queued on device removal, the test never completes the device removal on Windows.
On Wine, driver_hid never receives the IRP_MN_SURPRISE_REMOVAL if we don't call it before removing the "children" in ntoskrnl.exe. I don't really understand who are the children there, but driver_hid apparently isn't.
Allow me to explain in detail. Apologies if any of this is old news to you. If nothing else I'll adapt it to a wiki page later ;-)
TLC PDO winehid.sys + hidclass.sys HID FDO
HID PDO winebus.sys root FDO
root PDO ntoskrnl.exe [PNP manager]
In the case of the HID test driver it's a little different:
TLC PDO driver_hid.dll + hidclass.sys root/HID FDO
root/HID PDO ntoskrnl.exe [PNP manager]
A root PNP device is the one passed to AddDevice for a root PNP driver. Usually it's intended to enumerate children; here we kind of cheat and make the root PnP device the same device as the HID device. It's a bit weird but it works.
The TLC (top-level collection) PDO is managed entirely within hidclass.sys; the minidriver never interacts with it. In theory there's supposed to be one or more of these; we don't handle the "or more" case yet.
The application only ever interacts with the TLC PDO. It's actually impossible to open the HID device stack (IIRC it fails with STATUS_UNSUCCESSFUL). In response to read/write/hidd ioctls, hidclass creates new irps and sends them to the HID device stack, i.e. the "HID FDO/HID PDO" pair in the above diagrams.
[Note that IRPs are always sent to the top of a stack first, so in this case the minidriver has a chance to handle it. In a real HID driver the minidriver would always handle it itself, rather than sending it down to the PDO. You can mentally replace winehid with hidusb, and winebus with (say) usbccgp. Obviously usbccgp doesn't know anything about HID. Our architecture is a little weird, though, for various reasons.]
There is usually no TLC FDO, but in theory there could be, and in practice I believe mouse and keyboard drivers attach to the TLC PDO.
On reflection, I think I may have confused myself, so this patch actually makes more sense than it initially did. It'd still be nice to have tests, though, and the commit message should ideally be more specific.
Thanks, this is definitely not old news, although it's still quite blurry.
From [1] I understand that the hid minidriver lives at the HID FDO level, which means that (minidriver, hidclass.sys) act as the bus FDO driver here.
Reading [2] or [3] doesn't make it completely explicit if the children should receive the removal IRP first, but from what I understand it is how it's generally supposed to work.
With that understanding I now believe it's correct to call the IRP on the children of the device first.
The problem I'm facing here comes from the way hidclass.sys PDO handles its removal. It waits for the reading thread to complete, which may be waiting on IRPs queued in the HID FDO.
We could either:
1) call the surprise removal first on the device stack, then call the normal removal on the stack, giving a chance for the HID FDO to cancel its queued IRPs.
or,
2) combine the waits between halt_event and the pending IRP in the HID PDO device thread, so that the thread can terminate even with a pending IRP.
Unrelated, but I'm not sure to understand why the HID PDO has to re-create IRPs every time it needs to call the minidriver (except for the device thread of course), wouldn't it be possible to just use the normal IRP propagation down the stack?
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/hid-architectu...
[2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...
[3] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...