On Fri, Oct 9, 2026 at 3:43 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [Medium] spi-hid: hid_driver_reset_resume() races with error recovery
> during resume
> --
>
> commit e43c402ebe88a8dd98c0bab5a81f64d825faf560
> Author: Jingyuan Liang <[email protected]>
>
> HID: spi-hid: add power management implementation
>
> This commit implements power management callbacks for the HID over SPI
> driver. It introduces suspend and resume operations, handling device
> power state and reset coordination during system transitions.
>
> > diff --git a/drivers/hid/spi-hid/spi-hid-core.c
> > b/drivers/hid/spi-hid/spi-hid-core.c
> > index f4e9b4e1294ed..66bfc251bd915 100644
> > --- a/drivers/hid/spi-hid/spi-hid-core.c
> > +++ b/drivers/hid/spi-hid/spi-hid-core.c
> [ ... ]
> > +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)) {
> [ ... ]
> > + 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: Medium]
> Does hid_driver_reset_resume() race with error recovery here?
>
> In spi_hid_resume() (drivers/hid/spi-hid/spi-hid-core.c), the power_lock is
> acquired at the beginning of the function and held until it returns.
>
> If a wakeup-capable device suspends while an error recovery is pending
> (the SPI_HID_ERROR flag is set), spi_hid_suspend() cancels reset_work and
> leaves the device powered on.
>
> During system resume, spi_hid_resume() reschedules reset_work to handle the
> pending error. However, because the device was left powered on, was_reset is
> false, causing spi_hid_resume() to skip spi_hid_wait_for_reset().
>
> It then immediately calls hid_driver_reset_resume() while still holding
> power_lock. Because reset_work requires power_lock to execute
> spi_hid_error_handler(), the scheduled error recovery is blocked until
> spi_hid_resume() returns.
>
> Consequently, hid_driver_reset_resume() attempts to communicate with the
> device synchronously while it is physically still in an unresponsive error
> state, which will time out or fail, aborting the device resume.
This needs an error to be pending exactly at suspend on a wakeup-enabled
device. Error recovery still runs right after resume, so I'd rather keep
the resume path simple.
>
> > + if (error) {
> > + dev_err(dev, "%s: failed to reset resume hid driver:
> > %d\n",
> > + __func__, error);
> > + return error;
> > + }
> > + }
> > + return 0;
> > +}
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=10
