Re: [PATCH v13 29/35] 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] Unhandled `-EINVAL` from `pm_runtime_get_if_active()` leads to unsafe unclocked hardware access and guaranteed PM usage count underflow.
- [Medium] `PHY_NOTIFY_PRE_RESET` skips asserting `PHYSOFTRST` while DWC3 is runtime suspending, undermining the patch's purpose.
--

--- Patch [29]: [PATCH v13 29/35] usb: dwc3: core: support PHY reset notifications ---
commit 6e8dbcf8839d758e54f1776486d2e59f6acec9ca
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: dwc3: core: support PHY reset notifications

This commit adds handling for PHY framework reset notifications to the DWC3
core. It asserts the PHYSOFTRST bit before the external PHY resets and
deasserts it afterward. This prevents the external USBDP PHY from entering
a corrupted state resulting in LCPLL lock failures.

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 517aa7f1486da..10678dd63419a 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
[ ... ]
> @@ -880,6 +881,103 @@ 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;
> +	int ret;
> +
> +	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.
> +		 *
> +		 * pm_runtime_get_if_active() returns 0 when suspended (skip),
> +		 * 1 when active (ref held), or -EINVAL when PM is disabled
> +		 * (device always active). In the -EINVAL case PM ref counting
> +		 * is a no-op, so the unconditional put in POST_RESET is safe.
> +		 */
> +		ret = pm_runtime_get_if_active(dwc->dev);
> +		if (!ret)
> +			return NOTIFY_OK;

[Severity: High]
If pm_runtime_get_if_active() returns -EINVAL (e.g. when PM is disabled),
this check only looks for 0 and allows execution to proceed without having
incremented the usage count.

Since the device might not actually be active or clocked when PM is disabled,
could this lead to unclocked hardware access on DWC3_GUSB3PIPECTL and a kernel
panic?

Also, the comment mentions that PM ref counting is a no-op for -EINVAL,
making the unconditional put in POST_RESET safe. However,
pm_runtime_put_autosuspend() unconditionally decrements the atomic usage
counter via rpm_drop_usage_count(). Won't this cause a usage count underflow?

[Severity: Medium]
If the device state is RPM_SUSPENDING, pm_runtime_get_if_active() returns 0.
Could an external PHY reset occur concurrently while dwc3_suspend_common() is
executing but before it disables clocks?

If so, this returns NOTIFY_OK and bypasses PHYSOFTRST assertion while the
DWC3 IP is still actively driving the PIPE interface, potentially causing the
exact LCPLL lock failure this patch is meant to fix.

> +
> +		/*
> +		 * 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);
> +		dwc->phy_reset_active |= BIT(port);
> +		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:
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		if (!(dwc->phy_reset_active & BIT(port))) {
> +			spin_unlock_irqrestore(&dwc->lock, flags);
> +			return NOTIFY_OK;
> +		}
> +
> +		dwc->phy_reset_active &= ~BIT(port);
> +
> +		/*
> +		 * Deassert PHY soft reset to reconnect the PIPE interface
> +		 * after PHY reinitialization.
> +		 */
> +		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);

[Severity: High]
If pm_runtime_get_if_active() returned -EINVAL earlier, won't this
unconditional put drop the PM reference and lead to a usage count underflow?

> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
[ ... ]
> +static void dwc3_phy_unregister_notifiers(struct dwc3 *dwc)
> +{
> +	int i;
> +
> +	for (i = 0; i < dwc->num_usb3_ports; i++)
> +		phy_unregister_notifier(dwc->usb3_generic_phy[i],
> +					&dwc->usb3_phy_nb[i].nb);
> +
> +	/* Release any PM references from in-flight resets */
> +	for (i = 0; i < dwc->num_usb3_ports; i++) {
> +		if (dwc->phy_reset_active & BIT(i))
> +			pm_runtime_put_autosuspend(dwc->dev);

[Severity: High]
Similarly, if a reset is in-flight when PM is disabled (meaning no reference
was acquired), won't this also cause a usage count underflow?

> +	}
> +	dwc->phy_reset_active = 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-rockchip-usbdp-cleanup-v13-0-6cb3e769d4c5@collabora.com?part=29

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