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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing error handling for pm_runtime_get_sync() in dwc3_usb3_phy_notify() can lead to hardware access on a suspended device and PM usage count leak.
- [High] AB-BA deadlock between PHY notifier rwsem and Runtime PM state lock during DWC3 runtime suspend.
- [High] Concurrent read-modify-write on DWC3_GUSB3PIPECTL can silently clear the newly added PHYSOFTRST bit.
- [Medium] The PM runtime usage counter is permanently leaked if the DWC3 driver unbinds mid-reset.
--

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

usb: dwc3: core: 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/core.c b/drivers/usb/dwc3/core.c
> index 517aa7f1486da..899decbd0da04 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
[ ... ]
> @@ -880,10 +886,78 @@ 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:
> +		pm_runtime_get_sync(dwc->dev);

[Severity: High]
Does this code handle the case where pm_runtime_get_sync() fails?

If pm_runtime_get_sync() returns a negative error code (failing to resume the
device), it still increments the PM usage counter. Since the return value is
ignored, the code will proceed to unconditionally access DWC3 hardware
registers via dwc3_readl() and dwc3_writel(). Accessing registers on a
suspended, clock-gated device can trigger a synchronous external abort
(SError) or bus fault, causing a kernel panic. 

Should this error path balance the usage counter via pm_runtime_put_noidle()
and return an error to prevent the leak and crash?

[Severity: High]
Can this pm_runtime_get_sync() call cause a deadlock during runtime suspend?

When a PHY reset notification arrives, the blocking_notifier_call_chain()
holds phy->notifier.rwsem (read lock), and dwc3_usb3_phy_notify() calls
pm_runtime_get_sync().

If DWC3 is concurrently runtime-suspending (in DEVICE mode, the state is
RPM_SUSPENDING), pm_runtime_get_sync() blocks waiting for the suspend to
finish.

The suspend thread calls dwc3_suspend_common() -> dwc3_core_exit() ->
dwc3_phy_unregister_notifiers(), which attempts to acquire
phy->notifier.rwsem (write lock) via blocking_notifier_chain_unregister():

dwc3_phy_unregister_notifiers()
    phy_unregister_notifier()
        blocking_notifier_chain_unregister() -> blocks on write lock

The two threads would mutually block, causing a strict AB-BA system deadlock.

> +
> +		/*
> +		 * 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);

[Severity: High]
Can the PHYSOFTRST bit be silently cleared by concurrent register accesses?

While dwc3_usb3_phy_notify() modifies DWC3_GUSB3PIPECTL holding dwc->lock,
the existing dwc3_enable_susphy() function also modifies this register using a
read-modify-write sequence, but without holding dwc->lock:

dwc3_enable_susphy() {
    ...
    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);
    ...
}

If dwc3_enable_susphy() executes concurrently (e.g., triggered via
__dwc3_set_mode() during a role switch when the PHY reset is also triggered),
it can read the register before PHYSOFTRST is set and write it back after,
inadvertently clearing the bit.

> +		break;
> +
> +	case PHY_NOTIFY_POST_RESET:
[ ... ]
> +		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);
> +}

[Severity: Medium]
Can this cause a PM runtime usage counter leak if the driver unbinds mid-reset?

PHY_NOTIFY_PRE_RESET increments the PM usage counter via pm_runtime_get_sync().

If the driver is unbound (dwc3_remove() -> dwc3_core_remove() ->
dwc3_core_exit()) before PHY_NOTIFY_POST_RESET is sent,
dwc3_phy_unregister_notifiers() cleanly unregisters the notifier.

Because the notifier is unregistered, the DWC3 driver never receives the
POST_RESET event, skipping the corresponding pm_runtime_put_autosuspend(). The
PM usage counter would remain unbalanced, preventing the device instance from
ever suspending if rebound.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=30