[PATCH] HID: logitech-hidpp: sync wheel multiplier on wheel mode changes

[PATCH] HID: logitech-hidpp: sync wheel multiplier on wheel mode changes

[Lauri Saurus]

The hid-logitech-hidpp driver enables high resolution scrolling on
device connect for capable HID++ 2.0 devices. Driver also reads the
wheel capability and caches the returned high resolution wheel scroll
multiplier, that is used for scroll scaling when handling wheel scroll
events.

Wheel mode can also be set externally through HID++ requests, which
can leave the cached multiplier stale and cause incorrect scroll
scaling. If external SetWheelMode HID++ request sets the mode to
low resolution, the cached multiplier is not updated accordingly. This
causes extremely slow scrolling since driver expects multiple wheel
scroll events per detent but is only getting one.

The fix listens for HID++ SetWheelMode request responses and updates
the wheel scroll multiplier based on the set high resolution scroll
mode. The fix has been tested with Logitech G502X lightspeed mouse.

Signed-off-by: Lauri Saurus <saurla@saurla.com>
---
 drivers/hid/hid-logitech-hidpp.c | 37 ++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ccbf28869a96..e80c4aa6115b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -207,6 +207,9 @@ struct hidpp_device {
 
 	u8 wireless_feature_index;
 
+	int hires_wheel_multiplier;
+	u8 hires_wheel_feature_index;
+
 	bool connected_once;
 };
 
@@ -3710,6 +3713,7 @@ static int hi_res_scroll_enable(struct hidpp_device *hidpp)
 		multiplier = 1;
 	}
 
+	hidpp->hires_wheel_multiplier = multiplier;
 	hidpp->vertical_wheel_counter.wheel_multiplier = multiplier;
 	hid_dbg(hidpp->hid_dev, "wheel multiplier = %d\n", multiplier);
 	return 0;
@@ -3720,6 +3724,7 @@ static int hidpp_initialize_hires_scroll(struct hidpp_device *hidpp)
 	int ret;
 	unsigned long capabilities;
 
+	hidpp->hires_wheel_feature_index = 0xff;
 	capabilities = hidpp->capabilities;
 
 	if (hidpp->protocol_major >= 2) {
@@ -3729,6 +3734,7 @@ static int hidpp_initialize_hires_scroll(struct hidpp_device *hidpp)
 					     &feature_index);
 		if (!ret) {
 			hidpp->capabilities |= HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL;
+			hidpp->hires_wheel_feature_index = feature_index;
 			hid_dbg(hidpp->hid_dev, "Detected HID++ 2.0 hi-res scroll wheel\n");
 			return 0;
 		}
@@ -3751,6 +3757,31 @@ static int hidpp_initialize_hires_scroll(struct hidpp_device *hidpp)
 	return 0;
 }
 
+static int hidpp20_hires_wheel_raw_event(struct hidpp_device *hidpp,
+						u8 *data, int size)
+{
+	if (hidpp->hires_wheel_feature_index == 0xff)
+		return 0;
+
+	if (size < 5)
+		return 0;
+
+	if (data[0] != REPORT_ID_HIDPP_LONG ||
+	    data[2] != hidpp->hires_wheel_feature_index)
+		return 0;
+
+	if ((data[3] & 0xf0) == CMD_HIRES_WHEEL_SET_WHEEL_MODE) {
+		u8 mode = data[4];
+		bool hires = (mode & 0x02) != 0;
+		int new_multiplier = (hires && hidpp->hires_wheel_multiplier > 0)
+			? hidpp->hires_wheel_multiplier : 1;
+		hidpp->vertical_wheel_counter.wheel_multiplier = new_multiplier;
+		return 1;
+	}
+
+	return 0;
+}
+
 /* -------------------------------------------------------------------------- */
 /* Generic HID++ devices                                                      */
 /* -------------------------------------------------------------------------- */
@@ -3947,6 +3978,12 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 			return ret;
 	}
 
+	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL) {
+		ret = hidpp20_hires_wheel_raw_event(hidpp, data, size);
+		if (ret != 0)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.54.0

Re: [PATCH] HID: logitech-hidpp: sync wheel multiplier on wheel mode changes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The logic incorrectly assumes that the device's response to the `CMD_HIRES_WHEEL_SET_WHEEL_MODE` command will echo the requested mode in its payload (`data[4]`), violating HID++ 2.0 protocol behavior.
- [Medium] The function `hidpp20_hires_wheel_raw_event` strictly checks for `REPORT_ID_HIDPP_LONG` (0x11), completely ignoring responses sent via Very Long (0x12) reports.
- [Low] Data race on `wheel_multiplier` and `hires_wheel_multiplier` due to unprotected concurrent accesses across interrupt and process contexts.
--

commit 83eeb25697adad15bd7ac874fb7119dc29516cf1
Author: Lauri Saurus <saurla@saurla.com>

HID: logitech-hidpp: sync wheel multiplier on wheel mode changes
    
The hid-logitech-hidpp driver enables high resolution scrolling on
device connect for capable HID++ 2.0 devices. Driver also reads the
wheel capability and caches the returned high resolution wheel scroll
multiplier, that is used for scroll scaling when handling wheel scroll
events.

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index b1330d23bd2d0..3afd8caf02911 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c

[ ... ]

> +static int hidpp20_hires_wheel_raw_event(struct hidpp_device *hidpp,
> +						u8 *data, int size)
> +{
> +	if (hidpp->hires_wheel_feature_index == 0xff)
> +		return 0;
> +
> +	if (size < 5)
> +		return 0;
> +
> +	if (data[0] != REPORT_ID_HIDPP_LONG ||
> +	    data[2] != hidpp->hires_wheel_feature_index)
> +		return 0;

Does this code need to handle VERY_LONG reports as well?

Userspace tools or newer devices might transmit FAP commands using
REPORT_ID_HIDPP_VERY_LONG (0x12). If the request is a VERY_LONG report, the
response will also be VERY_LONG, and this check would drop it, failing to
update the multiplier.

> +
> +	if ((data[3] & 0xf0) == CMD_HIRES_WHEEL_SET_WHEEL_MODE) {
> +		u8 mode = data[4];
> +		bool hires = (mode & 0x02) != 0;

Does the HID++ 2.0 protocol guarantee that SET command responses will echo
the requested mode in the payload?

In the FAP protocol, responses to SET commands often return an empty
(zero-filled) payload. If the device returns a zero payload, data[4] would
be 0x00. The code would then evaluate hires = false and forcefully set the
multiplier to 1, even if high-resolution mode was successfully requested.

> +		int new_multiplier = (hires && hidpp->hires_wheel_multiplier > 0)
> +			? hidpp->hires_wheel_multiplier : 1;
> +		hidpp->vertical_wheel_counter.wheel_multiplier = new_multiplier;

Is there a potential data race on wheel_multiplier and
hires_wheel_multiplier here?

Looking at hi_res_scroll_enable(), it writes to these variables from
process context via the workqueue. Concurrently, this event handler reads
and writes to them from the HID raw event delivery path in interrupt context.
There does not appear to be any synchronization protecting these shared
variables across contexts.

> +		return 1;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518192649.245691-1-saurla@saurla.com?part=1