Re: [PATCH 2/2] HID: logitech-hidpp: fix Bolt wheel mode handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid-logitech-hidpp: high-resolution scrolling is no longer enabled by default
- [Medium] hid-logitech-hidpp: failed capability read permanently breaks scroll wheel
--

commit ca5a9ddace3102c7dc2ef5b518171ea6cf49e055
Author: Erik Håkansson <erikhakan@gmail.com>

HID: logitech-hidpp: fix Bolt wheel mode handling

This patch modifies how high-resolution wheel settings are handled for Bolt
devices in the HID++ driver. It preserves the device's user-selected resolution
and inversion settings while enabling HID++ notifications for Bolt. It also
tracks diverted wheel events to apply the correct multiplier for high- and
low-resolution events.

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 166980be57b7c..55e9ac5e5ab78 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c

[ ... ]

> @@ -3912,16 +3939,43 @@ static int hi_res_scroll_enable(struct hidpp_device *hidpp)
>  {
>  	int ret;
>  	u8 multiplier = 1;
> +	bool high_resolution = true;
> +	const bool is_bolt = hidpp_is_bolt_child(hidpp->hid_dev);
> +
> +	if (is_bolt)
> +		hidpp->hires_wheel_diverted = false;
>  
>  	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL) {
> -		bool use_hidpp = hidpp_is_bolt_child(hidpp->hid_dev);
> +		u8 mode;
> +
> +		/* Preserve the persistent mode, except for Bolt event routing. */
> +		ret = hidpp_hrw_get_wheel_mode(hidpp, &mode);
> +		if (ret == 0) {
> +			bool invert = mode & HIRES_WHEEL_MODE_INVERT;
> +			bool use_hidpp = mode & HIRES_WHEEL_MODE_USE_HIDPP;
> +
> +			high_resolution = mode & HIRES_WHEEL_MODE_HIGH_RESOLUTION;
> +
> +			/* Preserve user-set resolution and inversion; enable HID++ for Bolt. */
> +			if (is_bolt && high_resolution && !use_hidpp) {
> +				ret = hidpp_hrw_set_wheel_mode(hidpp, invert,
> +							       high_resolution, true);
> +				if (ret == 0)
> +					use_hidpp = true;
> +			}

[Severity: High]
Does this code intentionally drop the default enablement of high-resolution
scrolling for non-Bolt devices and low-resolution Bolt devices?

The previous code unconditionally enabled high-resolution mode by calling
hidpp_hrw_set_wheel_mode(hidpp, false, true, use_hidpp). The new code appears
to remove this entirely for non-Bolt devices, and only enables it for Bolt
devices if they are already in high-res mode (is_bolt && high_resolution &&
!use_hidpp).

Could this permanently disable high-resolution scrolling out of the box for
these devices?

>  
> -		ret = hidpp_hrw_set_wheel_mode(hidpp, false, true, use_hidpp);
> -		if (ret == 0)
> -			ret = hidpp_hrw_get_wheel_capability(hidpp, &multiplier);
> +			if (ret == 0)
> +				ret = hidpp_hrw_get_wheel_capability(hidpp, &multiplier);
> +			if (ret == 0)
> +				hidpp->hires_wheel_diverted = use_hidpp;

[Severity: Medium]
What happens if hidpp_hrw_get_wheel_capability() fails due to an intermittent
RF timeout or IO error?

If the hardware is already in use_hidpp mode (or was successfully set to it),
but the capability read fails, hidpp->hires_wheel_diverted will not be
updated to match the hardware state.

Could this leave the driver desynchronized from the hardware, causing the raw
event handler to permanently drop subsequent HID++ wheel events?

> +		}
>  	} else if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922-feature-bolt-fix-v1-0-63b0fa8da0d3@gmail.com?part=2