| Message ID | 20221205210354.11846-2-andrew.smirnov@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Handling of non-numbered feature reports by hidraw | expand |
Hi On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > Report ID of zero is a special case handling ID-less reports and in > that case we should omit report ID from the payload being sent to the > backend. > > Without this change UHID_DEV_NUMBERED_{FEATURE,OUTPUT}_REPORTS doesn't > represent a semantical difference. > > Cc: David Rheinsberg <david.rheinsberg@gmail.com> > Cc: Jiri Kosina <jikos@kernel.org> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: linux-input@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-usb@vger.kernel.org > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > drivers/hid/uhid.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c > index 2a918aeb0af1..7551120215e8 100644 > --- a/drivers/hid/uhid.c > +++ b/drivers/hid/uhid.c > @@ -273,11 +273,11 @@ static int uhid_hid_get_report(struct hid_device *hid, unsigned char rnum, > } > > static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum, > - const u8 *buf, size_t count, u8 rtype) > + u8 *buf, size_t count, u8 rtype) > { > struct uhid_device *uhid = hid->driver_data; > struct uhid_event *ev; > - int ret; > + int ret, skipped_report_id = 0; > > if (!READ_ONCE(uhid->running) || count > UHID_DATA_MAX) > return -EIO; > @@ -286,6 +286,15 @@ static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum, > if (!ev) > return -ENOMEM; > > + /* Byte 0 is the report number. Report data starts at byte 1.*/ > + buf[0] = rnum; > + if (buf[0] == 0x0) { > + /* Don't send the Report ID */ > + buf++; > + count--; > + skipped_report_id = 1; > + } > + In HID core, the buffer is filled by a call to hid_output_report() in __hid_request(). And hid_output_report() only writes the ID if it is non-zero. So your patch looks like it is duplicating this logic? In which scenario is the report-ID not skipped exactly? Regardless, if you want to mess with the buffer, you should do that after the memcpy(). I don't see why we should mess with the buffer from HID core, when we have our own, anyway. David
) On Mon, Dec 12, 2022 at 7:23 AM David Rheinsberg <david.rheinsberg@gmail.com> wrote: > > Hi > > On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > > > Report ID of zero is a special case handling ID-less reports and in > > that case we should omit report ID from the payload being sent to the > > backend. > > > > Without this change UHID_DEV_NUMBERED_{FEATURE,OUTPUT}_REPORTS doesn't > > represent a semantical difference. > > > > Cc: David Rheinsberg <david.rheinsberg@gmail.com> > > Cc: Jiri Kosina <jikos@kernel.org> > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > Cc: linux-input@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-usb@vger.kernel.org > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > --- > > drivers/hid/uhid.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c > > index 2a918aeb0af1..7551120215e8 100644 > > --- a/drivers/hid/uhid.c > > +++ b/drivers/hid/uhid.c > > @@ -273,11 +273,11 @@ static int uhid_hid_get_report(struct hid_device *hid, unsigned char rnum, > > } > > > > static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum, > > - const u8 *buf, size_t count, u8 rtype) > > + u8 *buf, size_t count, u8 rtype) > > { > > struct uhid_device *uhid = hid->driver_data; > > struct uhid_event *ev; > > - int ret; > > + int ret, skipped_report_id = 0; > > > > if (!READ_ONCE(uhid->running) || count > UHID_DATA_MAX) > > return -EIO; > > @@ -286,6 +286,15 @@ static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum, > > if (!ev) > > return -ENOMEM; > > > > + /* Byte 0 is the report number. Report data starts at byte 1.*/ > > + buf[0] = rnum; > > + if (buf[0] == 0x0) { > > + /* Don't send the Report ID */ > > + buf++; > > + count--; > > + skipped_report_id = 1; > > + } > > + > > In HID core, the buffer is filled by a call to hid_output_report() in > __hid_request(). And hid_output_report() only writes the ID if it is > non-zero. So your patch looks like it is duplicating this logic? It would be in this scenario. But then I think it also means that USBHID will incorrectly strip an extra byte of the payload if it's zero for reports that don't have a report id, right? So maybe the fix for this is to get rid of payload adjustment in set/send paths in USBHID and move the adjustment to hidraw? > In which scenario is the report-ID not skipped exactly? The call chain in my use case is as follows: hidraw_ioctl(HIDIOCSFEATURE) -> hid_hw_raw_request() -> uhid_hid_raw_request() -> uhid_hid_set_report() > > Regardless, if you want to mess with the buffer, you should do that > after the memcpy(). I don't see why we should mess with the buffer > from HID core, when we have our own, anyway. > I was just mimicking code from USBHID, to make it clear it served the same purpose, that buf[0] = rnum; isn't strictly necessary and could be dropped.
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 2a918aeb0af1..7551120215e8 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -273,11 +273,11 @@ static int uhid_hid_get_report(struct hid_device *hid, unsigned char rnum, } static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum, - const u8 *buf, size_t count, u8 rtype) + u8 *buf, size_t count, u8 rtype) { struct uhid_device *uhid = hid->driver_data; struct uhid_event *ev; - int ret; + int ret, skipped_report_id = 0; if (!READ_ONCE(uhid->running) || count > UHID_DATA_MAX) return -EIO; @@ -286,6 +286,15 @@ static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum, if (!ev) return -ENOMEM; + /* Byte 0 is the report number. Report data starts at byte 1.*/ + buf[0] = rnum; + if (buf[0] == 0x0) { + /* Don't send the Report ID */ + buf++; + count--; + skipped_report_id = 1; + } + ev->type = UHID_SET_REPORT; ev->u.set_report.rnum = rnum; ev->u.set_report.rtype = rtype; @@ -306,7 +315,7 @@ static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum, if (uhid->report_buf.u.set_report_reply.err) ret = -EIO; else - ret = count; + ret = count + skipped_report_id; unlock: mutex_unlock(&uhid->report_lock);
Report ID of zero is a special case handling ID-less reports and in that case we should omit report ID from the payload being sent to the backend. Without this change UHID_DEV_NUMBERED_{FEATURE,OUTPUT}_REPORTS doesn't represent a semantical difference. Cc: David Rheinsberg <david.rheinsberg@gmail.com> Cc: Jiri Kosina <jikos@kernel.org> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-usb@vger.kernel.org Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- drivers/hid/uhid.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)