[PATCH v2 5/5] winebus.sys: Handle IRP_MN_SURPRISE_REMOVAL and set removed flag.

[Rémi Bernon]

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 at 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/understanding-when-remove-irps-are-issued
-- 
Rémi Bernon <rbernon at codeweavers.com>