Re: [PATCH v2 12/33] HID: logitech-hidpp: mark device online on unified battery 0x1004 events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] HID: logitech-hidpp: 0x1000/0x1004 collision in
  hidpp20_battery_event_1004()
--

commit ac0aae22ff219439085bc6982cf3dda4b4fdc13a
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

HID: logitech-hidpp: mark device online on unified battery 0x1004 events

This commit marks hidpp->battery.online as true and includes
!hidpp->battery.online in the changed check when handling unified battery
status events in hidpp20_battery_event_1004().

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 28402baf04fb6..f717f03084548 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -1761,12 +1761,15 @@ static int hidpp20_battery_event_1004(struct hidpp_device *hidpp,
>  	status = hidpp20_unifiedbattery_map_status(hidpp, params[2], params[3]);
>  	level = hidpp20_unifiedbattery_map_level(hidpp, params[1]);
>  
> -	changed = status != hidpp->battery.status ||
> +	changed = !hidpp->battery.online ||
> +		  status != hidpp->battery.status ||
>  		  (state_of_charge != hidpp->battery.capacity &&
>  		   hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE) ||
>  		  (level != hidpp->battery.level &&
>  		   hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS);
>  
> +	hidpp->battery.online = true;
> +
>  	if (changed) {
>  		hidpp->battery.capacity = state_of_charge;
>  		hidpp->battery.status = status;

[Severity: High]
Does this cause a regression on feature 0x1000 devices by overwriting
hidpp->battery when a charging event arrives, and corrupt 0x1004 events by
running hidpp20_battery_event_1000() first?

Both feature 0x1000 and feature 0x1004 store their index in
hidpp->battery.feature_index and use event ID 0x00, and
hidpp_raw_hidpp_event() calls both handlers unconditionally:

drivers/hid/hid-logitech-hidpp.c:hidpp_raw_hidpp_event() {
	...
	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
		ret = hidpp20_battery_event_1000(hidpp, data, size);
		if (ret != 0)
			return ret;
		ret = hidpp20_battery_event_1004(hidpp, data, size);
		if (ret != 0)
			return ret;
	...
}

On a 0x1000 device, a charging report sets hidpp->battery.online to false
in hidpp20_battery_event_1000() and returns 0.  With this change,
hidpp20_battery_event_1004() then sees !hidpp->battery.online, forces
changed to true, sets hidpp->battery.online back to true, and overwrites
capacity, status, and level using 0x1004 decoding, which also prevents the
0 to 1 online transition needed to schedule reset_hi_res_work when the
charging cable is unplugged.

Conversely, on a 0x1004 device, hidpp20_battery_event_1000() runs first on
every 0x1004 event and mutates hidpp->battery.online before
hidpp20_battery_event_1004() checks !hidpp->battery.online, setting online
to false on every charging event and true on discharging or full reconnect
events.

Gating hidpp20_battery_event_1000() and hidpp20_battery_event_1004() on
HIDPP_CAPABILITY_UNIFIED_BATTERY would avoid both handlers matching the
same event.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-hid-logitech-hidpp-v2-0-30743e91d83b@gmail.com?part=12