| Message ID | a542b260-7eb1-8917-9379-e945003d7817@fourwalledcubicle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix HOG profile incorrectly stripping off read report bytes. | expand |
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.
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 >
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 > > >
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:
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(-) 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;