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. > + 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
