* [PATCH] Fix HOG profile incorrectly stripping off read report bytes.
@ 2020-11-20 1:42 Dean Camera
2020-11-20 17:49 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 4+ messages in thread
From: Dean Camera @ 2020-11-20 1:42 UTC (permalink / raw)
To: linux-bluetooth
If the HID subsystem requests a HID report to be read from the
device, we currently incorrectly strip off the first byte of the
response, if the device has report IDs set in the HID report
descriptor.
This is incorrect; unlike USB HID, the report ID is *not* included
in the HOG profile's HID reports, and instead exists out of band
in a descriptor on the report's bluetooth characteristic in the
device.
In this patch, we remove the erroneous stripping of the first
byte of the report, and (if report IDs are enabled) prepend the
report ID to the front of the result. This makes the HID report
returned indentical in format to that of a USB HID report, so
that the upper HID drivers can consume HOG device reports in the
same way as USB.
---
profiles/input/hog-lib.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
index 78018aad3..49d459e21 100644
--- a/profiles/input/hog-lib.c
+++ b/profiles/input/hog-lib.c
@@ -779,7 +779,8 @@ fail:
static void get_report_cb(guint8 status, const guint8 *pdu, guint16 len,
gpointer user_data)
{
- struct bt_hog *hog = user_data;
+ struct report *report = user_data;
+ struct bt_hog *hog = report->hog;
struct uhid_event rsp;
int err;
@@ -808,13 +809,16 @@ static void get_report_cb(guint8 status, const
guint8 *pdu, guint16 len,
--len;
++pdu;
+
if (hog->has_report_id && len > 0) {
- --len;
- ++pdu;
+ rsp.u.get_report_reply.size = len + 1;
+ rsp.u.get_report_reply.data[0] = report->id;
+ memcpy(&rsp.u.get_report_reply.data[1], pdu, len);
+ }
+ else {
+ rsp.u.get_report_reply.size = len;
+ memcpy(rsp.u.get_report_reply.data, pdu, len);
}
-
- rsp.u.get_report_reply.size = len;
- memcpy(rsp.u.get_report_reply.data, pdu, len);
exit:
rsp.u.get_report_reply.err = status;
@@ -846,7 +850,7 @@ static void get_report(struct uhid_event *ev, void
*user_data)
hog->getrep_att = gatt_read_char(hog->attrib,
report->value_handle,
- get_report_cb, hog);
+ get_report_cb, report);
if (!hog->getrep_att) {
err = ENOMEM;
goto fail;
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix HOG profile incorrectly stripping off read report bytes.
2020-11-20 1:42 [PATCH] Fix HOG profile incorrectly stripping off read report bytes Dean Camera
@ 2020-11-20 17:49 ` Luiz Augusto von Dentz
[not found] ` <CAHdu5-74egP-m3pUPKEb_TWHRm21DMnbqE2K119wGoO9TgXioQ@mail.gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2020-11-20 17:49 UTC (permalink / raw)
To: Dean Camera; +Cc: linux-bluetooth
Hi Dean,
On Thu, Nov 19, 2020 at 5:47 PM Dean Camera <dean@fourwalledcubicle.com> wrote:
>
> If the HID subsystem requests a HID report to be read from the
> device, we currently incorrectly strip off the first byte of the
> response, if the device has report IDs set in the HID report
> descriptor.
>
> This is incorrect; unlike USB HID, the report ID is *not* included
> in the HOG profile's HID reports, and instead exists out of band
> in a descriptor on the report's bluetooth characteristic in the
> device.
>
> In this patch, we remove the erroneous stripping of the first
> byte of the report, and (if report IDs are enabled) prepend the
> report ID to the front of the result. This makes the HID report
> returned indentical in format to that of a USB HID report, so
> that the upper HID drivers can consume HOG device reports in the
> same way as USB.
> ---
> profiles/input/hog-lib.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> index 78018aad3..49d459e21 100644
> --- a/profiles/input/hog-lib.c
> +++ b/profiles/input/hog-lib.c
> @@ -779,7 +779,8 @@ fail:
> static void get_report_cb(guint8 status, const guint8 *pdu, guint16 len,
> gpointer user_data)
> {
> - struct bt_hog *hog = user_data;
> + struct report *report = user_data;
> + struct bt_hog *hog = report->hog;
> struct uhid_event rsp;
> int err;
>
> @@ -808,13 +809,16 @@ static void get_report_cb(guint8 status, const
> guint8 *pdu, guint16 len,
>
> --len;
> ++pdu;
> +
> if (hog->has_report_id && len > 0) {
> - --len;
> - ++pdu;
> + rsp.u.get_report_reply.size = len + 1;
> + rsp.u.get_report_reply.data[0] = report->id;
> + memcpy(&rsp.u.get_report_reply.data[1], pdu, len);
> + }
> + else {
> + rsp.u.get_report_reply.size = len;
> + memcpy(rsp.u.get_report_reply.data, pdu, len);
> }
> -
> - rsp.u.get_report_reply.size = len;
> - memcpy(rsp.u.get_report_reply.data, pdu, len);
>
> exit:
> rsp.u.get_report_reply.err = status;
> @@ -846,7 +850,7 @@ static void get_report(struct uhid_event *ev, void
> *user_data)
>
> hog->getrep_att = gatt_read_char(hog->attrib,
> report->value_handle,
> - get_report_cb, hog);
> + get_report_cb, report);
> if (!hog->getrep_att) {
> err = ENOMEM;
> goto fail;
> --
> 2.29.2.windows.2
>
Applied, thanks.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix HOG profile incorrectly stripping off read report bytes.
[not found] ` <CAHdu5-74egP-m3pUPKEb_TWHRm21DMnbqE2K119wGoO9TgXioQ@mail.gmail.com>
@ 2020-11-20 22:00 ` Dean Camera
[not found] ` <CAHdu5-6MgMc9DQK=pAH0qHJYRy+aaJV98bG1ct+2tXyRv2yx-w@mail.gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Dean Camera @ 2020-11-20 22:00 UTC (permalink / raw)
To: mathieu.stephan, Luiz Augusto von Dentz; +Cc: linux-bluetooth
The existing behaviour for a HOG device with report IDs was broken, and
likely wouldn't have worked at all as the first byte of the response was
being discarded.
The missing prepended report ID is theoretically something that would
cause an observable difference to userspace, but as it's only added in
the multiple report case that was broken I don't think anyone will be
affected.
Single report HOG devices make up for the majority of consumer HID
devices, which is probably why this has gone unnoticed for so long, and
the behaviour of those is unaffected by my patch.
The only use case I can see that would be broken would be a userspace
app using HIDRAW to communicate with a multiple report HOG device, which
was already tolerant of missing the first byte if the report.
Cheers,
- Dean
On 21/11/2020 6:56 am, mathieu.stephan@gmail.com wrote:
> Hello All,
>
> Is there a way to communicate to users that particular change?
> I'm reacting as this is something our team heavily relies upon
> (https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91
> <https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91>)
> and I'm guessing we're far from the only ones :)
>
> Regards,
> Mathieu
>
> On Fri, Nov 20, 2020 at 6:52 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com <mailto:luiz.dentz@gmail.com>> wrote:
>
> Hi Dean,
>
> On Thu, Nov 19, 2020 at 5:47 PM Dean Camera
> <dean@fourwalledcubicle.com <mailto:dean@fourwalledcubicle.com>> wrote:
> >
> > If the HID subsystem requests a HID report to be read from the
> > device, we currently incorrectly strip off the first byte of the
> > response, if the device has report IDs set in the HID report
> > descriptor.
> >
> > This is incorrect; unlike USB HID, the report ID is *not* included
> > in the HOG profile's HID reports, and instead exists out of band
> > in a descriptor on the report's bluetooth characteristic in the
> > device.
> >
> > In this patch, we remove the erroneous stripping of the first
> > byte of the report, and (if report IDs are enabled) prepend the
> > report ID to the front of the result. This makes the HID report
> > returned indentical in format to that of a USB HID report, so
> > that the upper HID drivers can consume HOG device reports in the
> > same way as USB.
> > ---
> > profiles/input/hog-lib.c | 18 +++++++++++-------
> > 1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > index 78018aad3..49d459e21 100644
> > --- a/profiles/input/hog-lib.c
> > +++ b/profiles/input/hog-lib.c
> > @@ -779,7 +779,8 @@ fail:
> > static void get_report_cb(guint8 status, const guint8 *pdu,
> guint16 len,
> > gpointer
> user_data)
> > {
> > - struct bt_hog *hog = user_data;
> > + struct report *report = user_data;
> > + struct bt_hog *hog = report->hog;
> > struct uhid_event rsp;
> > int err;
> >
> > @@ -808,13 +809,16 @@ static void get_report_cb(guint8 status, const
> > guint8 *pdu, guint16 len,
> >
> > --len;
> > ++pdu;
> > +
> > if (hog->has_report_id && len > 0) {
> > - --len;
> > - ++pdu;
> > + rsp.u.get_report_reply.size = len + 1;
> > + rsp.u.get_report_reply.data[0] = report->id;
> > + memcpy(&rsp.u.get_report_reply.data[1], pdu, len);
> > + }
> > + else {
> > + rsp.u.get_report_reply.size = len;
> > + memcpy(rsp.u.get_report_reply.data, pdu, len);
> > }
> > -
> > - rsp.u.get_report_reply.size = len;
> > - memcpy(rsp.u.get_report_reply.data, pdu, len);
> >
> > exit:
> > rsp.u.get_report_reply.err = status;
> > @@ -846,7 +850,7 @@ static void get_report(struct uhid_event *ev,
> void
> > *user_data)
> >
> > hog->getrep_att = gatt_read_char(hog->attrib,
> > report->value_handle,
> > - get_report_cb, hog);
> > + get_report_cb,
> report);
> > if (!hog->getrep_att) {
> > err = ENOMEM;
> > goto fail;
> > --
> > 2.29.2.windows.2
> >
>
> Applied, thanks.
>
> --
> Luiz Augusto von Dentz
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix HOG profile incorrectly stripping off read report bytes.
[not found] ` <CAHdu5-6MgMc9DQK=pAH0qHJYRy+aaJV98bG1ct+2tXyRv2yx-w@mail.gmail.com>
@ 2020-11-20 22:22 ` Dean Camera
0 siblings, 0 replies; 4+ messages in thread
From: Dean Camera @ 2020-11-20 22:22 UTC (permalink / raw)
To: mathieu.stephan; +Cc: Luiz Augusto von Dentz, linux-bluetooth
Mathieu,
Quite possibly! I discovered this while trying to implement a user-space
application for a commercial product, which uses multiple reports quite
heavily, as it's a lot more complex than the usual keyboard or mouse.
I hope your project works as expected once this patch is applied, and
that it leads to more companies developing more complex HID products now
that it should work correctly. You shouldn't need any special "is
Bluetooth" handling with this fix, as the reports should appear
identical to a regular USB HID device (as it should be).
The HID report byte has caused untold amounts of bugs and confusion
after it was decided to omit it for single report devices in the
original USB HID spec to save space on the wire, which was a neccesary
evil at the time to make the basic reports fit into a Low Speed USB
device packet, but still causes trouble today. The fact that it's not
sent over the air along with the report on HOG just compounds the confusion.
- Dean
On 21/11/2020 9:10 am, mathieu.stephan@gmail.com wrote:
> Hello Dean and thanks for your reply,
>
> I had completely missed the preprended byte would only be there for
> multiple HOG devices.
> I fully agree with your assessment, as we indeed discovered segfaults
> when our 2 reports HoG would connect to Linux
> (https://github.com/mooltipass/moolticute/issues/671
> <https://github.com/mooltipass/moolticute/issues/671>).
> That makes me think we (until recently, maybe) are the only ones to have
> such a device and (unlucky) scenario.
>
> Mathieu
>
> On Fri, Nov 20, 2020 at 11:00 PM Dean Camera <dean@fourwalledcubicle.com
> <mailto:dean@fourwalledcubicle.com>> wrote:
>
> The existing behaviour for a HOG device with report IDs was broken, and
> likely wouldn't have worked at all as the first byte of the response
> was
> being discarded.
>
> The missing prepended report ID is theoretically something that would
> cause an observable difference to userspace, but as it's only added in
> the multiple report case that was broken I don't think anyone will be
> affected.
>
> Single report HOG devices make up for the majority of consumer HID
> devices, which is probably why this has gone unnoticed for so long, and
> the behaviour of those is unaffected by my patch.
>
> The only use case I can see that would be broken would be a userspace
> app using HIDRAW to communicate with a multiple report HOG device,
> which
> was already tolerant of missing the first byte if the report.
>
> Cheers,
> - Dean
>
> On 21/11/2020 6:56 am, mathieu.stephan@gmail.com
> <mailto:mathieu.stephan@gmail.com> wrote:
> > Hello All,
> >
> > Is there a way to communicate to users that particular change?
> > I'm reacting as this is something our team heavily relies upon
> >
> (https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91
> <https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91>
>
> >
> <https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91
> <https://github.com/mooltipass/moolticute/blob/master/src/MPDevice_linux.cpp#L91>>)
>
> > and I'm guessing we're far from the only ones :)
> >
> > Regards,
> > Mathieu
> >
> > On Fri, Nov 20, 2020 at 6:52 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com <mailto:luiz.dentz@gmail.com>
> <mailto:luiz.dentz@gmail.com <mailto:luiz.dentz@gmail.com>>> wrote:
> >
> > Hi Dean,
> >
> > On Thu, Nov 19, 2020 at 5:47 PM Dean Camera
> > <dean@fourwalledcubicle.com
> <mailto:dean@fourwalledcubicle.com>
> <mailto:dean@fourwalledcubicle.com
> <mailto:dean@fourwalledcubicle.com>>> wrote:
> > >
> > > If the HID subsystem requests a HID report to be read from the
> > > device, we currently incorrectly strip off the first byte
> of the
> > > response, if the device has report IDs set in the HID report
> > > descriptor.
> > >
> > > This is incorrect; unlike USB HID, the report ID is *not*
> included
> > > in the HOG profile's HID reports, and instead exists out
> of band
> > > in a descriptor on the report's bluetooth characteristic
> in the
> > > device.
> > >
> > > In this patch, we remove the erroneous stripping of the first
> > > byte of the report, and (if report IDs are enabled)
> prepend the
> > > report ID to the front of the result. This makes the HID
> report
> > > returned indentical in format to that of a USB HID report, so
> > > that the upper HID drivers can consume HOG device reports
> in the
> > > same way as USB.
> > > ---
> > > profiles/input/hog-lib.c | 18 +++++++++++-------
> > > 1 file changed, 11 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/profiles/input/hog-lib.c
> b/profiles/input/hog-lib.c
> > > index 78018aad3..49d459e21 100644
> > > --- a/profiles/input/hog-lib.c
> > > +++ b/profiles/input/hog-lib.c
> > > @@ -779,7 +779,8 @@ fail:
> > > static void get_report_cb(guint8 status, const guint8 *pdu,
> > guint16 len,
> > >
> gpointer
> > user_data)
> > > {
> > > - struct bt_hog *hog = user_data;
> > > + struct report *report = user_data;
> > > + struct bt_hog *hog = report->hog;
> > > struct uhid_event rsp;
> > > int err;
> > >
> > > @@ -808,13 +809,16 @@ static void get_report_cb(guint8
> status, const
> > > guint8 *pdu, guint16 len,
> > >
> > > --len;
> > > ++pdu;
> > > +
> > > if (hog->has_report_id && len > 0) {
> > > - --len;
> > > - ++pdu;
> > > + rsp.u.get_report_reply.size = len + 1;
> > > + rsp.u.get_report_reply.data[0] = report->id;
> > > + memcpy(&rsp.u.get_report_reply.data[1],
> pdu, len);
> > > + }
> > > + else {
> > > + rsp.u.get_report_reply.size = len;
> > > + memcpy(rsp.u.get_report_reply.data, pdu, len);
> > > }
> > > -
> > > - rsp.u.get_report_reply.size = len;
> > > - memcpy(rsp.u.get_report_reply.data, pdu, len);
> > >
> > > exit:
> > > rsp.u.get_report_reply.err = status;
> > > @@ -846,7 +850,7 @@ static void get_report(struct
> uhid_event *ev,
> > void
> > > *user_data)
> > >
> > > hog->getrep_att = gatt_read_char(hog->attrib,
> > >
> report->value_handle,
> > > -
> get_report_cb, hog);
> > > + get_report_cb,
> > report);
> > > if (!hog->getrep_att) {
> > > err = ENOMEM;
> > > goto fail;
> > > --
> > > 2.29.2.windows.2
> > >
> >
> > Applied, thanks.
> >
> > --
> > Luiz Augusto von Dentz
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-20 22:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 1:42 [PATCH] Fix HOG profile incorrectly stripping off read report bytes Dean Camera
2020-11-20 17:49 ` Luiz Augusto von Dentz
[not found] ` <CAHdu5-74egP-m3pUPKEb_TWHRm21DMnbqE2K119wGoO9TgXioQ@mail.gmail.com>
2020-11-20 22:00 ` Dean Camera
[not found] ` <CAHdu5-6MgMc9DQK=pAH0qHJYRy+aaJV98bG1ct+2tXyRv2yx-w@mail.gmail.com>
2020-11-20 22:22 ` Dean Camera
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.