Re: [PATCH v9 08/38] phy: rockchip: usbdp: Keep clocks running on PHY re-init

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `rk_udphy_dp_phy_power_on()` returns 0 (success) when the DP mode is unsupported by the current Type-C state, leading callers to access clock-gated MMIO registers and causing a Synchronous External Abort (SError).
--

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

    phy: rockchip: usbdp: Keep clocks running on PHY re-init

This commit modifies the PHY mode change logic in rk_udphy_power_on() to avoid
unnecessarily toggling the clocks off and on again. It skips the clock disable
and setup steps, directly calling rk_udphy_init() instead.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 5d0e4f82de2f8..2b27fd1c15d8d 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1022,8 +1022,7 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  		if (udphy->mode == UDPHY_MODE_DP)
>  			rk_udphy_u3_port_disable(udphy, true);
>  
> -		rk_udphy_disable(udphy);
> -		ret = rk_udphy_setup(udphy);
> +		ret = rk_udphy_init(udphy);

[Severity: High]
This isn't a bug introduced by this patch, but rk_udphy_power_on() returns 0
(success) when the requested mode is not supported by the current Type-C
state, which can lead to a Synchronous External Abort (SError).

If rk_udphy_dp_phy_power_on() calls rk_udphy_power_on() before a Type-C Alt
Mode is negotiated, rk_udphy_power_on() returns 0 without enabling the APB
clocks:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_on() {
	if (!(udphy->mode & mode)) {
		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
		return 0;
	}
...
}

Because rk_udphy_power_on() returns success, rk_udphy_dp_phy_power_on()
assumes the PHY is powered on and proceeds to access clock-gated MMIO
registers:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_power_on() {
...
	ret = rk_udphy_power_on(udphy, UDPHY_MODE_DP);
	if (ret)
		goto unlock;

	rk_udphy_dplane_enable(udphy, dp_lanes);
...
}

Here, rk_udphy_dplane_enable() performs read/modify/write operations on
udphy->pma_regmap. Accessing these registers with disabled APB clocks
triggers an SError system crash.

The same vulnerability exists in rk_udphy_dp_phy_power_off() and
rk_udphy_dp_phy_configure(), which unconditionally access the PMA registers
before updating or checking the power state.

Could rk_udphy_power_on() return an error code instead of 0 when the mode
is unsupported, or could the callers verify if the PHY actually powered on
before accessing the registers?

>  		if (ret)
>  			return ret;
>  		udphy->mode_change = false;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-rockchip-usbdp-cleanup-v9-0-e31efbb62d2e@collabora.com?part=8