From: "Zebediah Figura (she/her)" Subject: Re: [PATCH 4/6] ntoskrnl.exe: Send IRP_MN_SURPRISE_REMOVAL before removing children. Message-Id: <21dab54a-fd54-a95f-8d10-c62d07d6c6b3@codeweavers.com> Date: Fri, 18 Jun 2021 16:06:03 -0500 In-Reply-To: <271c8c1f-ff25-5eaa-cb94-f4c42f7582f3@codeweavers.com> References: <20210618120611.703993-1-rbernon@codeweavers.com> <20210618120611.703993-4-rbernon@codeweavers.com> <271c8c1f-ff25-5eaa-cb94-f4c42f7582f3@codeweavers.com> 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-device-in-a-function-driver >> >> >> [1] >> https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an-irp-mn-surprise-removal-request >> >> >> [2] >> https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-device-in-a-bus-drive >> >> > > 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.