[PATCH v2 5/5] winebus.sys: Handle IRP_MN_SURPRISE_REMOVAL and set removed flag.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Tue Jun 29 12:31:09 CDT 2021
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 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
>
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.
More information about the wine-devel
mailing list