Re: [PATCH] usb: core: raise root-hub autosuspend delay to stop a busy-loop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: core: usb_runtime_suspend() clobbers root-hub autosuspend_delay
--

commit 60580cc9fa143b8fbfd60c5978507dea53f76384
Author: Jianping Liu <frankjpliu@tencent.com>

usb: core: raise root-hub autosuspend delay to stop a busy-loop

This commit updates usb_runtime_suspend() to set the root-hub autosuspend
delay to 1000 ms when runtime suspend fails while the delay is 0, and
resets any non-zero delay back to 0 when runtime suspend later succeeds.

> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index d3a61ebc6afb0..30965d6ffd677 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -2008,6 +2008,20 @@ int usb_runtime_suspend(struct device *dev)
>  	if (status == -EAGAIN || status == -EBUSY)
>  		usb_mark_last_busy(udev);
>  
> +	/*
> +	 * Root hubs use 0ms delay, so a persistent -EBUSY retries with no
> +	 * wait and pins one CPU at 100%. Set autosuspend_delay to 1000ms
> +	 * on fail, then back to 0 ms on success.
> +	 */
> +	if (!udev->parent) {
> +		int delay = READ_ONCE(dev->power.autosuspend_delay);
> +
> +		if (unlikely(status != 0 && delay == 0))
> +			pm_runtime_set_autosuspend_delay(dev, 1000);
> +		if (unlikely(status == 0 && delay != 0))
> +			pm_runtime_set_autosuspend_delay(dev, 0);
> +	}

[Severity: Medium]
Can this unconditionally overwrite user-configured or driver-configured
dev->power.autosuspend_delay values on root hubs and drop the runtime PM
usage reference when racing with negative delay writes?

First, userspace can configure a custom positive delay on a root hub via
autosuspend_delay_ms_store() in drivers/base/power/sysfs.c or
autosuspend_store() in drivers/usb/core/sysfs.c, and child USB device
drivers may also adjust their parent hub's delay during probe as noted in
hub_probe() in drivers/usb/core/hub.c. Because usb_runtime_suspend() does
not track whether the non-zero delay was set by a prior suspend failure,
won't the first successful root-hub runtime suspend see status == 0 and
delay != 0 and reset the custom delay back to 0?

Conversely, if a transient suspend failure occurs while delay == 0 (for
instance, -EBUSY when a device connects to a port) and the newly connected
device keeps the root hub active afterward, dev->power.autosuspend_delay
remains left at 1000 ms in sysfs.

Second, __rpm_callback() in drivers/base/power/runtime.c releases
dev->power.lock while dev->power.runtime_status is RPM_SUSPENDING before
invoking usb_runtime_suspend(). If userspace concurrently writes a negative
delay to disable autosuspend via autosuspend_store() or
autosuspend_delay_ms_store(), pm_runtime_set_autosuspend_delay() calls
update_autosuspend() in drivers/base/power/runtime.c:

drivers/base/power/runtime.c:update_autosuspend() {
	if (dev->power.use_autosuspend && delay < 0) {

		/* If it used to be allowed then prevent it. */
		if (!old_use || old_delay >= 0) {
			atomic_inc(&dev->power.usage_count);
			rpm_resume(dev, 0);
		} else {
			trace_rpm_usage(dev, 0);
		}
	}

	/* Runtime suspend should be allowed now. */
	else {

		/* If it used to be prevented then allow it. */
		if (old_use && old_delay < 0)
			atomic_dec(&dev->power.usage_count);

		/* Maybe we can autosuspend now. */
		rpm_idle(dev, RPM_AUTO);
	}
}

Inside rpm_resume(), because runtime_status is RPM_SUSPENDING, the caller
releases dev->power.lock and sleeps on dev->power.wait_queue waiting for
the suspend callback to complete.

When usb_suspend_both() finishes in usb_runtime_suspend():

1. If status == 0, READ_ONCE(dev->power.autosuspend_delay) sees the negative
   delay (delay != 0) and calls pm_runtime_set_autosuspend_delay(dev, 0).
2. If status != 0 and READ_ONCE(dev->power.autosuspend_delay) read 0 just
   before the concurrent writer set the negative delay, it calls
   pm_runtime_set_autosuspend_delay(dev, 1000).

In both cases, wouldn't update_autosuspend() see old_use && old_delay < 0,
overwrite the negative delay with 0 or 1000, and execute
atomic_dec(&dev->power.usage_count), undoing the usage_count increment and
re-enabling autosuspend?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028041859.1061836-1-frankljpliu@gmail.com?part=1