Re: [PATCH v12 31/36] phy: rockchip: usbdp: Add phy reset notification support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Shared `phy_reset_count` across multiple USB3 ports in DWC3 notifier causes stuck `PHYSOFTRST` and unbalanced PM runtime refcounts.
--

--- Patch [31]: [PATCH v12 31/36] phy: rockchip: usbdp: Add phy reset notification support ---
commit a652d5b92503989cfd858925aecf81d1ed290bb7
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Add phy reset notification support
    
This commit adds support for reset notifications to gracefully handle
the PHY being disabled. It is intended to prevent corrupting the
controller's internal state and resolve "cmn ana lcpll lock timeout"
errors after device replugs.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index af915cc377275..0333e846ce343 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1005,24 +1005,39 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  	}
>  
>  	if (udphy->status == UDPHY_MODE_NONE) {
> +		phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_PRE_RESET);
> +

[Severity: High]
Can this cause a port to become permanently stuck in reset due to how the
DWC3 core tracks reset notifications?

Looking at the DWC3 notifier callback in dwc3_usb3_phy_notify(), it uses a
single controller-wide atomic counter (phy_reset_count) rather than tracking
the state on a per-port basis:

drivers/usb/dwc3/core.c:dwc3_usb3_phy_notify() {
    ...
    atomic_inc(&dwc->phy_reset_count);
    ...
}

If this PHY notification is triggered concurrently across multiple USB3 ports,
could the shared counter lead to unbalanced state? For example, if Port 0
executes PHY_NOTIFY_PRE_RESET while the controller is suspended
(pm_runtime_get_if_active <= 0), it skips the sequence and does not increment
the counter.

If the controller then wakes up and Port 1 concurrently executes
PHY_NOTIFY_PRE_RESET, it increments the shared counter to 1 and asserts
Port 1's reset.

When Port 0 later finishes its PHY setup and executes PHY_NOTIFY_POST_RESET,
it sees the shared counter is 1. Does it incorrectly process the post-reset,
decrementing the counter to 0 and dropping a PM refcount?

If so, when Port 1 finally executes PHY_NOTIFY_POST_RESET, it would see the
shared counter is 0 and abort early, potentially leaving Port 1 permanently
stuck with PHYSOFTRST asserted and unbalanced PM runtime refcounts.

Does the state need to be tracked on a per-port basis in the DWC3 core before
enabling these notifications for multi-port controllers?

> +		rk_udphy_u3_port_disable(udphy, true);
> +		udelay(10);
> +
>  		ret = rk_udphy_setup(udphy);
> -		if (ret)
> +		if (ret) {
> +			phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);
>  			return ret;
> +		}
>  
>  		if (!udphy->hs && udphy->mode & UDPHY_MODE_USB)
>  			rk_udphy_u3_port_disable(udphy, false);
>  		udphy->phy_needs_reinit = false;
> +
> +		phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);

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