On Mon, Oct 19, 2026 at 12:36 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s)
> to consider:
> - [High] hid: spi-hid: disable_irq() before cancel_work_sync() in
> spi_hid_suspend()
> - [High] hid: spi-hid: unchecked deassert_reset() and reset timeout in
> resume/suspend
> - [High] hid: spi-hid: holding power_lock in spi_hid_wait_for_reset() blocks
> recovery
> - [Medium] hid: spi-hid: spi_hid_resume() runs reset_resume before pending
> reset_work
> - [Medium] HID: spi-hid: skipped power-up in spi_hid_resume() when wakeup is
> enabled
> --
>
> commit 943f2d7ec87adee993c65a27b7655a39b4d0ef5c
> Author: Jingyuan Liang <[email protected]>
>
> HID: spi-hid: add power management implementation
>
> This commit implements system sleep power management callbacks for the
> HID over SPI transport driver, handling device suspend, resume, reset
> synchronization, and wake IRQ setup.
>
> > diff --git a/drivers/hid/spi-hid/spi-hid-core.c
> > b/drivers/hid/spi-hid/spi-hid-core.c
> > index 96805c10bfe36..c2232d5959b88 100644
> > --- a/drivers/hid/spi-hid/spi-hid-core.c
> > +++ b/drivers/hid/spi-hid/spi-hid-core.c
>
> [ ... ]
>
> > @@ -242,6 +250,148 @@ 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 void 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;
> > + }
> > +
> > + set_bit(SPI_HID_READY, &shid->flags);
> > +}
>
> [Severity: High]
> What happens if an error occurs during the post-reset handshake while a
> caller is waiting in spi_hid_wait_for_reset()?
>
> Both spi_hid_resume() and the power_down() rollback path in
> spi_hid_suspend() hold shid->power_lock across spi_hid_wait_for_reset()
> and hid_driver_reset_resume().
>
> If a transient bus error, header validation error, or descriptor error
> occurs in spi_hid_dev_irq(), spi_hid_reset_response(), or
> spi_hid_dev_desc_response(), the driver sets SPI_HID_ERROR and schedules
> shid->reset_work to run spi_hid_error_handler():
>
> spi_hid_reset_work()
> spi_hid_error_handler()
> guard(mutex)(&shid->power_lock);
>
> Because spi_hid_wait_for_reset() is sleeping with shid->power_lock held,
> spi_hid_error_handler() blocks on shid->power_lock and cannot perform any
> of its SPI_HID_MAX_RESET_ATTEMPTS recovery resets until
> spi_hid_wait_for_reset() times out after 2000 ms and spi_hid_resume() or
> spi_hid_suspend() returns. That leaves SPI_HID_READY cleared when
> hid_driver_reset_resume() runs, and delays the error reset until after
> resume finishes.
Will fix in v7: resume no longer calls hid_driver_reset_resume() when the
device is not ready. Delaying the error reset until resume returns is
intentional.
>
> > +
> > +static int spi_hid_suspend(struct spi_hid *shid)
> > +{
> > + int error;
> > + struct device *dev = &shid->spi->dev;
> > +
> > + /* 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);
> > + return error;
> > + }
> > + }
> > + }
> > +
> > + /*
> > + * disable_irq() waits for a running IRQ thread, so no new reset_work
> > + * can be queued after it returns. Then wait for any queued or running
> > + * instance. Must not hold power_lock here: reset_work takes it.
> > + */
> > + disable_irq(shid->spi->irq);
> > + cancel_work_sync(&shid->reset_work);
>
> [Severity: High]
> Can calling disable_irq(shid->spi->irq) before
> cancel_work_sync(&shid->reset_work) starve an in-flight synchronous
> request in spi_hid_reset_work()?
>
> If shid->reset_work is already running when spi_hid_suspend() disables the
> IRQ, it may be executing a synchronous request in:
>
> spi_hid_reset_work()
> spi_hid_reset_response() / spi_hid_create_device() /
> spi_hid_refresh_device()
> spi_hid_sync_request()
> __spi_hid_sync_request()
> wait_for_completion_timeout(&shid->output_done, ...)
>
> Since shid->output_done is completed by spi_hid_response_handler() from
> the threaded IRQ handler spi_hid_dev_irq(), disabling the IRQ first
> prevents the response interrupt from being serviced while
> cancel_work_sync() waits for reset_work.
>
> This stalls cancel_work_sync() for the full SPI_HID_RESP_TIMEOUT, fails
> the request with -ETIMEDOUT, clears SPI_HID_READY, sets SPI_HID_ERROR,
> and triggers dev_WARN_ONCE() in spi_hid_reset_response():
>
> error = spi_hid_sync_request(shid, &report);
> if (error) {
> dev_WARN_ONCE(dev, true,
> "Failed to send device descriptor request:
> %d\n", error);
> set_bit(SPI_HID_ERROR, &shid->flags);
> schedule_work(&shid->reset_work);
> }
>
> For wakeup-enabled devices, this also leaves the device suspended with
> SPI_HID_READY cleared (causing spi_hid_input_report_handler() to drop
> wakeup input reports) and SPI_HID_ERROR pending. In addition, because
> shid->power_lock is released after hid_driver_suspend() and before
> disable_irq(), spi_hid_create_device() or spi_hid_refresh_device() in
> spi_hid_reset_work() can register a new shid->hid in that window that
> never has hid_driver_suspend() called on it.
Will fix this in v7.
>
> > +
> > + 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\n",
> > __func__);
> > + shid->regulator_error_count++;
> > + shid->regulator_last_error = error;
> > + /*
> > + * Undo partial suspend before returning
> > error.
> > + * Complete the reset; RESET_PENDING stays set
> > + * until the device sends its reset response.
> > + */
> > +
> > shid->ops->sleep_minimal_reset_delay(shid->ops);
> > + reinit_completion(&shid->reset_done);
> > + shid->ops->deassert_reset(shid->ops);
> > + enable_irq(shid->spi->irq);
> > + if (shid->hid) {
> > + spi_hid_wait_for_reset(shid);
> > + hid_driver_reset_resume(shid->hid);
> > + }
> > + return error;
> > + }
> > +
> > + 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);
> > + if (test_bit(SPI_HID_DEAD, &shid->flags)) {
> > + /* Stay off; only balance disable_irq() in suspend. */
> > + enable_irq(shid->spi->irq);
> > + return 0;
> > + }
> > +
> > + if (!device_may_wakeup(dev)) {
> > + if (shid->power_state == HIDSPI_OFF) {
>
> [Severity: Medium]
> Can gating the power-up path on !device_may_wakeup(dev) and
> shid->power_state == HIDSPI_OFF leave the device unpowered across resume
> or hibernation restore?
This is addressed in 11/11.
>
> In this commit, if shid->power_state is HIDSPI_OFF while
> device_may_wakeup(dev) is true (for example, if power/wakeup is enabled
> while suspended or after a failed power_up()), spi_hid_resume() skips
> powering up and resetting the device. A later commit in the series ("HID:
> spi-hid: add panel follower support") removes the outer
> !device_may_wakeup(dev) check in spi_hid_resume(), but hibernation still
> has a related problem when wakeup is enabled.
>
> Because spi_hid_core_pm uses SYSTEM_SLEEP_PM_OPS(spi_hid_core_pm_suspend,
> spi_hid_core_pm_resume), .freeze, .thaw, .poweroff, and .restore all call
> spi_hid_suspend() and spi_hid_resume(). When device_may_wakeup(dev) is
> true during hibernation:
>
> 1. During .freeze, spi_hid_suspend() sees device_may_wakeup(dev) and skips
> clearing SPI_HID_READY, asserting reset, calling ops->power_down(), and
> setting shid->power_state = HIDSPI_OFF. The hibernation image therefore
> saves shid->power_state as HIDSPI_ON (and conf->supply_enabled as true
> in spi-hid-of).
>
> 2. During .poweroff, spi_hid_suspend() again skips powering down the
> device, and platform power is then removed by S4 or by the boot
> kernel's .freeze before image restoration.
>
> 3. During .restore, spi_hid_resume() sees shid->power_state == HIDSPI_ON
> from the restored image, skips assert_reset(), power_up(),
> deassert_reset(), and spi_hid_wait_for_reset(), and calls
> hid_driver_reset_resume(shid->hid) while the hardware is unpowered or
> held in reset.
Will address this in a follow-up patch.
>
> > + 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\n",
> > __func__);
> > + shid->regulator_error_count++;
> > + shid->regulator_last_error = 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);
> > + shid->ops->deassert_reset(shid->ops);
> > + was_reset = true;
> > + }
> > + }
> > +
> > + 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: High]
> Should spi_hid_resume() and the power_down() error rollback in
> spi_hid_suspend() check the return value of shid->ops->deassert_reset()
> and handle timeouts from spi_hid_wait_for_reset()?
>
> Unlike spi_hid_core_probe() and spi_hid_error_handler(), both sites ignore
> the return value of shid->ops->deassert_reset(shid->ops) (which evaluates
> the ACPI _RST method in spi_hid_acpi_deassert_reset() or drives the reset
> GPIO in spi_hid_of_deassert_reset()).
>
> If deassert_reset() fails, or if the device fails to respond before
> spi_hid_wait_for_reset() times out, spi_hid_wait_for_reset() only logs a
> warning and returns without setting SPI_HID_READY, setting SPI_HID_ERROR,
> or scheduling shid->reset_work (and if shid->hid is NULL,
> spi_hid_wait_for_reset() is not called at all).
>
> Both functions then unconditionally call hid_driver_reset_resume(shid->hid)
> while SPI_HID_READY is still 0, causing any synchronous HID requests
> during reset_resume to fail with -ENODEV in __spi_hid_send_output_report(),
> and leaving the device stuck in SPI_HID_RESET_PENDING with no recovery
> scheduled.
Will fix this in v7.
>
> [Severity: Medium]
> Also in spi_hid_resume(), what happens when a wakeup-enabled device
> (was_reset == false) resumes while reset or recovery work is pending?
>
> When SPI_HID_RESET_RESPONSE, SPI_HID_CREATE_DEVICE, or SPI_HID_ERROR was
> pending when spi_hid_suspend() cancelled shid->reset_work, SPI_HID_READY
> is 0.
>
> In spi_hid_resume(), schedule_work(&shid->reset_work) queues the worker
> asynchronously while shid->power_lock is still held, spi_hid_wait_for_reset()
> is skipped because was_reset is false, and hid_driver_reset_resume(shid->hid)
> is called immediately before shid->reset_work can complete the reset and
> set SPI_HID_READY.
>
> As a result, __spi_hid_send_output_report() rejects any feature or output
> reports sent by hid_driver_reset_resume() with -ENODEV:
>
> /* While not READY, only (re)init descriptor requests may be sent. */
> if (report->report_type != DEVICE_DESCRIPTOR &&
> report->report_type != REPORT_DESCRIPTOR &&
> !test_bit(SPI_HID_READY, &shid->flags))
> return -ENODEV;
>
Will fix this in v7.
> > + if (error) {
> > + dev_err(dev, "%s: failed to reset resume hid driver:
> > %d\n",
> > + __func__, error);
> > + return error;
> > + }
> > + }
> > + return 0;
> > +}
>
> [ ... ]
>
> > @@ -1454,6 +1620,25 @@ void spi_hid_core_remove(struct spi_device *spi)
> > }
> > EXPORT_SYMBOL_GPL(spi_hid_core_remove);
> >
> > +static int spi_hid_core_pm_suspend(struct device *dev)
> > +{
> > + struct spi_hid *shid = dev_get_drvdata(dev);
> > +
> > + return spi_hid_suspend(shid);
> > +}
> > +
> > +static int spi_hid_core_pm_resume(struct device *dev)
> > +{
> > + struct spi_hid *shid = dev_get_drvdata(dev);
> > +
> > + return spi_hid_resume(shid);
> > +}
> > +
> > +const struct dev_pm_ops spi_hid_core_pm = {
> > + SYSTEM_SLEEP_PM_OPS(spi_hid_core_pm_suspend, spi_hid_core_pm_resume)
> > +};
> > +EXPORT_SYMBOL_GPL(spi_hid_core_pm);
>
> [ ... ]
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=10
