[PATCH v2 0/2] MR1448: hid: parser and HidP_SetUsageValue fix
This fixes two things: * HID report descriptor parser enum for local items. Close inspection of page 40 of [Device Class Definition for HID 1.11](https://www.usb.org/document-library/device-class-definition-hid-111) reveals a jump between Designator Maximum (0101) and String Index (0111). This caused a controller of mine to not get recognized. * HidP_SetUsageValue for items that define multiple controls (with Report Count > 1). Attempting to set one value would result in HIDP_STATUS_BUFFER_TOO_SMALL. -- v2: hid: Fix HidP_SetUsageValue for usage ranges https://gitlab.winehq.org/wine/wine/-/merge_requests/1448
From: Matthew Tran <0e4ef622(a)gmail.com> --- dlls/hidparse.sys/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/hidparse.sys/main.c b/dlls/hidparse.sys/main.c index 9c753c0f365..7058093e03c 100644 --- a/dlls/hidparse.sys/main.c +++ b/dlls/hidparse.sys/main.c @@ -93,7 +93,7 @@ enum TAG_LOCAL_DESIGNATOR_INDEX, TAG_LOCAL_DESIGNATOR_MINIMUM, TAG_LOCAL_DESIGNATOR_MAXIMUM, - TAG_LOCAL_STRING_INDEX, + TAG_LOCAL_STRING_INDEX = 0x7, TAG_LOCAL_STRING_MINIMUM, TAG_LOCAL_STRING_MAXIMUM, TAG_LOCAL_DELIMITER -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1448
From: Matthew Tran <0e4ef622(a)gmail.com> The original set_usage_value was renamed to set_usage_value_array. --- dlls/hid/hidp.c | 46 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/dlls/hid/hidp.c b/dlls/hid/hidp.c index a1c5805c19f..2fedb3f9586 100644 --- a/dlls/hid/hidp.c +++ b/dlls/hid/hidp.c @@ -495,16 +495,31 @@ NTSTATUS WINAPI HidP_SetScaledUsageValue( HIDP_REPORT_TYPE report_type, USAGE us return enum_value_caps( preparsed, report_type, report_len, &filter, set_scaled_usage_value, ¶ms, &count ); } +struct set_usage_value_params +{ + ULONG value; + void *report_buf; + USAGE usage; +}; + static NTSTATUS set_usage_value( const struct hid_value_caps *caps, void *user ) { - struct usage_value_params *params = user; - ULONG bit_count = caps->bit_size * caps->report_count; - unsigned char *report_buf; + struct set_usage_value_params *params = user; + unsigned char *report_buf, start_bit, usage_index; - if ((bit_count + 7) / 8 > params->value_len) return HIDP_STATUS_BUFFER_TOO_SMALL; + if (caps->flags & HID_VALUE_CAPS_IS_RANGE) + { + usage_index = params->usage - caps->usage_min; + report_buf = (unsigned char *)params->report_buf + caps->start_byte + usage_index * caps->bit_size / 8; + start_bit = caps->start_bit + (usage_index * caps->bit_size) % 8; + } + else + { + report_buf = (unsigned char *)params->report_buf + caps->start_byte; + start_bit = caps->start_bit; + } - report_buf = (unsigned char *)params->report_buf + caps->start_byte; - copy_bits( report_buf, params->value_buf, bit_count, caps->start_bit ); + copy_bits( report_buf, (unsigned char*) ¶ms->value, caps->bit_size, start_bit ); return HIDP_STATUS_NULL; } @@ -512,7 +527,7 @@ static NTSTATUS set_usage_value( const struct hid_value_caps *caps, void *user ) NTSTATUS WINAPI HidP_SetUsageValue( HIDP_REPORT_TYPE report_type, USAGE usage_page, USHORT collection, USAGE usage, ULONG value, PHIDP_PREPARSED_DATA preparsed_data, char *report_buf, ULONG report_len ) { - struct usage_value_params params = {.value_buf = &value, .value_len = sizeof(value), .report_buf = report_buf}; + struct set_usage_value_params params = {.value = value, .report_buf = report_buf, .usage = usage}; struct hid_preparsed_data *preparsed = (struct hid_preparsed_data *)preparsed_data; struct caps_filter filter = {.values = TRUE, .usage_page = usage_page, .collection = collection, .usage = usage}; USHORT count = 1; @@ -526,6 +541,21 @@ NTSTATUS WINAPI HidP_SetUsageValue( HIDP_REPORT_TYPE report_type, USAGE usage_pa return enum_value_caps( preparsed, report_type, report_len, &filter, set_usage_value, ¶ms, &count ); } +static NTSTATUS set_usage_value_array( const struct hid_value_caps *caps, void *user ) +{ + struct usage_value_params *params = user; + ULONG bit_count = caps->bit_size * caps->report_count; + unsigned char *report_buf; + + if (caps->flags & HID_VALUE_CAPS_IS_RANGE) return HIDP_STATUS_NOT_VALUE_ARRAY; + if ((bit_count + 7) / 8 > params->value_len) return HIDP_STATUS_BUFFER_TOO_SMALL; + + report_buf = (unsigned char *)params->report_buf + caps->start_byte; + copy_bits( report_buf, params->value_buf, bit_count, caps->start_bit ); + + return HIDP_STATUS_NULL; +} + NTSTATUS WINAPI HidP_SetUsageValueArray( HIDP_REPORT_TYPE report_type, USAGE usage_page, USHORT collection, USAGE usage, char *value_buf, USHORT value_len, PHIDP_PREPARSED_DATA preparsed_data, char *report_buf, ULONG report_len ) @@ -542,7 +572,7 @@ NTSTATUS WINAPI HidP_SetUsageValueArray( HIDP_REPORT_TYPE report_type, USAGE usa if (!report_len) return HIDP_STATUS_INVALID_REPORT_LENGTH; filter.report_id = report_buf[0]; - return enum_value_caps( preparsed, report_type, report_len, &filter, set_usage_value, ¶ms, &count ); + return enum_value_caps( preparsed, report_type, report_len, &filter, set_usage_value_array, ¶ms, &count ); } struct set_usage_params -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1448
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=126495 Your paranoid android. === debian11 (32 bit report) === d3d8: stateblock: Timeout visual: Timeout d3d9: d3d9ex: Timeout device: Timeout stateblock: Timeout visual: Timeout d3dcompiler_43: asm: Timeout blob: Timeout hlsl_d3d11: Timeout hlsl_d3d9: Timeout reflection: Timeout d3dcompiler_46: asm: Timeout blob: Timeout hlsl_d3d11: Timeout hlsl_d3d9: Timeout reflection: Timeout d3dcompiler_47: asm: Timeout blob: Timeout hlsl_d3d11: Timeout hlsl_d3d9: Timeout reflection: Timeout d3drm: d3drm: Timeout Report validation errors: advapi32:security has no test summary line (early exit of the main process?) advapi32:security has unaccounted for todo messages vector: Timeout === debian11 (build log) === WineRunWineTest.pl:error: The task timed out
Thanks for spotting this! Do you think you could add a few tests to check the HidP_SetUsageValue change? Like in `test_hidp` (the descriptor being in `test_hid_driver`) in `dlls/dinput/tests/hid.c`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1448#note_16548
participants (4)
-
Marvin -
Matthew Tran -
Matthew Tran (＠0e4ef622)
-
Rémi Bernon