Re: [PATCH v4 10/11] HID: spi-hid: add power management implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] `spi_hid_resume()` calls `hid_driver_reset_resume()` synchronously before the device signals readiness.
- [High] Error path in `spi_hid_resume()` creates unbalanced IRQ disable depth for wakeup-capable devices.
- [High] Missing synchronization between system suspend and `reset_work` causes a TOCTOU race on `power_state`.
- [High] System suspend cuts device power without synchronizing against ongoing SPI HID requests.
- [High] Unbalanced `enable_irq()` in `spi_hid_resume()` triggers a kernel warning if the device was already `HIDSPI_OFF`.
- [Medium] Conditional cleanup of the wake IRQ leads to a memory leak if userspace disables wakeup.

Pre-existing issues:
- [Critical] Returning `IRQ_HANDLED` for an unpowered device causes an infinite IRQ storm.
--

commit 57585bfd26214992cdd5246d2df2a4622693a915
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add power management implementation

This commit implements HID over SPI driver power management callbacks, adding
suspend and resume functions for system power state transitions.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 698e72102c11e..517f069134775 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c

[ ... ]

> @@ -245,6 +247,96 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
>  	}
>  }
>  
> +static int spi_hid_suspend(struct spi_hid *shid)
> +{
> +	int error;
> +	struct device *dev = &shid->spi->dev;
> +
> +	guard(mutex)(&shid->power_lock);

[Severity: High]
Does this suspend routine need to synchronize against ongoing SPI I/O?

HID I/O routines like spi_hid_ll_output_report() and spi_hid_ll_raw_request()
acquire output_lock and io_lock to serialize transfers, but they do not check
or lock power_state. Because spi_hid_suspend() acquires power_lock to disable
the device's regulator but does not acquire output_lock or io_lock, nor does
it clear SPI_HID_READY, could it cut hardware power concurrently while a kernel
thread is actively executing spi_write() on another CPU, leading to hardware
bus timeouts?

[Severity: High]
Does this function also need to flush or cancel the asynchronous reset_work
before cutting device power?

If it doesn't synchronize with the workqueue, could there be a race where
spi_hid_reset_response() locklessly reads power_state as HIDSPI_ON, gets
preempted, and allows spi_hid_suspend() to power down the device? If the
workqueue then resumes and calls spi_hid_sync_request(), it would initiate
SPI transmissions to an unpowered device.

> +	if (shid->power_state == HIDSPI_OFF)
> +		return 0;

[Severity: High]
If this early return is taken, will the subsequent resume cause an unbalanced
IRQ enable warning?

In spi_hid_suspend(), this early return completely bypasses the call to
disable_irq(shid->spi->irq). However, upon system resume, spi_hid_resume()
unconditionally calls enable_irq(shid->spi->irq). If the IRQ disable depth
was not incremented during suspend, could this unconditional enable drop the
depth below zero and trigger a WARN(1, "Unbalanced enable for IRQ %d\n")?

> +
> +	if (shid->hid) {
> +		error = hid_driver_suspend(shid->hid, PMSG_SUSPEND);
> +		if (error) {
> +			dev_err(dev, "%s failed to suspend hid driver: %d\n",
> +				__func__, error);
> +			return error;
> +		}
> +	}
> +
> +	disable_irq(shid->spi->irq);
> +
> +	if (!device_may_wakeup(dev)) {
> +		set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +
> +		shid->ops->assert_reset(shid->ops);
> +
> +		error = shid->ops->power_down(shid->ops);
> +		if (error) {
> +			dev_err(dev, "%s: could not power down\n", __func__);
> +			shid->regulator_error_count++;
> +			shid->regulator_last_error = error;
> +			/* Undo partial suspend before returning error */
> +			shid->ops->deassert_reset(shid->ops);
> +			clear_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +			enable_irq(shid->spi->irq);
> +			if (shid->hid)
> +				hid_driver_reset_resume(shid->hid);
> +			return error;
> +		}
> +
> +		shid->power_state = HIDSPI_OFF;
> +	}
> +	return 0;
> +}
> +
> +static int spi_hid_resume(struct spi_hid *shid)
> +{
> +	int error;
> +	struct device *dev = &shid->spi->dev;
> +
> +	guard(mutex)(&shid->power_lock);
> +
> +	if (!device_may_wakeup(dev)) {
> +		if (shid->power_state == HIDSPI_OFF) {
> +			shid->ops->assert_reset(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\n", __func__);
> +				shid->regulator_error_count++;
> +				shid->regulator_last_error = error;
> +				return error;
> +			}
> +			shid->power_state = HIDSPI_ON;
> +			shid->ops->deassert_reset(shid->ops);
> +		}
> +	}
> +
> +	enable_irq(shid->spi->irq);
> +
> +	if (shid->hid) {
> +		error = hid_driver_reset_resume(shid->hid);

[Severity: High]
Does this code need to wait for the device's asynchronous RESET_RESPONSE
before sending further commands?

During a full resume cycle, the device is powered up, its reset line is
deasserted, and enable_irq() is called. The hardware then begins booting
and will signal readiness via an asynchronous RESET_RESPONSE interrupt.
However, spi_hid_resume() does not wait for this interrupt and synchronously
executes hid_driver_reset_resume() immediately. Because the SPI_HID_READY
flag is left set during suspend, any HID initialization commands generated
by the upper layer's resume callback will be immediately transmitted over
the SPI bus to a device that is still booting. Could this result in bus
timeouts and failed resume cycles?

> +		if (error) {
> +			dev_err(dev, "%s: failed to reset resume hid driver: %d\n",
> +				__func__, error);
> +			/* Undo partial resume before returning error */
> +			disable_irq(shid->spi->irq);
> +			if (!device_may_wakeup(dev)) {
> +				set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +				shid->ops->assert_reset(shid->ops);
> +				shid->ops->power_down(shid->ops);
> +				shid->power_state = HIDSPI_OFF;
> +			}

[Severity: High]
Can this error path create an unbalanced IRQ disable depth for
wakeup-capable devices?

If a device is wakeup-capable, spi_hid_suspend() skips powering down the
device (leaving power_state at HIDSPI_ON) but still calls disable_irq().
In spi_hid_resume(), enable_irq() is called, bringing the IRQ depth to 0.

If hid_driver_reset_resume() subsequently fails, this error recovery path
calls disable_irq() to undo the resume, but skips setting HIDSPI_OFF due
to the if (!device_may_wakeup(dev)) condition. Because the PM core ignores
resume errors and continues, the device remains at HIDSPI_ON.

On the next system suspend cycle, spi_hid_suspend() will see HIDSPI_ON and
execute disable_irq() a second time. A later successful resume will only call
enable_irq() once, leaving the device's interrupt permanently disabled.

> +			return error;
> +		}
> +	}
> +	return 0;
> +}

[ ... ]

> @@ -795,6 +887,11 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
>  	trace_spi_hid_header_transfer(shid);
>  
>  	scoped_guard(mutex, &shid->io_lock) {
> +		if (shid->power_state == HIDSPI_OFF) {
> +			dev_warn(dev, "Device is off, ignoring interrupt\n");
> +			goto out;
> +		}

[Severity: Critical]
This isn't a bug introduced by this patch, but should this path return IRQ_NONE
instead of IRQ_HANDLED?

In spi_hid_error_handler(), if the device exceeds maximum reset attempts,
it is aborted and powered off (HIDSPI_OFF). The function then returns,
releasing guard(disable_irq)(&shid->spi->irq) and explicitly re-enabling
the IRQ line.

If the active-low interrupt line floats low due to power loss, it immediately
triggers. This threaded handler detects HIDSPI_OFF, logs a warning, and
returns IRQ_HANDLED via goto out.

Returning IRQ_HANDLED circumvents the kernel's spurious interrupt detector.
Because the physical line remains asserted and the handler claims success
without clearing the hardware source, the genirq subsystem immediately
re-fires the interrupt. Can this lead to a 100% CPU lockup?

[ ... ]

> @@ -1251,10 +1343,19 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
>  		dev_err(dev, "%s: unable to request threaded IRQ\n", __func__);
>  		return error;
>  	}
> +	if (device_may_wakeup(dev)) {
> +		error = dev_pm_set_wake_irq(dev, spi->irq);
> +		if (error) {
> +			dev_err(dev, "%s: failed to set wake IRQ\n", __func__);
> +			return error;
> +		}
> +	}
>  
>  	error = shid->ops->power_up(shid->ops);
>  	if (error) {
>  		dev_err(dev, "%s: could not power up\n", __func__);
> +		if (device_may_wakeup(dev))
> +			dev_pm_clear_wake_irq(dev);

[Severity: Medium]
Will this conditional cleanup leak memory if userspace alters the wakeup
settings?

In spi_hid_core_probe(), dev_pm_set_wake_irq() is executed if
device_may_wakeup(dev) is true. The cleanup path conditionally calls
dev_pm_clear_wake_irq(dev) based on the same check.

However, device_may_wakeup() evaluates dev->power.should_wakeup, which is
mutable state controllable by userspace via sysfs
(/sys/devices/.../power/wakeup). If userspace disables wakeup after the
device is probed, the check fails, bypassing the cleanup and permanently
leaking the allocated wake_irq memory while leaving a dangling pointer in
dev->power.wakeirq. Should dev_pm_clear_wake_irq() be called unconditionally
since it handles missing wake IRQs gracefully?

[ ... ]

> @@ -1284,9 +1385,31 @@ void spi_hid_core_remove(struct spi_device *spi)
>  	error = shid->ops->power_down(shid->ops);
>  	if (error)
>  		dev_err(dev, "failed to disable regulator\n");
> +
> +	if (device_may_wakeup(dev))
> +		dev_pm_clear_wake_irq(dev);

[Severity: Medium]
As noted above, should dev_pm_clear_wake_irq() be called unconditionally here
to avoid leaking the wake_irq structure if userspace disables device wakeup?

>  }
>  EXPORT_SYMBOL_GPL(spi_hid_core_remove);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=10