Re: [PATCH v13 25/35] phy: rockchip: usbdp: Avoid xHCI SErrors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The initial setup path for `UDPHY_MODE_NONE` incorrectly re-enables the USB3 port when the device only supports high-speed (`udphy->hs == true`), overriding intentional configuration.
- [High] The driver relies on the dynamically mutating `udphy->mode` to filter PHY power-off requests, which can permanently leak enabled clocks if the Type-C mode changes between power-on and power-off.
--

--- Patch [25]: [PATCH v13 25/35] phy: rockchip: usbdp: Avoid xHCI SErrors ---
commit c94fbaf2a245e81d949d0da907a2acb799ce1846
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

    phy: rockchip: usbdp: Avoid xHCI SErrors
    
    The USBDP PHY provides the PIPE clock to the USB3 controller, which
    means the PHY must be fully running when anything tries to access
    the xHCI registers.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index fba35510d88ce..c7f09040185b6 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1000,23 +992,24 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  	}
>  
>  	if (udphy->status == UDPHY_MODE_NONE) {
> -		udphy->mode_change = false;
>  		ret = rk_udphy_setup(udphy);
>  		if (ret)
>  			return ret;
>  
>  		if (udphy->mode & UDPHY_MODE_USB)
>  			rk_udphy_u3_port_disable(udphy, false);

[Severity: High]
This isn't a bug introduced by this patch, but should the check above also 
verify !udphy->hs? 

When the PHY is configured for high-speed only, rk_udphy_usb3_phy_init() 
intentionally disables the USB3 port. If DP is later initialized and enters 
this udphy->status == UDPHY_MODE_NONE branch, it appears the USB3 port will be
unconditionally re-enabled, overriding the explicit high-speed configuration.

You added the !udphy->hs check to the re-init branch below, which prevents 
this issue there. Does the initial setup branch need the same check?

> -	} else if (udphy->mode_change) {
> -		udphy->mode_change = false;
> -		udphy->status = UDPHY_MODE_NONE;
> -		if (udphy->mode == UDPHY_MODE_DP)
> -			rk_udphy_u3_port_disable(udphy, true);
> +		udphy->phy_needs_reinit = false;
> +	} else if (udphy->phy_needs_reinit) {
> +		rk_udphy_u3_port_disable(udphy, true);
> +		udelay(10);
>  
> -		rk_udphy_disable(udphy);
> -		ret = rk_udphy_setup(udphy);
> +		ret = rk_udphy_init(udphy);
>  		if (ret)
>  			return ret;
> +
> +		if (!udphy->hs && udphy->mode & UDPHY_MODE_USB)
> +			rk_udphy_u3_port_disable(udphy, false);
> +		udphy->phy_needs_reinit = false;
>  	}
>  
>  	udphy->status |= mode;

[ ... ]

> @@ -1303,35 +1274,30 @@ static const struct phy_ops rk_udphy_dp_phy_ops = {
>  static int rk_udphy_usb3_phy_init(struct phy *phy)
>  {
>  	struct rk_udphy *udphy = phy_get_drvdata(phy);
> -	int ret = 0;
>  
> -	mutex_lock(&udphy->mutex);
> +	guard(mutex)(&udphy->mutex);
> +
>  	/* DP only or high-speed, disable U3 port */
>  	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
>  		rk_udphy_u3_port_disable(udphy, true);
> -		goto unlock;
> +		return 0;
>  	}
>  
> -	ret = rk_udphy_power_on(udphy, UDPHY_MODE_USB);
> -
> -unlock:
> -	mutex_unlock(&udphy->mutex);
> -	return ret;
> +	return rk_udphy_power_on(udphy, UDPHY_MODE_USB);
>  }
>  
>  static int rk_udphy_usb3_phy_exit(struct phy *phy)
>  {
>  	struct rk_udphy *udphy = phy_get_drvdata(phy);
>  
> -	mutex_lock(&udphy->mutex);
> +	guard(mutex)(&udphy->mutex);
> +
>  	/* DP only or high-speed */
>  	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
> -		goto unlock;
> +		return 0;

[Severity: High]
This is a pre-existing issue, but does evaluating the dynamically mutable
udphy->mode here risk leaking resources?

If a Type-C mux event changes udphy->mode (e.g., switching from USB+DP to 
DP-only) while the PHY is active, udphy->mode & UDPHY_MODE_USB will become
false.

When the USB framework later powers off the PHY, this check will abort the
exit early. The USB bit in udphy->status will remain set permanently, and 
rk_udphy_power_off() will never fully disable the PHY and its clocks.

Should this instead check the software power state (udphy->status) rather
than the requested hardware mode?

>  
>  	rk_udphy_power_off(udphy, UDPHY_MODE_USB);
>  
> -unlock:
> -	mutex_unlock(&udphy->mutex);
>  	return 0;
>  }

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