HID: vivaldi: fix handling devices not using numbered reports
PATCH 1/1 HID: vivaldi: fix handling devices not using numbered reports
Unfortunately details of USB HID transport bled into HID core and handling of numbered/unnumbered reports is quite a mess, with hid_report_len() calculating the length according to USB rules, and hid_hw_raw_request() adding report ID to the buffer for both numbered and unnumbered reports.
Untangling it all requres a lot of changes in HID, so for now let's handle this in the driver.
---
CrOS folks, please help testing this as I do not have the affected hardware.
Thanks!
drivers/hid/hid-vivaldi.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)
drivers/hid/hid-vivaldi.c +28 -6
@@ -71,10 +71,11 @@ static void vivaldi_feature_mapping(struct hid_device *hdev, struct hid_usage *usage) {struct vivaldi_data *drvdata =hid_get_drvdata (hdev); +struct hid_report *report = field->report;int fn_key;int ret;u32 report_len; -u8 *buf; +u8 *report_data, *buf;if (field->logical !=HID_USAGE_FN_ROW_PHYSMAP || (usage->hid &HID_USAGE_PAGE ) !=HID_UP_ORDINAL )
@@ -86,12 +87,24 @@ static void vivaldi_feature_mapping(struct hid_device *hdev,if (fn_key > drvdata->max_function_row_key) drvdata->max_function_row_key = fn_key; - buf =hid_alloc_report_buf (field->report,GFP_KERNEL ); -if (!buf) + report_data = buf =hid_alloc_report_buf (report,GFP_KERNEL ); +if (!report_data)return ; - report_len =hid_report_len (field->report); - ret =hid_hw_raw_request (hdev, field->report->id, buf, + report_len =hid_report_len (report); +if (!report->id) { +/* +* hid_hw_raw_request() will stuff report ID (which will be 0) +* into the first byte of the buffer even for unnumbered +* reports, so we need to account for this to avoid getting +* -EOVERFLOW in return. +* Note that hid_alloc_report_buf() adds 7 bytes to the size +* so we can safely say that we have space for an extra byte. +*/ + report_len++; + } + + ret =hid_hw_raw_request (hdev, field->report->id, report_data, report_len,HID_FEATURE_REPORT ,HID_REQ_GET_REPORT );if (ret <0 ) {
@@ -100,7 +113,16 @@ static void vivaldi_feature_mapping(struct hid_device *hdev, goto out; } - ret =hid_report_raw_event (hdev,HID_FEATURE_REPORT , buf, +if (!report->id) { +/* +* Undo the damage from hid_hw_raw_request() for unnumbered +* reports. +*/ + report_data++; + report_len--; + } + + ret =hid_report_raw_event (hdev,HID_FEATURE_REPORT , report_data, report_len,0 );if (ret) { dev_warn(&hdev->dev, "failed to report feature %d\n",
Signature · 5 lines
2.34.1.575.g55b058a8bb-goog -- Dmitry
Quoting Dmitry Torokhov (2022-01-07 12:09:36)
Quoted 6 lines
Unfortunately details of USB HID transport bled into HID core and handling of numbered/unnumbered reports is quite a mess, with
hid_report_len()calculating the length according to USB rules, andhid_hw_raw_request()adding report ID to the buffer for both numbered and unnumbered reports.
Quoted 6 lines
Untangling it all requres a lot of changes in HID, so for now let's handle this in the driver.
Fixes 14c9c014babe ("HID: add vivaldi HID driver")Signed-off-by Dmitry Torokhov <[email protected]>---
This silences a warning I see printed from this driver
hid-vivaldi 0003:18D1:504C.0002: failed to fetch feature 0
and then I see the 'function_row_physmap' attribute in sysfs is non-zero now. Thanks!
Quoted 54 lines · Dmitry Torokhov
Unfortunately details of USB HID transport bled into HID core and handling of numbered/unnumbered reports is quite a mess, with
hid_report_len()calculating the length according to USB rules, andhid_hw_raw_request()adding report ID to the buffer for both numbered and unnumbered reports.Untangling it all requres a lot of changes in HID, so for now let's handle this in the driver.
Fixes 14c9c014babe ("HID: add vivaldi HID driver")Signed-off-by Dmitry Torokhov <[email protected]>---
CrOS folks, please help testing this as I do not have the affected hardware.
Thanks!
drivers/hid/hid-vivaldi.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)drivers/hid/hid-vivaldi.c +8
-5@@ -71,10 +71,11 @@ static void vivaldi_feature_mapping(struct hid_device *hdev, struct hid_usage *usage) {struct vivaldi_data *drvdata =hid_get_drvdata (hdev); +struct hid_report *report = field->report;int fn_key;int ret;u32 report_len; -u8 *buf; +u8 *report_data, *buf;if (field->logical !=HID_USAGE_FN_ROW_PHYSMAP || (usage->hid &HID_USAGE_PAGE ) !=HID_UP_ORDINAL )@@ -86,12 +87,24 @@ static void vivaldi_feature_mapping(struct hid_device *hdev,if (fn_key > drvdata->max_function_row_key) drvdata->max_function_row_key = fn_key; - buf =hid_alloc_report_buf (field->report,GFP_KERNEL ); -if (!buf) + report_data = buf =hid_alloc_report_buf (report,GFP_KERNEL ); +if (!report_data)return ; - report_len =hid_report_len (field->report); - ret = hid_hw_raw_request(hdev, field->report->id, buf, + report_len =hid_report_len (report); +if (!report->id) { +/* +* hid_hw_raw_request() will stuff report ID (which will be 0)
drivers/hid/hid-vivaldi.c static void vivaldi_feature_mapping(struct hid_device *hdev, · line 107
Quoted 10 lines
drivers/hid/hid-vivaldi.c +10
-0+* into the first byte of the buffer even for unnumbered +* reports, so we need to account for this to avoid getting +* -EOVERFLOW in return. +* Note that hid_alloc_report_buf() adds 7 bytes to the size +* so we can safely say that we have space for an extra byte. +*/ + report_len++; + } + + ret = hid_hw_raw_request(hdev, field->report->id, report_data,
This can be changed to "report->id", sorry I missed it.
Quoted 11 lines
drivers/hid/hid-vivaldi.c +3
-1report_len,HID_FEATURE_REPORT , HID_REQ_GET_REPORT);if (ret <0 ) {@@ -100,7 +113,16 @@ static void vivaldi_feature_mapping(struct hid_device *hdev, goto out; } - ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, +if (!report->id) { +/* +* Undo the damage from hid_hw_raw_request() for unnumbered
drivers/hid/hid-vivaldi.c static void vivaldi_feature_mapping(struct hid_device *hdev, · line 128
Quoted 10 lines
drivers/hid/hid-vivaldi.c +7
-0+* reports. +*/ + report_data++; + report_len--; + } + + ret =hid_report_raw_event (hdev,HID_FEATURE_REPORT , report_data, report_len,0 );if (ret) { dev_warn(&hdev->dev, "failed to report feature %d\n",
Thanks.
Signature · 1 line
Dmitry
Jiri Kosina
maintainerQuoted 5 lines · Dmitry Torokhov
Unfortunately details of USB HID transport bled into HID core and handling of numbered/unnumbered reports is quite a mess, with
hid_report_len()calculating the length according to USB rules, andhid_hw_raw_request()adding report ID to the buffer for both numbered and unnumbered reports.
Yeah, I agree that's unfortunate, and untangling this has been on my TODO for quite some time :/
Quoted 45 lines
Fixes 14c9c014babe ("HID: add vivaldi HID driver")Signed-off-by Dmitry Torokhov <[email protected]>---
CrOS folks, please help testing this as I do not have the affected hardware.
Thanks!
drivers/hid/hid-vivaldi.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)drivers/hid/hid-vivaldi.c +8
-5@@ -71,10 +71,11 @@ static void vivaldi_feature_mapping(struct hid_device *hdev, struct hid_usage *usage) {struct vivaldi_data *drvdata =hid_get_drvdata (hdev); +struct hid_report *report = field->report;int fn_key;int ret;u32 report_len; -u8 *buf; +u8 *report_data, *buf;if (field->logical !=HID_USAGE_FN_ROW_PHYSMAP || (usage->hid &HID_USAGE_PAGE ) !=HID_UP_ORDINAL )@@ -86,12 +87,24 @@ static void vivaldi_feature_mapping(struct hid_device *hdev,if (fn_key > drvdata->max_function_row_key) drvdata->max_function_row_key = fn_key; - buf =hid_alloc_report_buf (field->report,GFP_KERNEL ); -if (!buf) + report_data = buf =hid_alloc_report_buf (report,GFP_KERNEL ); +if (!report_data)return ; - report_len =hid_report_len (field->report); - ret = hid_hw_raw_request(hdev, field->report->id, buf, + report_len =hid_report_len (report); +if (!report->id) { +/* +* hid_hw_raw_request() will stuff report ID (which will be 0)
drivers/hid/hid-vivaldi.c static void vivaldi_feature_mapping(struct hid_device *hdev, · line 107
Quoted 10 lines
drivers/hid/hid-vivaldi.c +10
-0+* into the first byte of the buffer even for unnumbered +* reports, so we need to account for this to avoid getting +* -EOVERFLOW in return. +* Note that hid_alloc_report_buf() adds 7 bytes to the size +* so we can safely say that we have space for an extra byte. +*/ + report_len++; + } + + ret = hid_hw_raw_request(hdev, field->report->id, report_data,
I've changed this to report->id and applied, thanks.
Signature · 2 lines
Jiri Kosina SUSE Labs