Re: [PATCH v7 11/11] HID: spi-hid: add panel follower support
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-24 01:10:48
Also in:
linux-devicetree, linux-input
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -1447,6 +1452,108 @@ static struct hid_ll_driver spi_hid_ll_driver = {
[ ... ]
quoted hunk ↗ jump to hunk
+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); +}
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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/20261024-send-upstream-v7-0-abad13174b43@chromium.org?part=11