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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing PHY_NOTIFY_POST_RESET notification on the rk_udphy_setup() error path.
- [High] The return value of phy_notify_reset(..., PHY_NOTIFY_PRE_RESET) is completely ignored.
- [High] The USB3 port is permanently left disabled after a PHY re-initialization.
--

--- Patch [31]: [PATCH v11 31/36] phy: rockchip: usbdp: Add phy reset notification support ---
commit 8e70526891d152fa8ddbd23ac5f83716e6e5bc55
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. This avoids corrupting the controller's internal state
and fixes lock timeout errors after device replugs.

I noticed a few potential regressions related to error handling and hardware
enablement in this patch.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1005,6 +1005,11 @@ 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]
Should the return value of phy_notify_reset() be checked here? If the
consumer's pre-reset notifier returns an error code, proceeding with the
hardware reset could potentially trigger xHCI SErrors, and unconditionally
calling the post-reset notification later might unbalance the PM runtime
reference.

> +
> +		rk_udphy_u3_port_disable(udphy, true);
> +		udelay(10);
> +
>  		ret = rk_udphy_setup(udphy);
>  		if (ret)
>  			return ret;

[Severity: High]
Does this error path leak a PM runtime reference by leaving the DWC3
controller permanently in a pre-reset state? It appears the early return
skips balancing the prior PHY_NOTIFY_PRE_RESET notification with a
PHY_NOTIFY_POST_RESET call.

[ ... ]

> @@ -1012,13 +1017,22 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  		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);
>  	} else if (udphy->phy_needs_reinit) {
> +		phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_PRE_RESET);

[Severity: High]
Similar to the check above, does this code need to handle errors returned
from phy_notify_reset() to avoid hardware issues or underflowing the
atomic reset count?

> +
>  		rk_udphy_u3_port_disable(udphy, true);
>  		udelay(10);
>  
>  		ret = rk_udphy_init(udphy);
> -		if (ret)
> +		if (ret) {
> +			phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);
>  			return ret;
> +		}
> +
> +		phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);
> +
>  		udphy->phy_needs_reinit = false;
>  	}

[Severity: High]
Is the USB3 port intentionally left disabled on this success path? Unlike
the cold power-on branch above it, there is no corresponding call to
rk_udphy_u3_port_disable(udphy, false) when rk_udphy_init() succeeds.
Could this permanently break USB3 connectivity after a PHY re-initialization
or mode change?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-rockchip-usbdp-cleanup-v11-0-a149ac60f76c@collabora.com?part=31