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

[sashiko-bot@kernel.org]

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