[PATCH 2/3] hidclass.sys: Keep pending IRPs with the report queues.
Aric Stewart
aric at codeweavers.com
Wed Sep 29 11:31:42 CDT 2021
Signed-off-by: Aric Stewart <aric at codeweavers.com>
On 9/24/21 3:07 AM, Rémi Bernon wrote:
> Since d15358518b83384b137e81b71729c4f47fac0665 we only complete one
> pending IRP per HID report, but there may be more than one IRP queued,
> from different readers.
>
> This causes trouble and report interleaving when more than one reader
> accesses a device at a time. We need to complete only one for each
> report queue instead.
>
> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
> ---
> dlls/hidclass.sys/device.c | 158 +++++++++++++++++++++----------------
> dlls/hidclass.sys/hid.h | 6 +-
> dlls/hidclass.sys/pnp.c | 20 +----
> 3 files changed, 98 insertions(+), 86 deletions(-)
>
> diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> index 21941a1802e..8f816bc0b2d 100644
> --- a/dlls/hidclass.sys/device.c
> +++ b/dlls/hidclass.sys/device.c
> @@ -33,45 +33,20 @@
>
> WINE_DEFAULT_DEBUG_CHANNEL(hid);
>
> -IRP *pop_irp_from_queue(BASE_DEVICE_EXTENSION *ext)
> -{
> - LIST_ENTRY *entry;
> - KIRQL old_irql;
> - IRP *irp = NULL;
> -
> - KeAcquireSpinLock(&ext->u.pdo.irp_queue_lock, &old_irql);
> -
> - while (!irp && (entry = RemoveHeadList(&ext->u.pdo.irp_queue)) != &ext->u.pdo.irp_queue)
> - {
> - irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
> - if (!IoSetCancelRoutine(irp, NULL))
> - {
> - /* cancel routine is already cleared, meaning that it was called. let it handle completion. */
> - InitializeListHead(&irp->Tail.Overlay.ListEntry);
> - irp = NULL;
> - }
> - }
> -
> - KeReleaseSpinLock(&ext->u.pdo.irp_queue_lock, old_irql);
> - return irp;
> -}
> -
> static void WINAPI read_cancel_routine(DEVICE_OBJECT *device, IRP *irp)
> {
> - BASE_DEVICE_EXTENSION *ext;
> - KIRQL old_irql;
> + struct hid_report_queue *queue = irp->Tail.Overlay.OriginalFileObject->FsContext;
> + KIRQL irql;
>
> TRACE("cancel %p IRP on device %p\n", irp, device);
>
> - ext = device->DeviceExtension;
> -
> IoReleaseCancelSpinLock(irp->CancelIrql);
>
> - KeAcquireSpinLock(&ext->u.pdo.irp_queue_lock, &old_irql);
> + KeAcquireSpinLock( &queue->lock, &irql );
>
> RemoveEntryList(&irp->Tail.Overlay.ListEntry);
>
> - KeReleaseSpinLock(&ext->u.pdo.irp_queue_lock, old_irql);
> + KeReleaseSpinLock( &queue->lock, irql );
>
> irp->IoStatus.Status = STATUS_CANCELLED;
> irp->IoStatus.Information = 0;
> @@ -107,6 +82,7 @@ static struct hid_report_queue *hid_report_queue_create( void )
> struct hid_report_queue *queue;
>
> if (!(queue = calloc( 1, sizeof(struct hid_report_queue) ))) return NULL;
> + InitializeListHead( &queue->irp_queue );
> KeInitializeSpinLock( &queue->lock );
> list_init( &queue->entry );
> queue->length = 32;
> @@ -116,8 +92,43 @@ static struct hid_report_queue *hid_report_queue_create( void )
> return queue;
> }
>
> +static IRP *hid_report_queue_pop_irp( struct hid_report_queue *queue )
> +{
> + LIST_ENTRY *entry;
> + IRP *irp = NULL;
> + KIRQL irql;
> +
> + KeAcquireSpinLock( &queue->lock, &irql );
> +
> + while (!irp && (entry = RemoveHeadList( &queue->irp_queue )) != &queue->irp_queue)
> + {
> + irp = CONTAINING_RECORD( entry, IRP, Tail.Overlay.ListEntry );
> + if (!IoSetCancelRoutine( irp, NULL ))
> + {
> + /* cancel routine is already cleared, meaning that it was called. let it handle completion. */
> + InitializeListHead( &irp->Tail.Overlay.ListEntry );
> + irp = NULL;
> + }
> + }
> +
> + KeReleaseSpinLock( &queue->lock, irql );
> + return irp;
> +}
> +
> +void hid_report_queue_remove_pending_irps( struct hid_report_queue *queue )
> +{
> + IRP *irp;
> +
> + while ((irp = hid_report_queue_pop_irp( queue )))
> + {
> + irp->IoStatus.Status = STATUS_DELETE_PENDING;
> + IoCompleteRequest( irp, IO_NO_INCREMENT );
> + }
> +}
> +
> void hid_report_queue_destroy( struct hid_report_queue *queue )
> {
> + hid_report_queue_remove_pending_irps( queue );
> while (queue->length--) hid_report_decref( queue->reports[queue->length] );
> list_remove( &queue->entry );
> free( queue );
> @@ -144,7 +155,30 @@ static NTSTATUS hid_report_queue_resize( struct hid_report_queue *queue, ULONG l
> return STATUS_SUCCESS;
> }
>
> -static void hid_report_queue_push( struct hid_report_queue *queue, struct hid_report *report )
> +static NTSTATUS hid_report_queue_push_irp( struct hid_report_queue *queue, IRP *irp )
> +{
> + KIRQL irql;
> +
> + KeAcquireSpinLock( &queue->lock, &irql );
> +
> + IoSetCancelRoutine( irp, read_cancel_routine );
> + if (irp->Cancel && !IoSetCancelRoutine( irp, NULL ))
> + {
> + /* IRP was canceled before we set cancel routine */
> + InitializeListHead( &irp->Tail.Overlay.ListEntry );
> + KeReleaseSpinLock( &queue->lock, irql );
> + return STATUS_CANCELLED;
> + }
> +
> + InsertTailList( &queue->irp_queue, &irp->Tail.Overlay.ListEntry );
> + irp->IoStatus.Status = STATUS_PENDING;
> + IoMarkIrpPending( irp );
> +
> + KeReleaseSpinLock( &queue->lock, irql );
> + return STATUS_PENDING;
> +}
> +
> +static void hid_report_queue_push_report( struct hid_report_queue *queue, struct hid_report *report )
> {
> ULONG i = queue->write_idx, next = i + 1;
> struct hid_report *prev;
> @@ -162,7 +196,7 @@ static void hid_report_queue_push( struct hid_report_queue *queue, struct hid_re
> hid_report_decref( prev );
> }
>
> -static struct hid_report *hid_report_queue_pop( struct hid_report_queue *queue )
> +static struct hid_report *hid_report_queue_pop_report( struct hid_report_queue *queue )
> {
> ULONG i = queue->read_idx, next = i + 1;
> struct hid_report *report;
> @@ -186,6 +220,7 @@ static void hid_device_queue_input( DEVICE_OBJECT *device, HID_XFER_PACKET *pack
> const BOOL polled = ext->u.pdo.information.Polled;
> struct hid_report *last_report, *report;
> struct hid_report_queue *queue;
> + LIST_ENTRY completed, *entry;
> RAWINPUT *rawinput;
> ULONG size;
> KIRQL irql;
> @@ -223,25 +258,34 @@ static void hid_device_queue_input( DEVICE_OBJECT *device, HID_XFER_PACKET *pack
> return;
> }
>
> + InitializeListHead( &completed );
> +
> KeAcquireSpinLock( &ext->u.pdo.report_queues_lock, &irql );
> LIST_FOR_EACH_ENTRY( queue, &ext->u.pdo.report_queues, struct hid_report_queue, entry )
> - hid_report_queue_push( queue, last_report );
> - KeReleaseSpinLock( &ext->u.pdo.report_queues_lock, irql );
> -
> - do
> {
> - if (!(irp = pop_irp_from_queue( ext ))) break;
> - queue = irp->Tail.Overlay.OriginalFileObject->FsContext;
> + hid_report_queue_push_report( queue, last_report );
>
> - if (!(report = hid_report_queue_pop( queue ))) hid_report_incref( (report = last_report) );
> - memcpy( irp->AssociatedIrp.SystemBuffer, report->buffer, desc->InputLength );
> - irp->IoStatus.Information = report->length;
> - irp->IoStatus.Status = STATUS_SUCCESS;
> - hid_report_decref( report );
> + do
> + {
> + if (!(irp = hid_report_queue_pop_irp( queue ))) break;
> + if (!(report = hid_report_queue_pop_report( queue ))) hid_report_incref( (report = last_report) );
> +
> + memcpy( irp->AssociatedIrp.SystemBuffer, report->buffer, desc->InputLength );
> + irp->IoStatus.Information = report->length;
> + irp->IoStatus.Status = STATUS_SUCCESS;
> + hid_report_decref( report );
>
> + InsertTailList( &completed, &irp->Tail.Overlay.ListEntry );
> + }
> + while (polled);
> + }
> + KeReleaseSpinLock( &ext->u.pdo.report_queues_lock, irql );
> +
> + while ((entry = RemoveHeadList( &completed )) != &completed)
> + {
> + irp = CONTAINING_RECORD( entry, IRP, Tail.Overlay.ListEntry );
> IoCompleteRequest( irp, IO_NO_INCREMENT );
> }
> - while (polled);
>
> hid_report_decref( last_report );
> }
> @@ -584,7 +628,6 @@ NTSTATUS WINAPI pdo_read(DEVICE_OBJECT *device, IRP *irp)
> HIDP_COLLECTION_DESC *desc = ext->u.pdo.device_desc.CollectionDesc;
> IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
> struct hid_report *report;
> - NTSTATUS status;
> BOOL removed;
> KIRQL irql;
>
> @@ -607,36 +650,19 @@ NTSTATUS WINAPI pdo_read(DEVICE_OBJECT *device, IRP *irp)
> }
>
> irp->IoStatus.Information = 0;
> - if ((report = hid_report_queue_pop( queue )))
> + if ((report = hid_report_queue_pop_report( queue )))
> {
> memcpy( irp->AssociatedIrp.SystemBuffer, report->buffer, desc->InputLength );
> irp->IoStatus.Information = report->length;
> irp->IoStatus.Status = STATUS_SUCCESS;
> hid_report_decref( report );
> - }
> - else
> - {
> - KeAcquireSpinLock(&ext->u.pdo.irp_queue_lock, &irql);
> -
> - IoSetCancelRoutine(irp, read_cancel_routine);
> - if (irp->Cancel && !IoSetCancelRoutine(irp, NULL))
> - {
> - /* IRP was canceled before we set cancel routine */
> - InitializeListHead(&irp->Tail.Overlay.ListEntry);
> - KeReleaseSpinLock(&ext->u.pdo.irp_queue_lock, irql);
> - return STATUS_CANCELLED;
> - }
> -
> - InsertTailList(&ext->u.pdo.irp_queue, &irp->Tail.Overlay.ListEntry);
> - irp->IoStatus.Status = STATUS_PENDING;
> - IoMarkIrpPending(irp);
>
> - KeReleaseSpinLock(&ext->u.pdo.irp_queue_lock, irql);
> + IoCompleteRequest( irp, IO_NO_INCREMENT );
> + return STATUS_SUCCESS;
> }
>
> - status = irp->IoStatus.Status;
> - if (status != STATUS_PENDING) IoCompleteRequest( irp, IO_NO_INCREMENT );
> - return status;
> + return hid_report_queue_push_irp( queue, irp );
> +
> }
>
> NTSTATUS WINAPI pdo_write(DEVICE_OBJECT *device, IRP *irp)
> diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h
> index 60f3d0fb57e..c2b1e48978d 100644
> --- a/dlls/hidclass.sys/hid.h
> +++ b/dlls/hidclass.sys/hid.h
> @@ -67,9 +67,6 @@ typedef struct _BASE_DEVICE_EXTENSION
>
> UNICODE_STRING link_name;
>
> - KSPIN_LOCK irp_queue_lock;
> - LIST_ENTRY irp_queue;
> -
> KSPIN_LOCK lock;
> BOOL removed;
>
> @@ -104,6 +101,7 @@ struct hid_report_queue
> ULONG read_idx;
> ULONG write_idx;
> struct hid_report *reports[512];
> + LIST_ENTRY irp_queue;
> };
>
> typedef struct _minidriver
> @@ -123,8 +121,8 @@ void call_minidriver( ULONG code, DEVICE_OBJECT *device, void *in_buff, ULONG in
>
> /* Internal device functions */
> void HID_StartDeviceThread(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
> +void hid_report_queue_remove_pending_irps( struct hid_report_queue *queue );
> void hid_report_queue_destroy( struct hid_report_queue *queue );
> -IRP *pop_irp_from_queue(BASE_DEVICE_EXTENSION *ext) DECLSPEC_HIDDEN;
>
> NTSTATUS WINAPI pdo_ioctl(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
> NTSTATUS WINAPI pdo_read(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
> diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c
> index 8c0c8adafe6..644a94616bb 100644
> --- a/dlls/hidclass.sys/pnp.c
> +++ b/dlls/hidclass.sys/pnp.c
> @@ -222,8 +222,6 @@ static void create_child(minidriver *minidriver, DEVICE_OBJECT *fdo)
> pdo_ext->u.pdo.parent_fdo = fdo;
> list_init( &pdo_ext->u.pdo.report_queues );
> KeInitializeSpinLock( &pdo_ext->u.pdo.report_queues_lock );
> - InitializeListHead(&pdo_ext->u.pdo.irp_queue);
> - KeInitializeSpinLock(&pdo_ext->u.pdo.irp_queue_lock);
> wcscpy(pdo_ext->device_id, fdo_ext->device_id);
> wcscpy(pdo_ext->instance_id, fdo_ext->instance_id);
> pdo_ext->class_guid = fdo_ext->class_guid;
> @@ -370,17 +368,6 @@ 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);
> @@ -482,8 +469,6 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
> break;
>
> case IRP_MN_REMOVE_DEVICE:
> - remove_pending_irps(ext);
> -
> send_wm_input_device_change(ext, GIDC_REMOVAL);
>
> IoSetDeviceInterfaceState(&ext->u.pdo.link_name, FALSE);
> @@ -518,7 +503,10 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp)
> ext->u.pdo.removed = TRUE;
> KeReleaseSpinLock(&ext->u.pdo.lock, irql);
>
> - remove_pending_irps(ext);
> + KeAcquireSpinLock( &ext->u.pdo.report_queues_lock, &irql );
> + LIST_FOR_EACH_ENTRY_SAFE( queue, next, &ext->u.pdo.report_queues, struct hid_report_queue, entry )
> + hid_report_queue_remove_pending_irps( queue );
> + KeReleaseSpinLock( &ext->u.pdo.report_queues_lock, irql );
>
> SetEvent(ext->u.pdo.halt_event);
> status = STATUS_SUCCESS;
>
More information about the wine-devel
mailing list