Re: [PATCH v16 4/6] usb: dwc3: rockchip: support PHY reset notifications

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc3: rockchip: PM usage counter underflow due to unbalanced put
- [High] usb: dwc3: rockchip: kernel panic on unclocked register access during removal
--

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

    usb: dwc3: rockchip: support PHY reset notifications
    
    On recent Rockchip platforms (at least RK3588 & RK3576), DWC3 IP is used
    with a USBDP PHY providing USB3 and DP. This PHY needs to be reset when
    the mode changes, which may happen when plugging in different USB-C
    devices.

> diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c
> index 4129aefd6a8b0..17b6c2ca9e192 100644
> --- a/drivers/usb/dwc3/dwc3-rockchip.c
> +++ b/drivers/usb/dwc3/dwc3-rockchip.c

[ ... ]

> +static int dwc3_usb3_phy_notify(struct notifier_block *nb,
> +				unsigned long action, void *data)
> +{
> +	struct dwc3_rk_phy_nb *pnb = container_of(nb, struct dwc3_rk_phy_nb, nb);
> +	struct dwc3_rockchip *dwc_rk = pnb->dwc_rk;
> +	struct dwc3 *dwc = &dwc_rk->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]
Does this error handling correctly account for -EINVAL?

When PM is disabled (such as during driver removal), 
pm_runtime_get_if_active() returns -EINVAL. Since the if (!ret) check
only evaluates to true for 0, the code falls through on -EINVAL without
holding a PM reference.

> +		/*
> +		 * 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_rk->phy_reset_active |= BIT(port);
> +		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));

[Severity: High]
Can accessing hardware registers here cause a kernel panic during removal?

Because dwc3_rk_phy_register_notifiers() uses devm_add_action_or_reset(),
the notifier is unregistered during devres cleanup, which happens after
dwc3_rockchip_remove() returns.

The dwc3_rockchip_remove() path calls dwc3_core_remove(), which disables PM
and shuts down hardware clocks via dwc3_core_exit().

If a PHY reset notification arrives during this window,
pm_runtime_get_if_active() returns -EINVAL, the code falls through to this
point, and we attempt to read registers via dwc3_readl() when clocks are
disabled. Accessing unclocked registers on ARM/Rockchip architectures causes
a synchronous external abort.

Should the driver explicitly unregister the hardware-accessing PHY notifiers
before tearing down the DWC3 core?

> +		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_rk->phy_reset_active & BIT(port))) {
> +			spin_unlock_irqrestore(&dwc->lock, flags);
> +			return NOTIFY_OK;
> +		}
> +
> +		dwc_rk->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]
Does this unconditional put cause a PM usage counter underflow if the initial
get returned -EINVAL?

As seen in __pm_runtime_suspend(), the usage counter is decremented via
rpm_drop_usage_count() before checking the PM enable status. If the counter
was > 0 (held by another component), dropping it prematurely could cause the
device to auto-suspend while in use.

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

[Severity: High]
Similarly, can this unconditional cleanup put cause corruption if the initial
get failed with -EINVAL?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-b4-rockchip-dwc3-rockchip-glue-v16-0-126a2e9133c3@collabora.com?part=4

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