[RFC PATCH 0/2] Handling of non-numbered feature reports by hidraw

[RFC PATCH 0/2] Handling of non-numbered feature reports by hidraw

[Andrey Smirnov]

Hi everyone,

I'm working on a firmware of a device that exposes a HID interface via
USB and/or BLE and uses, among other things, non-numbered feature
reports. Included in this series are two paches I had to create in
order for hidraw devices created for aforementioned subsystems to
behave in the same way when exerciesd by the same test tool.

I don't know if the patches are acceptable as-is WRT to not breaking
existing userspace, hence the RFC tag.

Andrey Smirnov (2):
  HID: uhid: Don't send the report ID if it's zero
  HID: usbhid: Don't include report ID zero into returned data

 drivers/hid/uhid.c            | 15 ++++++++++++---
 drivers/hid/usbhid/hid-core.c | 14 --------------
 2 files changed, 12 insertions(+), 17 deletions(-)

--
2.34.1

[RFC PATCH 1/2] HID: uhid: Don't send the report ID if it's zero

[Andrey Smirnov]

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;
+	}
+
 	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);
-- 
2.34.1

[RFC PATCH 2/2] HID: usbhid: Don't include report ID zero into returned data

[Andrey Smirnov]

Report ID of zero is a special case for ID-less reports, which by
definition do not have report ID as a part of their payload. Not
returning an extra zero also matches hidraw documentation,
specifically:

      For devices which do not use numbered reports, set the first
      byte to 0.  The returned report buffer will contain the report
      number in the first byte, followed by the report data read from
      the device.  For devices which do not use numbered reports, the
      report data will begin at the first byte of the returned buffer.

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/usbhid/hid-core.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index be4c731aaa65..575f09003602 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -874,18 +874,8 @@ static int usbhid_get_raw_report(struct hid_device *hid,
 	struct usb_device *dev = hid_to_usb_dev(hid);
 	struct usb_interface *intf = usbhid->intf;
 	struct usb_host_interface *interface = intf->cur_altsetting;
-	int skipped_report_id = 0;
 	int ret;
 
-	/* Byte 0 is the report number. Report data starts at byte 1.*/
-	buf[0] = report_number;
-	if (report_number == 0x0) {
-		/* Offset the return buffer by 1, so that the report ID
-		   will remain in byte 0. */
-		buf++;
-		count--;
-		skipped_report_id = 1;
-	}
 	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
 		HID_REQ_GET_REPORT,
 		USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
@@ -893,10 +883,6 @@ static int usbhid_get_raw_report(struct hid_device *hid,
 		interface->desc.bInterfaceNumber, buf, count,
 		USB_CTRL_SET_TIMEOUT);
 
-	/* count also the report id */
-	if (ret > 0 && skipped_report_id)
-		ret++;
-
 	return ret;
 }
 
-- 
2.34.1

Re: [RFC PATCH 0/2] Handling of non-numbered feature reports by hidraw

[David Rheinsberg]

Hi

On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> I'm working on a firmware of a device that exposes a HID interface via
> USB and/or BLE and uses, among other things, non-numbered feature
> reports. Included in this series are two paches I had to create in
> order for hidraw devices created for aforementioned subsystems to
> behave in the same way when exerciesd by the same test tool.
>
> I don't know if the patches are acceptable as-is WRT to not breaking
> existing userspace, hence the RFC tag.

Can you elaborate why you remove the special handling from USBHID but
add it to UHID? They both operate logically on the same level, so
shouldn't we simply adjust uhid to include the report-id in buf[0]?

Also, you override buf[0] in UHID, so I wonder what UHID currently
returns there?

IOW, can you elaborate a bit what the current behavior of each of the
involved modules is, and what behavior you would expect? This would
allow to better understand what you are trying to achieve. The more
context you can give, the easier it is to understand what happens
there.

Thanks!
David

Re: [RFC PATCH 0/2] Handling of non-numbered feature reports by hidraw

[Andrey Smirnov]

On Thu, Dec 8, 2022 at 7:46 AM David Rheinsberg
<david.rheinsberg@gmail.com> wrote:
>
> Hi
>
> On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > I'm working on a firmware of a device that exposes a HID interface via
> > USB and/or BLE and uses, among other things, non-numbered feature
> > reports. Included in this series are two paches I had to create in
> > order for hidraw devices created for aforementioned subsystems to
> > behave in the same way when exerciesd by the same test tool.
> >
> > I don't know if the patches are acceptable as-is WRT to not breaking
> > existing userspace, hence the RFC tag.
>
> Can you elaborate why you remove the special handling from USBHID but
> add it to UHID? They both operate logically on the same level, so
> shouldn't we simply adjust uhid to include the report-id in buf[0]?
>
> Also, you override buf[0] in UHID, so I wonder what UHID currently
> returns there?
>
> IOW, can you elaborate a bit what the current behavior of each of the
> involved modules is, and what behavior you would expect? This would
> allow to better understand what you are trying to achieve. The more
> context you can give, the easier it is to understand what happens
> there.
>

Sorry it's not very clear, so the difference between the cases is that
in the case of UHID the report ID ends up being included as a part of
"SET_FEATURE", so BlueZ checks UHID_DEV_NUMBERED_FEATURE_REPORTS,
which is not set (correctly) and tries to send the whole payload. This
ends up as a maxlen + 1 (extra byte) write to a property that is
maxlen long, which gets rejected by device's BLE stack.

In the case of USBHID the problem happens in "GET_FEATURE" path. When
userspace reads the expected data back it gets an extra 0 prepended to
the payload, so all of the actual payload has an offset of 1. This
doesn't happen with UHID, which I think is the correct behavior here.

Hopefully that explains the difference, let me know if something is unclear

Re: [RFC PATCH 1/2] HID: uhid: Don't send the report ID if it's zero

[David Rheinsberg]

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

Re: [RFC PATCH 0/2] Handling of non-numbered feature reports by hidraw

[David Rheinsberg]

Hi

On Thu, 8 Dec 2022 at 21:59, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> On Thu, Dec 8, 2022 at 7:46 AM David Rheinsberg
> <david.rheinsberg@gmail.com> wrote:
> >
> > Hi
> >
> > On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > I'm working on a firmware of a device that exposes a HID interface via
> > > USB and/or BLE and uses, among other things, non-numbered feature
> > > reports. Included in this series are two paches I had to create in
> > > order for hidraw devices created for aforementioned subsystems to
> > > behave in the same way when exerciesd by the same test tool.
> > >
> > > I don't know if the patches are acceptable as-is WRT to not breaking
> > > existing userspace, hence the RFC tag.
> >
> > Can you elaborate why you remove the special handling from USBHID but
> > add it to UHID? They both operate logically on the same level, so
> > shouldn't we simply adjust uhid to include the report-id in buf[0]?
> >
> > Also, you override buf[0] in UHID, so I wonder what UHID currently
> > returns there?
> >
> > IOW, can you elaborate a bit what the current behavior of each of the
> > involved modules is, and what behavior you would expect? This would
> > allow to better understand what you are trying to achieve. The more
> > context you can give, the easier it is to understand what happens
> > there.
> >
>
> Sorry it's not very clear, so the difference between the cases is that
> in the case of UHID the report ID ends up being included as a part of
> "SET_FEATURE", so BlueZ checks UHID_DEV_NUMBERED_FEATURE_REPORTS,
> which is not set (correctly) and tries to send the whole payload. This
> ends up as a maxlen + 1 (extra byte) write to a property that is
> maxlen long, which gets rejected by device's BLE stack.
>
> In the case of USBHID the problem happens in "GET_FEATURE" path. When
> userspace reads the expected data back it gets an extra 0 prepended to
> the payload, so all of the actual payload has an offset of 1. This
> doesn't happen with UHID, which I think is the correct behavior here.
>
> Hopefully that explains the difference, let me know if something is unclear

Yes, thanks, I completely missed that. Lets continue discussion on the patches.

Thanks!
David

Re: [RFC PATCH 1/2] HID: uhid: Don't send the report ID if it's zero

[Andrey Smirnov]

)

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.