Re: [PATCH v7 05/11] HID: spi-hid: add HID SPI protocol implementation
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-24 01:13:50
Also in:
linux-devicetree, linux-input
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -172,17 +333,96 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
[ ... ]
quoted hunk ↗ jump to hunk
+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? [ ... ]
quoted hunk ↗ jump to hunk
@@ -324,13 +752,206 @@ static int spi_hid_create_device(struct spi_hid *shid)
[ ... ]
quoted hunk ↗ jump to hunk
+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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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