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

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 [off-list ref]
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.

[ ... ]

[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;
+}

[ ... ]

[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?

[ ... ]

[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?

[ ... ]

[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