| Message ID | YdieAFj0ppmAtQxS@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: vivaldi: fix handling devices not using numbered reports | expand |
Quoting Dmitry Torokhov (2022-01-07 12:09:36) > 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. > > Fixes: 14c9c014babe ("HID: add vivaldi HID driver") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- 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! Tested-by: Stephen Boyd <swboyd@chromium.org> # CoachZ
On Fri, Jan 07, 2022 at 12:09:36PM -0800, Dmitry Torokhov wrote: > 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. > > Fixes: 14c9c014babe ("HID: add vivaldi HID driver") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > > 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(-) > > diff --git a/drivers/hid/hid-vivaldi.c b/drivers/hid/hid-vivaldi.c > index cd7ada48b1d9..1804de1ef9b8 100644 > --- a/drivers/hid/hid-vivaldi.c > +++ b/drivers/hid/hid-vivaldi.c > @@ -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, This can be changed to "report->id", sorry I missed it. > 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", Thanks.
On Fri, 7 Jan 2022, Dmitry Torokhov wrote: > 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. Yeah, I agree that's unfortunate, and untangling this has been on my TODO for quite some time :/ > Fixes: 14c9c014babe ("HID: add vivaldi HID driver") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > > 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(-) > > diff --git a/drivers/hid/hid-vivaldi.c b/drivers/hid/hid-vivaldi.c > index cd7ada48b1d9..1804de1ef9b8 100644 > --- a/drivers/hid/hid-vivaldi.c > +++ b/drivers/hid/hid-vivaldi.c > @@ -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, I've changed this to report->id and applied, thanks.
diff --git a/drivers/hid/hid-vivaldi.c b/drivers/hid/hid-vivaldi.c index cd7ada48b1d9..1804de1ef9b8 100644 --- a/drivers/hid/hid-vivaldi.c +++ b/drivers/hid/hid-vivaldi.c @@ -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",
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. Fixes: 14c9c014babe ("HID: add vivaldi HID driver") Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- 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(-)