[PATCH 1/5] wineusb.sys: Introduce new remove_pending_irps helper.

[Rémi Bernon]

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/wineusb.sys/wineusb.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/dlls/wineusb.sys/wineusb.c b/dlls/wineusb.sys/wineusb.c
index b8cefdaff5e..8fb1d75f121 100644
--- a/dlls/wineusb.sys/wineusb.c
+++ b/dlls/wineusb.sys/wineusb.c
@@ -363,6 +363,20 @@ static NTSTATUS query_id(const struct usb_device *device, IRP *irp, BUS_QUERY_ID
     return STATUS_SUCCESS;
 }
 
+static void remove_pending_irps(struct usb_device *device)
+{
+    LIST_ENTRY *entry;
+    IRP *irp;
+
+    while ((entry = RemoveHeadList(&device->irp_list)) != &device->irp_list)
+    {
+        irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
+        irp->IoStatus.Status = STATUS_DELETE_PENDING;
+        irp->IoStatus.Information = 0;
+        IoCompleteRequest(irp, IO_NO_INCREMENT);
+    }
+}
+
 static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp)
 {
     IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp);
@@ -393,17 +407,8 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp)
             break;
 
         case IRP_MN_REMOVE_DEVICE:
-        {
-            LIST_ENTRY *entry;
-
             EnterCriticalSection(&wineusb_cs);
-            while ((entry = RemoveHeadList(&device->irp_list)) != &device->irp_list)
-            {
-                irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
-                irp->IoStatus.Status = STATUS_DELETE_PENDING;
-                irp->IoStatus.Information = 0;
-                IoCompleteRequest(irp, IO_NO_INCREMENT);
-            }
+            remove_pending_irps(device);
             LeaveCriticalSection(&wineusb_cs);
 
             if (device->removed)
@@ -419,7 +424,6 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp)
 
             ret = STATUS_SUCCESS;
             break;
-        }
 
         default:
             FIXME("Unhandled minor function %#x.\n", stack->MinorFunction);
-- 
2.32.0

[PATCH 2/5] wineusb.sys: Handle IRP_MN_SURPRISE_REMOVAL and set removed flag.

[Rémi Bernon]

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/wineusb.sys/wineusb.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/dlls/wineusb.sys/wineusb.c b/dlls/wineusb.sys/wineusb.c
index 8fb1d75f121..a72095409ff 100644
--- a/dlls/wineusb.sys/wineusb.c
+++ b/dlls/wineusb.sys/wineusb.c
@@ -145,7 +145,6 @@ static void remove_usb_device(libusb_device *libusb_device)
         if (device->libusb_device == libusb_device)
         {
             list_remove(&device->entry);
-            device->removed = TRUE;
             break;
         }
     }
@@ -402,26 +401,24 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp)
         }
 
         case IRP_MN_START_DEVICE:
-        case IRP_MN_SURPRISE_REMOVAL:
             ret = STATUS_SUCCESS;
             break;
 
-        case IRP_MN_REMOVE_DEVICE:
+        case IRP_MN_SURPRISE_REMOVAL:
             EnterCriticalSection(&wineusb_cs);
             remove_pending_irps(device);
+            device->removed = TRUE;
             LeaveCriticalSection(&wineusb_cs);
+            ret = STATUS_SUCCESS;
+            break;
 
-            if (device->removed)
-            {
-                libusb_unref_device(device->libusb_device);
-                libusb_close(device->handle);
+        case IRP_MN_REMOVE_DEVICE:
+            remove_pending_irps(device);
 
-                irp->IoStatus.Status = STATUS_SUCCESS;
-                IoCompleteRequest(irp, IO_NO_INCREMENT);
-                IoDeleteDevice(device->device_obj);
-                return STATUS_SUCCESS;
-            }
+            libusb_unref_device(device->libusb_device);
+            libusb_close(device->handle);
 
+            IoDeleteDevice(device->device_obj);
             ret = STATUS_SUCCESS;
             break;
 
-- 
2.32.0

[PATCH 2/5] wineusb.sys: Handle IRP_MN_SURPRISE_REMOVAL and set removed flag.

[Zebediah Figura (she/her)]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>

[PATCH 3/5] wineusb.sys: Return STATUS_DELETE_PENDING when device is removed.

[Rémi Bernon]

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/wineusb.sys/wineusb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/dlls/wineusb.sys/wineusb.c b/dlls/wineusb.sys/wineusb.c
index a72095409ff..fae297915fc 100644
--- a/dlls/wineusb.sys/wineusb.c
+++ b/dlls/wineusb.sys/wineusb.c
@@ -726,9 +726,21 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device_obj, IRP *irp
     ULONG code = stack->Parameters.DeviceIoControl.IoControlCode;
     struct usb_device *device = device_obj->DeviceExtension;
     NTSTATUS status = STATUS_NOT_IMPLEMENTED;
+    BOOL removed;
 
     TRACE("device_obj %p, irp %p, code %#x.\n", device_obj, irp, code);
 
+    EnterCriticalSection(&wineusb_cs);
+    removed = device->removed;
+    LeaveCriticalSection(&wineusb_cs);
+
+    if (removed)
+    {
+        irp->IoStatus.Status = STATUS_DELETE_PENDING;
+        IoCompleteRequest(irp, IO_NO_INCREMENT);
+        return STATUS_DELETE_PENDING;
+    }
+
     switch (code)
     {
         case IOCTL_INTERNAL_USB_SUBMIT_URB:
-- 
2.32.0

[PATCH 3/5] wineusb.sys: Return STATUS_DELETE_PENDING when device is removed.

[Zebediah Figura (she/her)]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>

[PATCH 4/5] hidclass.sys: Stop accepting IRPs after device removal.

[Rémi Bernon]

Handle IRP_MN_SURPRISE_REMOVAL and notify device thread to stop it, but
only wait for it in IRP_MN_REMOVE_DEVICE, as it's probably waiting for
an IRP sent to the lower driver, which needs to be notified of removal
too first.

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/hidclass.sys/device.c | 39 ++++++++++++++++++++++++++++++++++++++
 dlls/hidclass.sys/hid.h    |  3 +++
 dlls/hidclass.sys/pnp.c    | 24 ++++++++++++++++++++++-
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
index 82366ad1888..d3f5fa90fc7 100644
--- a/dlls/hidclass.sys/device.c
+++ b/dlls/hidclass.sys/device.c
@@ -431,11 +431,24 @@ NTSTATUS WINAPI pdo_ioctl(DEVICE_OBJECT *device, IRP *irp)
     NTSTATUS rc = STATUS_SUCCESS;
     IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
     BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
+    BOOL removed;
+    KIRQL irql;
 
     irp->IoStatus.Information = 0;
 
     TRACE("device %p ioctl(%x)\n", device, irpsp->Parameters.DeviceIoControl.IoControlCode);
 
+    KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
+    removed = ext->u.pdo.removed;
+    KeReleaseSpinLock(&ext->u.pdo.lock, irql);
+
+    if (removed)
+    {
+        irp->IoStatus.Status = STATUS_DELETE_PENDING;
+        IoCompleteRequest(irp, IO_NO_INCREMENT);
+        return STATUS_DELETE_PENDING;
+    }
+
     switch (irpsp->Parameters.DeviceIoControl.IoControlCode)
     {
         case IOCTL_HID_GET_POLL_FREQUENCY_MSEC:
@@ -588,6 +601,19 @@ NTSTATUS WINAPI pdo_read(DEVICE_OBJECT *device, IRP *irp)
     NTSTATUS rc = STATUS_SUCCESS;
     IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
     int ptr = -1;
+    BOOL removed;
+    KIRQL irql;
+
+    KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
+    removed = ext->u.pdo.removed;
+    KeReleaseSpinLock(&ext->u.pdo.lock, irql);
+
+    if (removed)
+    {
+        irp->IoStatus.Status = STATUS_DELETE_PENDING;
+        IoCompleteRequest(irp, IO_NO_INCREMENT);
+        return STATUS_DELETE_PENDING;
+    }
 
     packet = malloc(buffer_size);
     ptr = PtrToUlong( irp->Tail.Overlay.OriginalFileObject->FsContext );
@@ -662,7 +688,20 @@ NTSTATUS WINAPI pdo_write(DEVICE_OBJECT *device, IRP *irp)
     const WINE_HIDP_PREPARSED_DATA *data = ext->u.pdo.preparsed_data;
     HID_XFER_PACKET packet;
     ULONG max_len;
+    BOOL removed;
     NTSTATUS rc;
+    KIRQL irql;
+
+    KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
+    removed = ext->u.pdo.removed;
+    KeReleaseSpinLock(&ext->u.pdo.lock, irql);
+
+    if (removed)
+    {
+        irp->IoStatus.Status = STATUS_DELETE_PENDING;
+        IoCompleteRequest(irp, IO_NO_INCREMENT);
+        return STATUS_DELETE_PENDING;
+    }
 
     irp->IoStatus.Information = 0;
 
diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h
index 5a502840691..863bb9a8bd4 100644
--- a/dlls/hidclass.sys/hid.h
+++ b/dlls/hidclass.sys/hid.h
@@ -67,6 +67,9 @@ typedef struct _BASE_DEVICE_EXTENSION
             KSPIN_LOCK irp_queue_lock;
             LIST_ENTRY irp_queue;
 
+            KSPIN_LOCK lock;
+            BOOL       removed;
+
             BOOL is_mouse;
             UNICODE_STRING mouse_link_name;
             BOOL is_keyboard;
diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c
index 5f59257cdf7..5d6fb3d068c 100644
--- a/dlls/hidclass.sys/pnp.c
+++ b/dlls/hidclass.sys/pnp.c
@@ -359,11 +359,24 @@ static NTSTATUS fdo_pnp(DEVICE_OBJECT *device, IRP *irp)
     }
 }
 
+static void remove_pending_irps(BASE_DEVICE_EXTENSION *ext)
+{
+    IRP *irp;
+
+    while ((irp = pop_irp_from_queue(ext)))
+    {
+        irp->IoStatus.Status = STATUS_DELETE_PENDING;
+        IoCompleteRequest(irp, IO_NO_INCREMENT);
+    }
+}
+
 static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
 {
     IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
     BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
     NTSTATUS status = irp->IoStatus.Status;
+    IRP *queued_irp;
+    KIRQL irql;
 
     TRACE("irp %p, minor function %#x.\n", irp, irpsp->MinorFunction);
 
@@ -453,12 +466,14 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
                 IoSetDeviceInterfaceState(&ext->u.pdo.mouse_link_name, TRUE);
             if (ext->u.pdo.is_keyboard)
                 IoSetDeviceInterfaceState(&ext->u.pdo.keyboard_link_name, TRUE);
+
+            ext->u.pdo.removed = FALSE;
             status = STATUS_SUCCESS;
             break;
 
         case IRP_MN_REMOVE_DEVICE:
         {
-            IRP *queued_irp;
+            remove_pending_irps(ext);
 
             send_wm_input_device_change(ext, GIDC_REMOVAL);
 
@@ -494,6 +509,13 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
         }
 
         case IRP_MN_SURPRISE_REMOVAL:
+            KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
+            ext->u.pdo.removed = TRUE;
+            KeReleaseSpinLock(&ext->u.pdo.lock, irql);
+
+            remove_pending_irps(ext);
+
+            SetEvent(ext->u.pdo.halt_event);
             status = STATUS_SUCCESS;
             break;
 
-- 
2.32.0

[PATCH 4/5] hidclass.sys: Stop accepting IRPs after device removal.

[Zebediah Figura (she/her)]

On 7/1/21 2:51 AM, Rémi Bernon wrote:
> Handle IRP_MN_SURPRISE_REMOVAL and notify device thread to stop it, but
> only wait for it in IRP_MN_REMOVE_DEVICE, as it's probably waiting for
> an IRP sent to the lower driver, which needs to be notified of removal
> too first.
>
> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
> ---
>   dlls/hidclass.sys/device.c | 39 ++++++++++++++++++++++++++++++++++++++
>   dlls/hidclass.sys/hid.h    |  3 +++
>   dlls/hidclass.sys/pnp.c    | 24 ++++++++++++++++++++++-
>   3 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> index 82366ad1888..d3f5fa90fc7 100644
> --- a/dlls/hidclass.sys/device.c
> +++ b/dlls/hidclass.sys/device.c
> @@ -431,11 +431,24 @@ NTSTATUS WINAPI pdo_ioctl(DEVICE_OBJECT *device, IRP *irp)
>       NTSTATUS rc = STATUS_SUCCESS;
>       IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
>       BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
> +    BOOL removed;
> +    KIRQL irql;
>
> irp->IoStatus.Information = 0;
>
> TRACE("device %p ioctl(%x)\n", device, irpsp->Parameters.DeviceIoControl.IoControlCode);
>
> +    KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
> +    removed = ext->u.pdo.removed;
> +    KeReleaseSpinLock(&ext->u.pdo.lock, irql);
> +
> +    if (removed)
> +    {
> +        irp->IoStatus.Status = STATUS_DELETE_PENDING;
> +        IoCompleteRequest(irp, IO_NO_INCREMENT);
> +        return STATUS_DELETE_PENDING;
> +    }
> +
>       switch (irpsp->Parameters.DeviceIoControl.IoControlCode)
>       {
>           case IOCTL_HID_GET_POLL_FREQUENCY_MSEC:
> @@ -588,6 +601,19 @@ NTSTATUS WINAPI pdo_read(DEVICE_OBJECT *device, IRP *irp)
>       NTSTATUS rc = STATUS_SUCCESS;
>       IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
>       int ptr = -1;
> +    BOOL removed;
> +    KIRQL irql;
> +
> +    KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
> +    removed = ext->u.pdo.removed;
> +    KeReleaseSpinLock(&ext->u.pdo.lock, irql);
> +
> +    if (removed)
> +    {
> +        irp->IoStatus.Status = STATUS_DELETE_PENDING;
> +        IoCompleteRequest(irp, IO_NO_INCREMENT);
> +        return STATUS_DELETE_PENDING;
> +    }
>
> packet = malloc(buffer_size);
>       ptr = PtrToUlong( irp->Tail.Overlay.OriginalFileObject->FsContext );
> @@ -662,7 +688,20 @@ NTSTATUS WINAPI pdo_write(DEVICE_OBJECT *device, IRP *irp)
>       const WINE_HIDP_PREPARSED_DATA *data = ext->u.pdo.preparsed_data;
>       HID_XFER_PACKET packet;
>       ULONG max_len;
> +    BOOL removed;
>       NTSTATUS rc;
> +    KIRQL irql;
> +
> +    KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
> +    removed = ext->u.pdo.removed;
> +    KeReleaseSpinLock(&ext->u.pdo.lock, irql);
> +
> +    if (removed)
> +    {
> +        irp->IoStatus.Status = STATUS_DELETE_PENDING;
> +        IoCompleteRequest(irp, IO_NO_INCREMENT);
> +        return STATUS_DELETE_PENDING;
> +    }
>
> irp->IoStatus.Information = 0;
>
> diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h
> index 5a502840691..863bb9a8bd4 100644
> --- a/dlls/hidclass.sys/hid.h
> +++ b/dlls/hidclass.sys/hid.h
> @@ -67,6 +67,9 @@ typedef struct _BASE_DEVICE_EXTENSION
>               KSPIN_LOCK irp_queue_lock;
>               LIST_ENTRY irp_queue;
>
> +            KSPIN_LOCK lock;
> +            BOOL       removed;

Style nitpick, but none of the other members of this structure are 
aligned...
> +
>               BOOL is_mouse;
>               UNICODE_STRING mouse_link_name;
>               BOOL is_keyboard;
> diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c
> index 5f59257cdf7..5d6fb3d068c 100644
> --- a/dlls/hidclass.sys/pnp.c
> +++ b/dlls/hidclass.sys/pnp.c
> @@ -359,11 +359,24 @@ static NTSTATUS fdo_pnp(DEVICE_OBJECT *device, IRP *irp)
>       }
>   }
>
> +static void remove_pending_irps(BASE_DEVICE_EXTENSION *ext)
> +{
> +    IRP *irp;
> +
> +    while ((irp = pop_irp_from_queue(ext)))
> +    {
> +        irp->IoStatus.Status = STATUS_DELETE_PENDING;
> +        IoCompleteRequest(irp, IO_NO_INCREMENT);
> +    }
> +}
> +
>   static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
>   {
>       IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
>       BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
>       NTSTATUS status = irp->IoStatus.Status;
> +    IRP *queued_irp;
> +    KIRQL irql;
>
> TRACE("irp %p, minor function %#x.\n", irp, irpsp->MinorFunction);
>
> @@ -453,12 +466,14 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
>                   IoSetDeviceInterfaceState(&ext->u.pdo.mouse_link_name, TRUE);
>               if (ext->u.pdo.is_keyboard)
>                   IoSetDeviceInterfaceState(&ext->u.pdo.keyboard_link_name, TRUE);
> +
> +            ext->u.pdo.removed = FALSE;
>               status = STATUS_SUCCESS;
>               break;
>
> case IRP_MN_REMOVE_DEVICE:
>           {
> -            IRP *queued_irp;
> +            remove_pending_irps(ext);

In that case there's no reason to delete queued IRPs later in the handler.
> send_wm_input_device_change(ext, GIDC_REMOVAL);
>
> @@ -494,6 +509,13 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
>           }
>
> case IRP_MN_SURPRISE_REMOVAL:
> +            KeAcquireSpinLock(&ext->u.pdo.lock, &irql);
> +            ext->u.pdo.removed = TRUE;
> +            KeReleaseSpinLock(&ext->u.pdo.lock, irql);
> +
> +            remove_pending_irps(ext);
> +
> +            SetEvent(ext->u.pdo.halt_event);
>               status = STATUS_SUCCESS;
>               break;

[PATCH 4/5] hidclass.sys: Stop accepting IRPs after device removal.

[Rémi Bernon]

On 7/1/21 5:16 PM, Zebediah Figura (she/her) wrote:
> > diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c
> > index 5f59257cdf7..5d6fb3d068c 100644
> > --- a/dlls/hidclass.sys/pnp.c
> > +++ b/dlls/hidclass.sys/pnp.c
> > @@ -359,11 +359,24 @@ static NTSTATUS fdo_pnp(DEVICE_OBJECT *device, 
> > IRP *irp)
> >       }
> >   }
> > +static void remove_pending_irps(BASE_DEVICE_EXTENSION *ext)
> > +{
> > +    IRP *irp;
> > +
> > +    while ((irp = pop_irp_from_queue(ext)))
> > +    {
> > +        irp->IoStatus.Status = STATUS_DELETE_PENDING;
> > +        IoCompleteRequest(irp, IO_NO_INCREMENT);
> > +    }
> > +}
> > +
> >   static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
> >   {
> >       IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
> >       BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
> >       NTSTATUS status = irp->IoStatus.Status;
> > +    IRP *queued_irp;
> > +    KIRQL irql;
> >       TRACE("irp %p, minor function %#x.\n", irp, irpsp->MinorFunction);
> > @@ -453,12 +466,14 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, 
> > IRP *irp)
> >
> > IoSetDeviceInterfaceState(&ext->u.pdo.mouse_link_name, TRUE);
> >               if (ext->u.pdo.is_keyboard)
> >
> > IoSetDeviceInterfaceState(&ext->u.pdo.keyboard_link_name, TRUE);
> > +
> > +            ext->u.pdo.removed = FALSE;
> >               status = STATUS_SUCCESS;
> >               break;
> >           case IRP_MN_REMOVE_DEVICE:
> >           {
> > -            IRP *queued_irp;
> > +            remove_pending_irps(ext);
>
> In that case there's no reason to delete queued IRPs later in the handler.

Well, according to [1] it's what we are supposed to do:
> 4. Prevent any new I/O operations on the device.
>
> [...]
>
> 5. Fail outstanding I/O requests on the device.

[1] 
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...

-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH 4/5] hidclass.sys: Stop accepting IRPs after device removal.

[Zebediah Figura (she/her)]

On 7/1/21 11:29 AM, Rémi Bernon wrote:
> On 7/1/21 5:16 PM, Zebediah Figura (she/her) wrote:
> > > diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c
> > > index 5f59257cdf7..5d6fb3d068c 100644
> > > --- a/dlls/hidclass.sys/pnp.c
> > > +++ b/dlls/hidclass.sys/pnp.c
> > > @@ -359,11 +359,24 @@ static NTSTATUS fdo_pnp(DEVICE_OBJECT *device,
> > > IRP *irp)
> > >        }
> > >    }
> > > +static void remove_pending_irps(BASE_DEVICE_EXTENSION *ext)
> > > +{
> > > +    IRP *irp;
> > > +
> > > +    while ((irp = pop_irp_from_queue(ext)))
> > > +    {
> > > +        irp->IoStatus.Status = STATUS_DELETE_PENDING;
> > > +        IoCompleteRequest(irp, IO_NO_INCREMENT);
> > > +    }
> > > +}
> > > +
> > >    static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
> > >    {
> > >        IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
> > >        BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
> > >        NTSTATUS status = irp->IoStatus.Status;
> > > +    IRP *queued_irp;
> > > +    KIRQL irql;
> > >        TRACE("irp %p, minor function %#x.\n", irp, irpsp->MinorFunction);
> > > @@ -453,12 +466,14 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device,
> > > IRP *irp)
> > >
> > > IoSetDeviceInterfaceState(&ext->u.pdo.mouse_link_name, TRUE);
> > >                if (ext->u.pdo.is_keyboard)
> > >
> > > IoSetDeviceInterfaceState(&ext->u.pdo.keyboard_link_name, TRUE);
> > > +
> > > +            ext->u.pdo.removed = FALSE;
> > >                status = STATUS_SUCCESS;
> > >                break;
> > >            case IRP_MN_REMOVE_DEVICE:
> > >            {
> > > -            IRP *queued_irp;
> > > +            remove_pending_irps(ext);
> >
> > In that case there's no reason to delete queued IRPs later in the handler.
>
> Well, according to [1] it's what we are supposed to do:
> > 4. Prevent any new I/O operations on the device.
> >
> > [...]
> >
> > 5. Fail outstanding I/O requests on the device.

As I understand step 4 corresponds to setting the 'removed' flag to 
TRUE. Also, I'm talking about IRP_MN_REMOVE_DEVICE here, and not 
IRP_MN_SURPRISE_REMOVAL. Currently we're failing outstanding I/O twice.
> [1]
> https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...

[PATCH 4/5] hidclass.sys: Stop accepting IRPs after device removal.

[Rémi Bernon]

On 7/1/21 6:43 PM, Zebediah Figura (she/her) wrote:
> On 7/1/21 11:29 AM, Rémi Bernon wrote:
> > On 7/1/21 5:16 PM, Zebediah Figura (she/her) wrote:
> > > > diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c
> > > > index 5f59257cdf7..5d6fb3d068c 100644
> > > > --- a/dlls/hidclass.sys/pnp.c
> > > > +++ b/dlls/hidclass.sys/pnp.c
> > > > @@ -359,11 +359,24 @@ static NTSTATUS fdo_pnp(DEVICE_OBJECT *device,
> > > > IRP *irp)
> > > >        }
> > > >    }
> > > > +static void remove_pending_irps(BASE_DEVICE_EXTENSION *ext)
> > > > +{
> > > > +    IRP *irp;
> > > > +
> > > > +    while ((irp = pop_irp_from_queue(ext)))
> > > > +    {
> > > > +        irp->IoStatus.Status = STATUS_DELETE_PENDING;
> > > > +        IoCompleteRequest(irp, IO_NO_INCREMENT);
> > > > +    }
> > > > +}
> > > > +
> > > >    static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
> > > >    {
> > > >        IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
> > > >        BASE_DEVICE_EXTENSION *ext = device->DeviceExtension;
> > > >        NTSTATUS status = irp->IoStatus.Status;
> > > > +    IRP *queued_irp;
> > > > +    KIRQL irql;
> > > >        TRACE("irp %p, minor function %#x.\n", irp, 
> > > > irpsp->MinorFunction);
> > > > @@ -453,12 +466,14 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device,
> > > > IRP *irp)
> > > > IoSetDeviceInterfaceState(&ext->u.pdo.mouse_link_name, TRUE);
> > > >                if (ext->u.pdo.is_keyboard)
> > > > IoSetDeviceInterfaceState(&ext->u.pdo.keyboard_link_name, TRUE);
> > > > +
> > > > +            ext->u.pdo.removed = FALSE;
> > > >                status = STATUS_SUCCESS;
> > > >                break;
> > > >            case IRP_MN_REMOVE_DEVICE:
> > > >            {
> > > > -            IRP *queued_irp;
> > > > +            remove_pending_irps(ext);
> > >
> > > In that case there's no reason to delete queued IRPs later in the 
> > > handler.
> >
> > Well, according to [1] it's what we are supposed to do:
> > > 4. Prevent any new I/O operations on the device.
> > >
> > > [...]
> > >
> > > 5. Fail outstanding I/O requests on the device.
>
> As I understand step 4 corresponds to setting the 'removed' flag to 
> TRUE. Also, I'm talking about IRP_MN_REMOVE_DEVICE here, and not 
> IRP_MN_SURPRISE_REMOVAL. Currently we're failing outstanding I/O twice.

Oh right, I thought this was referring the next hunk.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH 5/5] ntoskrnl.exe: Send IRP_MN_SURPRISE_REMOVAL to the device stack first.

[Rémi Bernon]

Instead of sending both IRP_MN_SURPRISE_REMOVAL and IRP_MN_REMOVE_DEVICE
to all children first. They may have pending IRPs sent to their parent
PDO driver, which need to be cancelled before IRP_MN_REMOVE_DEVICE can
complete. This is the case for hidclass.sys and its thread calling the
lower driver for instance.

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/ntoskrnl.exe/pnp.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c
index e425712d738..d8eb62bc17a 100644
--- a/dlls/ntoskrnl.exe/pnp.c
+++ b/dlls/ntoskrnl.exe/pnp.c
@@ -350,21 +350,20 @@ static void enumerate_new_device( DEVICE_OBJECT *device, HDEVINFO set )
     start_device( device, set, &sp_device );
 }
 
-static void remove_device( DEVICE_OBJECT *device )
+static void send_remove_device_irp( DEVICE_OBJECT *device, UCHAR code )
 {
     struct wine_device *wine_device = CONTAINING_RECORD(device, struct wine_device, device_obj);
 
-    TRACE("Removing device %p.\n", device);
+    TRACE( "Removing device %p, code %x.\n", device, code );
 
     if (wine_device->children)
     {
         ULONG i;
         for (i = 0; i < wine_device->children->Count; ++i)
-            remove_device( wine_device->children->Objects[i] );
+            send_remove_device_irp( wine_device->children->Objects[i], code );
     }
 
-    send_pnp_irp( device, IRP_MN_SURPRISE_REMOVAL );
-    send_pnp_irp( device, IRP_MN_REMOVE_DEVICE );
+    send_pnp_irp( device, code );
 }
 
 static BOOL device_in_list( const DEVICE_RELATIONS *list, const DEVICE_OBJECT *device )
@@ -441,7 +440,8 @@ static void handle_bus_relations( DEVICE_OBJECT *parent )
             if (!device_in_list( relations, child ))
             {
                 TRACE("Removing device %p.\n", child);
-                remove_device( child );
+                send_remove_device_irp( child, IRP_MN_SURPRISE_REMOVAL );
+                send_remove_device_irp( child, IRP_MN_REMOVE_DEVICE );
             }
             ObDereferenceObject( child );
         }
@@ -1105,7 +1105,10 @@ void pnp_manager_stop_driver( struct wine_driver *driver )
     struct root_pnp_device *device, *next;
 
     LIST_FOR_EACH_ENTRY_SAFE( device, next, &driver->root_pnp_devices, struct root_pnp_device, entry )
-        remove_device( device->device );
+    {
+        send_remove_device_irp( device->device, IRP_MN_SURPRISE_REMOVAL );
+        send_remove_device_irp( device->device, IRP_MN_REMOVE_DEVICE );
+    }
 }
 
 void pnp_manager_stop(void)
@@ -1179,7 +1182,8 @@ void CDECL wine_enumerate_root_devices( const WCHAR *driver_name )
     {
         TRACE("Removing device %s.\n", debugstr_w(pnp_device->id));
 
-        remove_device( pnp_device->device );
+        send_remove_device_irp( pnp_device->device, IRP_MN_SURPRISE_REMOVAL );
+        send_remove_device_irp( pnp_device->device, IRP_MN_REMOVE_DEVICE );
     }
 
     list_move_head( &driver->root_pnp_devices, &new_list );
-- 
2.32.0

[Zebediah Figura (she/her)]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>