Re: [PATCH] HID: logitech-hidpp: do not overwrite the device's hi-res wheel mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid: incorrect caching of hi-res wheel multiplier in low-res mode
--

commit 243fa986b83024ae16437c2e5c919019982c426b
Author: Roman Stingler <roman.stingler@gmail.com>

HID: logitech-hidpp: do not overwrite the device's hi-res wheel mode
    
This commit modifies hi_res_scroll_enable() to read the current wheel mode
from the device instead of always forcing high-resolution mode on connect.
This prevents the driver from overwriting a persistent mode configured by
userspace.
    
Link: https://lore.kernel.org/all/20260920094508.39682-1-roman.stingler@gmail.com/

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 493763a12518e..338477bfcaebe 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c

[ ... ]

> @@ -3910,8 +3910,16 @@ static int hi_res_scroll_enable(struct hidpp_device *hidpp)
>  	u8 multiplier = 1;
>  
>  	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL) {
> -		ret = hidpp_hrw_set_wheel_mode(hidpp, false, true, false);
> -		if (ret == 0)
> +		u8 mode;
> +
> +		/*
> +		 * The wheel mode is persistent state in the device, so read it
> +		 * rather than overwriting it, and scale to match. A device
> +		 * left in hi-res still gets the multiplier it needs; one the
> +		 * user configured for low resolution is left alone.
> +		 */
> +		ret = hidpp_hrw_get_wheel_mode(hidpp, &mode);
> +		if (ret == 0 && (mode & BIT(1)))
>  			ret = hidpp_hrw_get_wheel_capability(hidpp, &multiplier);

[Severity: High]
Does skipping the capability query when in low-resolution mode introduce a
regression if the device is later switched to high-resolution mode?

If the device is probed in low-resolution mode, mode & BIT(1) is false and
hidpp_hrw_get_wheel_capability() is skipped. The local variable multiplier
remains 1.

Later in hi_res_scroll_enable(), this fallback value is cached permanently:

    hidpp->hires_wheel_multiplier = multiplier;
    hidpp->vertical_wheel_counter.wheel_multiplier = multiplier;

If a userspace configuration tool (such as Solaar) later switches the device
to high-resolution mode via HID++ commands, the driver will use this cached
multiplier of 1 instead of the hardware's true capability.

Since the kernel relies on hires_wheel_multiplier to scale physical
high-resolution wheel ticks, a multiplier of 1 causes each physical sub-detent
tick to be incorrectly reported as a full 120-unit detent, making the scroll
wheel move wildly fast.

Should the true high-resolution multiplier capability be queried and stored
regardless of the current active mode?

>  	} else if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL) {
>  		ret = hidpp_hrs_set_highres_scrolling_mode(hidpp, true,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923181203.422097-1-roman.stingler@gmail.com?part=1