Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] HID: spi-hid: unrecovered deassert_reset() failure in spi_hid_resume() - [Medium] HID: spi-hid: improper wakeup teardown for panel followers in probe --
commit 35d9a38c1b6ffa8dbb3ab740ec9ce120c5b2e7a1 Author: Jingyuan Liang <[email protected]> HID: spi-hid: add panel follower support This commit adds DRM panel follower support to spi-hid so that a follower device powers up and suspends in coordination with its panel rather than during initial probe and standard system PM callbacks. > diff --git a/drivers/hid/spi-hid/spi-hid-core.c > b/drivers/hid/spi-hid/spi-hid-core.c > index 3b8874ca0e1cc..496b33cdf3d46 100644 > --- a/drivers/hid/spi-hid/spi-hid-core.c > +++ b/drivers/hid/spi-hid/spi-hid-core.c [ ... ] > @@ -325,28 +328,30 @@ static int spi_hid_resume(struct spi_hid *shid) > return 0; > } > > - if (!device_may_wakeup(dev)) { > - if (shid->power_state == HIDSPI_OFF) { > - shid->ops->assert_reset(shid->ops); > + /* > + * Check power_state, not device_may_wakeup(): power/wakeup can change > + * while the device is off. > + */ > + if (shid->power_state == HIDSPI_OFF) { > + shid->ops->assert_reset(shid->ops); > > - shid->ops->sleep_minimal_reset_delay(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; > + 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: Medium] What happens if shid->ops->deassert_reset(shid->ops) fails in spi_hid_resume()? For non-followers, initial power-up goes through spi_hid_dev_init(), which powers down the device and returns the error when deassert_reset() fails. For panel followers, spi_hid_core_probe() leaves shid->power_state as HIDSPI_OFF and shid->hid as NULL, and spi_hid_panel_follower_work() relies on spi_hid_resume() for the initial power-up as well as subsequent resumes. When deassert_reset() fails in spi_hid_resume(), was_reset remains false while shid->power_state is set to HIDSPI_ON and the IRQ is enabled with the device still held in reset: drivers/hid/spi-hid/spi-hid-core.c:spi_hid_resume() { ... 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; ... } On the first power-up of a panel follower, shid->hid is NULL, so spi_hid_resume() returns 0 without scheduling shid->reset_work. On subsequent resumes where shid->hid is non-NULL, was_reset is false and SPI_HID_READY was cleared during spi_hid_suspend(), so spi_hid_resume() again returns 0 without setting SPI_HID_ERROR or scheduling shid->reset_work. Should a deassert_reset() failure in spi_hid_resume() trigger recovery (or clean up and return the error) so the device is not left powered on in reset and permanently uninitialized? > @@ -1447,6 +1452,108 @@ static struct hid_ll_driver spi_hid_ll_driver = { [ ... ] > +static int spi_hid_register_panel_follower(struct spi_hid *shid) > +{ > + struct device *dev = &shid->spi->dev; > + > + shid->panel_follower.funcs = &spi_hid_panel_follower_prepare_funcs; > + > + /* > + * If we're not in control of our own power up/power down then we can't > + * do the logic to manage wakeups. Give a warning if a user thought > + * that was possible then force the capability off. > + */ > + if (device_can_wakeup(dev)) { > + dev_warn(dev, "Can't wakeup if following panel\n"); > + device_set_wakeup_capable(dev, false); > + } > + > + return drm_panel_add_follower(dev, &shid->panel_follower); > +} [ ... ] > @@ -1523,21 +1618,19 @@ int spi_hid_core_probe(struct spi_device *spi, struct > spihid_ops *ops, > } > } > > - error = shid->ops->power_up(shid->ops); > - if (error) { > - dev_err(dev, "%s: could not power up: %d\n", __func__, error); > - return error; > - } > - > - error = shid->ops->deassert_reset(shid->ops); > - if (error) { > - dev_err(dev, "%s: failed to deassert reset: %d\n", __func__, > error); > - shid->ops->power_down(shid->ops); > - return error; > + if (shid->is_panel_follower) { > + /* Like the IRQ: the first panel_prepared resume enables it. */ > + disable_work(&shid->reset_work); > + error = spi_hid_register_panel_follower(shid); [Severity: Medium] Does calling device_set_wakeup_capable(dev, false) in spi_hid_register_panel_follower() after devm_pm_set_wake_irq() in spi_hid_core_probe() leave the wake IRQ configured and leak dev->power.wakeup if wakeup was enabled on the device before probe? In spi_hid_core_probe(), if the firmware node marked the device as wakeup capable (for example via acpi_bind_one()), device_can_wakeup(dev) is true and devm_pm_set_wake_irq(dev, spi->irq) attaches a wake IRQ before shid->is_panel_follower is checked: drivers/hid/spi-hid/spi-hid-core.c:spi_hid_core_probe() { ... if (device_can_wakeup(dev)) { error = devm_pm_set_wake_irq(dev, spi->irq); if (error) { dev_err(dev, "%s: failed to set wake IRQ\n", __func__); return error; } } if (shid->is_panel_follower) { /* Like the IRQ: the first panel_prepared resume enables it. */ disable_work(&shid->reset_work); error = spi_hid_register_panel_follower(shid); ... } Furthermore, if a wakeup_source was attached to dev->power.wakeup (for example via sysfs power/wakeup before async or deferred probe completes), calling device_set_wakeup_capable(dev, false) directly sets dev->power.can_wakeup = false without detaching the wakeup source first. Subsequent calls to device_wakeup_disable(dev), including from device_pm_remove() on unbind, check dev->power.can_wakeup and return early: drivers/base/power/wakeup.c:device_wakeup_disable() { struct wakeup_source *ws; if (!dev || !dev->power.can_wakeup) return; ws = device_wakeup_detach(dev); wakeup_source_unregister(ws); } Should spi_hid_register_panel_follower() use device_init_wakeup(dev, false) (which calls device_wakeup_disable(dev) before clearing wakeup capability), and should spi_hid_core_probe() skip devm_pm_set_wake_irq() when shid->is_panel_follower is true? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
