Re: [PATCH v7 05/11] HID: spi-hid: add HID SPI protocol implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] HID: spi-hid: spi_hid_reset_work() runs pre-error work before SPI_HID_ERROR
- [Medium] HID: spi-hid: sync requests not aborted or serialized on reset in error path
- [Medium] HID: spi-hid: duplicate reset scheduled on timeout in spi_hid_get_request()
--

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

HID: spi-hid: add HID SPI protocol implementation

This commit implements the HID over SPI protocol in spi-hid-core.c,
adding interrupt handling, input report processing, synchronous output
request/response tracking, and error recovery via device reset.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 54c278ac8803b..cd4d763840098 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -172,17 +333,96 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
[ ... ]
> +static void spi_hid_error_handler(struct spi_hid *shid)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	int error;
> +
> +	guard(mutex)(&shid->power_lock);
> +	if (shid->power_state == HIDSPI_OFF)
> +		return;
> +
> +	disable_irq(shid->spi->irq);
> +
> +	if (shid->reset_attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
> +		dev_err(dev, "unresponsive device, aborting\n");
> +		spi_hid_stop_hid(shid);
> +		shid->ops->assert_reset(shid->ops);
> +		error = shid->ops->power_down(shid->ops);
> +		if (error)
> +			dev_err(dev, "failed to disable regulator: %d\n", error);
> +		shid->power_state = HIDSPI_OFF;
> +		/* Dead device: leave the IRQ permanently disabled. */
> +		return;
> +	}
> +
> +	clear_bit(SPI_HID_READY, &shid->flags);
> +	set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +
> +	shid->ops->assert_reset(shid->ops);
> +
> +	shid->power_state = HIDSPI_OFF;
> +
> +	/*
> +	 * We want to cancel pending reset work as the device is being reset
> +	 * to recover from an error. cancel_work_sync will put us in a deadlock
> +	 * because this function is scheduled in 'reset_work' and we should
> +	 * avoid waiting for itself.
> +	 */
> +	cancel_work(&shid->reset_work);
> +
> +	/* Drop work requested before this reset. */
> +	clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
> +	clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
> +	clear_bit(SPI_HID_ERROR, &shid->flags);
> +
> +	shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> +	shid->power_state = HIDSPI_ON;
> +
> +	shid->ops->deassert_reset(shid->ops);
> +
> +	enable_irq(shid->spi->irq);
> +}

[Severity: Medium]
What happens if a synchronous request is waiting on shid->output_done in
__spi_hid_sync_request() while an interrupt error in spi_hid_dev_irq() ->
spi_hid_irq_error() or a spontaneous RESET_RESPONSE in
spi_hid_process_input_report() triggers a reset?

In __spi_hid_sync_request(), the caller holds shid->output_lock while
waiting in wait_for_completion_timeout(&shid->output_done, ...). However,
spi_hid_error_handler() only acquires shid->power_lock: it does not
serialize with shid->output_lock or shid->io_lock, nor does it clear
shid->expected_response or complete shid->output_done to abort an in-flight
request.

Because of this:
1. A thread in __spi_hid_send_output_report() that already passed the
   test_bit(SPI_HID_READY, &shid->flags) check before acquiring
   shid->io_lock can perform spi_hid_output() on the SPI bus while
   spi_hid_error_handler() has asserted reset and set shid->power_state to
   HIDSPI_OFF.
2. When the device finishes resetting and sends RESET_RESPONSE,
   spi_hid_reset_response() -> spi_hid_sync_request() blocks on
   shid->output_lock until the pre-reset waiter's 1000 ms timeout expires.
3. Once wait_for_completion_timeout() in __spi_hid_sync_request() finally
   times out, the pre-reset caller sets SPI_HID_ERROR and schedules
   shid->reset_work again, triggering a second hardware reset after the
   first recovery completes and incrementing shid->reset_attempts a second
   time.

Should the error and reset paths abort any in-flight request (or serialize
with shid->output_lock / shid->io_lock) so that pre-reset waiters do not
stall recovery and schedule a stale reset on timeout?

[ ... ]
> @@ -324,13 +752,206 @@ static int spi_hid_create_device(struct spi_hid *shid)
[ ... ]
> +static void spi_hid_reset_work(struct work_struct *work)
> +{
> +	struct spi_hid *shid =
> +		container_of(work, struct spi_hid, reset_work);
> +	struct device *dev = &shid->spi->dev;
> +	int error = 0;
> +	bool resched = false;
> +
> +	if (test_and_clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags)) {
> +		spi_hid_reset_response(shid);
> +		resched = true;
> +	} else if (test_and_clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags)) {
> +		guard(mutex)(&shid->power_lock);
> +		if (shid->power_state != HIDSPI_OFF) {
> +			if (!shid->hid) {
> +				error = spi_hid_create_device(shid);
> +				if (error) {
> +					dev_err(dev, "%s: Failed to create hid device: %d\n",
> +						__func__, error);
> +				}
> +			} else {
> +				spi_hid_refresh_device(shid);
> +			}
> +		} else {
> +			dev_err(dev, "%s: Powered off, returning\n", __func__);
> +		}
> +		resched = true;
> +	} else if (test_and_clear_bit(SPI_HID_ERROR, &shid->flags)) {
> +		spi_hid_error_handler(shid);
> +	}

[Severity: Medium]
Should spi_hid_reset_work() check SPI_HID_ERROR before
SPI_HID_RESET_RESPONSE and SPI_HID_CREATE_DEVICE?

In spi_hid_error_handler(), the code clears SPI_HID_RESET_RESPONSE and
SPI_HID_CREATE_DEVICE with the comment:

	/* Drop work requested before this reset. */
	clear_bit(SPI_HID_RESET_RESPONSE, &shid->flags);
	clear_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
	clear_bit(SPI_HID_ERROR, &shid->flags);

However, because spi_hid_error_handler() is only called from the final
else-if branch in spi_hid_reset_work(), it cannot run while
SPI_HID_RESET_RESPONSE or SPI_HID_CREATE_DEVICE is set. If an error sets
SPI_HID_ERROR while either of those bits is pending (for example, an IRQ
error in spi_hid_dev_irq() -> spi_hid_irq_error() after
spi_hid_dev_desc_response() sets SPI_HID_CREATE_DEVICE),
spi_hid_reset_work() will first execute spi_hid_reset_response() or
spi_hid_create_device() / spi_hid_refresh_device(), reschedule itself, and
only then run spi_hid_error_handler() to immediately reset the newly
created or refreshed HID device.

[ ... ]
> @@ -355,6 +976,15 @@ static int spi_hid_get_request(struct spi_hid *shid, u8 content_id,
>  		dev_err(dev,
>  			"Expected get request response not received! Error %d\n",
>  			error);
> +		/*
> +		 * -ENODEV: the device is not ready (e.g. a reset is in
> +		 * progress), so the GET_FEATURE request was not sent.
> +		 * Don't start another reset on top of the one already running.
> +		 */
> +		if (error != -ENODEV) {
> +			set_bit(SPI_HID_ERROR, &shid->flags);
> +			schedule_work(&shid->reset_work);
> +		}
>  		return error;
>  	}

[Severity: Medium]
Won't a timed-out request in spi_hid_get_request() schedule a duplicate
reset?

When __spi_hid_sync_request() times out waiting on shid->output_done, it
already sets SPI_HID_ERROR and schedules shid->reset_work before returning
-ETIMEDOUT:

	dev_err(dev, "Response timed out\n");
	/* The device may still answer later: resync it with a reset. */
	set_bit(SPI_HID_ERROR, &shid->flags);
	schedule_work(&shid->reset_work);
	return -ETIMEDOUT;

Because spi_hid_reset_work() -> spi_hid_error_handler() does not take
shid->output_lock, the worker thread on another CPU can immediately begin
running spi_hid_reset_work() and clear SPI_HID_ERROR while
spi_hid_get_request() (and similarly spi_hid_report_descriptor_request()
and spi_hid_reset_response()) receives -ETIMEDOUT and sets SPI_HID_ERROR
and schedules shid->reset_work a second time. This triggers two
back-to-back hardware resets and increments shid->reset_attempts twice for
a single timeout.

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