From: "Zebediah Figura (she/her)" Subject: Re: [PATCH 5/6] ntoskrnl.exe/tests: Add tests to read/write reports on device. Message-Id: Date: Fri, 18 Jun 2021 14:49:00 -0500 In-Reply-To: <77c06b61-aadd-171a-3a35-d09e6c6060c6@codeweavers.com> References: <20210618120611.703993-1-rbernon@codeweavers.com> <20210618120611.703993-5-rbernon@codeweavers.com> <77c06b61-aadd-171a-3a35-d09e6c6060c6@codeweavers.com> 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 >>> --- >>>   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.