Received-SPF: pass (google.com: domain of linux-kernel+bounces-145713-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <fe2980e3-3204-4572-9c7c-1e960727e1d4@redhat.com>
Date: Mon, 15 Apr 2024 20:31:14 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Logitech Anywhere 3SB support
To: Benjamin Tissoires <bentiss@kernel.org>, kde@carewolf.com
Cc: lains@riseup.net, hadess@hadess.net, jikos@kernel.org,
 benjamin.tissoires@redhat.com, linux-input@vger.kernel.org,
 linux-kernel@vger.kernel.org, Allan Sandfeld Jensen <allan.jensen@qt.io>
References: <20240413095453.14816-1-kde@carewolf.com>
 <ntsifcsfo5i6xisxbgfjdpe4uenygqxrt3v5sceflgipznw6cb@gnhvkjmglrtg>
Content-Language: en-US, nl
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <ntsifcsfo5i6xisxbgfjdpe4uenygqxrt3v5sceflgipznw6cb@gnhvkjmglrtg>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Hi,

On 4/15/24 5:54 PM, Benjamin Tissoires wrote:
> [Ccing Hans as well for input]
> 
> On Apr 13 2024, kde@carewolf.com wrote:
>> From: Allan Sandfeld Jensen <allan.jensen@qt.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?

FWIW I'm also not in favor of stretching drivers/hid/hid-logitech-dj.c
even further to also support the new bolt stuff.

AFAIK the new bolt stuff is significantly different.

Allan, I see in your other reply that you are mainly after
highres scrolling and since the bolt receiver does not do
per paired device addressing I wonder if you cannot just
get that by treating the bolt receiver as a wired HIDPP
device and just directly listing it as such in
hid-logitech-hidpp.c ?

The whole purpose of hid-logitech-dj.c is to create 1 virtual
hidpp devices per paired device and with bolt that is not
possible, so I think that we should circumvent hid-logitech-dj.c
for bolt and if we want to use any hidpp features do so
by directly listing the receivers in hid-logitech-hidpp.c .

Regards,

Hans


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