All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
@ 2022-08-25 12:29 Bastien Nocera
  2022-08-25 15:59 ` Nestor Lopez Casado
  2022-08-30 11:37 ` Bastien Nocera
  0 siblings, 2 replies; 13+ messages in thread
From: Bastien Nocera @ 2022-08-25 12:29 UTC (permalink / raw)
  To: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input,
	Nestor Lopez Casado

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-25 12:29 [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices Bastien Nocera
@ 2022-08-25 15:59 ` Nestor Lopez Casado
  2022-08-25 16:27   ` Bastien Nocera
  2022-08-25 20:47   ` Peter F. Patel-Schneider
  2022-08-30 11:37 ` Bastien Nocera
  1 sibling, 2 replies; 13+ messages in thread
From: Nestor Lopez Casado @ 2022-08-25 15:59 UTC (permalink / raw)
  To: Bastien Nocera
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input,
	Peter F. Patel-Schneider

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
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-25 15:59 ` Nestor Lopez Casado
@ 2022-08-25 16:27   ` Bastien Nocera
  2022-08-25 20:47   ` Peter F. Patel-Schneider
  1 sibling, 0 replies; 13+ messages in thread
From: Bastien Nocera @ 2022-08-25 16:27 UTC (permalink / raw)
  To: Nestor Lopez Casado
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input,
	Peter F. Patel-Schneider

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-25 15:59 ` Nestor Lopez Casado
  2022-08-25 16:27   ` Bastien Nocera
@ 2022-08-25 20:47   ` Peter F. Patel-Schneider
  2022-08-26 14:35     ` Bastien Nocera
                       ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: Peter F. Patel-Schneider @ 2022-08-25 20:47 UTC (permalink / raw)
  To: Nestor Lopez Casado, Bastien Nocera
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input

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
> > 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-25 20:47   ` Peter F. Patel-Schneider
@ 2022-08-26 14:35     ` Bastien Nocera
  2022-08-26 15:31       ` Peter F. Patel-Schneider
  2022-08-26 15:37       ` Peter F. Patel-Schneider
  2022-08-29 11:52     ` Bastien Nocera
  2022-08-29 13:45     ` Bastien Nocera
  2 siblings, 2 replies; 13+ messages in thread
From: Bastien Nocera @ 2022-08-26 14:35 UTC (permalink / raw)
  To: Peter F. Patel-Schneider, Nestor Lopez Casado
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-26 14:35     ` Bastien Nocera
@ 2022-08-26 15:31       ` Peter F. Patel-Schneider
  2022-08-26 15:37       ` Peter F. Patel-Schneider
  1 sibling, 0 replies; 13+ messages in thread
From: Peter F. Patel-Schneider @ 2022-08-26 15:31 UTC (permalink / raw)
  To: Bastien Nocera, Nestor Lopez Casado
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input

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




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-26 14:35     ` Bastien Nocera
  2022-08-26 15:31       ` Peter F. Patel-Schneider
@ 2022-08-26 15:37       ` Peter F. Patel-Schneider
  2022-08-29 13:41         ` Bastien Nocera
  1 sibling, 1 reply; 13+ messages in thread
From: Peter F. Patel-Schneider @ 2022-08-26 15:37 UTC (permalink / raw)
  To: Bastien Nocera, Nestor Lopez Casado
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input

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





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-25 20:47   ` Peter F. Patel-Schneider
  2022-08-26 14:35     ` Bastien Nocera
@ 2022-08-29 11:52     ` Bastien Nocera
  2022-08-29 13:45     ` Bastien Nocera
  2 siblings, 0 replies; 13+ messages in thread
From: Bastien Nocera @ 2022-08-29 11:52 UTC (permalink / raw)
  To: Peter F. Patel-Schneider, Nestor Lopez Casado
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input

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);


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-26 15:37       ` Peter F. Patel-Schneider
@ 2022-08-29 13:41         ` Bastien Nocera
  2022-08-29 14:20           ` Peter F. Patel-Schneider
  0 siblings, 1 reply; 13+ messages in thread
From: Bastien Nocera @ 2022-08-29 13:41 UTC (permalink / raw)
  To: Peter F. Patel-Schneider, Nestor Lopez Casado
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-25 20:47   ` Peter F. Patel-Schneider
  2022-08-26 14:35     ` Bastien Nocera
  2022-08-29 11:52     ` Bastien Nocera
@ 2022-08-29 13:45     ` Bastien Nocera
  2022-08-29 14:01       ` Peter F. Patel-Schneider
  2 siblings, 1 reply; 13+ messages in thread
From: Bastien Nocera @ 2022-08-29 13:45 UTC (permalink / raw)
  To: Peter F. Patel-Schneider, Nestor Lopez Casado
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input

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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-29 13:45     ` Bastien Nocera
@ 2022-08-29 14:01       ` Peter F. Patel-Schneider
  0 siblings, 0 replies; 13+ messages in thread
From: Peter F. Patel-Schneider @ 2022-08-29 14:01 UTC (permalink / raw)
  To: Bastien Nocera, Nestor Lopez Casado
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input

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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-29 13:41         ` Bastien Nocera
@ 2022-08-29 14:20           ` Peter F. Patel-Schneider
  0 siblings, 0 replies; 13+ messages in thread
From: Peter F. Patel-Schneider @ 2022-08-29 14:20 UTC (permalink / raw)
  To: Bastien Nocera, Nestor Lopez Casado
  Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input


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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices
  2022-08-25 12:29 [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices Bastien Nocera
  2022-08-25 15:59 ` Nestor Lopez Casado
@ 2022-08-30 11:37 ` Bastien Nocera
  1 sibling, 0 replies; 13+ messages in thread
From: Bastien Nocera @ 2022-08-30 11:37 UTC (permalink / raw)
  To: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input,
	Nestor Lopez Casado

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-08-30 11:39 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-25 12:29 [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices Bastien Nocera
2022-08-25 15:59 ` Nestor Lopez Casado
2022-08-25 16:27   ` Bastien Nocera
2022-08-25 20:47   ` Peter F. Patel-Schneider
2022-08-26 14:35     ` Bastien Nocera
2022-08-26 15:31       ` Peter F. Patel-Schneider
2022-08-26 15:37       ` Peter F. Patel-Schneider
2022-08-29 13:41         ` Bastien Nocera
2022-08-29 14:20           ` Peter F. Patel-Schneider
2022-08-29 11:52     ` Bastien Nocera
2022-08-29 13:45     ` Bastien Nocera
2022-08-29 14:01       ` Peter F. Patel-Schneider
2022-08-30 11:37 ` Bastien Nocera

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.