Re: [PATCH v18 5/7] usb: dwc3: rockchip: add USBDP PHY reset quirk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: dwc3: rockchip: dwc3_rockchip_phy_pre_reset() skips RPM_RESUMING state
- [High] usb: dwc3: rockchip: unsynchronized runtime PM in dwc3_rockchip_phy_reset()
- [Medium] usb: dwc3: rockchip: race between PHY reset quirk and __dwc3_set_mode()
--

commit 175128dd0be21c712a951377a4fee0fe496c4cf7
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: dwc3: rockchip: add USBDP PHY reset quirk

This commit adds a reset handler for the Rockchip USBDP PHY in the Rockchip
DWC3 glue driver to assert and deassert DWC3_GUSB3PIPECTL_PHYSOFTRST around
external PHY resets, preventing DWC3 PIPE traffic from interfering with PHY
PLL locking. It also exports the dwc3_readl and dwc3_writel tracepoint
symbols for use by the glue driver.

> diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c
> index 4129aefd6a8b0..75e647e04cc52 100644
> --- a/drivers/usb/dwc3/dwc3-rockchip.c
> +++ b/drivers/usb/dwc3/dwc3-rockchip.c
> @@ -2,11 +2,105 @@
>  /* Copyright (c) 2026, Collabora Ltd. */
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/phy/rockchip-usbdp.h>
>  #include <linux/pm_runtime.h>
>  #include "glue.h"
> +#include "io.h"
>  
>  struct dwc3_rockchip {
>  	struct dwc3		dwc;
> +	u8			phy_reset_active;
> +};
> +
> +static void dwc3_rockchip_phy_pre_reset(struct dwc3_rockchip *dwc_rk, int port)
> +{
> +	struct dwc3 *dwc = &dwc_rk->dwc;
> +	u32 reg;
> +
> +	/*
> +	 * If the device is runtime suspended, there is nothing to do and RPM
> +	 * resume will do the necessary things. There is no need for a RPM
> +	 * reference either, since DWC3 runtime suspend/resume will call phy
> +	 * init/exit and that is synchronized against ongoing resets.
> +	 */
> +	if (!pm_runtime_active(dwc->dev))
> +		return;

[Severity: High]
Does checking pm_runtime_active(dwc->dev) here miss the case where DWC3 is in
the middle of runtime resume or runtime suspend?

In drivers/base/power/runtime.c:rpm_resume(), the PM core sets
dev->power.runtime_status to RPM_RESUMING before invoking the driver's
runtime_resume callback, and only updates it to RPM_ACTIVE after the callback
returns:

drivers/base/power/runtime.c:rpm_resume() {
	...
	__update_runtime_status(dev, RPM_RESUMING);

	callback = RPM_GET_CALLBACK(dev, runtime_resume);

	dev_pm_disable_wake_irq_check(dev, false);
	retval = rpm_callback(callback, dev);
	...
}

Meanwhile, include/linux/pm_runtime.h:pm_runtime_active() only returns true
when runtime_status is RPM_ACTIVE (or runtime PM is disabled):

static inline bool pm_runtime_active(struct device *dev)
{
	return dev->power.runtime_status == RPM_ACTIVE
		|| dev->power.disable_depth;
}

During runtime resume in DWC3_GCTL_PRTCAP_DEVICE mode, the call chain is:

dwc3_rockchip_runtime_resume()
  -> dwc3_runtime_resume()
    -> dwc3_resume_common()
      -> dwc3_core_init_for_resume()
        -> reset_control_deassert(dwc->reset)
        -> dwc3_clk_enable(dwc)
        -> dwc3_core_init(dwc)
          -> dwc3_phy_setup(dwc)
          -> dwc3_phy_init(dwc)
            -> phy_init()
              -> rk_udphy_usb3_phy_init()
                -> rk_udphy_power_on()
                  -> rk_udphy_run_reset_handler(udphy, true)
                    -> dwc3_rockchip_phy_pre_reset()

At that point, DWC3 reset has been deasserted, clocks are enabled, and
dwc3_phy_setup() has configured DWC3_GUSB3PIPECTL(0), but
dev->power.runtime_status is still RPM_RESUMING. When rk_udphy_power_on()
runs the reset handler (because udphy->status == UDPHY_MODE_NONE or
udphy->phy_needs_reinit is true after a USB-C reconnect or orientation
change), won't dwc3_rockchip_phy_pre_reset() see !pm_runtime_active(dwc->dev)
and return early without asserting DWC3_GUSB3PIPECTL_PHYSOFTRST, causing
rk_udphy_status_check() to fail LCPLL lock and failing dwc3_runtime_resume()
(as well as skipping the VBUS notification in the follow-up commit "usb:
dwc3: rockchip: fix USB-C reconnect in gadget mode")?

Similarly, during runtime suspend, rpm_suspend() sets runtime_status to
RPM_SUSPENDING before invoking the driver callback. While
dwc3_suspend_common() is executing dwc3_gadget_suspend(), synchronize_irq(),
or waiting on udphy->mutex in dwc3_phy_exit() before clocks are disabled and
reset is asserted, wouldn't a concurrent PHY reset via
rk_udphy_dp_phy_power_on() also see !pm_runtime_active(dwc->dev) and skip
asserting DWC3_GUSB3PIPECTL_PHYSOFTRST while DWC3 is still running?

> +
> +	/*
> +	 * Assert USB3 PHY soft reset within DWC3 before the external
> +	 * PHY resets. This disconnects the PIPE interface, preventing
> +	 * the DWC3 from interfering with PHY reinitialization and
> +	 * avoiding LCPLL lock failures.
> +	 */
> +	scoped_guard(spinlock_irqsave, &dwc->lock) {
> +		dwc_rk->phy_reset_active |= BIT(port);
> +		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
> +		reg |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
> +	}
> +}

[Severity: Medium]
Can the PHY reset callbacks race with DWC3 role switching in
__dwc3_set_mode()?

When a USB-C device with DisplayPort AltMode is plugged in, unplugged, or
undergoes a role swap, tcpm_mux_set() updates the mux/orientation state
(setting udphy->phy_needs_reinit = true in rk_udphy_orien_sw_set() or
rk_udphy_typec_mux_set()) and calls usb_role_switch_set_role() ->
dwc3_usb_role_switch_set() -> dwc3_set_mode(), queuing __dwc3_set_mode() on
system_freezable_wq. Concurrently, DP HPD/link enablement invokes
phy_power_on(dp->phy) -> rk_udphy_dp_phy_power_on() -> rk_udphy_power_on()
under udphy->mutex.

First, __dwc3_set_mode() calls dwc3_host_exit(), dwc3_gadget_exit(), and
dwc3_host_init() -> dwc3_xhci_plat_start(), all of which call
dwc3_enable_susphy() without holding dwc->lock:

drivers/usb/dwc3/core.c:dwc3_enable_susphy() {
	...
	for (i = 0; i < dwc->num_usb3_ports; i++) {
		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(i));
		if (enable && !dwc->dis_u3_susphy_quirk)
			reg |= DWC3_GUSB3PIPECTL_SUSPHY;
		else
			reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;

		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(i), reg);
	}
}

Could an unlocked read-modify-write in dwc3_enable_susphy() interleave with
dwc3_rockchip_phy_pre_reset() or dwc3_rockchip_phy_post_reset() and clear
DWC3_GUSB3PIPECTL_PHYSOFTRST during the PHY reset or re-assert it after
dwc3_rockchip_phy_post_reset() clears it?

Second, between dwc3_rockchip_phy_pre_reset() and
dwc3_rockchip_phy_post_reset(), rk_udphy_init() sleeps in usleep_range() and
polls PLL status in regmap_read_poll_timeout() while holding only
udphy->mutex. Since __dwc3_set_mode() holds only dwc->mutex, can
__dwc3_set_mode() assert and deassert DWC3_GCTL_CORESOFTRESET during the PHY
reset window:

drivers/usb/dwc3/core.c:__dwc3_set_mode() {
	...
		reg = dwc3_readl(dwc, DWC3_GCTL);
		reg |= DWC3_GCTL_CORESOFTRESET;
		dwc3_writel(dwc, DWC3_GCTL, reg);

		msleep(100);

		reg = dwc3_readl(dwc, DWC3_GCTL);
		reg &= ~DWC3_GCTL_CORESOFTRESET;
		dwc3_writel(dwc, DWC3_GCTL, reg);
	...
}

resetting DWC3 global registers and clearing DWC3_GUSB3PIPECTL_PHYSOFTRST
while the USBDP PHY reset is still in progress?

> +
> +static void dwc3_rockchip_phy_post_reset(struct dwc3_rockchip *dwc_rk, int port)
> +{
> +	struct dwc3 *dwc = &dwc_rk->dwc;
> +	u32 reg;
> +
> +	scoped_guard(spinlock_irqsave, &dwc->lock) {
> +		if (!(dwc_rk->phy_reset_active & BIT(port)))
> +			return;
> +
> +		dwc_rk->phy_reset_active &= ~BIT(port);
> +
> +		/* Restore normal operation */
> +		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
> +		reg &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
> +	}
> +}

[Severity: High]
Is it safe to rely on DWC3 runtime suspend/resume calling phy_init() and
phy_exit() instead of holding a runtime PM reference across the PHY reset?

In drivers/usb/dwc3/core.c, dwc3_suspend_common() and dwc3_resume_common()
only call dwc3_core_exit() (which calls dwc3_phy_exit() -> phy_exit()) and
dwc3_core_init_for_resume() (which calls dwc3_core_init() -> dwc3_phy_init()
-> phy_init()) during runtime PM when in DWC3_GCTL_PRTCAP_DEVICE mode. In
DWC3_GCTL_PRTCAP_HOST and DWC3_GCTL_PRTCAP_OTG modes, runtime suspend and
resume skip dwc3_core_exit() and dwc3_core_init_for_resume():

drivers/usb/dwc3/core.c:dwc3_suspend_common() {
	...
	case DWC3_GCTL_PRTCAP_HOST:
		if (!PMSG_IS_AUTO(msg) &&
		    (!device_may_wakeup(dwc->dev) || dwc->needs_full_reinit)) {
			dwc3_core_exit(dwc);
			break;
		}
	...
	case DWC3_GCTL_PRTCAP_OTG:
		/* do nothing during runtime_suspend */
		if (PMSG_IS_AUTO(msg))
			break;
	...
}

Because phy_exit() is not called on runtime suspend in host or OTG modes,
rk_udphy_usb3_phy_exit() does not run, udphy->mutex is not acquired by
runtime suspend/resume, and udphy->sw_mode retains UDPHY_MODE_USB while
dwc->dev is runtime-suspended.

If dwc->dev is RPM_ACTIVE in DWC3_GCTL_PRTCAP_HOST or DWC3_GCTL_PRTCAP_OTG
mode when dwc3_rockchip_phy_pre_reset() runs (for example, via
rk_udphy_dp_phy_power_on() -> rk_udphy_power_on()), it sets
dwc_rk->phy_reset_active |= BIT(port) without holding a runtime PM reference.
While rk_udphy_power_on() sleeps in rk_udphy_init(), dwc->dev can
runtime-suspend concurrently without blocking on udphy->mutex, allowing the
USB power domain (RK3588_PD_USB / RK3576_PD_USB) to power down. When
dwc3_rockchip_phy_post_reset() then executes, won't it see
dwc_rk->phy_reset_active & BIT(port) set and call dwc3_readl() and
dwc3_writel() on DWC3_GUSB3PIPECTL(port) while dwc->dev is runtime-suspended,
triggering an arm64 SError?

Conversely, if dwc->dev is already runtime-suspended in DWC3_GCTL_PRTCAP_HOST
or DWC3_GCTL_PRTCAP_OTG mode while DWC3 clocks and reset remain active (and
the shared USB power domain is kept on by sibling controllers), won't
dwc3_rockchip_phy_pre_reset() see !pm_runtime_active(dwc->dev) and return
early without asserting DWC3_GUSB3PIPECTL_PHYSOFTRST, causing the USBDP PHY
reset to fail LCPLL lock while subsequent runtime resume in host or OTG mode
does not reinitialize the DWC3 core or PHY?

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-b4-rockchip-dwc3-rockchip-glue-v18-0-60000d03fca2@collabora.com?part=5