linux-kernel - Re: [PATCH] Logitech Anywhere 3SB support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ntsifcsfo5i6xisxbgfjdpe4uenygqxrt3v5sceflgipznw6cb@gnhvkjmglrtg>
Date: Mon, 15 Apr 2024 17:54:57 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: kde@...ewolf.com
Cc: lains@...eup.net, hadess@...ess.net, jikos@...nel.org, 
	benjamin.tissoires@...hat.com, linux-input@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Hans de Goede <hdegoede@...hat.com>, Allan Sandfeld Jensen <allan.jensen@...io>
Subject: Re: [PATCH] Logitech Anywhere 3SB support

[Ccing Hans as well for input]

On Apr 13 2024, kde@...ewolf.com wrote:
> From: Allan Sandfeld Jensen <allan.jensen@...io>
> 

FWIW, this patch neesd a commit description and signed-offs

> ---
>  drivers/hid/hid-ids.h            |  1 +
>  drivers/hid/hid-logitech-dj.c    | 10 +++++++++-
>  drivers/hid/hid-logitech-hidpp.c |  2 ++
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 2235d78784b1..4b79c4578d32 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -849,6 +849,7 @@
>  #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1	0xc539
>  #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1	0xc53f
>  #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_POWERPLAY	0xc53a
> +#define USB_DEVICE_ID_LOGITECH_BOLT_RECEIVER		0xc548
>  #define USB_DEVICE_ID_SPACETRAVELLER	0xc623
>  #define USB_DEVICE_ID_SPACENAVIGATOR	0xc626
>  #define USB_DEVICE_ID_DINOVO_DESKTOP	0xc704
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index c358778e070b..92b41ae5a47c 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -120,6 +120,7 @@ enum recvr_type {
>  	recvr_type_27mhz,
>  	recvr_type_bluetooth,
>  	recvr_type_dinovo,
> +	recvr_type_bolt,

I am *really* hesitant in integrating the bolt receiver into
logitech-dj.ko:
- the bolt receiver is not capable of making the distinction between the
  source of the events (so only one mouse/keyboard can be used at the
  time)
- we still have a couple of outstanding and impossible to debug issues
  with some high resolution mice connected over the unifying receiver,
  and adding one more receiver makes me nervous
- I have a strong feeling by reading the code that the keyboard part
  will fail (there is a comment "For the keyboard, we can reuse the same
  report by using the second byte which is constant in the USB HID
  report descriptor." though I can't seem to find this constant report
  on the bolt receiver)
- what are the benefits of adding it?
- will it break fwupd?

>  };
>  
>  struct dj_report {
> @@ -1068,6 +1069,7 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev,
>  		workitem.reports_supported |= STD_KEYBOARD;
>  		break;
>  	case 0x0f:
> +	case 0x10:
>  	case 0x11:
>  		device_type = "eQUAD Lightspeed 1.2";
>  		logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem);
> @@ -1430,7 +1432,8 @@ static int logi_dj_ll_parse(struct hid_device *hid)
>  		dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n",
>  			__func__, djdev->reports_supported);
>  		if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp ||
> -		    djdev->dj_receiver_dev->type == recvr_type_mouse_only)
> +		    djdev->dj_receiver_dev->type == recvr_type_mouse_only ||
> +		    djdev->dj_receiver_dev->type == recvr_type_bolt)
>  			rdcat(rdesc, &rsize, mse_high_res_descriptor,
>  			      sizeof(mse_high_res_descriptor));
>  		else if (djdev->dj_receiver_dev->type == recvr_type_27mhz)
> @@ -1773,6 +1776,7 @@ static int logi_dj_probe(struct hid_device *hdev,
>  	case recvr_type_dj:		no_dj_interfaces = 3; break;
>  	case recvr_type_hidpp:		no_dj_interfaces = 2; break;
>  	case recvr_type_gaming_hidpp:	no_dj_interfaces = 3; break;
> +	case recvr_type_bolt:		no_dj_interfaces = 4; break;

4? The device I have here only has 3 (unless I misremember how this is
supposed to be working).

>  	case recvr_type_mouse_only:	no_dj_interfaces = 2; break;
>  	case recvr_type_27mhz:		no_dj_interfaces = 2; break;
>  	case recvr_type_bluetooth:	no_dj_interfaces = 2; break;
> @@ -1950,6 +1954,10 @@ static const struct hid_device_id logi_dj_receivers[] = {
>  	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>  		USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2),
>  	 .driver_data = recvr_type_dj},
> +	{ /* Logitech bolt receiver (0xc548) */
> +	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> +		USB_DEVICE_ID_LOGITECH_BOLT_RECEIVER),
> +	 .driver_data = recvr_type_bolt},
>  
>  	{ /* Logitech Nano mouse only receiver (0xc52f) */
>  	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 3c00e6ac8e76..509142982daa 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -4380,6 +4380,8 @@ static const struct hid_device_id hidpp_devices[] = {
>  	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb023) },
>  	{ /* MX Master 3S mouse over Bluetooth */
>  	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb034) },
> +	{ /* MX Anywhere 3SB mouse over Bluetooth */
> +	  HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb038) },

That I can accept, however know that there is a regression in bluez
0.73[0] (but it should be fixed in 0.74)

Cheers,
Benjamin


[0] https://github.com/bluez/bluez/issues/778#issuecomment-2048870358

>  	{}
>  };
>  
> -- 
> 2.39.2
>