[PATCH 5/6] ntoskrnl.exe/tests: Add tests to read/write reports on device.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Fri Jun 18 14:49:00 CDT 2021
On 6/18/21 1:45 PM, Rémi Bernon wrote:
> 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.
You probably want spinlocks (KeAcquireSpinLock, KeReleaseSpinLock). You
do have to be careful about what you do inside of a spinlock, though. In
particular, I'm not sure it's safe to call IoCompleteRequest() inside of
a spinlock.
>
>>> + 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.
>
It seems that you actually can't handle custom ioctls either on the PDO
or FDO, which is kind of annoying, but in lieu of that we could create
another device.
If you want to use a thread, you can use PsCreateSystemThread(). Being
able to test completion behaviour from user space may be helpful in
general, though.
More information about the wine-devel
mailing list