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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: spi-hid: unhandled deassert_reset() failure in spi_hid_resume()
- [Medium] HID: spi-hid: race and missed hid_driver_reset_resume() in spi_hid_resume()
--

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

HID: spi-hid: add power management implementation

This patch adds system sleep power management callbacks to the HID over SPI
transport driver, handling device power down, reset synchronization, and wake
IRQ setup across suspend and resume.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 4ff93d22715cd..3b8874ca0e1cc 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c

[ ... ]

> @@ -231,6 +239,147 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
>  	}
>  }
>  
> +/*
> + * Wait for the device descriptor response after reset; the device then takes
> + * requests. Set READY here: the report descriptor check that normally sets it
> + * needs power_lock, which callers hold.
> + */
> +static int spi_hid_wait_for_reset(struct spi_hid *shid)
> +{
> +	unsigned long timeout = msecs_to_jiffies(SPI_HID_RESET_TIMEOUT +
> +						 SPI_HID_RESP_TIMEOUT);
> +
> +	if (!wait_for_completion_timeout(&shid->reset_done, timeout)) {
> +		dev_warn(&shid->spi->dev,
> +			 "Device descriptor not received after reset\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	set_bit(SPI_HID_READY, &shid->flags);
> +	return 0;
> +}
> +
> +static int spi_hid_suspend(struct spi_hid *shid)
> +{
> +	int error;
> +	struct device *dev = &shid->spi->dev;
> +
> +	/*
> +	 * Stop reset_work until resume; pending steps keep their flags. The
> +	 * IRQ stays on so a request in flight gets its response.
> +	 */
> +	disable_work_sync(&shid->reset_work);
> +
> +	/* HID driver suspend may talk to the device; responses need the IRQ. */
> +	scoped_guard(mutex, &shid->power_lock) {
> +		if (shid->power_state != HIDSPI_OFF && shid->hid) {
> +			error = hid_driver_suspend(shid->hid, PMSG_SUSPEND);
> +			if (error) {
> +				dev_err(dev, "%s failed to suspend hid driver: %d\n",
> +					__func__, error);
> +				enable_work(&shid->reset_work);
> +				/* Redo a schedule_work() dropped while disabled. */
> +				schedule_work(&shid->reset_work);
> +				return error;
> +			}
> +		}
> +	}
> +
> +	/* disable_irq() waits for a running IRQ thread. */
> +	disable_irq(shid->spi->irq);
> +
> +	guard(mutex)(&shid->power_lock);
> +	if (!device_may_wakeup(dev)) {
> +		if (shid->power_state != HIDSPI_OFF) {
> +			/* Device is fully reset on resume; drop stale state. */
> +			clear_bit(SPI_HID_READY, &shid->flags);
> +			clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
> +			clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
> +			clear_bit(SPI_HID_ERROR, &shid->flags);
> +			set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +
> +			shid->ops->assert_reset(shid->ops);
> +
> +			error = shid->ops->power_down(shid->ops);
> +			if (error)
> +				dev_err(dev, "%s: could not power down: %d\n", __func__, error);
> +
> +			/* Held in reset either way; resume resets it. */
> +			shid->power_state = HIDSPI_OFF;
> +		}
> +	}
> +	return 0;
> +}
> +
> +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);
> +	enable_work(&shid->reset_work);
> +	if (test_bit(SPI_HID_DEAD, &shid->flags)) {
> +		/* Stay off; only balance disable_*() in suspend. */
> +		enable_irq(shid->spi->irq);
> +		return 0;
> +	}
> +
> +	if (!device_may_wakeup(dev)) {
> +		if (shid->power_state == HIDSPI_OFF) {
> +			shid->ops->assert_reset(shid->ops);
> +
> +			shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> +			error = shid->ops->power_up(shid->ops);
> +			if (error) {
> +				dev_err(dev, "%s: could not power up: %d\n", __func__, error);
> +				/* Suspend runs next even if resume fails. */
> +				enable_irq(shid->spi->irq);
> +				return error;
> +			}
> +			shid->power_state = HIDSPI_ON;
> +			reinit_completion(&shid->reset_done);
> +			error = shid->ops->deassert_reset(shid->ops);
> +			if (error)
> +				dev_err(dev, "%s: failed to deassert reset: %d\n",
> +					__func__, error);
> +			else
> +				was_reset = true;
> +		}
> +	}

[Severity: High]
What happens in spi_hid_resume() if shid->ops->deassert_reset(shid->ops)
fails (or if was_reset is true while shid->hid is still NULL and the reset
times out)?

When deassert_reset() fails in spi_hid_error_handler(), the driver sets
SPI_HID_ERROR and schedules shid->reset_work because a device held in reset
will not assert a reset-response IRQ:

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_error_handler() {
	...
	error = shid->ops->deassert_reset(shid->ops);
	if (error) {
		dev_err(dev, "failed to deassert reset: %d\n", error);
		set_bit(SPI_HID_ERROR, &shid->flags);
		schedule_work(&shid->reset_work);
	}
}

In spi_hid_resume(), however, a deassert_reset() failure only logs the error
and leaves was_reset false. Because spi_hid_suspend() cleared SPI_HID_READY,
SPI_HID_RESET_RESPONSE, SPI_HID_CREATE_DEVICE, and SPI_HID_ERROR before
powering down, shid->reset_work is not scheduled, spi_hid_wait_for_reset() is
skipped, and spi_hid_resume() returns 0 when !test_bit(SPI_HID_READY,
&shid->flags).

Similarly, because spi_hid_wait_for_reset() is gated inside if (shid->hid), if
shid->hid was not yet created before suspend and the post-resume reset times
out, nothing sets SPI_HID_ERROR or schedules shid->reset_work.

Could this leave the device permanently held in reset with its regulator
enabled in HIDSPI_ON and no recovery work queued?

> +
> +	enable_irq(shid->spi->irq);
> +
> +	/*
> +	 * For devices that may wake up, process work that was pending while
> +	 * suspend had reset_work disabled. 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)) {
> +			/* No reset response: let recovery retry. */
> +			set_bit(SPI_HID_ERROR, &shid->flags);
> +			schedule_work(&shid->reset_work);
> +			return -ETIMEDOUT;
> +		}
> +		/* HID requests would fail while the device is not ready. */
> +		if (!test_bit(SPI_HID_READY, &shid->flags))
> +			return 0;
> +		error = hid_driver_reset_resume(shid->hid);
> +		if (error) {
> +			dev_err(dev, "%s: failed to reset resume hid driver: %d\n",
> +				__func__, error);
> +			return error;
> +		}
> +	}
> +	return 0;
> +}

[Severity: Medium]
On the wakeup path (when device_may_wakeup(dev) is true and was_reset is
false), how does hid_driver_reset_resume(shid->hid) interact with a pending
reset_work?

When spi_hid_irq_error() sets SPI_HID_ERROR or spi_hid_process_input_report()
sets SPI_HID_RESET_RESPONSE in the IRQ thread, SPI_HID_READY is not cleared
immediately. Instead, SPI_HID_READY is cleared later when shid->reset_work
runs spi_hid_error_handler() (which must first acquire shid->power_lock) or
spi_hid_reset_response().

Because spi_hid_resume() holds shid->power_lock across the entire function,
scheduling shid->reset_work and immediately checking test_bit(SPI_HID_READY,
&shid->flags) means:

1. If SPI_HID_ERROR or SPI_HID_RESET_RESPONSE was set while SPI_HID_READY is
still set, spi_hid_resume() sees SPI_HID_READY set and calls
hid_driver_reset_resume(shid->hid) right before shid->reset_work runs and
resets the device.

2. Conversely, if SPI_HID_READY was already cleared (for example, after a sync
request timeout or while SPI_HID_CREATE_DEVICE is pending), spi_hid_resume()
returns 0 early at !test_bit(SPI_HID_READY, &shid->flags). Once
shid->reset_work later completes in spi_hid_refresh_device():

drivers/hid/spi-hid/spi-hid-core.c:spi_hid_refresh_device() {
	...
	/* Same report descriptor, so no need to create a new hid device. */
	if (new_crc32 == shid->report_descriptor_crc32) {
		shid->reset_attempts = 0;
		set_bit(SPI_HID_READY, &shid->flags);
		return;
	}
}

it sets SPI_HID_READY and returns without calling
hid_driver_reset_resume(shid->hid), leaving the HID driver in the suspended
state set by spi_hid_suspend().

Should spi_hid_resume() coordinate with pending reset work on the wakeup path
so hid_driver_reset_resume() runs after the reset completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-send-upstream-v7-0-abad13174b43@chromium.org?part=10