[PATCH v2 0/1] MR11204: hidclass: Return the full report length from IOCTL_HID_WRITE_REPORT.

[Thomas Portal (＠OursCodeur)]

### What

WriteFile on a numbered HID output report returns one byte less than the buffer the application submitted. hidclass subtracts the report ID byte from the transfer length on the IOCTL_HID_WRITE_REPORT path (`params->padding = 1 - offset`). Windows returns the full length with the report ID byte included.

### Why it matters

An application that watches the WriteFile return value to advance an upload never makes progress. The Elgato Stream Deck desktop app hits this: its per-key image upload resends the first page forever because the count it gets back is always one short, so the keys never render. Removing the subtraction lets the upload finish and the keys draw.

### How I checked the Windows behaviour

A small Win32 probe writes an OutputReportByteLength buffer to the device and reads back the count. On Windows OutputReportByteLength is 1024 and WriteFile returns 1024. hidapi uses the same convention: hid_write returns report length + 1 (the report ID byte) for a numbered report.

### The test

test_write_file already asserts the Windows value (a numbered report returns report_len), and it passes on Windows. It also passed on Wine, for the wrong reason. The mock bus driver returns report_len + 1, and the hidclass subtraction cancelled the extra byte. A real minidriver returns the bytes it wrote, which is report_len, so nothing cancels the subtraction and the WriteFile result is short by one.

This sets the mock's returned length to report_len, matching what a real bus reports, and drops the padding. The asserted WriteFile results do not change (report_len for both numbered and unnumbered reports). Only the mock's intermediate value and the hidclass compensation change.

The hid test passes with both changes and no failures. Reverting the hidclass change on its own turns test_write_file red, so the test guards the behaviour directly.

For history: 94e5945102 (2021) introduced the subtraction, and a7d67c1c90 (2022) carried it forward as the padding field during the async rewrite.

Signed-off-by: Thomas Portal [e-mail]

--
  v2: hidclass: Return the full report length from IOCTL_HID_WRITE_REPORT.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11204

[PATCH v2 1/1] hidclass: Return the full report length from IOCTL_HID_WRITE_REPORT.

[Thomas Portal]

From: Thomas Portal <[e-mail]>

Writing a numbered HID output report with WriteFile returns one byte less
than the application passed in. hid_device_xfer_report subtracts the
report ID byte from the completed transfer length. Windows returns the
full length with the report ID byte included, so an application that
follows the returned count to drive an upload stalls when the count comes
up short. The Elgato Stream Deck image upload does exactly that.

The dinput test did not catch this. Its mock bus driver returns a transfer
length of report_len + 1, and the subtraction cancelled the extra byte. A
real minidriver such as winebus returns the number of bytes written, which
is the full report length, so against a real device the subtraction leaves
WriteFile one byte short.

Drop the padding and have the mock return a realistic transfer length.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59891
Signed-off-by: Thomas Portal <[e-mail]>
---
 dlls/dinput/tests/hid.c    | 14 +++-----------
 dlls/hidclass.sys/device.c |  4 ----
 2 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/dlls/dinput/tests/hid.c b/dlls/dinput/tests/hid.c
index 7c42ac54f74..b7b743009d7 100644
--- a/dlls/dinput/tests/hid.c
+++ b/dlls/dinput/tests/hid.c
@@ -1427,7 +1427,7 @@ static void test_write_file( HANDLE file, int report_id, ULONG report_len )
         .report_id = report_id,
         .report_len = report_len - (report_id ? 0 : 1),
         .report_buf = {report_id ? report_id : 0xcd,0xcd,0xcd,0xcd,0xcd},
-        .ret_length = 3,
+        .ret_length = report_len,
         .ret_status = STATUS_SUCCESS,
     };
 
@@ -1463,16 +1463,8 @@ static void test_write_file( HANDLE file, int report_id, ULONG report_len )
         ret = WriteFile( file, report, report_len, &length, NULL );
     }
 
-    if (report_id)
-    {
-        ok( ret, "WriteFile failed, last error %lu\n", GetLastError() );
-        ok( length == 2, "WriteFile wrote %lu\n", length );
-    }
-    else
-    {
-        ok( ret, "WriteFile failed, last error %lu\n", GetLastError() );
-        ok( length == 3, "WriteFile wrote %lu\n", length );
-    }
+    ok( ret, "WriteFile failed, last error %lu\n", GetLastError() );
+    ok( length == report_len, "WriteFile wrote %lu\n", length );
 
     set_hid_expect( file, NULL, 0 );
 }
diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
index 5b7a675db7f..5624fd08e24 100644
--- a/dlls/hidclass.sys/device.c
+++ b/dlls/hidclass.sys/device.c
@@ -416,7 +416,6 @@ static const WCHAR *find_device_string( const WCHAR *device_id, ULONG index )
 struct completion_params
 {
     HID_XFER_PACKET packet;
-    ULONG padding;
     IRP *irp;
 };
 
@@ -428,7 +427,6 @@ static NTSTATUS CALLBACK xfer_completion( DEVICE_OBJECT *device, IRP *irp, void
     TRACE( "device %p, irp %p, context %p\n", device, irp, context );
 
     orig_irp->IoStatus = irp->IoStatus;
-    orig_irp->IoStatus.Information -= params->padding;
     IoCompleteRequest( orig_irp, IO_NO_INCREMENT );
 
     free( params );
@@ -498,8 +496,6 @@ static NTSTATUS hid_device_xfer_report( struct phys_device *pdo, ULONG code, IRP
                                              sizeof(params->packet), TRUE, NULL, NULL );
         break;
     case IOCTL_HID_WRITE_REPORT:
-        params->padding = 1 - offset;
-        /* fallthrough */
     case IOCTL_HID_SET_FEATURE:
     case IOCTL_HID_SET_OUTPUT_REPORT:
         params->packet.reportBufferLen = report_len - offset;
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/11204

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/dinput/tests/hid.c:
> .report_id = report_id,
>          .report_len = report_len - (report_id ? 0 : 1),
>          .report_buf = {report_id ? report_id : 0xcd,0xcd,0xcd,0xcd,0xcd},
> -        .ret_length = 3,
> +        .ret_length = report_len,

This test was meant to catch the fact that hidclass.sys ignores what the minidriver returns and uses the output report length regardless. I'm not sure it's very useful or that it matters much, but still, it seems that this change is simply hiding that fact. Using a larger value here, like 5, still ends up with the value fixed up to the right buffer size so it's not just a matter of subtraction cancelling the returned value.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11204#note_144186