| Message ID | 4542beb149883ab5dfdcfd7f6bb4b516e5c1bcdb.camel@hadess.net (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices | expand |
Adding Peter, as he maintains Solaar, one popular application that supports Logitech bluetooth devices. @Peter F. Patel-Schneider -nestor On Thu, Aug 25, 2022 at 2:29 PM Bastien Nocera <hadess@hadess.net> wrote: > > Probe for HID++ support over Bluetooth for all the Logitech Bluetooth > devices. As Logitech doesn't have a list of Bluetooth devices that > support HID++ over Bluetooth, probe every device. The HID++ driver > will fall back to plain HID if the device does not support HID++. > > Note that this change might cause upower to export 2 batteries for > certain Bluetooth LE devices which export their battery information > through the Bluetooth BATT profile. This particular bug is tracked at: > https://gitlab.freedesktop.org/upower/upower/-/issues/166 > > Tested with a Logitech Signature M650 mouse, over Bluetooth > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > --- > > Note that I could not test whether the Harmony PS3 (handled in hid-sony.c) > or DiNovo Edge keyboard (handled in hid-input.c) devices would correctly fallback > to those drivers in that case. > > Ways to test this would be appreciated (or merge this, and wait for feedback...) > > drivers/hid/hid-logitech-hidpp.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index 81de88ab2ecc..86e7a38d8a9a 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -4347,6 +4347,9 @@ static const struct hid_device_id hidpp_devices[] = { > { /* MX Master 3 mouse over Bluetooth */ > HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb023), > .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + > + { /* And try to enable HID++ for all the Logitech Bluetooth devices */ > + HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_ANY, USB_VENDOR_ID_LOGITECH, HID_ANY_ID) }, > {} > }; > > -- > 2.37.2 >
On Thu, 2022-08-25 at 17:59 +0200, Nestor Lopez Casado wrote: > Adding Peter, as he maintains Solaar, one popular application that > supports Logitech bluetooth devices. This patch should make no difference to Solaar.
This patch will cause more use of a part of the driver that constructs messages that do not conform to the HID++ 2.0 specification. This makes now a good time to fix the parts of the driver that construct non-conforming messages. More information follows. The Logitech HID++2.0 Draft Specification at https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf specifies (on pages 2 and 3) that the low-order nibble of the function (command) byte is non-zero. The HID++ driver at https://github.com/torvalds/linux/blob/master/drivers/hid/hid-logitech-hidpp.c has 1 in that nibble for some commands, #define CMD_ROOT_GET_FEATURE 0x01 #define CMD_ROOT_GET_PROTOCOL_VERSION 0x11 #define CMD_GET_DEVICE_NAME_TYPE_GET_COUNT 0x01 #define CMD_GET_DEVICE_NAME_TYPE_GET_DEVICE_NAME 0x11 #define CMD_GET_DEVICE_NAME_TYPE_GET_TYPE 0x21 But other commands have zero in that nibble, namely #define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_LEVEL_STATUS 0x00 #define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_CAPABILITY 0x10 #define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00 #define CMD_UNIFIED_BATTERY_GET_CAPABILITIES 0x00 #define CMD_UNIFIED_BATTERY_GET_STATUS 0x10 #define CMD_HI_RESOLUTION_SCROLLING_SET_HIGHRES_SCROLLING_MODE 0x10 #define CMD_HIRES_WHEEL_GET_WHEEL_CAPABILITY 0x00 #define CMD_HIRES_WHEEL_SET_WHEEL_MODE 0x20 #define CMD_SOLAR_SET_LIGHT_MEASURE 0x00 #define CMD_TOUCHPAD_FW_ITEMS_SET 0x10 This results in non-conforming messages being sent to devices. As devices are obligated to return this nibble intact they produce non- conforming responses as well. (Their other option would be to reject the messages.) This confuses other software that correctly uses this nibble to distinguish between device response messages and device event messages. In particular, the response to the unified battery command to get the capabilities comes back with a 0x00 function byte which is indistinguishable from a spontaneous notification message from the device for a battery status event. Other software trying to communicate with the device (e.g., Solaar) sees a unified battery status notification and will generally end up with incorrect information about the device. I suspect that this is actually happening and is the cause of the Solaar bug report https://github.com/pwr-Solaar/Solaar/issues/1718 There is also the possibility that the driver confuses a notification from the device as the response to a command that it sent. When this happens it would be likely that the actual response would be treated as a notification. The fix is to modify all the CMD definitions in the code to have 1 in their low-order nibble. peter PS: There is another HID++ 2.0 feature that reports battery information, 0x1F20 ADC Measurement, but that is not in the driver code. On Thu, 2022-08-25 at 17:59 +0200, Nestor Lopez Casado wrote: > Adding Peter, as he maintains Solaar, one popular application that > supports Logitech bluetooth devices. > @Peter F. Patel-Schneider > -nestor > > On Thu, Aug 25, 2022 at 2:29 PM Bastien Nocera <hadess@hadess.net> > wrote: > > > > Probe for HID++ support over Bluetooth for all the Logitech > > Bluetooth > > devices. As Logitech doesn't have a list of Bluetooth devices that > > support HID++ over Bluetooth, probe every device. The HID++ driver > > will fall back to plain HID if the device does not support HID++. > > > > Note that this change might cause upower to export 2 batteries for > > certain Bluetooth LE devices which export their battery information > > through the Bluetooth BATT profile. This particular bug is tracked > > at: > > https://gitlab.freedesktop.org/upower/upower/-/issues/166 > > > > Tested with a Logitech Signature M650 mouse, over Bluetooth > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > --- > > > > Note that I could not test whether the Harmony PS3 (handled in hid- > > sony.c) > > or DiNovo Edge keyboard (handled in hid-input.c) devices would > > correctly fallback > > to those drivers in that case. > > > > Ways to test this would be appreciated (or merge this, and wait for > > feedback...) > > > > drivers/hid/hid-logitech-hidpp.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid- > > logitech-hidpp.c > > index 81de88ab2ecc..86e7a38d8a9a 100644 > > --- a/drivers/hid/hid-logitech-hidpp.c > > +++ b/drivers/hid/hid-logitech-hidpp.c > > @@ -4347,6 +4347,9 @@ static const struct hid_device_id > > hidpp_devices[] = { > > { /* MX Master 3 mouse over Bluetooth */ > > HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb023), > > .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > + > > + { /* And try to enable HID++ for all the Logitech Bluetooth > > devices */ > > + HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_ANY, > > USB_VENDOR_ID_LOGITECH, HID_ANY_ID) }, > > {} > > }; > > > > -- > > 2.37.2 > >
On Thu, 2022-08-25 at 16:47 -0400, Peter F. Patel-Schneider wrote: > This patch will cause more use of a part of the driver that > constructs > messages that do not conform to the HID++ 2.0 specification. This > makes now a good time to fix the parts of the driver that construct > non-conforming messages. More information follows. This would cause problems, but not any worse than adding the product IDs individually, which is what we're trying to avoid. > This results in non-conforming messages being sent to devices. As > devices are obligated to return this nibble intact they produce non- > conforming responses as well. (Their other option would be to reject > the messages.) This confuses other software that correctly uses this > nibble to distinguish between device response messages and device > event > messages. I don't understand how this... > In particular, the response to the unified battery command to get the > capabilities comes back with a 0x00 function byte which is > indistinguishable from a spontaneous notification message from the > device for a battery status event. Other software trying to > communicate with the device (e.g., Solaar) sees a unified battery > status notification and will generally end up with incorrect > information about the device. I suspect that this is actually > happening and is the cause of the Solaar bug report > https://github.com/pwr-Solaar/Solaar/issues/1718 ...could cause this. Can you explain what the messages would look like in both cases, and how they could be misinterpreted as a 15 vs. 85 percent battery level? > There is also the possibility that the driver confuses a notification > from the device as the response to a command that it sent. When this > happens it would be likely that the actual response would be treated > as > a notification. > > > The fix is to modify all the CMD definitions in the code to have 1 in > their low-order nibble. All in all, I don't see those bugs as blocking the integration of the patch discussed above, I see it as a way to expose those bugs and possibly a way to make them more urgent. Filipe, were those problems known/already reported? Cheers
The bad interaction caused by the bug goes like this. The driver tries to get the battery information and finds out that the HID++ 2.0 device has the Unified Battery feature (0x1004) at index 0x08. It sends a command to the device to report the capabilities its Unified Battery feature supports using #define CMD_UNIFIED_BATTERY_GET_CAPABILITIES 0x00 for the command and software identifier nibbles in byte 4 of the message, which ends up being 11 FF 0800 00000000000000000000000000000000 This is a non-conforming message because the software identifier nibble (the low-order nibble of byte 4) is 0x0. The device does not respond with an error indicating that the message is non-conforming but instead treats the message as conformant and responds with the capabilities of its Unified Battery feature 11 FF 0800 0F030000000000000000000000000000 copying the device number, feature index, function, and software identifier, as required. The rest of the response indicates that the feature on the device supports all four qualitative battery levels, is rechargeable, and also reports battery levels in percentages. Devices with this feature also emit notification messages when their batteries change level. Notification messages are distinguished from response messages by having a 0x0 software identification nibble. So this response is read by other software that is listening to the raw HID messages from the device and interpreted as an event notification. In this case the feature has an event notification with function 0x0 that is emitted when status of the device's battery is changed. The fifth byte of these notification messages is the percentage of battery energy available. So the software concludes that the device's battery has 15% of its energy remaining. If the driver is changed to #define CMD_UNIFIED_BATTERY_GET_CAPABILITIES 0x01 then the command sent would be 11 FF 0801 00000000000000000000000000000000 which is conforming, and the response would be 11 FF 0801 0F030000000000000000000000000000 which is not the same as any event notification message. Other bad interactions are also possible, including the the driver misinterpreting an event notification as a response to a message that it sent to the device. peter On 8/26/22 10:35, Bastien Nocera wrote: > On Thu, 2022-08-25 at 16:47 -0400, Peter F. Patel-Schneider wrote: >> This patch will cause more use of a part of the driver that >> constructs >> messages that do not conform to the HID++ 2.0 specification. This >> makes now a good time to fix the parts of the driver that construct >> non-conforming messages. More information follows. > This would cause problems, but not any worse than adding the product > IDs individually, which is what we're trying to avoid. > >> This results in non-conforming messages being sent to devices. As >> devices are obligated to return this nibble intact they produce non- >> conforming responses as well. (Their other option would be to reject >> the messages.) This confuses other software that correctly uses this >> nibble to distinguish between device response messages and device >> event >> messages. > I don't understand how this... > >> In particular, the response to the unified battery command to get the >> capabilities comes back with a 0x00 function byte which is >> indistinguishable from a spontaneous notification message from the >> device for a battery status event. Other software trying to >> communicate with the device (e.g., Solaar) sees a unified battery >> status notification and will generally end up with incorrect >> information about the device. I suspect that this is actually >> happening and is the cause of the Solaar bug report >> https://github.com/pwr-Solaar/Solaar/issues/1718 > ...could cause this. Can you explain what the messages would look like > in both cases, and how they could be misinterpreted as a 15 vs. 85 > percent battery level? > >> There is also the possibility that the driver confuses a notification >> from the device as the response to a command that it sent. When this >> happens it would be likely that the actual response would be treated >> as >> a notification. >> >> >> The fix is to modify all the CMD definitions in the code to have 1 in >> their low-order nibble. > All in all, I don't see those bugs as blocking the integration of the > patch discussed above, I see it as a way to expose those bugs and > possibly a way to make them more urgent. > > Filipe, were those problems known/already reported? > > Cheers I reported https://bugzilla.kernel.org/show_bug.cgi?id=215699 which is a different problem with the same cause, albeit in a different constant in the driver. peter
Looking at https://bugzilla.kernel.org/show_bug.cgi?id=215699 reminded me of another problem with the driver code. Several HID++ 2.0 features, including the HiRes Wheel, have commands with bit fields in them and set all the bits in the bit field at once. But when the driver code sets the high-resolution bit for this feature it also sets two other bits, ignoring their current setting. This prevents other software from reliably using these two other bits, one of which is for reporting wheel movement in the opposite direction, a. k. a. natural scrolling. It would be useful for the driver code to first get the other bits and set them to their retrieved values. peter
On Thu, 2022-08-25 at 16:47 -0400, Peter F. Patel-Schneider wrote: > This patch will cause more use of a part of the driver that > constructs > messages that do not conform to the HID++ 2.0 specification. This > makes now a good time to fix the parts of the driver that construct > non-conforming messages. More information follows. > > > The Logitech HID++2.0 Draft Specification at > https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf > specifies (on pages 2 and 3) that the low-order nibble of the > function > (command) byte is non-zero. > > The HID++ driver at > https://github.com/torvalds/linux/blob/master/drivers/hid/hid-logitech-hidpp.c > has 1 in that nibble for some commands, > > #define CMD_ROOT_GET_FEATURE 0x01 > #define CMD_ROOT_GET_PROTOCOL_VERSION 0x11 > > #define CMD_GET_DEVICE_NAME_TYPE_GET_COUNT 0x01 > #define CMD_GET_DEVICE_NAME_TYPE_GET_DEVICE_NAME 0x11 > #define CMD_GET_DEVICE_NAME_TYPE_GET_TYPE 0x21 > > But other commands have zero in that nibble, namely > > #define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_LEVEL_STATUS 0x00 > #define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_CAPABILITY 0x10 > > #define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00 > > #define CMD_UNIFIED_BATTERY_GET_CAPABILITIES 0x00 > #define CMD_UNIFIED_BATTERY_GET_STATUS 0x10 > > #define CMD_HI_RESOLUTION_SCROLLING_SET_HIGHRES_SCROLLING_MODE 0x10 > > #define CMD_HIRES_WHEEL_GET_WHEEL_CAPABILITY 0x00 > #define CMD_HIRES_WHEEL_SET_WHEEL_MODE 0x20 > > #define CMD_SOLAR_SET_LIGHT_MEASURE 0x00 > > #define CMD_TOUCHPAD_FW_ITEMS_SET 0x10 Sounds like none of those commands should have anything set in that lower nibble, but this patch should be enough to take care of either case. I don't know whether the "rap" commands are used at all for HID++ 2.0 commands, or just for HID++ 1.0. Is the software ID something random that the software in question chooses? I chose 0x06 as we're at Linux version 6.0... diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 86e7a38d8a9a..4c430b24b6bc 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -41,6 +41,9 @@ module_param(disable_tap_to_click, bool, 0644); MODULE_PARM_DESC(disable_tap_to_click, "Disable Tap-To-Click mode reporting for touchpads (only on the K400 currently)."); +/* Define a non-zero software ID to identify our own responses */ +#define LINUX_KERNEL_SW_ID 0x06 + #define REPORT_ID_HIDPP_SHORT 0x10 #define REPORT_ID_HIDPP_LONG 0x11 #define REPORT_ID_HIDPP_VERY_LONG 0x12 @@ -343,7 +346,7 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp, else message->report_id = REPORT_ID_HIDPP_LONG; message->fap.feature_index = feat_index; - message->fap.funcindex_clientid = funcindex_clientid; + message->fap.funcindex_clientid = funcindex_clientid | LINUX_KERNEL_SW_ID; memcpy(&message->fap.params, params, param_count); ret = hidpp_send_message_sync(hidpp, message, response);
On Fri, 2022-08-26 at 11:37 -0400, Peter F. Patel-Schneider wrote: > Looking at > https://bugzilla.kernel.org/show_bug.cgi?id=215699 reminded me of > another problem with the driver code. Several HID++ 2.0 features, > including > the HiRes Wheel, have commands with bit fields in them and set all > the bits in > the bit field at once. But when the driver code sets the high- > resolution bit > for this feature it also sets two other bits, ignoring their current > setting. > This prevents other software from reliably using these two other > bits, one of > which is for reporting wheel movement in the opposite direction, a. > k. a. > natural scrolling. > > It would be useful for the driver code to first get the other bits > and set > them to their retrieved values. Do you have any other examples of this? For the classic Linux desktop, this particular bug isn't something we'd hit, as natural scrolling is implemented at the higher levels (libinput) rather than something we expect the driver to support.
On Thu, 2022-08-25 at 16:47 -0400, Peter F. Patel-Schneider wrote: > > PS: There is another HID++ 2.0 feature that reports battery > information, 0x1F20 ADC Measurement, but that is not in the driver > code. Do you have documentation for this one?
Only partial documentation and I can't distribute it. peter On 8/29/22 09:45, Bastien Nocera wrote: > On Thu, 2022-08-25 at 16:47 -0400, Peter F. Patel-Schneider wrote: >> PS: There is another HID++ 2.0 feature that reports battery >> information, 0x1F20 ADC Measurement, but that is not in the driver >> code. > Do you have documentation for this one?
On 8/29/22 09:41, Bastien Nocera wrote: > On Fri, 2022-08-26 at 11:37 -0400, Peter F. Patel-Schneider wrote: >> Looking at >> https://bugzilla.kernel.org/show_bug.cgi?id=215699 reminded me of >> another problem with the driver code. Several HID++ 2.0 features, >> including >> the HiRes Wheel, have commands with bit fields in them and set all >> the bits in >> the bit field at once. But when the driver code sets the high- >> resolution bit >> for this feature it also sets two other bits, ignoring their current >> setting. >> This prevents other software from reliably using these two other >> bits, one of >> which is for reporting wheel movement in the opposite direction, a. >> k. a. >> natural scrolling. >> >> It would be useful for the driver code to first get the other bits >> and set >> them to their retrieved values. > Do you have any other examples of this? For the classic Linux desktop, > this particular bug isn't something we'd hit, as natural scrolling is > implemented at the higher levels (libinput) rather than something we > expect the driver to support. I looked and I didn't find any other examples of this in the driver. The only HID++ command with this feature that is used by the driver, as far as I could tell, is #define CMD_HIRES_WHEEL_SET_WHEEL_MODE 0x20 of #define HIDPP_PAGE_HIRES_WHEEL 0x2121 The way Solaar users see this bug is that if they use this feature to set reverse scrolling sometimes the driver flips the bit off after Solaar flips the bit on. If a user doesn't use Solaar or some other software that changes devices behaviour using HID++ commands then they won't see any problem. (The could have a multi-host mouse, use Logitech software to flip the bit, and then when they change hosts to a Linux host the Linux driver would flip the bit.) peter
On Thu, 2022-08-25 at 14:29 +0200, Bastien Nocera wrote: > Probe for HID++ support over Bluetooth for all the Logitech Bluetooth > devices. As Logitech doesn't have a list of Bluetooth devices that > support HID++ over Bluetooth, probe every device. The HID++ driver > will fall back to plain HID if the device does not support HID++. > > Note that this change might cause upower to export 2 batteries for > certain Bluetooth LE devices which export their battery information > through the Bluetooth BATT profile. This particular bug is tracked > at: > https://gitlab.freedesktop.org/upower/upower/-/issues/166 > > Tested with a Logitech Signature M650 mouse, over Bluetooth > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > --- > > Note that I could not test whether the Harmony PS3 (handled in hid- > sony.c) > or DiNovo Edge keyboard (handled in hid-input.c) devices would > correctly fallback > to those drivers in that case. The special case of the DiNovo Edge keyboard and Harmony remote should be fixed in the v2 version.
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 81de88ab2ecc..86e7a38d8a9a 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -4347,6 +4347,9 @@ static const struct hid_device_id hidpp_devices[] = { { /* MX Master 3 mouse over Bluetooth */ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb023), .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, + + { /* And try to enable HID++ for all the Logitech Bluetooth devices */ + HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_ANY, USB_VENDOR_ID_LOGITECH, HID_ANY_ID) }, {} };
Probe for HID++ support over Bluetooth for all the Logitech Bluetooth devices. As Logitech doesn't have a list of Bluetooth devices that support HID++ over Bluetooth, probe every device. The HID++ driver will fall back to plain HID if the device does not support HID++. Note that this change might cause upower to export 2 batteries for certain Bluetooth LE devices which export their battery information through the Bluetooth BATT profile. This particular bug is tracked at: https://gitlab.freedesktop.org/upower/upower/-/issues/166 Tested with a Logitech Signature M650 mouse, over Bluetooth Signed-off-by: Bastien Nocera <hadess@hadess.net> --- Note that I could not test whether the Harmony PS3 (handled in hid-sony.c) or DiNovo Edge keyboard (handled in hid-input.c) devices would correctly fallback to those drivers in that case. Ways to test this would be appreciated (or merge this, and wait for feedback...) drivers/hid/hid-logitech-hidpp.c | 3 +++ 1 file changed, 3 insertions(+)