On 6/18/21 6:06 PM, Zebediah Figura (she/her) wrote:
On 6/18/21 7:06 AM, Rémi Bernon wrote:
Marking input report read requests as pending and completing them on write, otherwise Windows keeps reading input reports and never finishes setting up the device.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com
dlls/ntoskrnl.exe/tests/driver_hid.c | 95 +++++++++++++++++++++++++++- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 70 ++++++++++++++++++-- 2 files changed, 157 insertions(+), 8 deletions(-)
diff --git a/dlls/ntoskrnl.exe/tests/driver_hid.c b/dlls/ntoskrnl.exe/tests/driver_hid.c index e684e2531db..eb81426823b 100644 --- a/dlls/ntoskrnl.exe/tests/driver_hid.c +++ b/dlls/ntoskrnl.exe/tests/driver_hid.c @@ -42,10 +42,32 @@ static UNICODE_STRING control_symlink; static unsigned int got_start_device; static DWORD report_id; +struct minidevice_extension +{ + LIST_ENTRY irp_queue; + BOOL removed; +};
I hate to nitpick, but I really don't like the naming here. I'd rather call this something like "struct hid_device". "mext" used below is also a kind of jarring name for a variable; it looks like "next".
Alright.
+static void cancel_pending_requests(DEVICE_OBJECT *device) +{ + HID_DEVICE_EXTENSION *ext = device->DeviceExtension; + struct minidevice_extension *mext = ext->MiniDeviceExtension; + LIST_ENTRY *entry; + IRP *irp;
+ while ((entry = RemoveHeadList(&mext->irp_queue)) != &mext->irp_queue) + { + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); + irp->IoStatus.Status = STATUS_CANCELLED; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } +}
static NTSTATUS WINAPI driver_pnp(DEVICE_OBJECT *device, IRP *irp) { IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); HID_DEVICE_EXTENSION *ext = device->DeviceExtension; + struct minidevice_extension *mext = ext->MiniDeviceExtension; if (winetest_debug > 1) trace("pnp %#x\n", stack->MinorFunction); @@ -53,6 +75,8 @@ static NTSTATUS WINAPI driver_pnp(DEVICE_OBJECT *device, IRP *irp) { case IRP_MN_START_DEVICE: ++got_start_device; + InitializeListHead(&mext->irp_queue); + mext->removed = FALSE; IoSetDeviceInterfaceState(&control_symlink, TRUE); irp->IoStatus.Status = STATUS_SUCCESS; break; @@ -60,10 +84,14 @@ static NTSTATUS WINAPI driver_pnp(DEVICE_OBJECT *device, IRP *irp) case IRP_MN_SURPRISE_REMOVAL: case IRP_MN_QUERY_REMOVE_DEVICE: case IRP_MN_STOP_DEVICE: + mext->removed = TRUE; + cancel_pending_requests(device); irp->IoStatus.Status = STATUS_SUCCESS; break; case IRP_MN_REMOVE_DEVICE: + mext->removed = TRUE; + cancel_pending_requests(device); IoSetDeviceInterfaceState(&control_symlink, FALSE); irp->IoStatus.Status = STATUS_SUCCESS; break; @@ -290,6 +318,16 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) REPORT_SIZE(1, 1), FEATURE(1, Data|Var|Abs), END_COLLECTION,
+ USAGE_PAGE(1, HID_USAGE_PAGE_LED), + USAGE(1, HID_USAGE_LED_GREEN), + COLLECTION(1, Report), + REPORT_ID_OR_USAGE_PAGE(1, report_id, 0), + USAGE_PAGE(1, HID_USAGE_PAGE_LED), + REPORT_COUNT(1, 8), + REPORT_SIZE(1, 1), + OUTPUT(1, Cnst|Var|Abs), + END_COLLECTION, END_COLLECTION, }; #undef REPORT_ID_OR_USAGE_PAGE @@ -297,6 +335,8 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) static BOOL test_failed; IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + HID_DEVICE_EXTENSION *ext = device->DeviceExtension; + struct minidevice_extension *mext = ext->MiniDeviceExtension; const ULONG in_size = stack->Parameters.DeviceIoControl.InputBufferLength; const ULONG out_size = stack->Parameters.DeviceIoControl.OutputBufferLength; const ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; @@ -378,7 +418,50 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) } if (out_size != expected_size) test_failed = TRUE; - ret = STATUS_NOT_IMPLEMENTED; + if (mext->removed) ret = STATUS_DEVICE_REMOVED; + else + { + InsertTailList(&mext->irp_queue, &irp->Tail.Overlay.ListEntry); + ret = STATUS_PENDING;
You need to call IoMarkIrpPending() if you're going to return STATUS_PENDING.
Sure.
+ } + break; + }
I don't think this is thread-safe. I know it's a test, but as long as we're in the kernel I'd really rather be careful; having to reboot a test VM is not fun.
Yeah, same as below I didn't know which primitive to use to make it thread safe, RtlCS isn't available there.
+ case IOCTL_HID_WRITE_REPORT: + { + HID_XFER_PACKET *packet = irp->UserBuffer; + ULONG expected_size = report_id ? 2 : 1; + LIST_ENTRY *entry; + todo_wine + ok(in_size == sizeof(*packet), "got input size %u\n", in_size); + todo_wine + ok(!out_size, "got output size %u\n", out_size); + todo_wine_if(!report_id) + ok(packet->reportBufferLen == expected_size, "got report size %u\n", packet->reportBufferLen);
+ if (report_id) + { + todo_wine_if(packet->reportBuffer[0] == 0xa5) + ok(packet->reportBuffer[0] == report_id, "got report id %x\n", packet->reportBuffer[0]); + } + else + { + todo_wine + ok(packet->reportBuffer[0] == 0xcd, "got first byte %x\n", packet->reportBuffer[0]); + }
+ if ((entry = RemoveHeadList(&mext->irp_queue)) != &mext->irp_queue) + { + IRP *tmp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); + memset(tmp->UserBuffer, 0xa5, 23); + if (report_id) ((char *)tmp->UserBuffer)[0] = report_id; + tmp->IoStatus.Information = 23; + tmp->IoStatus.Status = STATUS_SUCCESS; + IoCompleteRequest(tmp, IO_NO_INCREMENT); + }
+ irp->IoStatus.Information = packet->reportBufferLen; + ret = STATUS_SUCCESS; break; }
This seems awkward, and may stymie future attempts to test output reports. Can we just use a custom ioctl instead?
Yeah I guess, whichever is fine. Initially I wanted to have a thread but then I couldn't find an easy example of how to create one in a driver.
It wasn't obvious if I could add a custom ioctl so I took the easiest path and used an existing one.