On 6/29/21 7:31 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 12:25 PM, Rémi Bernon wrote:
On 6/29/21 7:17 PM, Zebediah Figura (she/her) wrote:
On 6/29/21 2:21 AM, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon rbernon@codeweavers.com
dlls/winebus.sys/main.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 897be25ed51..f3e63e87aa2 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -374,10 +374,6 @@ void bus_unlink_hid_device(DEVICE_OBJECT *device) EnterCriticalSection(&device_list_cs); list_remove(&pnp_device->entry); LeaveCriticalSection(&device_list_cs);
- EnterCriticalSection(&ext->cs); - ext->removed = TRUE; - LeaveCriticalSection(&ext->cs); } void bus_remove_hid_device(DEVICE_OBJECT *device) @@ -700,11 +696,21 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) status = STATUS_SUCCESS; break; + case IRP_MN_SURPRISE_REMOVAL: + EnterCriticalSection(&ext->cs); + remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); + break;
This is fine, I guess, but it makes the lock in remove_pending_irps() redundant.
case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device; + EnterCriticalSection(&ext->cs); remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); ext->vtbl->free_device(device);
I believe that ntoskrnl should guarantee we won't receive any IRPs after IRP_MN_REMOVE_DEVICE, which makes this hunk redundant.
Although we do, I don't think there's guarantees that IRP_MN_SURPRISE_REMOVAL is always sent first.
According to [1] the sequence could be IRP_MN_START_DEVICE -> IRP_MN_REMOVE_DEVICE -but probably no IRP could be queued in between- or IRP_MN_QUERY_REMOVE_DEVICE -> IRP_MN_REMOVE_DEVICE -which we don't support.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/understandi...
Right, but either way we shouldn't get any IRPs *after* IRP_MN_REMOVE_DEVICE. So nothing should be checking ext->removed, or trying to access the CS.
I don't see exactly what prevents this, but alright. FWIW the CS is currently acquired in IRP_MN_REMOVE_DEVICE, even before these patches.
The only way I can see that this is indeed unnecessary is if IRP_MN_REMOVE_DEVICE is special and only sent when all the handles to the device have been closed and all IRPs have been processed (either completed or marked pending).
(And if there's no such guarantees I can easily see how this could go wrong if IRP_MN_REMOVE_DEVICE completes before another IRP finished its processing)
As far as I can tell from the IRP_MN_REMOVE_DEVICE call sites, we just send it when the device is removed from the list, and it doesn't look obvious to me that there's such guarantees.