Re: [PATCH v7 11/11] HID: spi-hid: add panel follower support

[sashiko-bot@kernel.org]

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 <jingyliang@chromium.org>

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/20261024-send-upstream-v7-0-abad13174b43@chromium.org?part=11