Re: [PATCH v5 10/11] HID: spi-hid: add power management implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] spi-hid: hid_driver_reset_resume() races with error recovery during resume
--

commit e43c402ebe88a8dd98c0bab5a81f64d825faf560
Author: Jingyuan Liang <jingyliang@chromium.org>

    HID: spi-hid: add power management implementation

This commit implements power management callbacks for the HID over SPI
driver. It introduces suspend and resume operations, handling device
power state and reset coordination during system transitions.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index f4e9b4e1294ed..66bfc251bd915 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> +static int spi_hid_resume(struct spi_hid *shid)
> +{
> +	int error;
> +	struct device *dev = &shid->spi->dev;
> +	bool was_reset = false;
> +
> +	guard(mutex)(&shid->power_lock);
> +	if (test_bit(SPI_HID_DEAD, &shid->flags)) {
[ ... ]
> +	enable_irq(shid->spi->irq);
> +
> +	/*
> +	 * For devices that may wake up, process work that was pending when
> +	 * suspend cancelled reset_work. Otherwise suspend already dropped it.
> +	 */
> +	if (test_bit(SPI_HID_RESET_RESPONSE, &shid->flags) ||
> +	    test_bit(SPI_HID_CREATE_DEVICE, &shid->flags) ||
> +	    test_bit(SPI_HID_ERROR, &shid->flags))
> +		schedule_work(&shid->reset_work);
> +
> +	if (shid->hid) {
> +		if (was_reset)
> +			spi_hid_wait_for_reset(shid);
> +		error = hid_driver_reset_resume(shid->hid);

[Severity: Medium]
Does hid_driver_reset_resume() race with error recovery here?

In spi_hid_resume() (drivers/hid/spi-hid/spi-hid-core.c), the power_lock is
acquired at the beginning of the function and held until it returns.

If a wakeup-capable device suspends while an error recovery is pending
(the SPI_HID_ERROR flag is set), spi_hid_suspend() cancels reset_work and
leaves the device powered on.

During system resume, spi_hid_resume() reschedules reset_work to handle the
pending error. However, because the device was left powered on, was_reset is
false, causing spi_hid_resume() to skip spi_hid_wait_for_reset().

It then immediately calls hid_driver_reset_resume() while still holding
power_lock. Because reset_work requires power_lock to execute
spi_hid_error_handler(), the scheduled error recovery is blocked until
spi_hid_resume() returns.

Consequently, hid_driver_reset_resume() attempts to communicate with the
device synchronously while it is physically still in an unresponsive error
state, which will time out or fail, aborting the device resume.

> +		if (error) {
> +			dev_err(dev, "%s: failed to reset resume hid driver: %d\n",
> +				__func__, error);
> +			return error;
> +		}
> +	}
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-send-upstream-v5-0-384af01da3ee@chromium.org?part=10