Skip to content

Commit

Permalink
hog: Fix stripping off read report bytes
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@abcminiuser @Vudentz
abcminiuser authored and Vudentz committed Nov 20, 2020
1 parent e892dd0 commit 35a2c50
Showing 1 changed file with 10 additions and 7 deletions.
17 changes: 10 additions & 7 deletions profiles/input/hog-lib.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,8 @@ static void set_report(struct uhid_event *ev, void *user_data)
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;

Expand Down Expand Up @@ -808,14 +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;
err = bt_uhid_send(hog->uhid, &rsp);
Expand Down Expand Up @@ -846,7 +849,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;
Expand Down

5 comments on commit 35a2c50

@Plagman
Copy link

@Plagman Plagman commented on 35a2c50 Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abcminiuser @Vudentz

Hello,

This broke handling of some Bluetooth controllers in the Steam client for all users once their distribution updated them to 5.56 [1]. I understand this was intended to fix something that was originally incorrect, but it doesn't seem correct to just shift behavior under pre-existing users of the library, breaking them in the process (we already had a long-standing workaround for the prior behavior). It would have been more appropriate to only change it for clients that were aware of the new behavior with some sort of version handshake.

In this case we are lucky to happen to be aware of the breakage (after the fact) and have some bandwidth to adjust our side, but there are probably other usecases that don't know they are broken yet, and that might never get around to changing it and stay broken forever. These instances contribute in damaging the idea of a possible healthy third-party app ecosystem for the Linux desktop. I strongly encourage folks maintaining load-bearing userland libraries to take a page from the kernel book and reconsider rolling out breaking changes like this. Alternative usually involve implementing a quirk or version handshake system to avoid them in the future. We understand this is more work and not everyone might be in a position to do it.

If there is no plan to do that, can you possibly advise as to whether there is a way for us to detect which side of this breaking change we are on, so we can disable the workaround only when appropriate? We'd rather not have to pick a subset of our users to throw under the trolley.

[1] ValveSoftware/steam-for-linux#7697 (comment)

@Plagman
Copy link

@Plagman Plagman commented on 35a2c50 Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of the pre-existing thread about this issue in the kernel linux-bluetooth mailing list before typing the above comment. The last message [1] is confusing to me, as it implies folks writing the software that uses BlueZ and upgrading their BlueZ version are the same people, which doesn't seem to be the case. As far as I can understand we have no control over what BlueZ version is used at runtime.

[1] https://lore.kernel.org/linux-bluetooth/9fed57047ba6145af64ac00579f8ffb7cd03a55d.camel@collabora.com/

@pmarreck
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way to apply a change like this idempotently, so that re-application is in the worst case just redundant processing effort and doesn’t actually break anything?

@Vudentz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way to apply a change like this idempotently, so that re-application is in the worst case just redundant processing effort and doesn’t actually break anything?

Im afraid in this case the old behavior was just broken the information of the report ID would be lost, but perhaps the kernel driver was able to detect somehow (perhaps because there is a bus information) the report ID was stripped and now with this fix that workaround no longer works causing incompatibilities between userspace and kernel driver, we could perhaps add some versioning but Im afraid with uHID that is currently not possible. Btw, I hope no-one had the brilliant idea of adding the report ID on the peripheral side since that would probably be considered an invalid behavior as per HIDS spec.

@kakra
Copy link

@kakra kakra commented on 35a2c50 Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already had a long-standing workaround for the prior behavior

While I support your claim that the Linux ecosystem should maintain better API/ABI stability (yes, that's really important), I think it would be an equally valid claim that instead of relying on a work-around or hack, the devs could also have reported the broken behavior to bluez and maybe even provide a patch to the providing software instead of working around downstream. After all, this is open-source. But I also get the point why this may not have happened: Usually, you are looking for a problem in your own software first, suspecting a bug there, and working around there, as you usually won't expect an upstream bug or even don't see the whole picture yet.

As far as I understand, this only affects devices using hidraw, so I guess it may generally be a bad idea to re-implement hidraw drivers in user-space when there is a proper kernel-space driver - as SDL does, with SteamInput seemingly using SDL. I'm seeing similar problems developing the xpadneo driver where SDL (and thus SteamInput) completely ignores the HID report descriptor and just assumes a specific report format (which is broken behavior if you asked me). So we can probably conclude that implementing workarounds or handlers for really low-level implementations should be expected and proper workarounds with validation of packet formats is needed.

Please sign in to comment.