[PATCH 1/6] ntoskrnl.exe/tests: Return success from IOCTL_HID_GET_STRING.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- This series should hopefully not interfere with the other hidclass.sys, but I'm actually sending it to get some feedback about PATCH 4. Here's a bit of context: Implementing support for reports in the HID test driver, instead of returning STATUS_NOT_IMPLEMENTED, prevent spurious reboots on Windows when reading the reports, so it has to be done. However, returning input reports immediately in non-polled mode makes Windows loop forever on device initialization, probably trying to drain any input report buffer. So we have to keep them as pending until they are read. This then causes a lock-up on Wine, unless we properly cancel the IRPs on device removal. But the lock-up happens before the minidriver gets notified of the removal, unless PATCH 4 is there. dlls/ntoskrnl.exe/tests/driver_hid.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/tests/driver_hid.c b/dlls/ntoskrnl.exe/tests/driver_hid.c index acb51d99816..01a3747cd29 100644 --- a/dlls/ntoskrnl.exe/tests/driver_hid.c +++ b/dlls/ntoskrnl.exe/tests/driver_hid.c @@ -386,7 +386,9 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) ok(!in_size, "got input size %u\n", in_size); ok(out_size == 128, "got output size %u\n", out_size); - ret = STATUS_NOT_IMPLEMENTED; + memcpy(irp->UserBuffer, L"Wine Test", sizeof(L"Wine Test")); + irp->IoStatus.Information = sizeof(L"Wine Test"); + ret = STATUS_SUCCESS; break; default: -- 2.31.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver_hid.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/dlls/ntoskrnl.exe/tests/driver_hid.c b/dlls/ntoskrnl.exe/tests/driver_hid.c index 01a3747cd29..e684e2531db 100644 --- a/dlls/ntoskrnl.exe/tests/driver_hid.c +++ b/dlls/ntoskrnl.exe/tests/driver_hid.c @@ -382,6 +382,24 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) break; } + case IOCTL_HID_GET_INPUT_REPORT: + { + HID_XFER_PACKET *packet = irp->UserBuffer; + ULONG expected_size = 23; + ok(!in_size, "got input size %u\n", in_size); + ok(out_size == sizeof(*packet), "got output size %u\n", out_size); + + ok(packet->reportId == report_id, "got packet report id %u\n", packet->reportId); + ok(packet->reportBufferLen == expected_size, "got packet buffer len %u\n", packet->reportBufferLen); + ok(!!packet->reportBuffer, "got packet buffer %p\n", packet->reportBuffer); + + memset(packet->reportBuffer, 0xa5, packet->reportBufferLen); + if (report_id) ((char *)packet->reportBuffer)[0] = report_id; + irp->IoStatus.Information = expected_size; + ret = STATUS_SUCCESS; + break; + } + case IOCTL_HID_GET_STRING: ok(!in_size, "got input size %u\n", in_size); ok(out_size == 128, "got output size %u\n", out_size); -- 2.31.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/hidclass.sys/device.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index 82366ad1888..bbb7205a2e8 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -587,8 +587,16 @@ NTSTATUS WINAPI pdo_read(DEVICE_OBJECT *device, IRP *irp) UINT buffer_size = RingBuffer_GetBufferSize(ext->u.pdo.ring_buffer); NTSTATUS rc = STATUS_SUCCESS; IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp); + const WINE_HIDP_PREPARSED_DATA *data = ext->u.pdo.preparsed_data; int ptr = -1; + if (irpsp->Parameters.Read.Length < data->caps.InputReportByteLength) + { + irp->IoStatus.Status = STATUS_INVALID_BUFFER_SIZE; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_INVALID_BUFFER_SIZE; + } + packet = malloc(buffer_size); ptr = PtrToUlong( irp->Tail.Overlay.OriginalFileObject->FsContext ); @@ -664,6 +672,13 @@ NTSTATUS WINAPI pdo_write(DEVICE_OBJECT *device, IRP *irp) ULONG max_len; NTSTATUS rc; + if (irpsp->Parameters.Write.Length < data->caps.OutputReportByteLength) + { + irp->IoStatus.Status = irpsp->Parameters.Write.Length ? STATUS_INVALID_PARAMETER : STATUS_INVALID_BUFFER_SIZE; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return irp->IoStatus.Status; + } + irp->IoStatus.Information = 0; TRACE_(hid_report)("Device %p Buffer length %i Buffer %p\n", device, irpsp->Parameters.Write.Length, irp->AssociatedIrp.SystemBuffer); -- 2.31.0
So that mini driver gets notified first and has a chance to cancel pending IRPs. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/pnp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index e425712d738..b249e8401d2 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -356,6 +356,8 @@ static void remove_device( DEVICE_OBJECT *device ) TRACE("Removing device %p.\n", device); + send_pnp_irp( device, IRP_MN_SURPRISE_REMOVAL ); + if (wine_device->children) { ULONG i; @@ -363,7 +365,6 @@ static void remove_device( DEVICE_OBJECT *device ) remove_device( wine_device->children->Objects[i] ); } - send_pnp_irp( device, IRP_MN_SURPRISE_REMOVAL ); send_pnp_irp( device, IRP_MN_REMOVE_DEVICE ); } -- 2.31.0
On 6/18/21 7:06 AM, Rémi Bernon wrote:
So that mini driver gets notified first and has a chance to cancel pending IRPs.
Which minidriver? Isn't it the responsibility of the child to terminate all pending IRPs (and disallow further ones) on removal? See also [0], [1], [2], which say that queued requests should be completed in (both, for some reason) IRP_MN_SURPRISE_REMOVAL and IRP_MN_REMOVE_DEVICE. I think this deserves tests. [0] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-... [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an... [2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
On 6/18/21 5:59 PM, Zebediah Figura (she/her) wrote:
On 6/18/21 7:06 AM, Rémi Bernon wrote:
So that mini driver gets notified first and has a chance to cancel pending IRPs.
Which minidriver?
Isn't it the responsibility of the child to terminate all pending IRPs (and disallow further ones) on removal? See also [0], [1], [2], which say that queued requests should be completed in (both, for some reason) IRP_MN_SURPRISE_REMOVAL and IRP_MN_REMOVE_DEVICE.
I think this deserves tests.
[0] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...
[2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
I really don't have the big picture yet, so I meant the one implemented in driver_hid.c. If it doesn't cancel the IRP it has queued on device removal, the test never completes the device removal on Windows. On Wine, driver_hid never receives the IRP_MN_SURPRISE_REMOVAL if we don't call it before removing the "children" in ntoskrnl.exe. I don't really understand who are the children there, but driver_hid apparently isn't. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 6/18/21 1:49 PM, Rémi Bernon wrote:
On 6/18/21 5:59 PM, Zebediah Figura (she/her) wrote:
On 6/18/21 7:06 AM, Rémi Bernon wrote:
So that mini driver gets notified first and has a chance to cancel pending IRPs.
Which minidriver?
Isn't it the responsibility of the child to terminate all pending IRPs (and disallow further ones) on removal? See also [0], [1], [2], which say that queued requests should be completed in (both, for some reason) IRP_MN_SURPRISE_REMOVAL and IRP_MN_REMOVE_DEVICE.
I think this deserves tests.
[0] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...
[2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
I really don't have the big picture yet, so I meant the one implemented in driver_hid.c. If it doesn't cancel the IRP it has queued on device removal, the test never completes the device removal on Windows.
On Wine, driver_hid never receives the IRP_MN_SURPRISE_REMOVAL if we don't call it before removing the "children" in ntoskrnl.exe. I don't really understand who are the children there, but driver_hid apparently isn't.
Allow me to explain in detail. Apologies if any of this is old news to you. If nothing else I'll adapt it to a wiki page later ;-) TLC PDO winehid.sys + hidclass.sys HID FDO ------------------------------------------------- HID PDO winebus.sys root FDO ------------------------------------------------- root PDO ntoskrnl.exe [PNP manager] In the case of the HID test driver it's a little different: TLC PDO driver_hid.dll + hidclass.sys root/HID FDO ------------------------------------------------- root/HID PDO ntoskrnl.exe [PNP manager] A root PNP device is the one passed to AddDevice for a root PNP driver. Usually it's intended to enumerate children; here we kind of cheat and make the root PnP device the same device as the HID device. It's a bit weird but it works. The TLC (top-level collection) PDO is managed entirely within hidclass.sys; the minidriver never interacts with it. In theory there's supposed to be one or more of these; we don't handle the "or more" case yet. The application only ever interacts with the TLC PDO. It's actually impossible to open the HID device stack (IIRC it fails with STATUS_UNSUCCESSFUL). In response to read/write/hidd ioctls, hidclass creates new irps and sends them to the HID device stack, i.e. the "HID FDO/HID PDO" pair in the above diagrams. [Note that IRPs are always sent to the top of a stack first, so in this case the minidriver has a chance to handle it. In a real HID driver the minidriver would always handle it itself, rather than sending it down to the PDO. You can mentally replace winehid with hidusb, and winebus with (say) usbccgp. Obviously usbccgp doesn't know anything about HID. Our architecture is a little weird, though, for various reasons.] There is usually no TLC FDO, but in theory there could be, and in practice I believe mouse and keyboard drivers attach to the TLC PDO. On reflection, I think I may have confused myself, so this patch actually makes more sense than it initially did. It'd still be nice to have tests, though, and the commit message should ideally be more specific.
On 6/18/21 11:06 PM, Zebediah Figura (she/her) wrote:
On 6/18/21 1:49 PM, Rémi Bernon wrote:
On 6/18/21 5:59 PM, Zebediah Figura (she/her) wrote:
On 6/18/21 7:06 AM, Rémi Bernon wrote:
So that mini driver gets notified first and has a chance to cancel pending IRPs.
Which minidriver?
Isn't it the responsibility of the child to terminate all pending IRPs (and disallow further ones) on removal? See also [0], [1], [2], which say that queued requests should be completed in (both, for some reason) IRP_MN_SURPRISE_REMOVAL and IRP_MN_REMOVE_DEVICE.
I think this deserves tests.
[0] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...
[2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
I really don't have the big picture yet, so I meant the one implemented in driver_hid.c. If it doesn't cancel the IRP it has queued on device removal, the test never completes the device removal on Windows.
On Wine, driver_hid never receives the IRP_MN_SURPRISE_REMOVAL if we don't call it before removing the "children" in ntoskrnl.exe. I don't really understand who are the children there, but driver_hid apparently isn't.
Allow me to explain in detail. Apologies if any of this is old news to you. If nothing else I'll adapt it to a wiki page later ;-)
TLC PDO winehid.sys + hidclass.sys HID FDO ------------------------------------------------- HID PDO winebus.sys root FDO ------------------------------------------------- root PDO ntoskrnl.exe [PNP manager]
In the case of the HID test driver it's a little different:
TLC PDO driver_hid.dll + hidclass.sys root/HID FDO ------------------------------------------------- root/HID PDO ntoskrnl.exe [PNP manager]
A root PNP device is the one passed to AddDevice for a root PNP driver. Usually it's intended to enumerate children; here we kind of cheat and make the root PnP device the same device as the HID device. It's a bit weird but it works.
The TLC (top-level collection) PDO is managed entirely within hidclass.sys; the minidriver never interacts with it. In theory there's supposed to be one or more of these; we don't handle the "or more" case yet.
The application only ever interacts with the TLC PDO. It's actually impossible to open the HID device stack (IIRC it fails with STATUS_UNSUCCESSFUL). In response to read/write/hidd ioctls, hidclass creates new irps and sends them to the HID device stack, i.e. the "HID FDO/HID PDO" pair in the above diagrams.
[Note that IRPs are always sent to the top of a stack first, so in this case the minidriver has a chance to handle it. In a real HID driver the minidriver would always handle it itself, rather than sending it down to the PDO. You can mentally replace winehid with hidusb, and winebus with (say) usbccgp. Obviously usbccgp doesn't know anything about HID. Our architecture is a little weird, though, for various reasons.]
There is usually no TLC FDO, but in theory there could be, and in practice I believe mouse and keyboard drivers attach to the TLC PDO.
On reflection, I think I may have confused myself, so this patch actually makes more sense than it initially did. It'd still be nice to have tests, though, and the commit message should ideally be more specific.
Thanks, this is definitely not old news, although it's still quite blurry. From [1] I understand that the hid minidriver lives at the HID FDO level, which means that (minidriver, hidclass.sys) act as the bus FDO driver here. Reading [2] or [3] doesn't make it completely explicit if the children should receive the removal IRP first, but from what I understand it is how it's generally supposed to work. With that understanding I now believe it's correct to call the IRP on the children of the device first. The problem I'm facing here comes from the way hidclass.sys PDO handles its removal. It waits for the reading thread to complete, which may be waiting on IRPs queued in the HID FDO. We could either: 1) call the surprise removal first on the device stack, then call the normal removal on the stack, giving a chance for the HID FDO to cancel its queued IRPs. or, 2) combine the waits between halt_event and the pending IRP in the HID PDO device thread, so that the thread can terminate even with a pending IRP. Unrelated, but I'm not sure to understand why the HID PDO has to re-create IRPs every time it needs to call the minidriver (except for the device thread of course), wouldn't it be possible to just use the normal IRP propagation down the stack? [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/hid-architectu... [2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an... [3] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-... -- Rémi Bernon <rbernon(a)codeweavers.com>
On 6/21/21 12:29 PM, Rémi Bernon wrote:
On 6/18/21 11:06 PM, Zebediah Figura (she/her) wrote:
On 6/18/21 1:49 PM, Rémi Bernon wrote:
On 6/18/21 5:59 PM, Zebediah Figura (she/her) wrote:
On 6/18/21 7:06 AM, Rémi Bernon wrote:
So that mini driver gets notified first and has a chance to cancel pending IRPs.
Which minidriver?
Isn't it the responsibility of the child to terminate all pending IRPs (and disallow further ones) on removal? See also [0], [1], [2], which say that queued requests should be completed in (both, for some reason) IRP_MN_SURPRISE_REMOVAL and IRP_MN_REMOVE_DEVICE.
I think this deserves tests.
[0] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...
[2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
I really don't have the big picture yet, so I meant the one implemented in driver_hid.c. If it doesn't cancel the IRP it has queued on device removal, the test never completes the device removal on Windows.
On Wine, driver_hid never receives the IRP_MN_SURPRISE_REMOVAL if we don't call it before removing the "children" in ntoskrnl.exe. I don't really understand who are the children there, but driver_hid apparently isn't.
Allow me to explain in detail. Apologies if any of this is old news to you. If nothing else I'll adapt it to a wiki page later ;-)
TLC PDO winehid.sys + hidclass.sys HID FDO ------------------------------------------------- HID PDO winebus.sys root FDO ------------------------------------------------- root PDO ntoskrnl.exe [PNP manager]
In the case of the HID test driver it's a little different:
TLC PDO driver_hid.dll + hidclass.sys root/HID FDO ------------------------------------------------- root/HID PDO ntoskrnl.exe [PNP manager]
A root PNP device is the one passed to AddDevice for a root PNP driver. Usually it's intended to enumerate children; here we kind of cheat and make the root PnP device the same device as the HID device. It's a bit weird but it works.
The TLC (top-level collection) PDO is managed entirely within hidclass.sys; the minidriver never interacts with it. In theory there's supposed to be one or more of these; we don't handle the "or more" case yet.
The application only ever interacts with the TLC PDO. It's actually impossible to open the HID device stack (IIRC it fails with STATUS_UNSUCCESSFUL). In response to read/write/hidd ioctls, hidclass creates new irps and sends them to the HID device stack, i.e. the "HID FDO/HID PDO" pair in the above diagrams.
[Note that IRPs are always sent to the top of a stack first, so in this case the minidriver has a chance to handle it. In a real HID driver the minidriver would always handle it itself, rather than sending it down to the PDO. You can mentally replace winehid with hidusb, and winebus with (say) usbccgp. Obviously usbccgp doesn't know anything about HID. Our architecture is a little weird, though, for various reasons.]
There is usually no TLC FDO, but in theory there could be, and in practice I believe mouse and keyboard drivers attach to the TLC PDO.
On reflection, I think I may have confused myself, so this patch actually makes more sense than it initially did. It'd still be nice to have tests, though, and the commit message should ideally be more specific.
Thanks, this is definitely not old news, although it's still quite blurry.
From [1] I understand that the hid minidriver lives at the HID FDO level, which means that (minidriver, hidclass.sys) act as the bus FDO driver here.
Yep, that's correct.
Reading [2] or [3] doesn't make it completely explicit if the children should receive the removal IRP first, but from what I understand it is how it's generally supposed to work.
So [4] actually explicitly says this. It's also something I gleaned from trying to run some driver, I think it may have even been the toaster sample. I added some quick tests here, and they seem to confirm it.
With that understanding I now believe it's correct to call the IRP on the children of the device first.
The problem I'm facing here comes from the way hidclass.sys PDO handles its removal. It waits for the reading thread to complete, which may be waiting on IRPs queued in the HID FDO.
We could either:
1) call the surprise removal first on the device stack, then call the normal removal on the stack, giving a chance for the HID FDO to cancel its queued IRPs. or,
2) combine the waits between halt_event and the pending IRP in the HID PDO device thread, so that the thread can terminate even with a pending IRP.
So it turns out that, as far as I can tell, the intended solution is mostly (1). Device removal is extremely complicated in Windows, and I've already gotten it wrong multiple times, but after some extensive reading and testing here's what I've found: * When a device is reported missing via IoInvalidateDeviceRelations, IRP_MN_SURPRISE_REMOVAL is sent. And it turns out that this patch is sort of correct: it is indeed sent to the parent before IRP_MN_REMOVE_DEVICE is sent to the child. However, children are still supposed to receive IRP_MN_SURPRISE_REMOVAL before their parents. It's a bit tricky to actually test this, though. You need a function driver, and that made my tests a lot more difficult (it wreaks havoc with WM_DEVICECHANGE for some reason). So I'm willing to retract my comment demanding more tests. * And it is also correct that a lower (parent) device is supposed to complete any IRP under its jurisdiction during IRP_MN_SURPRISE_REMOVAL. [2] prescribes this, and the toaster sample bears it out. * When the parent device is removed via SetupDiRemoveDevice, it does not receive IRP_MN_SURPRISE_REMOVAL. Instead, it receives IRP_MN_QUERY_REMOVE. This is sent recursively, just like IRP_MN_SURPRISE_REMOVAL, and if successful IRP_MN_REMOVE_DEVICE is sent. * Microsoft doesn't document that you're supposed to complete IRPs in IRP_MN_QUERY_REMOVE. But a quick test shows that hidclass depends on it, and all of the few samples in the Windows 7 WDK that I checked seem to do so as well. I'll have to check if the native third-party HID driver I have does as well...
Unrelated, but I'm not sure to understand why the HID PDO has to re-create IRPs every time it needs to call the minidriver (except for the device thread of course), wouldn't it be possible to just use the normal IRP propagation down the stack?
The tricky thing about it is that it's not the same stack. There's a driver stack, and then there's *the* driver stack, and unfortunately I don't even think that Microsoft uses different terminology to refer to either. Generally speaking, an IRP can only be sent down a single FDO->(filter)->PDO chain. But the PDO might need to make some query about the driver stack below it, e.g. as a HID device might need to make a USB request.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/hid-architectu...
[2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...
[3] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/removing-a-...
[4] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-an...
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(a)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; +}; + +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; + } + break; + } + + 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; } @@ -389,7 +472,9 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) ok(!in_size, "got input size %u\n", in_size); ok(out_size == sizeof(*packet), "got output size %u\n", out_size); + todo_wine_if(report_id) ok(packet->reportId == report_id, "got packet report id %u\n", packet->reportId); + todo_wine_if(report_id) ok(packet->reportBufferLen == expected_size, "got packet buffer len %u\n", packet->reportBufferLen); ok(!!packet->reportBuffer, "got packet buffer %p\n", packet->reportBuffer); @@ -414,8 +499,11 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) ret = STATUS_NOT_IMPLEMENTED; } - irp->IoStatus.Status = ret; - IoCompleteRequest(irp, IO_NO_INCREMENT); + if (ret != STATUS_PENDING) + { + irp->IoStatus.Status = ret; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } return ret; } @@ -475,6 +563,7 @@ NTSTATUS WINAPI DriverEntry(DRIVER_OBJECT *driver, UNICODE_STRING *registry) { .Revision = HID_REVISION, .DriverObject = driver, + .DeviceExtensionSize = sizeof(struct minidevice_extension), .RegistryPath = registry, }; UNICODE_STRING name_str; diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 5453af8ff1c..df661327b41 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1609,8 +1609,9 @@ static void test_hidp(HANDLE file, int report_id) .Usage = HID_USAGE_GENERIC_JOYSTICK, .UsagePage = HID_USAGE_PAGE_GENERIC, .InputReportByteLength = 24, + .OutputReportByteLength = 2, .FeatureReportByteLength = 18, - .NumberLinkCollectionNodes = 8, + .NumberLinkCollectionNodes = 9, .NumberInputButtonCaps = 13, .NumberInputValueCaps = 7, .NumberInputDataIndices = 43, @@ -1623,8 +1624,9 @@ static void test_hidp(HANDLE file, int report_id) .Usage = HID_USAGE_GENERIC_JOYSTICK, .UsagePage = HID_USAGE_PAGE_GENERIC, .InputReportByteLength = 23, + .OutputReportByteLength = 2, .FeatureReportByteLength = 17, - .NumberLinkCollectionNodes = 8, + .NumberLinkCollectionNodes = 9, .NumberInputButtonCaps = 13, .NumberInputValueCaps = 7, .NumberInputDataIndices = 43, @@ -1766,8 +1768,8 @@ static void test_hidp(HANDLE file, int report_id) .LinkUsage = HID_USAGE_GENERIC_JOYSTICK, .LinkUsagePage = HID_USAGE_PAGE_GENERIC, .CollectionType = 1, - .NumberOfChildren = 5, - .FirstChild = 7, + .NumberOfChildren = 6, + .FirstChild = 8, }, { .LinkUsage = HID_USAGE_GENERIC_JOYSTICK, @@ -2569,6 +2571,64 @@ static void test_hidp(HANDLE file, int report_id) todo_wine ok(!memcmp(buffer, buffer + 16, 16), "unexpected report value\n"); + memset(report, 0xcd, sizeof(report)); + status = HidP_InitializeReportForID(HidP_Input, report_id, preparsed_data, report, caps.InputReportByteLength); + todo_wine_if(report_id) + ok(status == HIDP_STATUS_SUCCESS, "HidP_InitializeReportForID returned %#x\n", status); + + SetLastError(0xdeadbeef); + ret = HidD_GetInputReport(file, report, caps.InputReportByteLength); + ok(ret, "HidD_GetInputReport failed, last error %u\n", GetLastError()); + + memset(report, 0xcd, sizeof(report)); + SetLastError(0xdeadbeef); + ret = ReadFile(file, report, 0, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_USER_BUFFER, "ReadFile failed, last error %u\n", GetLastError()); + ok(value == 0, "ReadFile returned %x\n", value); + SetLastError(0xdeadbeef); + ret = ReadFile(file, report, caps.InputReportByteLength - 1, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_USER_BUFFER, "ReadFile failed, last error %u\n", GetLastError()); + ok(value == 0, "ReadFile returned %x\n", value); + + SetLastError(0xdeadbeef); + ret = WriteFile(file, report, 0, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_USER_BUFFER, "WriteFile failed, last error %u\n", GetLastError()); + ok(value == 0, "WriteFile returned %x\n", value); + SetLastError(0xdeadbeef); + ret = WriteFile(file, report, caps.OutputReportByteLength - 1, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_PARAMETER, "WriteFile failed, last error %u\n", GetLastError()); + ok(value == 0, "WriteFile returned %x\n", value); + + memset(report, 0xcd, sizeof(report)); + report[0] = 0xa5; + SetLastError(0xdeadbeef); + ret = WriteFile(file, report, caps.OutputReportByteLength, &value, NULL); + if (report_id) + { + todo_wine + ok(!ret && GetLastError() == ERROR_INVALID_PARAMETER, "WriteFile succeeded, last error %u\n", GetLastError()); + todo_wine + ok(value == 0, "WriteFile returned %x\n", value); + + SetLastError(0xdeadbeef); + report[0] = report_id; + ret = WriteFile(file, report, caps.OutputReportByteLength, &value, NULL); + ok(ret, "WriteFile failed, last error %u\n", GetLastError()); + ok(value == caps.OutputReportByteLength, "WriteFile returned %x\n", value); + } + else + { + ok(ret, "WriteFile failed, last error %u\n", GetLastError()); + ok(value == caps.OutputReportByteLength, "WriteFile returned %x\n", value); + } + + memset( report, 0xcd, sizeof(report) ); + SetLastError(0xdeadbeef); + ret = ReadFile( file, report, caps.InputReportByteLength, &value, NULL ); + ok(ret, "ReadFile failed, last error %u\n", GetLastError()); + ok(value == caps.InputReportByteLength, "ReadFile returned %x\n", value); + ok(report[0] == report_id, "unexpected report data\n"); + HidD_FreePreparsedData(preparsed_data); CloseHandle(file); } @@ -2616,7 +2676,7 @@ static void test_hid_device(DWORD report_id) todo_wine ok(found, "didn't find device\n"); - file = CreateFileA(iface_detail->DevicePath, FILE_READ_ACCESS, + file = CreateFileA(iface_detail->DevicePath, FILE_READ_ACCESS | FILE_WRITE_ACCESS, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL); ok(file != INVALID_HANDLE_VALUE, "got error %u\n", GetLastError()); -- 2.31.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=92750 Your paranoid android. === w7u_2qxl (32 bit report) === ntoskrnl.exe: ntoskrnl.c:2621: Test failed: report 0: WriteFile failed, last error 87 ntoskrnl.c:2622: Test failed: report 0: WriteFile returned 0 ntoskrnl.c:2628: Test failed: report 0: ReadFile failed, last error 1167 ntoskrnl.c:2629: Test failed: report 0: ReadFile returned 0 ntoskrnl.c:2630: Test failed: report 0: unexpected report data === w7u_el (32 bit report) === ntoskrnl.exe: driver.c:759: Test failed: got 0 ntoskrnl.c:2621: Test failed: report 0: WriteFile failed, last error 87 ntoskrnl.c:2622: Test failed: report 0: WriteFile returned 0 ntoskrnl: Timeout === w1064_tsign (64 bit report) === ntoskrnl.exe: driver.c:272: Test failed: Got unexpected test_load_image_notify_count 0.
On 6/18/21 2:46 PM, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=92750
Your paranoid android.
=== w7u_2qxl (32 bit report) ===
ntoskrnl.exe: ntoskrnl.c:2621: Test failed: report 0: WriteFile failed, last error 87 ntoskrnl.c:2622: Test failed: report 0: WriteFile returned 0 ntoskrnl.c:2628: Test failed: report 0: ReadFile failed, last error 1167 ntoskrnl.c:2629: Test failed: report 0: ReadFile returned 0 ntoskrnl.c:2630: Test failed: report 0: unexpected report data
=== w7u_el (32 bit report) ===
ntoskrnl.exe: driver.c:759: Test failed: got 0 ntoskrnl.c:2621: Test failed: report 0: WriteFile failed, last error 87 ntoskrnl.c:2622: Test failed: report 0: WriteFile returned 0 ntoskrnl: Timeout
=== w1064_tsign (64 bit report) ===
ntoskrnl.exe: driver.c:272: Test failed: Got unexpected test_load_image_notify_count 0.
I incorrectly considered W10 behavior to be standard, and it looks like older versions didn't check the report IDs on write, so I'll have to fix these last two patches. -- Rémi Bernon <rbernon(a)codeweavers.com>
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(a)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".
+ +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.
+ } + 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.
+ 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?
@@ -389,7 +472,9 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) ok(!in_size, "got input size %u\n", in_size); ok(out_size == sizeof(*packet), "got output size %u\n", out_size);
+ todo_wine_if(report_id) ok(packet->reportId == report_id, "got packet report id %u\n", packet->reportId); + todo_wine_if(report_id) ok(packet->reportBufferLen == expected_size, "got packet buffer len %u\n", packet->reportBufferLen); ok(!!packet->reportBuffer, "got packet buffer %p\n", packet->reportBuffer);
@@ -414,8 +499,11 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) ret = STATUS_NOT_IMPLEMENTED; }
- irp->IoStatus.Status = ret; - IoCompleteRequest(irp, IO_NO_INCREMENT); + if (ret != STATUS_PENDING) + { + irp->IoStatus.Status = ret; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } return ret; }
@@ -475,6 +563,7 @@ NTSTATUS WINAPI DriverEntry(DRIVER_OBJECT *driver, UNICODE_STRING *registry) { .Revision = HID_REVISION, .DriverObject = driver, + .DeviceExtensionSize = sizeof(struct minidevice_extension), .RegistryPath = registry, }; UNICODE_STRING name_str; diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 5453af8ff1c..df661327b41 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1609,8 +1609,9 @@ static void test_hidp(HANDLE file, int report_id) .Usage = HID_USAGE_GENERIC_JOYSTICK, .UsagePage = HID_USAGE_PAGE_GENERIC, .InputReportByteLength = 24, + .OutputReportByteLength = 2, .FeatureReportByteLength = 18, - .NumberLinkCollectionNodes = 8, + .NumberLinkCollectionNodes = 9, .NumberInputButtonCaps = 13, .NumberInputValueCaps = 7, .NumberInputDataIndices = 43, @@ -1623,8 +1624,9 @@ static void test_hidp(HANDLE file, int report_id) .Usage = HID_USAGE_GENERIC_JOYSTICK, .UsagePage = HID_USAGE_PAGE_GENERIC, .InputReportByteLength = 23, + .OutputReportByteLength = 2, .FeatureReportByteLength = 17, - .NumberLinkCollectionNodes = 8, + .NumberLinkCollectionNodes = 9, .NumberInputButtonCaps = 13, .NumberInputValueCaps = 7, .NumberInputDataIndices = 43, @@ -1766,8 +1768,8 @@ static void test_hidp(HANDLE file, int report_id) .LinkUsage = HID_USAGE_GENERIC_JOYSTICK, .LinkUsagePage = HID_USAGE_PAGE_GENERIC, .CollectionType = 1, - .NumberOfChildren = 5, - .FirstChild = 7, + .NumberOfChildren = 6, + .FirstChild = 8, }, { .LinkUsage = HID_USAGE_GENERIC_JOYSTICK, @@ -2569,6 +2571,64 @@ static void test_hidp(HANDLE file, int report_id) todo_wine ok(!memcmp(buffer, buffer + 16, 16), "unexpected report value\n");
+ memset(report, 0xcd, sizeof(report)); + status = HidP_InitializeReportForID(HidP_Input, report_id, preparsed_data, report, caps.InputReportByteLength); + todo_wine_if(report_id) + ok(status == HIDP_STATUS_SUCCESS, "HidP_InitializeReportForID returned %#x\n", status); + + SetLastError(0xdeadbeef); + ret = HidD_GetInputReport(file, report, caps.InputReportByteLength); + ok(ret, "HidD_GetInputReport failed, last error %u\n", GetLastError()); + + memset(report, 0xcd, sizeof(report)); + SetLastError(0xdeadbeef); + ret = ReadFile(file, report, 0, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_USER_BUFFER, "ReadFile failed, last error %u\n", GetLastError()); + ok(value == 0, "ReadFile returned %x\n", value); + SetLastError(0xdeadbeef); + ret = ReadFile(file, report, caps.InputReportByteLength - 1, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_USER_BUFFER, "ReadFile failed, last error %u\n", GetLastError()); + ok(value == 0, "ReadFile returned %x\n", value); + + SetLastError(0xdeadbeef); + ret = WriteFile(file, report, 0, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_USER_BUFFER, "WriteFile failed, last error %u\n", GetLastError()); + ok(value == 0, "WriteFile returned %x\n", value); + SetLastError(0xdeadbeef); + ret = WriteFile(file, report, caps.OutputReportByteLength - 1, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_PARAMETER, "WriteFile failed, last error %u\n", GetLastError()); + ok(value == 0, "WriteFile returned %x\n", value); + + memset(report, 0xcd, sizeof(report)); + report[0] = 0xa5; + SetLastError(0xdeadbeef); + ret = WriteFile(file, report, caps.OutputReportByteLength, &value, NULL); + if (report_id) + { + todo_wine + ok(!ret && GetLastError() == ERROR_INVALID_PARAMETER, "WriteFile succeeded, last error %u\n", GetLastError()); + todo_wine + ok(value == 0, "WriteFile returned %x\n", value); + + SetLastError(0xdeadbeef); + report[0] = report_id; + ret = WriteFile(file, report, caps.OutputReportByteLength, &value, NULL); + ok(ret, "WriteFile failed, last error %u\n", GetLastError()); + ok(value == caps.OutputReportByteLength, "WriteFile returned %x\n", value); + } + else + { + ok(ret, "WriteFile failed, last error %u\n", GetLastError()); + ok(value == caps.OutputReportByteLength, "WriteFile returned %x\n", value); + } + + memset( report, 0xcd, sizeof(report) ); + SetLastError(0xdeadbeef); + ret = ReadFile( file, report, caps.InputReportByteLength, &value, NULL ); + ok(ret, "ReadFile failed, last error %u\n", GetLastError()); + ok(value == caps.InputReportByteLength, "ReadFile returned %x\n", value); + ok(report[0] == report_id, "unexpected report data\n"); + HidD_FreePreparsedData(preparsed_data); CloseHandle(file); } @@ -2616,7 +2676,7 @@ static void test_hid_device(DWORD report_id)
todo_wine ok(found, "didn't find device\n");
- file = CreateFileA(iface_detail->DevicePath, FILE_READ_ACCESS, + file = CreateFileA(iface_detail->DevicePath, FILE_READ_ACCESS | FILE_WRITE_ACCESS, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL); ok(file != INVALID_HANDLE_VALUE, "got error %u\n", GetLastError());
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(a)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(a)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 <rbernon(a)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.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver_hid.c | 19 +++++++++++++++++-- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 24 ++++++++++++++++-------- 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/driver_hid.c b/dlls/ntoskrnl.exe/tests/driver_hid.c index eb81426823b..ed8a0021347 100644 --- a/dlls/ntoskrnl.exe/tests/driver_hid.c +++ b/dlls/ntoskrnl.exe/tests/driver_hid.c @@ -41,6 +41,7 @@ static UNICODE_STRING control_symlink; static unsigned int got_start_device; static DWORD report_id; +static DWORD polled_mode; struct minidevice_extension { @@ -419,6 +420,13 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) if (out_size != expected_size) test_failed = TRUE; if (mext->removed) ret = STATUS_DEVICE_REMOVED; + else if (polled_mode) + { + memset(irp->UserBuffer, 0xa5, expected_size); + if (report_id) ((char *)irp->UserBuffer)[0] = report_id; + irp->IoStatus.Information = expected_size; + ret = STATUS_SUCCESS; + } else { InsertTailList(&mext->irp_queue, &irp->Tail.Overlay.ListEntry); @@ -450,7 +458,7 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) ok(packet->reportBuffer[0] == 0xcd, "got first byte %x\n", packet->reportBuffer[0]); } - if ((entry = RemoveHeadList(&mext->irp_queue)) != &mext->irp_queue) + if (!polled_mode && (entry = RemoveHeadList(&mext->irp_queue)) != &mext->irp_queue) { IRP *tmp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); memset(tmp->UserBuffer, 0xa5, 23); @@ -474,7 +482,7 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) todo_wine_if(report_id) ok(packet->reportId == report_id, "got packet report id %u\n", packet->reportId); - todo_wine_if(report_id) + todo_wine_if(packet->reportBufferLen == 22 || packet->reportBufferLen == 24) ok(packet->reportBufferLen == expected_size, "got packet buffer len %u\n", packet->reportBufferLen); ok(!!packet->reportBuffer, "got packet buffer %p\n", packet->reportBuffer); @@ -585,6 +593,13 @@ NTSTATUS WINAPI DriverEntry(DRIVER_OBJECT *driver, UNICODE_STRING *registry) ok(!ret, "ZwQueryValueKey returned %#x\n", ret); memcpy(&report_id, buffer + info_size, size - info_size); + RtlInitUnicodeString(&name_str, L"PolledMode"); + size = info_size + sizeof(polled_mode); + ret = ZwQueryValueKey(hkey, &name_str, KeyValuePartialInformation, buffer, size, &size); + ok(!ret, "ZwQueryValueKey returned %#x\n", ret); + memcpy(&polled_mode, buffer + info_size, size - info_size); + params.DevicesArePolled = polled_mode; + driver->DriverExtension->AddDevice = driver_add_device; driver->DriverUnload = driver_unload; driver->MajorFunction[IRP_MJ_PNP] = driver_pnp; diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index df661327b41..fee0368fe6b 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1600,7 +1600,7 @@ static inline void check_hidp_value_caps_(int line, HIDP_VALUE_CAPS *caps, const } } -static void test_hidp(HANDLE file, int report_id) +static void test_hidp(HANDLE file, int report_id, int polled_mode) { const HIDP_CAPS expect_hidp_caps[] = { @@ -2625,15 +2625,18 @@ static void test_hidp(HANDLE file, int report_id) memset( report, 0xcd, sizeof(report) ); SetLastError(0xdeadbeef); ret = ReadFile( file, report, caps.InputReportByteLength, &value, NULL ); + todo_wine_if(polled_mode) ok(ret, "ReadFile failed, last error %u\n", GetLastError()); + todo_wine_if(polled_mode) ok(value == caps.InputReportByteLength, "ReadFile returned %x\n", value); + todo_wine_if(polled_mode) ok(report[0] == report_id, "unexpected report data\n"); HidD_FreePreparsedData(preparsed_data); CloseHandle(file); } -static void test_hid_device(DWORD report_id) +static void test_hid_device(DWORD report_id, DWORD polled_mode) { char buffer[200]; SP_DEVICE_INTERFACE_DETAIL_DATA_A *iface_detail = (void *)buffer; @@ -2648,7 +2651,7 @@ static void test_hid_device(DWORD report_id) HDEVINFO set; HANDLE file; - winetest_push_context("report %d", report_id); + winetest_push_context("report %d, polled %d", report_id, polled_mode); set = SetupDiGetClassDevsA(&GUID_DEVINTERFACE_HID, NULL, NULL, DIGCF_DEVICEINTERFACE | DIGCF_PRESENT); ok(set != INVALID_HANDLE_VALUE, "failed to get device list, error %#x\n", GetLastError()); @@ -2680,7 +2683,7 @@ static void test_hid_device(DWORD report_id) FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL); ok(file != INVALID_HANDLE_VALUE, "got error %u\n", GetLastError()); - test_hidp(file, report_id); + test_hidp(file, report_id, polled_mode); CloseHandle(file); @@ -2692,7 +2695,7 @@ static void test_hid_device(DWORD report_id) winetest_pop_context(); } -static void test_hid_driver(struct testsign_context *ctx, DWORD report_id) +static void test_hid_driver(struct testsign_context *ctx, DWORD report_id, DWORD polled_mode) { static const char hardware_id[] = "test_hardware_id\0"; char path[MAX_PATH], dest[MAX_PATH], *filepart; @@ -2717,6 +2720,9 @@ static void test_hid_driver(struct testsign_context *ctx, DWORD report_id) status = RegSetValueExW(hkey, L"ReportID", 0, REG_DWORD, (void *)&report_id, sizeof(report_id)); ok(!status, "RegSetValueExW returned %#x\n", status); + status = RegSetValueExW(hkey, L"PolledMode", 0, REG_DWORD, (void *)&polled_mode, sizeof(polled_mode)); + ok(!status, "RegSetValueExW returned %#x\n", status); + load_resource(L"driver_hid.dll", driver_filename); ret = MoveFileExW(driver_filename, L"winetest.sys", MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING); ok(ret, "failed to move file, error %u\n", GetLastError()); @@ -2764,7 +2770,7 @@ static void test_hid_driver(struct testsign_context *ctx, DWORD report_id) /* Tests. */ - test_hid_device(report_id); + test_hid_device(report_id, polled_mode); /* Clean up. */ @@ -2895,8 +2901,10 @@ START_TEST(ntoskrnl) test_pnp_driver(&ctx); subtest("driver_hid"); - test_hid_driver(&ctx, 0); - test_hid_driver(&ctx, 1); + test_hid_driver(&ctx, 0, FALSE); + test_hid_driver(&ctx, 1, FALSE); + test_hid_driver(&ctx, 0, TRUE); + test_hid_driver(&ctx, 1, TRUE); out: testsign_cleanup(&ctx); -- 2.31.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=92751 Your paranoid android. === w7u_2qxl (32 bit report) === ntoskrnl.exe: ntoskrnl.c:2621: Test failed: report 0, polled 0: WriteFile failed, last error 87 ntoskrnl.c:2622: Test failed: report 0, polled 0: WriteFile returned 0 ntoskrnl.c:2629: Test failed: report 0, polled 0: ReadFile failed, last error 1167 ntoskrnl.c:2631: Test failed: report 0, polled 0: ReadFile returned 0 ntoskrnl.c:2633: Test failed: report 0, polled 0: unexpected report data ntoskrnl.c:2621: Test failed: report 0, polled 1: WriteFile failed, last error 87 ntoskrnl.c:2622: Test failed: report 0, polled 1: WriteFile returned 0 === w7u_el (32 bit report) === ntoskrnl.exe: driver.c:759: Test failed: got 0 ntoskrnl.c:2621: Test failed: report 0, polled 0: WriteFile failed, last error 87 ntoskrnl.c:2622: Test failed: report 0, polled 0: WriteFile returned 0 ntoskrnl: Timeout === debiant2 (32 bit Arabic:Morocco report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (67920 bytes) === debiant2 (32 bit German report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (67920 bytes) === debiant2 (32 bit Japanese:Japan report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (67920 bytes)
participants (3)
-
Marvin -
Rémi Bernon -
Zebediah Figura (she/her)