[PATCH 5/6] ntoskrnl.exe/tests: Add tests to read/write reports on device.
Rémi Bernon
rbernon at codeweavers.com
Fri Jun 18 13:45:36 CDT 2021
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 at 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.
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list