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

[Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>]

On 10/23/2026 4:12 AM, Vladyslav Tymoshchuk wrote:
> 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 the system suspend and resume
> callbacks with a mutex, so that a role switch can't run while the
> core or the glue is being suspended or resumed.
> 
> 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-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>

Regards,
Krishna,

> ---
> Changes in v2:
> - Hold role_lock across the whole of dwc3_qcom_pm_suspend() and
>    dwc3_qcom_pm_resume(). In v1 a role switch could still run during
>    dwc3_pm_suspend() or dwc3_pm_resume() and bypass the deferral, as
>    the Sashiko review pointed out [4].
> - Link to v1: https://lore.kernel.org/all/20261018143126.3978-1-tvlad744@gmail.com/
> 
> The review also asked about the direct_complete path, which skips
> these callbacks. It is not taken for this driver: every
> qcom,snps-dwc3 node in the tree has a power domain, and
> genpd_prepare() never returns 1. Where it is taken, the role switch
> is dropped as it is without this patch, and the hardware is not
> touched.
> 
> 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 and without the pmic_glink RFC [5]. 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 15 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.
> - with a test-only msleep() under role_lock, so that a role switch
>    comes in while the lock is held:
>    - 3 s in dwc3_qcom_pm_resume() after dwc3_pm_resume(): the switch
>      from a cable plug during s2idle waited for the lock and then set
>      the override directly;
>    - 20 s in dwc3_qcom_pm_suspend() before dwc3_pm_suspend(): the
>      switch from an unplug waited for the lock and was deferred. The
>      unplug aborted the suspend, and dwc3_qcom_pm_resume() applied the
>      update. No reset.
> 
> 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]. Since v1, I have also sent an RFC
> that holds back the PMIC GLINK notifications over system sleep [5].
> With it, an unplug during s2idle on SM8750 no longer reaches this
> notifier. This patch is needed either way: the RFC does not hold back
> a role switch that is already in flight when the system suspend
> starts, and callers of usb_role_switch_set_role() outside pmic_glink,
> TCPM's kthread worker for one, are not frozen.
> 
> 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/
> [4] https://lore.kernel.org/all/sashiko-outbox-176222@kernel.org/
> [5] https://lore.kernel.org/all/20261022164151.592-1-tvlad744@gmail.com/
> 
>   drivers/usb/dwc3/dwc3-qcom.c | 61 +++++++++++++++++++++++++++++-------
>   1 file changed, 50 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index ac68b4218b56..b22a72e3c1b9 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),
> @@ -752,6 +775,12 @@ static int dwc3_qcom_pm_suspend(struct device *dev)
>   	bool wakeup = device_may_wakeup(dev);
>   	int ret;
>   
> +	/*
> +	 * A role switch that comes in now waits until the glue is suspended
> +	 * and is then deferred, see dwc3_qcom_set_role_notifier().
> +	 */
> +	guard(mutex)(&qcom->role_lock);
> +
>   	ret = dwc3_pm_suspend(&qcom->dwc);
>   	if (ret)
>   		return ret;
> @@ -772,6 +801,8 @@ static int dwc3_qcom_pm_resume(struct device *dev)
>   	bool wakeup = device_may_wakeup(dev);
>   	int ret;
>   
> +	guard(mutex)(&qcom->role_lock);
> +
>   	ret = dwc3_qcom_resume(qcom, wakeup);
>   	if (ret)
>   		return ret;
> @@ -779,10 +810,18 @@ static int dwc3_qcom_pm_resume(struct device *dev)
>   	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.
> +	 */
> +	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