Re: [PATCH v12 30/36] usb: dwc3: core: support PHY reset notifications

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The global `phy_reset_count` atomic variable is incorrectly used to track the reset state of multiple independent USB3 ports, and is read non-atomically, leading to cross-port state corruption, TOCTOU race conditions, and unbalanced PM usage counters.
- [High] The PHY reset workaround is completely bypassed if Runtime PM is disabled in the kernel configuration or via sysfs, causing the very hardware corruption the patch intends to fix.
--

--- Patch [30]: [PATCH v12 30/36] usb: dwc3: core: support PHY reset notifications ---
commit 9e1e85d93d9ab9e98188f6d4d342fd431f334513
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: dwc3: core: support PHY reset notifications

This commit handles external PHY reset notifications on Rockchip platforms
by asserting PHYSOFTRST before the PHY disables and deasserting it when the
PHY returns. This prevents the DWC3 IP from corrupting the PHY's internal
state and failing to lock PLL clocks.

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 517aa7f1486da..4d0b4c1c73f11 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c

[ ... ]

> @@ -880,6 +881,90 @@ static int dwc3_phy_init(struct dwc3 *dwc)
>  	return ret;
>  }
>  
> +static int dwc3_usb3_phy_notify(struct notifier_block *nb,
> +				unsigned long action, void *data)
> +{
> +	struct dwc3_phy_nb *pnb = container_of(nb, struct dwc3_phy_nb, nb);
> +	struct dwc3 *dwc = pnb->dwc;
> +	int port = pnb->port_index;
> +	unsigned long flags;
> +	u32 reg;
> +
> +	switch (action) {
> +	case PHY_NOTIFY_PRE_RESET:
> +		/*
> +		 * If already suspended, the resume path will reinit GUSB3PIPECTL
> +		 * via dwc3_core_init(). A forced resume is not possible as that
> +		 * would call phy_init() resulting in a deadlock. Due to the
> +		 * phy_init() in the resume path there is also no need to block
> +		 * async RPM resume on our side, since the PHY synchronizes it
> +		 * for us.
> +		 */
> +		if (pm_runtime_get_if_active(dwc->dev) <= 0)
> +			return NOTIFY_OK;

[Severity: High]
If CONFIG_PM is disabled, or runtime PM is disabled for this device via
sysfs, pm_runtime_get_if_active() returns -EINVAL.

Will this check treat the -EINVAL error code as suspended and bypass the
workaround completely, even though the DWC3 IP is actively running?

Also, if the check is later modified to proceed on -EINVAL, would the
POST_RESET path underflow the PM counter? pm_runtime_get_if_active()
doesn't increment the usage count when returning -EINVAL, but POST_RESET
unconditionally calls pm_runtime_put_autosuspend().

> +
> +		atomic_inc(&dwc->phy_reset_count);
> +
> +		/*
> +		 * 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.
> +		 */
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
> +		reg |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		break;
> +
> +	case PHY_NOTIFY_POST_RESET:
> +		if (!atomic_read(&dwc->phy_reset_count))
> +			return NOTIFY_OK;

[Severity: High]
Can this global count cause state corruption on multi-port systems if
notifications interleave with PM state changes?

For example, if Port 0 starts a reset while suspended, it skips the PRE_RESET
workaround and doesn't increment phy_reset_count.

If the controller resumes and Port 1 starts a reset, it increments the
global phy_reset_count to 1.

When Port 0 finishes its reset and hits POST_RESET, it sees phy_reset_count
as 1. Won't it erroneously clear PHYSOFTRST for Port 0 (which was never set),
call pm_runtime_put_autosuspend() unbalanced, and decrement the count to 0?

If Port 1 then finishes its reset, won't it see phy_reset_count at 0 and
skip its cleanup, leaving Port 1 permanently stuck in PHYSOFTRST?

> +		/*
> +		 * Deassert PHY soft reset to reconnect the PIPE interface
> +		 * after PHY reinitialization.
> +		 */
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
> +		reg &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +		pm_runtime_put_autosuspend(dwc->dev);
> +		atomic_dec(&dwc->phy_reset_count);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-rockchip-usbdp-cleanup-v12-0-8b41a9a9bef0@collabora.com?part=30

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy