Re: [PATCH] usb: dwc3: qcom: Defer VBUS override update during system suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: dwc3: qcom: premature pm_suspended clear in dwc3_qcom_pm_resume()
- [High] usb: dwc3: qcom: race between dwc3_qcom_set_role_notifier() and core PM callbacks
- [Medium] usb: dwc3: qcom: role switch dropped or overwritten during system sleep
--

commit a81658c1fd87525e81eccc5d37bb7630d869a4d0
Author: Vladyslav Tymoshchuk <tvlad744@gmail.com>

usb: dwc3: qcom: Defer VBUS override update during system suspend

This patch serializes dwc3_qcom_set_role_notifier() against system suspend
and resume using a mutex and defers QSCRATCH VBUS override updates while
the glue is suspended for system sleep until dwc3_qcom_pm_resume() runs.

> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index ac68b4218b56b..2997479988c74 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
[ ... ]
> @@ -561,27 +567,40 @@ static int dwc3_qcom_setup_irq(struct dwc3_qcom *qcom, struct platform_device *p
>  static void dwc3_qcom_set_role_notifier(struct dwc3 *dwc, enum usb_role next_role)
>  {
>  	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
> +	bool enable;
> +
> +	guard(mutex)(&qcom->role_lock);
>  
>  	if (qcom->current_role == next_role)
>  		return;
>  
> +	enable = qcom->current_role != USB_ROLE_DEVICE;
> +
> +	/*
> +	 * Not every caller of usb_role_switch_set_role() is frozen during
> +	 * system sleep, e.g. the UCSI connector work can run in the noirq
> +	 * resume phase. The glue clocks are off then and runtime PM can't
> +	 * turn them on, so leave QSCRATCH alone and let
> +	 * dwc3_qcom_pm_resume() apply the update.
> +	 */
> +	if (qcom->pm_suspended) {
> +		qcom->vbus_override_pending = true;
> +		qcom->vbus_override = enable;
> +		qcom->current_role = next_role;
> +		return;
> +	}
> +
>  	if (pm_runtime_resume_and_get(qcom->dev)) {
>  		dev_dbg(qcom->dev, "Failed to resume device\n");
>  		return;
>  	}

[Severity: Medium]
Can a role switch be dropped during system sleep when the PM core takes the
direct_complete path?

dwc3_qcom_dev_pm_ops wires up dwc3_qcom_prepare(), which calls
dwc3_pm_prepare() and returns 1 when the controller is runtime-suspended in
DWC3_GCTL_PRTCAP_DEVICE mode and has no pinctrl state. When no genpd
overrides .prepare and device_may_wakeup(dev) is false (for example, when
wakeup-source is not present or no gadget driver is bound), device_suspend()
takes the direct_complete path: it calls pm_runtime_disable(dev) while the
device is RPM_SUSPENDED and skips both dwc3_qcom_pm_suspend() and
dwc3_qcom_pm_resume().

In that state, qcom->pm_suspended remains false while runtime PM is disabled
and the glue clocks are off. If dwc3_qcom_set_role_notifier() runs during
system sleep, it sees qcom->pm_suspended == false and calls
pm_runtime_resume_and_get(qcom->dev), which fails with -EACCES and returns
without updating qcom->current_role or recording the VBUS override update.

>  
> -	if (qcom->current_role == USB_ROLE_DEVICE)
> -		dwc3_qcom_vbus_override_enable(qcom, false);
> -	else if (qcom->current_role != USB_ROLE_DEVICE)
> -		dwc3_qcom_vbus_override_enable(qcom, true);
> +	qcom->vbus_override_pending = false;
> +	dwc3_qcom_vbus_override_enable(qcom, enable);
>  
>  	pm_runtime_mark_last_busy(qcom->dev);
>  	pm_runtime_put_sync(qcom->dev);
>  
> -	/*
> -	 * Current role changes via usb_role_switch_set_role callback protected
> -	 * internally by mutex lock.
> -	 */
>  	qcom->current_role = next_role;
>  }
[ ... ]
> @@ -756,6 +779,8 @@ static int dwc3_qcom_pm_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	guard(mutex)(&qcom->role_lock);
> +
>  	ret = dwc3_qcom_suspend(qcom, wakeup);
>  	if (ret)
>  		return ret;

[Severity: High]
What happens if dwc3_qcom_set_role_notifier() runs while dwc3_pm_suspend()
is executing, before dwc3_qcom_pm_suspend() acquires qcom->role_lock and
sets qcom->pm_suspended = true?

If the device enters system suspend while already runtime-suspended
(RPM_SUSPENDED, qcom->is_suspended == true) with direct_complete not used
(for example, when wakeup is enabled), dwc3_pm_suspend() ->
dwc3_suspend_common() sees pm_runtime_suspended(dwc->dev) in device mode
and returns 0 without suspending the gadget or exiting the core.

Because the PM core holds a runtime PM usage reference from device_prepare()
and only disables runtime PM later in device_suspend_late(), a concurrent
dwc3_qcom_set_role_notifier() running before guard(mutex)(&qcom->role_lock)
sees qcom->pm_suspended == false and calls pm_runtime_resume_and_get(). That
invokes dwc3_qcom_runtime_resume(), turning on the glue clocks (setting
qcom->is_suspended = false) and resuming the DWC3 core, PHYs, and gadget via
dwc3_runtime_resume(), while the subsequent pm_runtime_put_sync() does not
suspend the device because the usage count is elevated.

dwc3_qcom_pm_suspend() then acquires qcom->role_lock and calls
dwc3_qcom_suspend(qcom, wakeup), which sees qcom->is_suspended == false and
disables the glue clocks and interconnects while the DWC3 core and PHYs
remain active.

Additionally, if the device was active in USB_ROLE_DEVICE and a role switch
to USB_ROLE_NONE runs during dwc3_pm_suspend() after dwc3_gadget_suspend(),
dwc3_qcom_set_role_notifier() sees qcom->pm_suspended == false, clears
qcom->vbus_override_pending, and disables the VBUS override in QSCRATCH
immediately. On the subsequent dwc3_pm_resume(), dwc3_gadget_resume() ->
dwc3_qcom_run_stop_notifier(dwc, true) re-enables the VBUS override, and
dwc3_qcom_pm_resume() will not clear it because qcom->vbus_override_pending
is false.

> @@ -772,17 +797,29 @@ static int dwc3_qcom_pm_resume(struct device *dev)
>  	bool wakeup = device_may_wakeup(dev);
>  	int ret;
>  
> -	ret = dwc3_qcom_resume(qcom, wakeup);
> -	if (ret)
> -		return ret;
> +	scoped_guard(mutex, &qcom->role_lock) {
> +		ret = dwc3_qcom_resume(qcom, wakeup);
> +		if (ret)
> +			return ret;
>  
> -	qcom->pm_suspended = false;
> +		qcom->pm_suspended = false;
> +	}
>  
>  	ret = dwc3_pm_resume(&qcom->dwc);
> -	if (ret)
> -		return ret;
>  
> -	return 0;
> +	/*
> +	 * Apply a VBUS override update from a role switch that came in while
> +	 * suspended. Do it after the core has resumed, as resuming the gadget
> +	 * enables the override and the role switch is the more recent event.
> +	 */
> +	scoped_guard(mutex, &qcom->role_lock) {
> +		if (qcom->vbus_override_pending) {
> +			dwc3_qcom_vbus_override_enable(qcom, qcom->vbus_override);
> +			qcom->vbus_override_pending = false;
> +		}
> +	}
> +
> +	return ret;
>  }

[Severity: High]
Should qcom->pm_suspended = false be moved into the second scoped_guard()
block after dwc3_pm_resume(&qcom->dwc) completes?

By clearing qcom->pm_suspended and releasing qcom->role_lock before calling
dwc3_pm_resume(&qcom->dwc), a concurrent dwc3_qcom_set_role_notifier() call
that arrives while dwc3_pm_resume() is executing will see
qcom->pm_suspended == false and take the active path instead of deferring
via qcom->vbus_override_pending:

1. If the device was in USB_ROLE_DEVICE and RPM_ACTIVE before suspend,
   pm_runtime_resume_and_get(qcom->dev) in dwc3_qcom_set_role_notifier()
   succeeds (rpm_resume() returns 1 while runtime PM is disabled in
   dwc3_pm_resume() because runtime_status and last_status are both
   RPM_ACTIVE). For a switch to USB_ROLE_NONE, dwc3_qcom_set_role_notifier()
   sets qcom->vbus_override_pending = false and disables the VBUS override
   in QSCRATCH. Moments later, dwc3_pm_resume() -> dwc3_resume_common() ->
   dwc3_gadget_resume() -> dwc3_gadget_soft_connect() ->
   dwc3_gadget_run_stop(dwc, true) -> dwc3_qcom_run_stop_notifier(dwc, true)
   calls dwc3_qcom_vbus_override_enable(qcom, true) without holding
   qcom->role_lock, racing on non-atomic QSCRATCH register accesses and
   re-enabling the VBUS override. When dwc3_qcom_pm_resume() reaches the
   second scoped_guard(), qcom->vbus_override_pending is false, leaving the
   VBUS override stuck enabled in QSCRATCH while qcom->current_role is
   USB_ROLE_NONE.

2. If the device was RPM_SUSPENDED before system suspend, inside
   dwc3_pm_resume():

   drivers/usb/dwc3/core.c:dwc3_pm_resume() {
       ...
       pm_runtime_disable(dev);
       ret = pm_runtime_set_active(dev);
       if (ret)
           goto out;

       ret = dwc3_resume_common(dwc, PMSG_RESUME);
       ...
   }

   pm_runtime_disable(dev) records dev->power.last_status = RPM_SUSPENDED,
   and pm_runtime_set_active(dev) updates runtime_status without changing
   last_status. When dwc3_qcom_set_role_notifier() calls
   pm_runtime_resume_and_get(qcom->dev) during this window, rpm_resume()
   returns -EACCES because disable_depth > 0 and last_status != RPM_ACTIVE,
   dropping the role switch and VBUS override update.

3. If the device was RPM_SUSPENDED before system suspend and
   dwc3_qcom_set_role_notifier() runs after qcom->pm_suspended = false is
   set but before dwc3_pm_resume() calls pm_runtime_disable(dev),
   pm_runtime_resume_and_get(qcom->dev) will synchronously invoke
   dwc3_qcom_runtime_resume() -> dwc3_runtime_resume(&qcom->dwc)
   concurrently with dwc3_pm_resume(&qcom->dwc).

Keeping qcom->pm_suspended = true until the second scoped_guard() block
after dwc3_pm_resume(&qcom->dwc) finishes would ensure role switches during
dwc3_pm_resume() are also deferred and applied once the core has resumed.

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