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

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

[Vladyslav Tymoshchuk]

dwc3_qcom_set_role_notifier() updates the VBUS override in QSCRATCH
synchronously, from the context of the usb_role_switch_set_role()
caller. The glue assumes that role switches only happen from a
freezable workqueue, but that holds for the dwc3 core's mode switch,
not for this notifier: the UCSI connector work, for one, runs on the
non-freezable system workqueue.

On SM8750, when the USB cable is unplugged while the system is in
s2idle, the UCSI connector work for the change runs before the devices
have resumed. It switches the role to USB_ROLE_NONE, and the notifier
accesses QSCRATCH while the glue clocks are still off, before
dwc3_qcom_pm_resume() turns them back on. The access does not complete
and the SoC resets. pm_runtime_resume_and_get() does not prevent this:
runtime PM is disabled at that point and the device is still marked
active, so it returns success without resuming anything.

While the glue is suspended for system sleep, only record the VBUS
override update and apply it from dwc3_qcom_pm_resume() once the
clocks are on. Apply it after the core has resumed, since resuming the
gadget enables the override and the role switch is the more recent
event. Serialize the notifier against system suspend and resume with a
mutex, so that a role switch can't race with the clocks being turned
off.

Fixes: 21188e8d6d75 ("usb: dwc3: qcom: Implement glue callbacks to facilitate runtime suspend")
Cc: stable@vger.kernel.org
Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>
---
Tested on SM8750 (OnePlus Pad 3; the board DT is not upstream yet),
v7.3-rc5 plus board patches, with the UCSI connector work still on
system_wq. The test kernel also has local ucsi_glink and qcom_battmgr
changes that make a cable plug or unplug wake the system from s2idle,
similar to Jishnu Prakash's ucsi_glink patch [3]:
- cable unplugged during s2idle: resumed in 2 of 2 runs; without this
  patch the SoC reset in all 6 earlier runs. The switch to
  USB_ROLE_NONE came 14-17 ms after the start of the resume, during
  the noirq phase, and was only recorded; dwc3_qcom_pm_resume()
  cleared the override about a second later.
- cable plugged in during s2idle, s2idle with the cable connected,
  and unplug/replug while awake: no change in behaviour.

I have not tested this patch without the wakeup changes.

The UCSI side is discussed in Abel Vesa's RFC to queue the connector
work on system_freezable_wq [1]. This patch is needed either way, as
not every caller of usb_role_switch_set_role() is frozen during system
sleep.

Faisal Hassan's SA8255P series [2] also changes dwc3_qcom_pm_suspend()
and dwc3_qcom_pm_resume() (the wakeup argument becomes a pm_message_t);
whichever goes in second needs a small rebase.

I used an AI assistant (Claude) for the analysis and to draft this
patch and its description; I reviewed the result and ran the tests on
my device.

[1] https://lore.kernel.org/all/20250205-ucsi-schedule-conn-worker-on-freezable-wq-v1-1-107d1356b77b@linaro.org/
[2] https://lore.kernel.org/all/20261010053246.2991984-3-faisal.hassan@oss.qualcomm.com/
[3] https://lore.kernel.org/all/20260722-ucsi_glink_wakeup-v2-1-b43dee56df24@oss.qualcomm.com/

 drivers/usb/dwc3/dwc3-qcom.c | 67 ++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index ac68b4218b56..2997479988c7 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -10,6 +10,7 @@
 #include <linux/irq.h>
 #include <linux/of_clk.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/kernel.h>
 #include <linux/interconnect.h>
 #include <linux/platform_device.h>
@@ -85,6 +86,11 @@ struct dwc3_qcom {
 	struct icc_path		*icc_path_apps;
 
 	enum usb_role		current_role;
+
+	/* Serializes the role notifier against system suspend and resume */
+	struct mutex		role_lock;
+	bool			vbus_override_pending;
+	bool			vbus_override;
 };
 
 #define to_dwc3_qcom(d) container_of((d), struct dwc3_qcom, dwc)
@@ -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;
 	}
 
-	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;
 }
 
@@ -624,6 +643,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 
 	qcom->dev = &pdev->dev;
 
+	ret = devm_mutex_init(dev, &qcom->role_lock);
+	if (ret)
+		return ret;
+
 	qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
 	if (IS_ERR(qcom->resets)) {
 		return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
@@ -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;
@@ -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;
 }
 
 static void dwc3_qcom_complete(struct device *dev)

base-commit: a90ee4305c4a5df72c11b31dacfdc76e00fcf78a
-- 
2.43.0

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

[sashiko-bot]

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