[Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Rafael J. Wysocki]

Bastien, Jiri,

Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech
Bluetooth devices") caused my Logitech Bluetooth mouse to become unusable.

Appended is the change I need to make it work again (note that adding the
device ID to unhandled_hidpp_devices[] doesn't help, so there must be some
significant enough difference in how the two cases are handled in the stack).

Here's what I get in the log without the patch below:

[   36.710574] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
[   36.710592] Bluetooth: HIDP socket layer initialized
[   36.724644] hid-generic 0005:046D:B016.0001: unknown main item tag 0x0
[   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
[   36.728036] input: Bluetooth Mouse M336/M337/M535 Consumer Control as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
[   36.728823] input: Bluetooth Mouse M336/M337/M535 Keyboard as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
[   36.731550] hid-generic 0005:046D:B016.0001: input,hidraw0: BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on 9c:b6:d0:96:8e:c8
[   36.833039] logitech-hidpp-device 0005:046D:B016.0001: unknown main item tag 0x0
[   36.999064] logitech-hidpp-device 0005:046D:B016.0001: Device not connected

and here's what I get with it:

[   43.642546] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
[   43.642559] Bluetooth: HIDP socket layer initialized
[   43.652898] hid-generic 0005:046D:B016.0001: unknown main item tag 0x0
[   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
[   43.655025] input: Bluetooth Mouse M336/M337/M535 Consumer Control as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
[   43.655400] input: Bluetooth Mouse M336/M337/M535 Keyboard as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
[   43.657521] hid-generic 0005:046D:B016.0001: input,hidraw0: BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on 9c:b6:d0:96:8e:c8

The only difference seems to be that in the former case the logitech-hidpp
driver tries to bind to the device, but I guess that is expected.  However,
when the device ID is added to unhandled_hidpp_devices[], the messages look
exactly like in the "good" case, but the mouse still doesn't work.

Thanks,
Rafael


---
 drivers/hid/hid-logitech-hidpp.c |    3 ---
 1 file changed, 3 deletions(-)

Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
===================================================================
--- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
+++ linux-pm/drivers/hid/hid-logitech-hidpp.c
@@ -4367,9 +4367,6 @@ static const struct hid_device_id hidpp_
 	{ /* MX5500 keyboard over Bluetooth */
 	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b),
 	  .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
-
-	{ /* 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) },
 	{}
 };
 

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Rafael J. Wysocki]

On Tue, Dec 6, 2022 at 3:58 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Bastien, Jiri,
>
> Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech
> Bluetooth devices") caused my Logitech Bluetooth mouse to become unusable.
>
> Appended is the change I need to make it work again (note that adding the
> device ID to unhandled_hidpp_devices[] doesn't help, so there must be some
> significant enough difference in how the two cases are handled in the stack).
>
> Here's what I get in the log without the patch below:
>
> [   36.710574] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> [   36.710592] Bluetooth: HIDP socket layer initialized
> [   36.724644] hid-generic 0005:046D:B016.0001: unknown main item tag 0x0
> [   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> [   36.728036] input: Bluetooth Mouse M336/M337/M535 Consumer Control as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> [   36.728823] input: Bluetooth Mouse M336/M337/M535 Keyboard as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> [   36.731550] hid-generic 0005:046D:B016.0001: input,hidraw0: BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on 9c:b6:d0:96:8e:c8
> [   36.833039] logitech-hidpp-device 0005:046D:B016.0001: unknown main item tag 0x0
> [   36.999064] logitech-hidpp-device 0005:046D:B016.0001: Device not connected
>
> and here's what I get with it:
>
> [   43.642546] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> [   43.642559] Bluetooth: HIDP socket layer initialized
> [   43.652898] hid-generic 0005:046D:B016.0001: unknown main item tag 0x0
> [   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> [   43.655025] input: Bluetooth Mouse M336/M337/M535 Consumer Control as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> [   43.655400] input: Bluetooth Mouse M336/M337/M535 Keyboard as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> [   43.657521] hid-generic 0005:046D:B016.0001: input,hidraw0: BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on 9c:b6:d0:96:8e:c8
>
> The only difference seems to be that in the former case the logitech-hidpp
> driver tries to bind to the device, but I guess that is expected.  However,
> when the device ID is added to unhandled_hidpp_devices[], the messages look
> exactly like in the "good" case, but the mouse still doesn't work.

Here's what happens.

- The logitech-hidpp driver is modular and is not present initially,
so hid-generic probes first (successfully).

- logitech-hidpp is loaded which causes the device to be reporobed due
to __hid_bus_driver_added().

- Because the ->match() callback in hid-generic returns 0 for the
device now, it is unbound from the device.

- The probing of logitech-hidpp fails (due to an error in ->probe()).

- The probing of hid-generic fails due to the failing ->match().

So in order for unhandled_hidpp_devices[] in logitech-hidpp to work
with hid-generic, __check_hid_generic() needs to be amended with a
change to run the ->match() callback of the "specific" driver being
tried.

I have a working patch for this that will be sent shortly.

Thanks!

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Bastien Nocera]

On Tue, 2022-12-06 at 15:58 +0100, Rafael J. Wysocki wrote:
> Bastien, Jiri,
> 
> Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> Logitech
> Bluetooth devices") caused my Logitech Bluetooth mouse to become
> unusable.
> 
> Appended is the change I need to make it work again (note that adding
> the
> device ID to unhandled_hidpp_devices[] doesn't help, so there must be
> some
> significant enough difference in how the two cases are handled in the
> stack).
> 
> Here's what I get in the log without the patch below:
> 
> [   36.710574] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> [   36.710592] Bluetooth: HIDP socket layer initialized
> [   36.724644] hid-generic 0005:046D:B016.0001: unknown main item tag
> 0x0
> [   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse as
> /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> [   36.728036] input: Bluetooth Mouse M336/M337/M535 Consumer Control
> as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> [   36.728823] input: Bluetooth Mouse M336/M337/M535 Keyboard as
> /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> [   36.731550] hid-generic 0005:046D:B016.0001: input,hidraw0:
> BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> 9c:b6:d0:96:8e:c8
> [   36.833039] logitech-hidpp-device 0005:046D:B016.0001: unknown
> main item tag 0x0
> [   36.999064] logitech-hidpp-device 0005:046D:B016.0001: Device not
> connected
> 
> and here's what I get with it:
> 
> [   43.642546] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> [   43.642559] Bluetooth: HIDP socket layer initialized
> [   43.652898] hid-generic 0005:046D:B016.0001: unknown main item tag
> 0x0
> [   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse as
> /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> [   43.655025] input: Bluetooth Mouse M336/M337/M535 Consumer Control
> as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> [   43.655400] input: Bluetooth Mouse M336/M337/M535 Keyboard as
> /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> [   43.657521] hid-generic 0005:046D:B016.0001: input,hidraw0:
> BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> 9c:b6:d0:96:8e:c8
> 
> The only difference seems to be that in the former case the logitech-
> hidpp
> driver tries to bind to the device, but I guess that is expected. 

There really shouldn't be that much difference between the 2 paths,
except that hid-logitech-hidpp.c will check that the device supports
HID++ in its report descriptors, and then start talking to it to check
whether it's connected.

Maybe the device doesn't support HID++?

Can you try running src/tools/hidpp-list-features from
https://github.com/cvuchener/hidpp on the hidraw device for the mouse?

> However,
> when the device ID is added to unhandled_hidpp_devices[], the
> messages look
> exactly like in the "good" case, but the mouse still doesn't work.

Given that this should be called without ever talking to the device,
that tells me that there might be a logic bug in the hid-core that uses
->probe. Benjamin?

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Bastien Nocera]

On Wed, 2022-12-07 at 09:58 +0100, Rafael J. Wysocki wrote:
> On Tue, Dec 6, 2022 at 3:58 PM Rafael J. Wysocki <rjw@rjwysocki.net>
> wrote:
> > 
> > Bastien, Jiri,
> > 
> > Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> > Logitech
> > Bluetooth devices") caused my Logitech Bluetooth mouse to become
> > unusable.
> > 
> > Appended is the change I need to make it work again (note that
> > adding the
> > device ID to unhandled_hidpp_devices[] doesn't help, so there must
> > be some
> > significant enough difference in how the two cases are handled in
> > the stack).
> > 
> > Here's what I get in the log without the patch below:
> > 
> > [   36.710574] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> > [   36.710592] Bluetooth: HIDP socket layer initialized
> > [   36.724644] hid-generic 0005:046D:B016.0001: unknown main item
> > tag 0x0
> > [   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > [   36.728036] input: Bluetooth Mouse M336/M337/M535 Consumer
> > Control as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > [   36.728823] input: Bluetooth Mouse M336/M337/M535 Keyboard as
> > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > [   36.731550] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > 9c:b6:d0:96:8e:c8
> > [   36.833039] logitech-hidpp-device 0005:046D:B016.0001: unknown
> > main item tag 0x0
> > [   36.999064] logitech-hidpp-device 0005:046D:B016.0001: Device
> > not connected
> > 
> > and here's what I get with it:
> > 
> > [   43.642546] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> > [   43.642559] Bluetooth: HIDP socket layer initialized
> > [   43.652898] hid-generic 0005:046D:B016.0001: unknown main item
> > tag 0x0
> > [   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > [   43.655025] input: Bluetooth Mouse M336/M337/M535 Consumer
> > Control as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > [   43.655400] input: Bluetooth Mouse M336/M337/M535 Keyboard as
> > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > [   43.657521] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > 9c:b6:d0:96:8e:c8
> > 
> > The only difference seems to be that in the former case the
> > logitech-hidpp
> > driver tries to bind to the device, but I guess that is expected. 
> > However,
> > when the device ID is added to unhandled_hidpp_devices[], the
> > messages look
> > exactly like in the "good" case, but the mouse still doesn't work.
> 
> Here's what happens.
> 
> - The logitech-hidpp driver is modular and is not present initially,
> so hid-generic probes first (successfully).
> 
> - logitech-hidpp is loaded which causes the device to be reporobed
> due
> to __hid_bus_driver_added().
> 
> - Because the ->match() callback in hid-generic returns 0 for the
> device now, it is unbound from the device.
> 
> - The probing of logitech-hidpp fails (due to an error in ->probe()).
> 
> - The probing of hid-generic fails due to the failing ->match().
> 
> So in order for unhandled_hidpp_devices[] in logitech-hidpp to work
> with hid-generic, __check_hid_generic() needs to be amended with a
> change to run the ->match() callback of the "specific" driver being
> tried.
> 
> I have a working patch for this that will be sent shortly.

I'm glad my belated questioning of the hid-generic fallback code was
correct ;)

My question about getting the output from the HID++ tool still stands
though.

[PATCH v1 0/2] HID: Fix regression resulting from commit 532223c8ac57

[Rafael J. Wysocki]

Jiri, Bastien,

This series of 2 patches fixes a regression introduced by commit 532223c8ac57
("HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices")
that caused my Logitech Bluetooth mouse to become unusable.

The problem is that the mouse doesn't work with HID++ and so it needs to be
added to the list of "unhandled" devices in logitech-hidpp (patch 2), but
this is not sufficient, because hid-generic needs to actually check whether
other drivers have ->match callbacks and what those callbacks return for
the given device (patch 1).

Thanks!

[PATCH v1 1/2] HID: generic: Add ->match() check to __check_hid_generic()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Some special HID drivers (for example, hid-logitech-hidpp) use ->match()
callbacks to reject specific devices that otherwise would match the
driver's device ID list, with the expectation that those devices will
be handled by some other drivers.  However, this doesn't work if
hid-generic is expected to bind to the given device, because its
->match() callback, hid_generic_match(), rejects all devices that match
device ID lists of the other HID drivers regardless of what is returned
by the other drivers' ->match() callbacks.

To make it work, amend the function used by hid_generic_match() for
checking an individual driver, __check_hid_generic(), with a check
involving the given driver's ->match() callback, so 0 is returned
when that callback rejects the device in question.

Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/hid/hid-generic.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/hid/hid-generic.c
===================================================================
--- linux-pm.orig/drivers/hid/hid-generic.c
+++ linux-pm/drivers/hid/hid-generic.c
@@ -31,7 +31,13 @@ static int __check_hid_generic(struct de
 	if (hdrv == &hid_generic)
 		return 0;
 
-	return hid_match_device(hdev, hdrv) != NULL;
+	if (!hid_match_device(hdev, hdrv))
+		return 0;
+
+	if (hdrv->match)
+		return hdrv->match(hdev, false);
+
+	return 1;
 }
 
 static bool hid_generic_match(struct hid_device *hdev,

[PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
work when HID++ is enabled for it, so add it to the list of devices
that are not handled by logitech-hidpp.

Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/hid/hid-logitech-hidpp.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
===================================================================
--- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
+++ linux-pm/drivers/hid/hid-logitech-hidpp.c
@@ -4274,6 +4274,7 @@ static const struct hid_device_id unhand
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_HARMONY_PS3) },
 	/* Handled in hid-generic */
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb016) },
 	{}
 };
 

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Rafael J. Wysocki]

On Wednesday, December 7, 2022 10:04:43 AM CET Bastien Nocera wrote:
> On Tue, 2022-12-06 at 15:58 +0100, Rafael J. Wysocki wrote:
> > Bastien, Jiri,
> > 
> > Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> > Logitech
> > Bluetooth devices") caused my Logitech Bluetooth mouse to become
> > unusable.
> > 
> > Appended is the change I need to make it work again (note that adding
> > the
> > device ID to unhandled_hidpp_devices[] doesn't help, so there must be
> > some
> > significant enough difference in how the two cases are handled in the
> > stack).
> > 
> > Here's what I get in the log without the patch below:
> > 
> > [   36.710574] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> > [   36.710592] Bluetooth: HIDP socket layer initialized
> > [   36.724644] hid-generic 0005:046D:B016.0001: unknown main item tag
> > 0x0
> > [   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > [   36.728036] input: Bluetooth Mouse M336/M337/M535 Consumer Control
> > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > [   36.728823] input: Bluetooth Mouse M336/M337/M535 Keyboard as
> > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > [   36.731550] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > 9c:b6:d0:96:8e:c8
> > [   36.833039] logitech-hidpp-device 0005:046D:B016.0001: unknown
> > main item tag 0x0
> > [   36.999064] logitech-hidpp-device 0005:046D:B016.0001: Device not
> > connected
> > 
> > and here's what I get with it:
> > 
> > [   43.642546] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> > [   43.642559] Bluetooth: HIDP socket layer initialized
> > [   43.652898] hid-generic 0005:046D:B016.0001: unknown main item tag
> > 0x0
> > [   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > [   43.655025] input: Bluetooth Mouse M336/M337/M535 Consumer Control
> > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > [   43.655400] input: Bluetooth Mouse M336/M337/M535 Keyboard as
> > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > [   43.657521] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > 9c:b6:d0:96:8e:c8
> > 
> > The only difference seems to be that in the former case the logitech-
> > hidpp
> > driver tries to bind to the device, but I guess that is expected. 
> 
> There really shouldn't be that much difference between the 2 paths,
> except that hid-logitech-hidpp.c will check that the device supports
> HID++ in its report descriptors, and then start talking to it to check
> whether it's connected.
> 
> Maybe the device doesn't support HID++?

Quite possibly.

> Can you try running src/tools/hidpp-list-features from
> https://github.com/cvuchener/hidpp on the hidraw device for the mouse?

OK, I'll do that.

> > However,
> > when the device ID is added to unhandled_hidpp_devices[], the
> > messages look
> > exactly like in the "good" case, but the mouse still doesn't work.
> 
> Given that this should be called without ever talking to the device,
> that tells me that there might be a logic bug in the hid-core that uses
> ->probe. Benjamin?

I've explained what happens in this message:

https://lore.kernel.org/lkml/CAJZ5v0jBo-_XnN2m0jeVdeTi7kjr6C3OSzc1NEJgav0srD0JGQ@mail.gmail.com/

and I've just posted patches that fix the issue for me.

Cheers!

Re: [PATCH v1 1/2] HID: generic: Add ->match() check to __check_hid_generic()

[Benjamin Tissoires]

On Wed, Dec 7, 2022 at 10:13 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Some special HID drivers (for example, hid-logitech-hidpp) use ->match()
> callbacks to reject specific devices that otherwise would match the
> driver's device ID list, with the expectation that those devices will
> be handled by some other drivers.  However, this doesn't work if
> hid-generic is expected to bind to the given device, because its
> ->match() callback, hid_generic_match(), rejects all devices that match
> device ID lists of the other HID drivers regardless of what is returned
> by the other drivers' ->match() callbacks.

Thanks Rafael for spotting that corner case in the ->match() processing.

>
> To make it work, amend the function used by hid_generic_match() for
> checking an individual driver, __check_hid_generic(), with a check
> involving the given driver's ->match() callback, so 0 is returned
> when that callback rejects the device in question.

Shouldn't we add that logic to hid_match_device() directly in
hid-core.c instead?
It feels wrong to have a function named "hid_match_device()" and have
to manually call later "->match()" on the driver itself.

Ack on the general idea anyway.

>
> Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/hid/hid-generic.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/hid/hid-generic.c
> ===================================================================
> --- linux-pm.orig/drivers/hid/hid-generic.c
> +++ linux-pm/drivers/hid/hid-generic.c
> @@ -31,7 +31,13 @@ static int __check_hid_generic(struct de
>         if (hdrv == &hid_generic)
>                 return 0;
>
> -       return hid_match_device(hdev, hdrv) != NULL;
> +       if (!hid_match_device(hdev, hdrv))
> +               return 0;
> +
> +       if (hdrv->match)
> +               return hdrv->match(hdev, false);
> +
> +       return 1;
>  }
>
>  static bool hid_generic_match(struct hid_device *hdev,
>
>
>

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Bastien Nocera]

On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
> work when HID++ is enabled for it,

This needs the output of the hidpp-list-features tool mentioned earlier
in the thread so we can avoid words like "evidently" and provide
concrete proof.

But why is it needed in this case? We purposefully try to avoid blanket
blocklists. The lack of HID++ can be probed, so the device should work
just as it used to (if the fallback code works).

We should only list devices that need special handling, and the ones
that don't work once HID++ was probed unsuccessfully.

>  so add it to the list of devices
> that are not handled by logitech-hidpp.
> 
> Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> Logitech Bluetooth devices")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/hid/hid-logitech-hidpp.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
> ===================================================================
> --- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
> +++ linux-pm/drivers/hid/hid-logitech-hidpp.c
> @@ -4274,6 +4274,7 @@ static const struct hid_device_id unhand
>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> USB_DEVICE_ID_LOGITECH_HARMONY_PS3) },
>         /* Handled in hid-generic */
>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD) },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb016) },
>         {}
>  };
>  
> 
> 
> 

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 10:16 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, December 7, 2022 10:04:43 AM CET Bastien Nocera wrote:
> > On Tue, 2022-12-06 at 15:58 +0100, Rafael J. Wysocki wrote:
> > > Bastien, Jiri,
> > >
> > > Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> > > Logitech
> > > Bluetooth devices") caused my Logitech Bluetooth mouse to become
> > > unusable.
> > >
> > > Appended is the change I need to make it work again (note that adding
> > > the
> > > device ID to unhandled_hidpp_devices[] doesn't help, so there must be
> > > some
> > > significant enough difference in how the two cases are handled in the
> > > stack).
> > >
> > > Here's what I get in the log without the patch below:
> > >
> > > [   36.710574] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> > > [   36.710592] Bluetooth: HIDP socket layer initialized
> > > [   36.724644] hid-generic 0005:046D:B016.0001: unknown main item tag
> > > 0x0
> > > [   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > > [   36.728036] input: Bluetooth Mouse M336/M337/M535 Consumer Control
> > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > > [   36.728823] input: Bluetooth Mouse M336/M337/M535 Keyboard as
> > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > > [   36.731550] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > > 9c:b6:d0:96:8e:c8
> > > [   36.833039] logitech-hidpp-device 0005:046D:B016.0001: unknown
> > > main item tag 0x0
> > > [   36.999064] logitech-hidpp-device 0005:046D:B016.0001: Device not
> > > connected
> > >
> > > and here's what I get with it:
> > >
> > > [   43.642546] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> > > [   43.642559] Bluetooth: HIDP socket layer initialized
> > > [   43.652898] hid-generic 0005:046D:B016.0001: unknown main item tag
> > > 0x0
> > > [   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > > [   43.655025] input: Bluetooth Mouse M336/M337/M535 Consumer Control
> > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > > [   43.655400] input: Bluetooth Mouse M336/M337/M535 Keyboard as
> > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > > [   43.657521] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > > 9c:b6:d0:96:8e:c8
> > >
> > > The only difference seems to be that in the former case the logitech-
> > > hidpp
> > > driver tries to bind to the device, but I guess that is expected.
> >
> > There really shouldn't be that much difference between the 2 paths,
> > except that hid-logitech-hidpp.c will check that the device supports
> > HID++ in its report descriptors, and then start talking to it to check
> > whether it's connected.
> >
> > Maybe the device doesn't support HID++?
>
> Quite possibly.
>
> > Can you try running src/tools/hidpp-list-features from
> > https://github.com/cvuchener/hidpp on the hidraw device for the mouse?
>
> OK, I'll do that.

Well, I would if I had a binary.

Otherwise, I have cmake 3.17 which apparently is too old, sorry.

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
> > work when HID++ is enabled for it,
>
> This needs the output of the hidpp-list-features tool mentioned earlier
> in the thread so we can avoid words like "evidently" and provide
> concrete proof.

Well, so point me to a binary of this, please.

> But why is it needed in this case?

Because it doesn't work otherwise.

> We purposefully try to avoid blanket
> blocklists. The lack of HID++ can be probed, so the device should work
> just as it used to (if the fallback code works).

No, because the hid-generic driver has no way to check that the probe
function of your driver fails for this particular device.  The probing
of hid-generic will fail so long as the device matches the device ID
list of any specific HID driver.  With patch [1/2] from this series
applied this is unless that specific driver has a ->match() callback
rejecting the given device.

You'd need a list of drivers that have been tried and failed somewhere
for that and AFAICS no such list is present in the code.

So a minimum fix for 6.1 that actually works for me is to add the
non-working device to the blocklist.  More sophisticated stuff can be
done later.

> We should only list devices that need special handling, and the ones
> that don't work once HID++ was probed unsuccessfully.
>
> >  so add it to the list of devices
> > that are not handled by logitech-hidpp.
> >
> > Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> > Logitech Bluetooth devices")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/hid/hid-logitech-hidpp.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
> > ===================================================================
> > --- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
> > +++ linux-pm/drivers/hid/hid-logitech-hidpp.c
> > @@ -4274,6 +4274,7 @@ static const struct hid_device_id unhand
> >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > USB_DEVICE_ID_LOGITECH_HARMONY_PS3) },
> >         /* Handled in hid-generic */
> >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD) },
> > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb016) },
> >         {}
> >  };

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Benjamin Tissoires]

On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
> > work when HID++ is enabled for it,
>
> This needs the output of the hidpp-list-features tool mentioned earlier
> in the thread so we can avoid words like "evidently" and provide
> concrete proof.
>
> But why is it needed in this case? We purposefully try to avoid blanket
> blocklists. The lack of HID++ can be probed, so the device should work
> just as it used to (if the fallback code works).

If I read the probe function correctly, we should have the HID++
reports present, so a static analysis will not allow us to detect that
information.

However, this reminds me of the Litra Glow[0]. On this device,
hidpp_root_get_protocol_version() also reports an error when it is
obvious it is connected.
And AFAICT, a BLE device is supposed to always be connected when it is
presented to the kernel (because disconnect is handled in the BLE
layer, in bluez).

Apparently Rafael's mouse is Bluetooth classic, so I have a doubt on
what happens when it goes into low power.

>
> We should only list devices that need special handling, and the ones
> that don't work once HID++ was probed unsuccessfully.
>

Given the current state of Rafael's mouse it goes into the second
category. But I suspect we should be smarter in the probe's decision
to consider if a device is connected or not.

Cheers,
Benjamin

[0] https://lore.kernel.org/linux-input/CABfF9mO3SQZvkQGOC09H5s7EEd2UGhpE=GYB46g_zF3aEOVn=Q@mail.gmail.com/

Re: [PATCH v1 1/2] HID: generic: Add ->match() check to __check_hid_generic()

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 10:27 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Dec 7, 2022 at 10:13 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Some special HID drivers (for example, hid-logitech-hidpp) use ->match()
> > callbacks to reject specific devices that otherwise would match the
> > driver's device ID list, with the expectation that those devices will
> > be handled by some other drivers.  However, this doesn't work if
> > hid-generic is expected to bind to the given device, because its
> > ->match() callback, hid_generic_match(), rejects all devices that match
> > device ID lists of the other HID drivers regardless of what is returned
> > by the other drivers' ->match() callbacks.
>
> Thanks Rafael for spotting that corner case in the ->match() processing.
>
> >
> > To make it work, amend the function used by hid_generic_match() for
> > checking an individual driver, __check_hid_generic(), with a check
> > involving the given driver's ->match() callback, so 0 is returned
> > when that callback rejects the device in question.
>
> Shouldn't we add that logic to hid_match_device() directly in
> hid-core.c instead?
> It feels wrong to have a function named "hid_match_device()" and have
> to manually call later "->match()" on the driver itself.

Well, I've followed the pattern present in hid_device_probe(), where
hid_match_device() is first called to check against the device ID list
and then ->match() is invoked later only if that doesn't fail.

Also changing hid_match_device() would change the way in which
hid_bus_match() works and that may lead to subsequent regressions,
potentially, so I'd rather avoid doing that ATM.

> Ack on the general idea anyway.

Thanks!

> >
> > Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/hid/hid-generic.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/hid/hid-generic.c
> > ===================================================================
> > --- linux-pm.orig/drivers/hid/hid-generic.c
> > +++ linux-pm/drivers/hid/hid-generic.c
> > @@ -31,7 +31,13 @@ static int __check_hid_generic(struct de
> >         if (hdrv == &hid_generic)
> >                 return 0;
> >
> > -       return hid_match_device(hdev, hdrv) != NULL;
> > +       if (!hid_match_device(hdev, hdrv))
> > +               return 0;
> > +
> > +       if (hdrv->match)
> > +               return hdrv->match(hdev, false);
> > +
> > +       return 1;
> >  }
> >
> >  static bool hid_generic_match(struct hid_device *hdev,
> >
> >
> >
>

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Bastien Nocera]

On Wed, 2022-12-07 at 10:36 +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 7, 2022 at 10:16 AM Rafael J. Wysocki <rjw@rjwysocki.net>
> wrote:
> > 
> > On Wednesday, December 7, 2022 10:04:43 AM CET Bastien Nocera
> > wrote:
> > > On Tue, 2022-12-06 at 15:58 +0100, Rafael J. Wysocki wrote:
> > > > Bastien, Jiri,
> > > > 
> > > > Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all
> > > > the
> > > > Logitech
> > > > Bluetooth devices") caused my Logitech Bluetooth mouse to
> > > > become
> > > > unusable.
> > > > 
> > > > Appended is the change I need to make it work again (note that
> > > > adding
> > > > the
> > > > device ID to unhandled_hidpp_devices[] doesn't help, so there
> > > > must be
> > > > some
> > > > significant enough difference in how the two cases are handled
> > > > in the
> > > > stack).
> > > > 
> > > > Here's what I get in the log without the patch below:
> > > > 
> > > > [   36.710574] Bluetooth: HIDP (Human Interface Emulation) ver
> > > > 1.2
> > > > [   36.710592] Bluetooth: HIDP socket layer initialized
> > > > [   36.724644] hid-generic 0005:046D:B016.0001: unknown main
> > > > item tag
> > > > 0x0
> > > > [   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > > > [   36.728036] input: Bluetooth Mouse M336/M337/M535 Consumer
> > > > Control
> > > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > > > [   36.728823] input: Bluetooth Mouse M336/M337/M535 Keyboard
> > > > as
> > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > > > [   36.731550] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > > > 9c:b6:d0:96:8e:c8
> > > > [   36.833039] logitech-hidpp-device 0005:046D:B016.0001:
> > > > unknown
> > > > main item tag 0x0
> > > > [   36.999064] logitech-hidpp-device 0005:046D:B016.0001:
> > > > Device not
> > > > connected
> > > > 
> > > > and here's what I get with it:
> > > > 
> > > > [   43.642546] Bluetooth: HIDP (Human Interface Emulation) ver
> > > > 1.2
> > > > [   43.642559] Bluetooth: HIDP socket layer initialized
> > > > [   43.652898] hid-generic 0005:046D:B016.0001: unknown main
> > > > item tag
> > > > 0x0
> > > > [   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > > > [   43.655025] input: Bluetooth Mouse M336/M337/M535 Consumer
> > > > Control
> > > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > > > [   43.655400] input: Bluetooth Mouse M336/M337/M535 Keyboard
> > > > as
> > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > > > [   43.657521] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > > > 9c:b6:d0:96:8e:c8
> > > > 
> > > > The only difference seems to be that in the former case the
> > > > logitech-
> > > > hidpp
> > > > driver tries to bind to the device, but I guess that is
> > > > expected.
> > > 
> > > There really shouldn't be that much difference between the 2
> > > paths,
> > > except that hid-logitech-hidpp.c will check that the device
> > > supports
> > > HID++ in its report descriptors, and then start talking to it to
> > > check
> > > whether it's connected.
> > > 
> > > Maybe the device doesn't support HID++?
> > 
> > Quite possibly.
> > 
> > > Can you try running src/tools/hidpp-list-features from
> > > https://github.com/cvuchener/hidpp on the hidraw device for the
> > > mouse?
> > 
> > OK, I'll do that.
> 
> Well, I would if I had a binary.
> 
> Otherwise, I have cmake 3.17 which apparently is too old, sorry.

Revert 308f240585380dd0af4d9f5bbec5eb01e103deca and it will just
require 3.12.

Or use Solaar from your distribution or one of the prebuilt packages
(https://github.com/pwr-Solaar/Solaar/#prebuilt-packages):
solaar -D /dev/hidrawX show

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Bastien Nocera]

On Wed, 2022-12-07 at 10:48 +0100, Benjamin Tissoires wrote:
> On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does
> > > not
> > > work when HID++ is enabled for it,
> > 
> > This needs the output of the hidpp-list-features tool mentioned
> > earlier
> > in the thread so we can avoid words like "evidently" and provide
> > concrete proof.
> > 
> > But why is it needed in this case? We purposefully try to avoid
> > blanket
> > blocklists. The lack of HID++ can be probed, so the device should
> > work
> > just as it used to (if the fallback code works).
> 
> If I read the probe function correctly, we should have the HID++
> reports present, so a static analysis will not allow us to detect
> that
> information.
> 
> However, this reminds me of the Litra Glow[0]. On this device,
> hidpp_root_get_protocol_version() also reports an error when it is
> obvious it is connected.

On the Litra Glow, the error isn't HIDPP_ERROR_RESOURCE_ERROR, but
HIDPP20_ERROR_UNSUPPORTED (0x09). I have a patch to add those constants
to the driver.

> And AFAICT, a BLE device is supposed to always be connected when it
> is
> presented to the kernel (because disconnect is handled in the BLE
> layer, in bluez).
> 
> Apparently Rafael's mouse is Bluetooth classic, so I have a doubt on
> what happens when it goes into low power.

It would probably just disconnect after a timeout. Reconnection isn't
as fast as with BLE, but it's fast enough.

> > We should only list devices that need special handling, and the
> > ones
> > that don't work once HID++ was probed unsuccessfully.
> > 
> 
> Given the current state of Rafael's mouse it goes into the second
> category. But I suspect we should be smarter in the probe's decision
> to consider if a device is connected or not.

Sure, and that's the data I'm trying to get out of the device.

> 
> Cheers,
> Benjamin
> 
> [0]
> https://lore.kernel.org/linux-input/CABfF9mO3SQZvkQGOC09H5s7EEd2UGhpE=GYB46g_zF3aEOVn=Q@mail.gmail.com/
> 

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Bastien Nocera]

On Wed, 2022-12-07 at 10:47 +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does
> > > not
> > > work when HID++ is enabled for it,
> > 
> > This needs the output of the hidpp-list-features tool mentioned
> > earlier
> > in the thread so we can avoid words like "evidently" and provide
> > concrete proof.
> 
> Well, so point me to a binary of this, please.
> 
> > But why is it needed in this case?
> 
> Because it doesn't work otherwise.
> 
> > We purposefully try to avoid blanket
> > blocklists. The lack of HID++ can be probed, so the device should
> > work
> > just as it used to (if the fallback code works).
> 
> No, because the hid-generic driver has no way to check that the probe
> function of your driver fails for this particular device.  The
> probing
> of hid-generic will fail so long as the device matches the device ID
> list of any specific HID driver.  With patch [1/2] from this series
> applied this is unless that specific driver has a ->match() callback
> rejecting the given device.
> 
> You'd need a list of drivers that have been tried and failed
> somewhere
> for that and AFAICS no such list is present in the code.

This code will start the generic HID stack if the device doesn't
support HID++:
        hidpp->supported_reports = hidpp_validate_device(hdev);

        if (!hidpp->supported_reports) {
                hid_set_drvdata(hdev, NULL);
                devm_kfree(&hdev->dev, hidpp);
                return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
        }

But in your case the device supports HID++ enough to go past that, but
fails to get the HID++ version, which makes the driver think it's not
connected, and just bails.

> So a minimum fix for 6.1 that actually works for me is to add the
> non-working device to the blocklist.  More sophisticated stuff can be
> done later.
> 
> > We should only list devices that need special handling, and the
> > ones
> > that don't work once HID++ was probed unsuccessfully.
> > 
> > >  so add it to the list of devices
> > > that are not handled by logitech-hidpp.
> > > 
> > > Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all
> > > the
> > > Logitech Bluetooth devices")
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c |    1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
> > > =================================================================
> > > ==
> > > --- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
> > > +++ linux-pm/drivers/hid/hid-logitech-hidpp.c
> > > @@ -4274,6 +4274,7 @@ static const struct hid_device_id unhand
> > >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_LOGITECH_HARMONY_PS3) },
> > >         /* Handled in hid-generic */
> > >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb016) },
> > >         {}
> > >  };

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Benjamin Tissoires]

On Wed, Dec 7, 2022 at 10:47 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net> wrote:
> >
> > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
> > > work when HID++ is enabled for it,
> >
> > This needs the output of the hidpp-list-features tool mentioned earlier
> > in the thread so we can avoid words like "evidently" and provide
> > concrete proof.
>
> Well, so point me to a binary of this, please.
>
> > But why is it needed in this case?
>
> Because it doesn't work otherwise.
>
> > We purposefully try to avoid blanket
> > blocklists. The lack of HID++ can be probed, so the device should work
> > just as it used to (if the fallback code works).
>
> No, because the hid-generic driver has no way to check that the probe
> function of your driver fails for this particular device.  The probing
> of hid-generic will fail so long as the device matches the device ID
> list of any specific HID driver.  With patch [1/2] from this series
> applied this is unless that specific driver has a ->match() callback
> rejecting the given device.
>
> You'd need a list of drivers that have been tried and failed somewhere
> for that and AFAICS no such list is present in the code.

That is the reason why I never wanted to enable HID++ on all Logitech
mice, and this comes back to bite us at the worst time possible, right
before the merge window opens :(

>
> So a minimum fix for 6.1 that actually works for me is to add the
> non-working device to the blocklist.  More sophisticated stuff can be
> done later.

Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I am
worried that you won't be the only one complaining we just killed
their mouse.
So I think the even wiser solution would be to delay (and so revert in
6.1 or 6.2) the 2 patches that enable hid++ on all logitech mice
(8544c812e43ab7bdf40458411b83987b8cba924d and
532223c8ac57605a10e46dc0ab23dcf01c9acb43).

Cheers,
Benjamin

>
> > We should only list devices that need special handling, and the ones
> > that don't work once HID++ was probed unsuccessfully.
> >
> > >  so add it to the list of devices
> > > that are not handled by logitech-hidpp.
> > >
> > > Fixes: 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the
> > > Logitech Bluetooth devices")
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c |    1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > Index: linux-pm/drivers/hid/hid-logitech-hidpp.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/hid/hid-logitech-hidpp.c
> > > +++ linux-pm/drivers/hid/hid-logitech-hidpp.c
> > > @@ -4274,6 +4274,7 @@ static const struct hid_device_id unhand
> > >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_LOGITECH_HARMONY_PS3) },
> > >         /* Handled in hid-generic */
> > >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_LOGITECH_DINOVO_EDGE_KBD) },
> > > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb016) },
> > >         {}
> > >  };
>

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 10:59 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2022-12-07 at 10:36 +0100, Rafael J. Wysocki wrote:
> > On Wed, Dec 7, 2022 at 10:16 AM Rafael J. Wysocki <rjw@rjwysocki.net>
> > wrote:
> > >
> > > On Wednesday, December 7, 2022 10:04:43 AM CET Bastien Nocera
> > > wrote:
> > > > On Tue, 2022-12-06 at 15:58 +0100, Rafael J. Wysocki wrote:
> > > > > Bastien, Jiri,
> > > > >
> > > > > Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all
> > > > > the
> > > > > Logitech
> > > > > Bluetooth devices") caused my Logitech Bluetooth mouse to
> > > > > become
> > > > > unusable.
> > > > >
> > > > > Appended is the change I need to make it work again (note that
> > > > > adding
> > > > > the
> > > > > device ID to unhandled_hidpp_devices[] doesn't help, so there
> > > > > must be
> > > > > some
> > > > > significant enough difference in how the two cases are handled
> > > > > in the
> > > > > stack).
> > > > >
> > > > > Here's what I get in the log without the patch below:
> > > > >
> > > > > [   36.710574] Bluetooth: HIDP (Human Interface Emulation) ver
> > > > > 1.2
> > > > > [   36.710592] Bluetooth: HIDP socket layer initialized
> > > > > [   36.724644] hid-generic 0005:046D:B016.0001: unknown main
> > > > > item tag
> > > > > 0x0
> > > > > [   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > > > > [   36.728036] input: Bluetooth Mouse M336/M337/M535 Consumer
> > > > > Control
> > > > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > > > > [   36.728823] input: Bluetooth Mouse M336/M337/M535 Keyboard
> > > > > as
> > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > > > > [   36.731550] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > > > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > > > > 9c:b6:d0:96:8e:c8
> > > > > [   36.833039] logitech-hidpp-device 0005:046D:B016.0001:
> > > > > unknown
> > > > > main item tag 0x0
> > > > > [   36.999064] logitech-hidpp-device 0005:046D:B016.0001:
> > > > > Device not
> > > > > connected
> > > > >
> > > > > and here's what I get with it:
> > > > >
> > > > > [   43.642546] Bluetooth: HIDP (Human Interface Emulation) ver
> > > > > 1.2
> > > > > [   43.642559] Bluetooth: HIDP socket layer initialized
> > > > > [   43.652898] hid-generic 0005:046D:B016.0001: unknown main
> > > > > item tag
> > > > > 0x0
> > > > > [   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse as
> > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input14
> > > > > [   43.655025] input: Bluetooth Mouse M336/M337/M535 Consumer
> > > > > Control
> > > > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input15
> > > > > [   43.655400] input: Bluetooth Mouse M336/M337/M535 Keyboard
> > > > > as
> > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input18
> > > > > [   43.657521] hid-generic 0005:046D:B016.0001: input,hidraw0:
> > > > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535] on
> > > > > 9c:b6:d0:96:8e:c8
> > > > >
> > > > > The only difference seems to be that in the former case the
> > > > > logitech-
> > > > > hidpp
> > > > > driver tries to bind to the device, but I guess that is
> > > > > expected.
> > > >
> > > > There really shouldn't be that much difference between the 2
> > > > paths,
> > > > except that hid-logitech-hidpp.c will check that the device
> > > > supports
> > > > HID++ in its report descriptors, and then start talking to it to
> > > > check
> > > > whether it's connected.
> > > >
> > > > Maybe the device doesn't support HID++?
> > >
> > > Quite possibly.
> > >
> > > > Can you try running src/tools/hidpp-list-features from
> > > > https://github.com/cvuchener/hidpp on the hidraw device for the
> > > > mouse?
> > >
> > > OK, I'll do that.
> >
> > Well, I would if I had a binary.
> >
> > Otherwise, I have cmake 3.17 which apparently is too old, sorry.
>
> Revert 308f240585380dd0af4d9f5bbec5eb01e103deca and it will just
> require 3.12.

OK

It says:

/dev/hidraw0 (device 0): Bluetooth Mouse M336/M337/M535 (046d:b016) HID++ 4.5

> Or use Solaar from your distribution or one of the prebuilt packages
> (https://github.com/pwr-Solaar/Solaar/#prebuilt-packages):
> solaar -D /dev/hidrawX show

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 11:05 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Dec 7, 2022 at 10:47 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Dec 7, 2022 at 10:29 AM Bastien Nocera <hadess@hadess.net> wrote:
> > >
> > > On Wed, 2022-12-07 at 10:12 +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Evidently, Logitech Bluetooth Mouse M336/M337/M535 (0xb016) does not
> > > > work when HID++ is enabled for it,
> > >
> > > This needs the output of the hidpp-list-features tool mentioned earlier
> > > in the thread so we can avoid words like "evidently" and provide
> > > concrete proof.
> >
> > Well, so point me to a binary of this, please.
> >
> > > But why is it needed in this case?
> >
> > Because it doesn't work otherwise.
> >
> > > We purposefully try to avoid blanket
> > > blocklists. The lack of HID++ can be probed, so the device should work
> > > just as it used to (if the fallback code works).
> >
> > No, because the hid-generic driver has no way to check that the probe
> > function of your driver fails for this particular device.  The probing
> > of hid-generic will fail so long as the device matches the device ID
> > list of any specific HID driver.  With patch [1/2] from this series
> > applied this is unless that specific driver has a ->match() callback
> > rejecting the given device.
> >
> > You'd need a list of drivers that have been tried and failed somewhere
> > for that and AFAICS no such list is present in the code.
>
> That is the reason why I never wanted to enable HID++ on all Logitech
> mice, and this comes back to bite us at the worst time possible, right
> before the merge window opens :(
>
> >
> > So a minimum fix for 6.1 that actually works for me is to add the
> > non-working device to the blocklist.  More sophisticated stuff can be
> > done later.
>
> Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I am
> worried that you won't be the only one complaining we just killed
> their mouse.
> So I think the even wiser solution would be to delay (and so revert in
> 6.1 or 6.2) the 2 patches that enable hid++ on all logitech mice
> (8544c812e43ab7bdf40458411b83987b8cba924d and
> 532223c8ac57605a10e46dc0ab23dcf01c9acb43).

Obviously that would work for me too, so it's your call.

Thanks!

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Jiri Kosina]

On Wed, 7 Dec 2022, Benjamin Tissoires wrote:

> Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I am 
> worried that you won't be the only one complaining we just killed their 
> mouse. So I think the even wiser solution would be to delay (and so 
> revert in 6.1 or 6.2) the 2 patches that enable hid++ on all logitech 
> mice (8544c812e43ab7bdf40458411b83987b8cba924d and 
> 532223c8ac57605a10e46dc0ab23dcf01c9acb43).

If we were not at -rc8 timeframe, I'd be in favor to coming up with proper 
fix still for 6.1. But as things currently are, let's just revert those 
and reschedule them with proper fix for 6.2+.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Bastien Nocera]

On Wed, 2022-12-07 at 11:07 +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 7, 2022 at 10:59 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Wed, 2022-12-07 at 10:36 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Dec 7, 2022 at 10:16 AM Rafael J. Wysocki
> > > <rjw@rjwysocki.net>
> > > wrote:
> > > > 
> > > > On Wednesday, December 7, 2022 10:04:43 AM CET Bastien Nocera
> > > > wrote:
> > > > > On Tue, 2022-12-06 at 15:58 +0100, Rafael J. Wysocki wrote:
> > > > > > Bastien, Jiri,
> > > > > > 
> > > > > > Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for
> > > > > > all
> > > > > > the
> > > > > > Logitech
> > > > > > Bluetooth devices") caused my Logitech Bluetooth mouse to
> > > > > > become
> > > > > > unusable.
> > > > > > 
> > > > > > Appended is the change I need to make it work again (note
> > > > > > that
> > > > > > adding
> > > > > > the
> > > > > > device ID to unhandled_hidpp_devices[] doesn't help, so
> > > > > > there
> > > > > > must be
> > > > > > some
> > > > > > significant enough difference in how the two cases are
> > > > > > handled
> > > > > > in the
> > > > > > stack).
> > > > > > 
> > > > > > Here's what I get in the log without the patch below:
> > > > > > 
> > > > > > [   36.710574] Bluetooth: HIDP (Human Interface Emulation)
> > > > > > ver
> > > > > > 1.2
> > > > > > [   36.710592] Bluetooth: HIDP socket layer initialized
> > > > > > [   36.724644] hid-generic 0005:046D:B016.0001: unknown
> > > > > > main
> > > > > > item tag
> > > > > > 0x0
> > > > > > [   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse
> > > > > > as
> > > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > 14
> > > > > > [   36.728036] input: Bluetooth Mouse M336/M337/M535
> > > > > > Consumer
> > > > > > Control
> > > > > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > 15
> > > > > > [   36.728823] input: Bluetooth Mouse M336/M337/M535
> > > > > > Keyboard
> > > > > > as
> > > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > 18
> > > > > > [   36.731550] hid-generic 0005:046D:B016.0001:
> > > > > > input,hidraw0:
> > > > > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535]
> > > > > > on
> > > > > > 9c:b6:d0:96:8e:c8
> > > > > > [   36.833039] logitech-hidpp-device 0005:046D:B016.0001:
> > > > > > unknown
> > > > > > main item tag 0x0
> > > > > > [   36.999064] logitech-hidpp-device 0005:046D:B016.0001:
> > > > > > Device not
> > > > > > connected
> > > > > > 
> > > > > > and here's what I get with it:
> > > > > > 
> > > > > > [   43.642546] Bluetooth: HIDP (Human Interface Emulation)
> > > > > > ver
> > > > > > 1.2
> > > > > > [   43.642559] Bluetooth: HIDP socket layer initialized
> > > > > > [   43.652898] hid-generic 0005:046D:B016.0001: unknown
> > > > > > main
> > > > > > item tag
> > > > > > 0x0
> > > > > > [   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse
> > > > > > as
> > > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > 14
> > > > > > [   43.655025] input: Bluetooth Mouse M336/M337/M535
> > > > > > Consumer
> > > > > > Control
> > > > > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > 15
> > > > > > [   43.655400] input: Bluetooth Mouse M336/M337/M535
> > > > > > Keyboard
> > > > > > as
> > > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > 18
> > > > > > [   43.657521] hid-generic 0005:046D:B016.0001:
> > > > > > input,hidraw0:
> > > > > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535]
> > > > > > on
> > > > > > 9c:b6:d0:96:8e:c8
> > > > > > 
> > > > > > The only difference seems to be that in the former case the
> > > > > > logitech-
> > > > > > hidpp
> > > > > > driver tries to bind to the device, but I guess that is
> > > > > > expected.
> > > > > 
> > > > > There really shouldn't be that much difference between the 2
> > > > > paths,
> > > > > except that hid-logitech-hidpp.c will check that the device
> > > > > supports
> > > > > HID++ in its report descriptors, and then start talking to it
> > > > > to
> > > > > check
> > > > > whether it's connected.
> > > > > 
> > > > > Maybe the device doesn't support HID++?
> > > > 
> > > > Quite possibly.
> > > > 
> > > > > Can you try running src/tools/hidpp-list-features from
> > > > > https://github.com/cvuchener/hidpp on the hidraw device for
> > > > > the
> > > > > mouse?
> > > > 
> > > > OK, I'll do that.
> > > 
> > > Well, I would if I had a binary.
> > > 
> > > Otherwise, I have cmake 3.17 which apparently is too old, sorry.
> > 
> > Revert 308f240585380dd0af4d9f5bbec5eb01e103deca and it will just
> > require 3.12.
> 
> OK
> 
> It says:
> 
> /dev/hidraw0 (device 0): Bluetooth Mouse M336/M337/M535 (046d:b016)
> HID++ 4.5

This is hidpp-list-devices, not hidpp-list-features.

Which of the 3 models above is it?

For comparison, this is what happens on my (newer Bluetooth LE/Bolt
mouse):
$ sudo ./_build/src/tools/hidpp-list-features /dev/hidraw5
Logitech Signature M650 Mouse (046d:b02a) is a HID++ 4.5 device
Feature 0x01: [0x0001] Feature set
Feature 0x02: [0x0003] Device FW version
Feature 0x03: [0x0005] Device name
Feature 0x04: [0x1d4b] Wireless device status
Feature 0x05: [0x0020] Reset
Feature 0x06: [0x0007] Device Friendly Name
Feature 0x07: [0x1004] ?
Feature 0x08: [0x1b04] Reprog controls v4
Feature 0x09: [0x1815] Hosts info
Feature 0x0a: [0x2250] ?
Feature 0x0b: [0x2130] Low-res wheel
Feature 0x0c: [0x2201] Adjustable dpi
Feature 0x0d: [0x00c3] ?
Feature 0x0e: [0x1802] Device reset (hidden, internal)
Feature 0x0f: [0x1803] ? (hidden, internal)
Feature 0x10: [0x1806] Configurable device properties (hidden,
internal)
Feature 0x11: [0x1816] ? (hidden, internal)
Feature 0x12: [0x1805] OOBState (hidden, internal)
Feature 0x13: [0x1830] ? (hidden, internal)
Feature 0x14: [0x1891] ? (hidden, internal)
Feature 0x15: [0x18a1] ? (hidden, internal)
Feature 0x16: [0x1e00] Enable hidden features (hidden)
Feature 0x17: [0x1e02] ? (hidden, internal)
Feature 0x18: [0x1e22] ? (hidden, internal)
Feature 0x19: [0x1602] ?
Feature 0x1a: [0x1eb0] ? (hidden, internal)
Feature 0x1b: [0x1861] ? (hidden, internal)
Feature 0x1c: [0x18b1] ? (hidden, internal)
Feature 0x1d: [0x920a] ? (hidden, internal)

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 11:51 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2022-12-07 at 11:07 +0100, Rafael J. Wysocki wrote:
> > On Wed, Dec 7, 2022 at 10:59 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > >
> > > On Wed, 2022-12-07 at 10:36 +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Dec 7, 2022 at 10:16 AM Rafael J. Wysocki
> > > > <rjw@rjwysocki.net>
> > > > wrote:
> > > > >
> > > > > On Wednesday, December 7, 2022 10:04:43 AM CET Bastien Nocera
> > > > > wrote:
> > > > > > On Tue, 2022-12-06 at 15:58 +0100, Rafael J. Wysocki wrote:
> > > > > > > Bastien, Jiri,
> > > > > > >
> > > > > > > Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for
> > > > > > > all
> > > > > > > the
> > > > > > > Logitech
> > > > > > > Bluetooth devices") caused my Logitech Bluetooth mouse to
> > > > > > > become
> > > > > > > unusable.
> > > > > > >
> > > > > > > Appended is the change I need to make it work again (note
> > > > > > > that
> > > > > > > adding
> > > > > > > the
> > > > > > > device ID to unhandled_hidpp_devices[] doesn't help, so
> > > > > > > there
> > > > > > > must be
> > > > > > > some
> > > > > > > significant enough difference in how the two cases are
> > > > > > > handled
> > > > > > > in the
> > > > > > > stack).
> > > > > > >
> > > > > > > Here's what I get in the log without the patch below:
> > > > > > >
> > > > > > > [   36.710574] Bluetooth: HIDP (Human Interface Emulation)
> > > > > > > ver
> > > > > > > 1.2
> > > > > > > [   36.710592] Bluetooth: HIDP socket layer initialized
> > > > > > > [   36.724644] hid-generic 0005:046D:B016.0001: unknown
> > > > > > > main
> > > > > > > item tag
> > > > > > > 0x0
> > > > > > > [   36.725860] input: Bluetooth Mouse M336/M337/M535 Mouse
> > > > > > > as
> > > > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > > 14
> > > > > > > [   36.728036] input: Bluetooth Mouse M336/M337/M535
> > > > > > > Consumer
> > > > > > > Control
> > > > > > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > > 15
> > > > > > > [   36.728823] input: Bluetooth Mouse M336/M337/M535
> > > > > > > Keyboard
> > > > > > > as
> > > > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > > 18
> > > > > > > [   36.731550] hid-generic 0005:046D:B016.0001:
> > > > > > > input,hidraw0:
> > > > > > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535]
> > > > > > > on
> > > > > > > 9c:b6:d0:96:8e:c8
> > > > > > > [   36.833039] logitech-hidpp-device 0005:046D:B016.0001:
> > > > > > > unknown
> > > > > > > main item tag 0x0
> > > > > > > [   36.999064] logitech-hidpp-device 0005:046D:B016.0001:
> > > > > > > Device not
> > > > > > > connected
> > > > > > >
> > > > > > > and here's what I get with it:
> > > > > > >
> > > > > > > [   43.642546] Bluetooth: HIDP (Human Interface Emulation)
> > > > > > > ver
> > > > > > > 1.2
> > > > > > > [   43.642559] Bluetooth: HIDP socket layer initialized
> > > > > > > [   43.652898] hid-generic 0005:046D:B016.0001: unknown
> > > > > > > main
> > > > > > > item tag
> > > > > > > 0x0
> > > > > > > [   43.653833] input: Bluetooth Mouse M336/M337/M535 Mouse
> > > > > > > as
> > > > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > > 14
> > > > > > > [   43.655025] input: Bluetooth Mouse M336/M337/M535
> > > > > > > Consumer
> > > > > > > Control
> > > > > > > as /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > > 15
> > > > > > > [   43.655400] input: Bluetooth Mouse M336/M337/M535
> > > > > > > Keyboard
> > > > > > > as
> > > > > > > /devices/pci0000:00/0000:00:14.0/usb1/1-7/1-
> > > > > > > 7:1.0/bluetooth/hci0/hci0:1/0005:046D:B016.0001/input/input
> > > > > > > 18
> > > > > > > [   43.657521] hid-generic 0005:046D:B016.0001:
> > > > > > > input,hidraw0:
> > > > > > > BLUETOOTH HID v12.03 Mouse [Bluetooth Mouse M336/M337/M535]
> > > > > > > on
> > > > > > > 9c:b6:d0:96:8e:c8
> > > > > > >
> > > > > > > The only difference seems to be that in the former case the
> > > > > > > logitech-
> > > > > > > hidpp
> > > > > > > driver tries to bind to the device, but I guess that is
> > > > > > > expected.
> > > > > >
> > > > > > There really shouldn't be that much difference between the 2
> > > > > > paths,
> > > > > > except that hid-logitech-hidpp.c will check that the device
> > > > > > supports
> > > > > > HID++ in its report descriptors, and then start talking to it
> > > > > > to
> > > > > > check
> > > > > > whether it's connected.
> > > > > >
> > > > > > Maybe the device doesn't support HID++?
> > > > >
> > > > > Quite possibly.
> > > > >
> > > > > > Can you try running src/tools/hidpp-list-features from
> > > > > > https://github.com/cvuchener/hidpp on the hidraw device for
> > > > > > the
> > > > > > mouse?
> > > > >
> > > > > OK, I'll do that.
> > > >
> > > > Well, I would if I had a binary.
> > > >
> > > > Otherwise, I have cmake 3.17 which apparently is too old, sorry.
> > >
> > > Revert 308f240585380dd0af4d9f5bbec5eb01e103deca and it will just
> > > require 3.12.
> >
> > OK
> >
> > It says:
> >
> > /dev/hidraw0 (device 0): Bluetooth Mouse M336/M337/M535 (046d:b016)
> > HID++ 4.5
>
> This is hidpp-list-devices, not hidpp-list-features.

Ah, sorry.

> Which of the 3 models above is it?

I have no idea.

> For comparison, this is what happens on my (newer Bluetooth LE/Bolt
> mouse):
> $ sudo ./_build/src/tools/hidpp-list-features /dev/hidraw5
> Logitech Signature M650 Mouse (046d:b02a) is a HID++ 4.5 device
> Feature 0x01: [0x0001] Feature set
> Feature 0x02: [0x0003] Device FW version
> Feature 0x03: [0x0005] Device name
> Feature 0x04: [0x1d4b] Wireless device status
> Feature 0x05: [0x0020] Reset
> Feature 0x06: [0x0007] Device Friendly Name
> Feature 0x07: [0x1004] ?
> Feature 0x08: [0x1b04] Reprog controls v4
> Feature 0x09: [0x1815] Hosts info
> Feature 0x0a: [0x2250] ?
> Feature 0x0b: [0x2130] Low-res wheel
> Feature 0x0c: [0x2201] Adjustable dpi
> Feature 0x0d: [0x00c3] ?
> Feature 0x0e: [0x1802] Device reset (hidden, internal)
> Feature 0x0f: [0x1803] ? (hidden, internal)
> Feature 0x10: [0x1806] Configurable device properties (hidden,
> internal)
> Feature 0x11: [0x1816] ? (hidden, internal)
> Feature 0x12: [0x1805] OOBState (hidden, internal)
> Feature 0x13: [0x1830] ? (hidden, internal)
> Feature 0x14: [0x1891] ? (hidden, internal)
> Feature 0x15: [0x18a1] ? (hidden, internal)
> Feature 0x16: [0x1e00] Enable hidden features (hidden)
> Feature 0x17: [0x1e02] ? (hidden, internal)
> Feature 0x18: [0x1e22] ? (hidden, internal)
> Feature 0x19: [0x1602] ?
> Feature 0x1a: [0x1eb0] ? (hidden, internal)
> Feature 0x1b: [0x1861] ? (hidden, internal)
> Feature 0x1c: [0x18b1] ? (hidden, internal)
> Feature 0x1d: [0x920a] ? (hidden, internal)

# hidpp-list-features /dev/hidraw1
Bluetooth Mouse M336/M337/M535 (046d:b016) is a HID++ 4.5 device
Feature 0x01: [0x0001] Feature set
Feature 0x02: [0x0003] Device FW version
Feature 0x03: [0x0005] Device name
Feature 0x04: [0x0020] Reset
Feature 0x05: [0x1e00] Enable hidden features (hidden)
Feature 0x06: [0x1800] Generic Test (hidden, internal)
Feature 0x07: [0x1000] Battery status
Feature 0x08: [0x1b04] Reprog controls v4
Feature 0x09: [0x2100] Vertical scrolling
Feature 0x0a: [0x2200] Mouse pointer
Feature 0x0b: [0x2205] Pointer speed
Feature 0x0c: [0x18b1] ? (hidden, internal)
Feature 0x0d: [0x2121] Hi-res wheel
Feature 0x0e: [0x1f03] ? (hidden, internal)

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Bastien Nocera]

On Wed, 2022-12-07 at 11:19 +0100, Jiri Kosina wrote:
> On Wed, 7 Dec 2022, Benjamin Tissoires wrote:
> 
> > Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I
> > am 
> > worried that you won't be the only one complaining we just killed
> > their 
> > mouse. So I think the even wiser solution would be to delay (and so
> > revert in 6.1 or 6.2) the 2 patches that enable hid++ on all
> > logitech 
> > mice (8544c812e43ab7bdf40458411b83987b8cba924d and 
> > 532223c8ac57605a10e46dc0ab23dcf01c9acb43).
> 
> If we were not at -rc8 timeframe, I'd be in favor to coming up with
> proper 
> fix still for 6.1. But as things currently are, let's just revert
> those 
> and reschedule them with proper fix for 6.2+.

Has anyone seen any other reports?

Because, honestly, seeing work that adds support for dozens of devices
getting tossed out at the last minute based on a single report with no
opportunity to fix the problem is really frustrating.

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 1:43 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2022-12-07 at 11:19 +0100, Jiri Kosina wrote:
> > On Wed, 7 Dec 2022, Benjamin Tissoires wrote:
> >
> > > Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I
> > > am
> > > worried that you won't be the only one complaining we just killed
> > > their
> > > mouse. So I think the even wiser solution would be to delay (and so
> > > revert in 6.1 or 6.2) the 2 patches that enable hid++ on all
> > > logitech
> > > mice (8544c812e43ab7bdf40458411b83987b8cba924d and
> > > 532223c8ac57605a10e46dc0ab23dcf01c9acb43).
> >
> > If we were not at -rc8 timeframe, I'd be in favor to coming up with
> > proper
> > fix still for 6.1. But as things currently are, let's just revert
> > those
> > and reschedule them with proper fix for 6.2+.
>
> Has anyone seen any other reports?
>
> Because, honestly, seeing work that adds support for dozens of devices
> getting tossed out at the last minute based on a single report with no
> opportunity to fix the problem is really frustrating.

Well, that's why I sent patches to address this particular case
without possibly breaking anything else.

Improvements can be made on top of them and the blocklist entry added
by patch [2/2] need not stay there forever, FWIW.

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Benjamin Tissoires]

On Wed, Dec 7, 2022 at 2:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Dec 7, 2022 at 1:43 PM Bastien Nocera <hadess@hadess.net> wrote:
> >
> > On Wed, 2022-12-07 at 11:19 +0100, Jiri Kosina wrote:
> > > On Wed, 7 Dec 2022, Benjamin Tissoires wrote:
> > >
> > > > Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I
> > > > am
> > > > worried that you won't be the only one complaining we just killed
> > > > their
> > > > mouse. So I think the even wiser solution would be to delay (and so
> > > > revert in 6.1 or 6.2) the 2 patches that enable hid++ on all
> > > > logitech
> > > > mice (8544c812e43ab7bdf40458411b83987b8cba924d and
> > > > 532223c8ac57605a10e46dc0ab23dcf01c9acb43).
> > >
> > > If we were not at -rc8 timeframe, I'd be in favor to coming up with
> > > proper
> > > fix still for 6.1. But as things currently are, let's just revert
> > > those
> > > and reschedule them with proper fix for 6.2+.
> >
> > Has anyone seen any other reports?

It's not so much about how many reports, but *what* the end result is.
If the device were working-ish, that would have been OK. But here the
device is completely ignored by the kernel which basically enters the
"no regression rule".

> >
> > Because, honestly, seeing work that adds support for dozens of devices
> > getting tossed out at the last minute based on a single report with no
> > opportunity to fix the problem is really frustrating.

I know, and I feel your pain as I was about to have the same last week
for HID-BPF. But as much as I hate dropping patches from the queue,
not being able to have at least a week to fix it properly ends up with
"fixes" that are broken and that might break other devices. Talking
from experience as my first fix from last week was exactly in that
category.

>
> Well, that's why I sent patches to address this particular case
> without possibly breaking anything else.

My concern is more that we now have a data point were the series broke
a device (pretty badly) and if (when) this happens shortly after 6.1
is getting released, we would have to say, oh yes, we know, so we need
to patch the kernel because our driver is buggy, and we knew it. This
is not acceptable, and I am sure that if Linus reads that thread he
would revert the 2 patches or maybe more.

>
> Improvements can be made on top of them and the blocklist entry added
> by patch [2/2] need not stay there forever, FWIW.
>

I need to check with Jiri, but there is a chance we can re-introduce
this in 6.2. This way we will have slightly more time to fix it in a
proper way.

Cheers,
Benjamin

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 2:25 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Dec 7, 2022 at 2:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Dec 7, 2022 at 1:43 PM Bastien Nocera <hadess@hadess.net> wrote:
> > >
> > > On Wed, 2022-12-07 at 11:19 +0100, Jiri Kosina wrote:
> > > > On Wed, 7 Dec 2022, Benjamin Tissoires wrote:
> > > >
> > > > > Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I
> > > > > am
> > > > > worried that you won't be the only one complaining we just killed
> > > > > their
> > > > > mouse. So I think the even wiser solution would be to delay (and so
> > > > > revert in 6.1 or 6.2) the 2 patches that enable hid++ on all
> > > > > logitech
> > > > > mice (8544c812e43ab7bdf40458411b83987b8cba924d and
> > > > > 532223c8ac57605a10e46dc0ab23dcf01c9acb43).
> > > >
> > > > If we were not at -rc8 timeframe, I'd be in favor to coming up with
> > > > proper
> > > > fix still for 6.1. But as things currently are, let's just revert
> > > > those
> > > > and reschedule them with proper fix for 6.2+.
> > >
> > > Has anyone seen any other reports?
>
> It's not so much about how many reports, but *what* the end result is.
> If the device were working-ish, that would have been OK. But here the
> device is completely ignored by the kernel which basically enters the
> "no regression rule".
>
> > >
> > > Because, honestly, seeing work that adds support for dozens of devices
> > > getting tossed out at the last minute based on a single report with no
> > > opportunity to fix the problem is really frustrating.
>
> I know, and I feel your pain as I was about to have the same last week
> for HID-BPF. But as much as I hate dropping patches from the queue,
> not being able to have at least a week to fix it properly ends up with
> "fixes" that are broken and that might break other devices. Talking
> from experience as my first fix from last week was exactly in that
> category.
>
> >
> > Well, that's why I sent patches to address this particular case
> > without possibly breaking anything else.
>
> My concern is more that we now have a data point were the series broke
> a device (pretty badly) and if (when) this happens shortly after 6.1
> is getting released, we would have to say, oh yes, we know, so we need
> to patch the kernel because our driver is buggy, and we knew it. This
> is not acceptable, and I am sure that if Linus reads that thread he
> would revert the 2 patches or maybe more.

Well, I agree.

> >
> > Improvements can be made on top of them and the blocklist entry added
> > by patch [2/2] need not stay there forever, FWIW.
> >
>
> I need to check with Jiri, but there is a chance we can re-introduce
> this in 6.2. This way we will have slightly more time to fix it in a
> proper way.

Sounds good to me.

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Bastien Nocera]

On Wed, 2022-12-07 at 13:43 +0100, Bastien Nocera wrote:
> On Wed, 2022-12-07 at 11:19 +0100, Jiri Kosina wrote:
> > On Wed, 7 Dec 2022, Benjamin Tissoires wrote:
> > 
> > > Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I
> > > am 
> > > worried that you won't be the only one complaining we just killed
> > > their 
> > > mouse. So I think the even wiser solution would be to delay (and
> > > so
> > > revert in 6.1 or 6.2) the 2 patches that enable hid++ on all
> > > logitech 
> > > mice (8544c812e43ab7bdf40458411b83987b8cba924d and 
> > > 532223c8ac57605a10e46dc0ab23dcf01c9acb43).
> > 
> > If we were not at -rc8 timeframe, I'd be in favor to coming up with
> > proper 
> > fix still for 6.1. But as things currently are, let's just revert
> > those 
> > and reschedule them with proper fix for 6.2+.
> 
> Has anyone seen any other reports?
> 
> Because, honestly, seeing work that adds support for dozens of
> devices
> getting tossed out at the last minute based on a single report with
> no
> opportunity to fix the problem is really frustrating.

FWIW, I went out to buy a Logitech device that uses Bluetooth Classic,
the only one I could find in 2 different shops among dozens of Logitech
devices, tested it, and it worked correctly.

Dec 07 15:17:49 classic kernel: logitech-hidpp-device 0005:046D:B342.000C: unknown main item tag 0x0
Dec 07 15:17:49 classic kernel: logitech-hidpp-device 0005:046D:B342.000C: HID++ 4.5 device connected.
Dec 07 15:17:50 classic kernel: input: Logitech Bluetooth Multi-Device Keyboard K380 as /devices/pci0000:00/0000:00:14.0/usb1/1-9/1-9:1.0/bluetooth/hci0/hci0:256/0005:046D:B342.000C/input/input36
Dec 07 15:17:50 classic kernel: logitech-hidpp-device 0005:046D:B342.000C: input,hidraw5: BLUETOOTH HID v42.01 Keyboard [Logitech Bluetooth Multi-Device Keyboard K380] on 8c:c6:81:15:0c:6f

$ sudo ./_build/src/tools/hidpp-list-features /dev/hidraw5 
Logitech Bluetooth Multi-Device Keyboard K380 (046d:b342) is a HID++ 4.5 device
Feature 0x01: [0x0001] Feature set
Feature 0x02: [0x0003] Device FW version
Feature 0x03: [0x0005] Device name
Feature 0x04: [0x0007] Device Friendly Name
Feature 0x05: [0x0020] Reset
Feature 0x06: [0x1000] Battery status
Feature 0x07: [0x1814] Change host
Feature 0x08: [0x1815] Hosts info
Feature 0x09: [0x1b04] Reprog controls v4
Feature 0x0a: [0x1e00] Enable hidden features (hidden)
Feature 0x0b: [0x40a2] New fn inversion
Feature 0x0c: [0x4220] Lock key state
Feature 0x0d: [0x4521] Keyboard disable
Feature 0x0e: [0x4531] Multiplatform

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Bastien Nocera]

On Wed, 2022-12-07 at 12:07 +0100, Rafael J. Wysocki wrote:
> # hidpp-list-features /dev/hidraw1
> Bluetooth Mouse M336/M337/M535 (046d:b016) is a HID++ 4.5 device
> Feature 0x01: [0x0001] Feature set
> Feature 0x02: [0x0003] Device FW version
> Feature 0x03: [0x0005] Device name
> Feature 0x04: [0x0020] Reset
> Feature 0x05: [0x1e00] Enable hidden features (hidden)
> Feature 0x06: [0x1800] Generic Test (hidden, internal)
> Feature 0x07: [0x1000] Battery status
> Feature 0x08: [0x1b04] Reprog controls v4
> Feature 0x09: [0x2100] Vertical scrolling
> Feature 0x0a: [0x2200] Mouse pointer
> Feature 0x0b: [0x2205] Pointer speed
> Feature 0x0c: [0x18b1] ? (hidden, internal)
> Feature 0x0d: [0x2121] Hi-res wheel
> Feature 0x0e: [0x1f03] ? (hidden, internal)

Would you be able to enable debugging for the hid subsystem to get some
debug data when getting the version from the device fails?

I can't see any problems in there that wouldn't also have impacted all
the other Logitech Bluetooth devices listed in the support devices
list.

If the problem is a timeout, maybe we should lower the timeouts we
currently have (5*HZ = 5 seconds, right?), so we can retry 5 times one
second instead.

Still, as I mentioned earlier, I can't reproduce the problem on another
Bluetooth Classic device...

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Rafael J. Wysocki]

On Wed, Dec 7, 2022 at 6:19 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2022-12-07 at 12:07 +0100, Rafael J. Wysocki wrote:
> > # hidpp-list-features /dev/hidraw1
> > Bluetooth Mouse M336/M337/M535 (046d:b016) is a HID++ 4.5 device
> > Feature 0x01: [0x0001] Feature set
> > Feature 0x02: [0x0003] Device FW version
> > Feature 0x03: [0x0005] Device name
> > Feature 0x04: [0x0020] Reset
> > Feature 0x05: [0x1e00] Enable hidden features (hidden)
> > Feature 0x06: [0x1800] Generic Test (hidden, internal)
> > Feature 0x07: [0x1000] Battery status
> > Feature 0x08: [0x1b04] Reprog controls v4
> > Feature 0x09: [0x2100] Vertical scrolling
> > Feature 0x0a: [0x2200] Mouse pointer
> > Feature 0x0b: [0x2205] Pointer speed
> > Feature 0x0c: [0x18b1] ? (hidden, internal)
> > Feature 0x0d: [0x2121] Hi-res wheel
> > Feature 0x0e: [0x1f03] ? (hidden, internal)
>
> Would you be able to enable debugging for the hid subsystem to get some
> debug data when getting the version from the device fails?

I guess I could, but I think that the device is just quirky.

At least the BT layer appears to think that it is connected.

Anyway, what exactly do you need?

> I can't see any problems in there that wouldn't also have impacted all
> the other Logitech Bluetooth devices listed in the support devices
> list.
>
> If the problem is a timeout, maybe we should lower the timeouts we
> currently have (5*HZ = 5 seconds, right?), so we can retry 5 times one
> second instead.

No, it doesn't take 5 sec to get a response from it.  It rather looks
like __hidpp_send_report() returns an error.

>
> Still, as I mentioned earlier, I can't reproduce the problem on another
> Bluetooth Classic device...

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8) #forregzbot

[Thorsten Leemhuis]

[Note: this mail contains only information for Linux kernel regression
tracking. Mails like these contain '#forregzbot' in the subject to make
then easy to spot and filter out. The author also tried to remove most
or all individuals from the list of recipients to spare them the hassle.]

On 06.12.22 15:58, Rafael J. Wysocki wrote:
> 
> Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech
> Bluetooth devices") caused my Logitech Bluetooth mouse to become unusable.
> [...]

Thanks for the report. To be sure below issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression
tracking bot:

#regzbot ^introduced 532223c8ac57
#regzbot title hid: Logitech BT mouse unusable
#regzbot monitor:
https://lore.kernel.org/all/20221207142433.1158329-1-benjamin.tissoires@redhat.com/
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.

Re: [PATCH v1 2/2] HID: logitech-hidpp: Add Bluetooth Mouse M336/M337/M535 to unhandled_hidpp_devices[]

[Benjamin Tissoires]

On Wed, Dec 7, 2022 at 3:24 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2022-12-07 at 13:43 +0100, Bastien Nocera wrote:
> > On Wed, 2022-12-07 at 11:19 +0100, Jiri Kosina wrote:
> > > On Wed, 7 Dec 2022, Benjamin Tissoires wrote:
> > >
> > > > Agree, but OTOH, Rafael, your mouse is not brand new AFAICT, so I
> > > > am
> > > > worried that you won't be the only one complaining we just killed
> > > > their
> > > > mouse. So I think the even wiser solution would be to delay (and
> > > > so
> > > > revert in 6.1 or 6.2) the 2 patches that enable hid++ on all
> > > > logitech
> > > > mice (8544c812e43ab7bdf40458411b83987b8cba924d and
> > > > 532223c8ac57605a10e46dc0ab23dcf01c9acb43).
> > >
> > > If we were not at -rc8 timeframe, I'd be in favor to coming up with
> > > proper
> > > fix still for 6.1. But as things currently are, let's just revert
> > > those
> > > and reschedule them with proper fix for 6.2+.
> >
> > Has anyone seen any other reports?
> >
> > Because, honestly, seeing work that adds support for dozens of
> > devices
> > getting tossed out at the last minute based on a single report with
> > no
> > opportunity to fix the problem is really frustrating.
>
> FWIW, I went out to buy a Logitech device that uses Bluetooth Classic,
> the only one I could find in 2 different shops among dozens of Logitech
> devices, tested it, and it worked correctly.

Again, I understand the frustration. But the problem is not so much
that we might or might not ever need another entry in that list. The
problem is that some devices were supported previously (not in a fancy
way), and now we have a chance to just disable those devices. Of
course, we could say "just rmmod hid-logitech-hidpp". I have already
been through that as well, and then you fight for 10 years on some
forums where everybody says that if you have an issue with your
touchscreen, just disable <insert any driver here> when the particular
touchscreen is *not* using that driver at all.

Anyway, let me write down my thoughts since yesterday:
1. Rafael already realized that the ->match() function was not working
outside any other driver than hid-generic (and this was the design at
the time)
2. We have an issue in hid-logitech-hidpp where during probe calling
hidpp_root_get_protocol_version() returns an error, when userspace
tools are working fine for the exact same command
3. IMO, the way hid-logitech-hidpp probe function is behaving is not
resilient enough to be able to have a generic catch-all, because there
is a non-zero chance the probe returns -ENODEV (see all the exit paths
that return -ENODEV in probe).

To solve 1, it needs a little bit of tinkering and Rafael already sent
a v1 for that. IMO we should refine it, but that's an already ongoing
process

To solve 2, Bastien already mentioned one piece of the puzzle (the
error code not being correctly reported and the signification changed
between HID++ 1.0 and 2.0). But I am still yet to understand why there
is a difference between userspace call of the function, and kernel
space.

To solve 3, I initially started to work on a simple, more resilient
probe in hid-logitech-hidpp. I thought that we could regroup all
device initialization we do in a hidpp_preinit() call, and if that
call fails, revert to the generic hid processing.

But then, looking at the bigger picture, it would make sense to not do
that exactly. Instead of returning 0 and handling the device through
hid-logitech-hidpp, maybe we should actually return -ENODEV, and have
a fallback mechanism in hid-core that says "it seems I have tried all
possible drivers, all of them failed, let's force hid-generic for this
one".

And as I type those lines, how about the cases when we actually want
to disable a USB interface from HID because it is legitimate to do so?

I'll need to think about this a little bit more.

To be able to reintroduce the bluetooth catch-all, I think we need to
solve 1 and 3. 2 would be nice to understand but is not preventing the
series from being merged back.

Cheers,
Benjamin

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Bastien Nocera]

On Wed, 2022-12-07 at 18:44 +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 7, 2022 at 6:19 PM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Wed, 2022-12-07 at 12:07 +0100, Rafael J. Wysocki wrote:
> > > # hidpp-list-features /dev/hidraw1
> > > Bluetooth Mouse M336/M337/M535 (046d:b016) is a HID++ 4.5 device
> > > Feature 0x01: [0x0001] Feature set
> > > Feature 0x02: [0x0003] Device FW version
> > > Feature 0x03: [0x0005] Device name
> > > Feature 0x04: [0x0020] Reset
> > > Feature 0x05: [0x1e00] Enable hidden features (hidden)
> > > Feature 0x06: [0x1800] Generic Test (hidden, internal)
> > > Feature 0x07: [0x1000] Battery status
> > > Feature 0x08: [0x1b04] Reprog controls v4
> > > Feature 0x09: [0x2100] Vertical scrolling
> > > Feature 0x0a: [0x2200] Mouse pointer
> > > Feature 0x0b: [0x2205] Pointer speed
> > > Feature 0x0c: [0x18b1] ? (hidden, internal)
> > > Feature 0x0d: [0x2121] Hi-res wheel
> > > Feature 0x0e: [0x1f03] ? (hidden, internal)
> > 
> > Would you be able to enable debugging for the hid subsystem to get
> > some
> > debug data when getting the version from the device fails?
> 
> I guess I could, but I think that the device is just quirky.
> 
> At least the BT layer appears to think that it is connected.
> 
> Anyway, what exactly do you need?
> 
> > I can't see any problems in there that wouldn't also have impacted
> > all
> > the other Logitech Bluetooth devices listed in the support devices
> > list.
> > 
> > If the problem is a timeout, maybe we should lower the timeouts we
> > currently have (5*HZ = 5 seconds, right?), so we can retry 5 times
> > one
> > second instead.
> 
> No, it doesn't take 5 sec to get a response from it.  It rather looks
> like __hidpp_send_report() returns an error.

Adding "debug" on the kernel command-line should be enough to get debug
out of hidpp_send_message_sync():
https://stackoverflow.com/a/63682160

Either that or turn all the dbg_hid() into hid_err() if you're going to
be compiling the kernel.

We're mainly interested in the error code from the device, as that's
what I'm guessing is caused the error to propagate.

> > Still, as I mentioned earlier, I can't reproduce the problem on
> > another
> > Bluetooth Classic device...

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8) #forregzbot

[Thorsten Leemhuis]

On 08.12.22 08:03, Thorsten Leemhuis wrote:
> [Note: this mail contains only information for Linux kernel regression
> tracking. Mails like these contain '#forregzbot' in the subject to make
> then easy to spot and filter out. The author also tried to remove most
> or all individuals from the list of recipients to spare them the hassle.]
> 
> On 06.12.22 15:58, Rafael J. Wysocki wrote:
>>
>> Commit 532223c8ac57 ("HID: logitech-hidpp: Enable HID++ for all the Logitech
>> Bluetooth devices") caused my Logitech Bluetooth mouse to become unusable.
>> [...]
> 
> Thanks for the report. To be sure below issue doesn't fall through the
> cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression
> tracking bot:
> 
> #regzbot ^introduced 532223c8ac57
> #regzbot title hid: Logitech BT mouse unusable
> #regzbot monitor:
> https://lore.kernel.org/all/20221207142433.1158329-1-benjamin.tissoires@redhat.com/
> #regzbot ignore-activity

#regzbot fix: a9d9e46c755a1

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Bastien Nocera]

On Thu, 2022-12-08 at 16:20 +0100, Bastien Nocera wrote:
> On Wed, 2022-12-07 at 18:44 +0100, Rafael J. Wysocki wrote:
> > On Wed, Dec 7, 2022 at 6:19 PM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > > 
> > > On Wed, 2022-12-07 at 12:07 +0100, Rafael J. Wysocki wrote:
> > > > # hidpp-list-features /dev/hidraw1
> > > > Bluetooth Mouse M336/M337/M535 (046d:b016) is a HID++ 4.5
> > > > device
> > > > Feature 0x01: [0x0001] Feature set
> > > > Feature 0x02: [0x0003] Device FW version
> > > > Feature 0x03: [0x0005] Device name
> > > > Feature 0x04: [0x0020] Reset
> > > > Feature 0x05: [0x1e00] Enable hidden features (hidden)
> > > > Feature 0x06: [0x1800] Generic Test (hidden, internal)
> > > > Feature 0x07: [0x1000] Battery status
> > > > Feature 0x08: [0x1b04] Reprog controls v4
> > > > Feature 0x09: [0x2100] Vertical scrolling
> > > > Feature 0x0a: [0x2200] Mouse pointer
> > > > Feature 0x0b: [0x2205] Pointer speed
> > > > Feature 0x0c: [0x18b1] ? (hidden, internal)
> > > > Feature 0x0d: [0x2121] Hi-res wheel
> > > > Feature 0x0e: [0x1f03] ? (hidden, internal)
> > > 
> > > Would you be able to enable debugging for the hid subsystem to
> > > get
> > > some
> > > debug data when getting the version from the device fails?
> > 
> > I guess I could, but I think that the device is just quirky.
> > 
> > At least the BT layer appears to think that it is connected.
> > 
> > Anyway, what exactly do you need?
> > 
> > > I can't see any problems in there that wouldn't also have
> > > impacted
> > > all
> > > the other Logitech Bluetooth devices listed in the support
> > > devices
> > > list.
> > > 
> > > If the problem is a timeout, maybe we should lower the timeouts
> > > we
> > > currently have (5*HZ = 5 seconds, right?), so we can retry 5
> > > times
> > > one
> > > second instead.
> > 
> > No, it doesn't take 5 sec to get a response from it.  It rather
> > looks
> > like __hidpp_send_report() returns an error.
> 
> Adding "debug" on the kernel command-line should be enough to get
> debug
> out of hidpp_send_message_sync():
> https://stackoverflow.com/a/63682160
> 
> Either that or turn all the dbg_hid() into hid_err() if you're going
> to
> be compiling the kernel.
> 
> We're mainly interested in the error code from the device, as that's
> what I'm guessing is caused the error to propagate.

Can you also check whether you had:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?id=8b7e58409b1813c58eea542d9f3b8db35b4ac1f7
in your git tree?

Would be great to know whether that commit helps at all.

Cheers

> > > Still, as I mentioned earlier, I can't reproduce the problem on
> > > another
> > > Bluetooth Classic device...
> 

Re: [Regression] Logitech BT mouse unusable after commit 532223c8ac57 (still in 6.1-rc8)

[Rafael J. Wysocki]

On Tue, Dec 13, 2022 at 5:14 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Thu, 2022-12-08 at 16:20 +0100, Bastien Nocera wrote:
> > On Wed, 2022-12-07 at 18:44 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Dec 7, 2022 at 6:19 PM Bastien Nocera <hadess@hadess.net>
> > > wrote:
> > > >
> > > > On Wed, 2022-12-07 at 12:07 +0100, Rafael J. Wysocki wrote:
> > > > > # hidpp-list-features /dev/hidraw1
> > > > > Bluetooth Mouse M336/M337/M535 (046d:b016) is a HID++ 4.5
> > > > > device
> > > > > Feature 0x01: [0x0001] Feature set
> > > > > Feature 0x02: [0x0003] Device FW version
> > > > > Feature 0x03: [0x0005] Device name
> > > > > Feature 0x04: [0x0020] Reset
> > > > > Feature 0x05: [0x1e00] Enable hidden features (hidden)
> > > > > Feature 0x06: [0x1800] Generic Test (hidden, internal)
> > > > > Feature 0x07: [0x1000] Battery status
> > > > > Feature 0x08: [0x1b04] Reprog controls v4
> > > > > Feature 0x09: [0x2100] Vertical scrolling
> > > > > Feature 0x0a: [0x2200] Mouse pointer
> > > > > Feature 0x0b: [0x2205] Pointer speed
> > > > > Feature 0x0c: [0x18b1] ? (hidden, internal)
> > > > > Feature 0x0d: [0x2121] Hi-res wheel
> > > > > Feature 0x0e: [0x1f03] ? (hidden, internal)
> > > >
> > > > Would you be able to enable debugging for the hid subsystem to
> > > > get
> > > > some
> > > > debug data when getting the version from the device fails?
> > >
> > > I guess I could, but I think that the device is just quirky.
> > >
> > > At least the BT layer appears to think that it is connected.
> > >
> > > Anyway, what exactly do you need?
> > >
> > > > I can't see any problems in there that wouldn't also have
> > > > impacted
> > > > all
> > > > the other Logitech Bluetooth devices listed in the support
> > > > devices
> > > > list.
> > > >
> > > > If the problem is a timeout, maybe we should lower the timeouts
> > > > we
> > > > currently have (5*HZ = 5 seconds, right?), so we can retry 5
> > > > times
> > > > one
> > > > second instead.
> > >
> > > No, it doesn't take 5 sec to get a response from it.  It rather
> > > looks
> > > like __hidpp_send_report() returns an error.
> >
> > Adding "debug" on the kernel command-line should be enough to get
> > debug
> > out of hidpp_send_message_sync():
> > https://stackoverflow.com/a/63682160
> >
> > Either that or turn all the dbg_hid() into hid_err() if you're going
> > to
> > be compiling the kernel.
> >
> > We're mainly interested in the error code from the device, as that's
> > what I'm guessing is caused the error to propagate.
>
> Can you also check whether you had:
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?id=8b7e58409b1813c58eea542d9f3b8db35b4ac1f7
> in your git tree?
>
> Would be great to know whether that commit helps at all.

No, it's not present in the kernels I've tested so far.