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 <[email protected]> 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/[email protected]?part=10
