Re: [PATCH 10/15] HID: hid-oxp: handle controller reinitialization across suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Late monocolor acknowledgments trigger spurious MCU reinitializations.
- [Low] Unnecessary 200ms delay when the transport write fails.
--

commit 0d410b2c1077d136f271fbc3070448d1e53eb0df
Author: Andrei Aldea <andrei1998@gmail.com>

HID: hid-oxp: handle controller reinitialization across suspend

This commit attempts to fix spurious MCU resets caused by confusing a monocolor
acknowledgment with an asynchronous MCU reset notification. It tracks monocolor
writes and consumes the matching acknowledgment. It also drains work queues on
suspend and re-enables them on resume, and queues a fallback reinitialization
after a set interval.

> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 7b366878fcb3e..e26e6a98294fe 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
[ ... ]
> @@ -417,11 +425,26 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
>  	if (data[0] != OXP_FID_GEN2_STATUS_EVENT)
>  		return 0;
>  
> +	/* A monocolor acknowledgment is not an MCU reset notification. */
> +	if (data[5] == OXP_STATUS_ACK) {
> +		scoped_guard(spinlock_irqsave, &cfg->rgb_reply_lock) {
> +			if (cfg->rgb_reply_pending &&
> +			    data[3] == cfg->rgb_reply_command &&
> +			    data[4] == cfg->rgb_reply_zone) {
> +				cfg->rgb_reply_pending = false;
> +				solicited = true;
> +			}
> +		}
> +		if (solicited)
> +			return 0;
> +	}
> +
>  	/* Sent ~6s after resume event, indicating the MCU has fully reset.
>  	 * Re-apply our settings after this has been received.
>  	 */
>  	if (data[3] == OXP_EFFECT_MONO_TRUE) {

[Severity: Medium]
Will this logic misidentify a late acknowledgment as a reset notification?

In mcu_property_out(), rgb_reply_pending is unconditionally cleared after
a strict 200ms timeout. If an acknowledgment from the MCU arrives after
that 200ms window, the solicited check above will fail because
cfg->rgb_reply_pending is already false. The late ACK will then fall
through to this check where data[3] == OXP_EFFECT_MONO_TRUE matches,
mistaking the late reply for an MCU reset and scheduling a spurious
controller reinitialization.

[ ... ]
> @@ -509,12 +546,18 @@ static int mcu_property_out(struct oxp_hid_cfg *cfg, u8 *header, size_t header_s
>  	dev_dbg(&cfg->hdev->dev, "raw data: [%*ph]\n", OXP_PACKET_SIZE, dmabuf);
>  
>  	ret = hid_hw_output_report(cfg->hdev, dmabuf, OXP_PACKET_SIZE);
> -	if (ret < 0)
> -		return ret;
> -
>  	/* MCU takes 200ms to be ready for another command. */
>  	msleep(200);

[Severity: Low]
Does this code unnecessarily delay 200ms on transport errors?

When hid_hw_output_report() returns a negative error code (e.g. if the
device is disconnected), this removed the early return but failed to
bypass the sleep, resulting in an unneeded 200ms delay.

> -	return ret == OXP_PACKET_SIZE ? 0 : -EIO;
> +	if (ret >= 0)
> +		ret = ret == OXP_PACKET_SIZE ? 0 : -EIO;
> +
> +	if (rgb_write) {
> +		scoped_guard(spinlock_irqsave, &cfg->rgb_reply_lock) {
> +			cfg->rgb_reply_pending = false;
> +		}
> +	}
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910032115.28669-1-andrei1998@gmail.com?part=10