Re: [PATCH v12 18/36] phy: rockchip: usbdp: Rename mode_change to phy_needs_reinit

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Hardware hang due to unclocked memory-mapped register access.
--

--- Patch [18]: [PATCH v12 18/36] phy: rockchip: usbdp: Rename mode_change to phy_needs_reinit ---
commit 879ba283150f41ae77236ca40a0536af8b8e8236
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Rename mode_change to phy_needs_reinit

This patch renames the mode_change property to phy_needs_reinit to better
reflect that the PHY requires re-initialization not just on mode changes, but
also on cable orientation switches. This prepares the driver to handle DP
re-init properly on orientation changes.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 24108816e3b95..e44d19c9d1193 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
[ ... ]
> @@ -951,15 +951,15 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)

[Severity: High]
This is a pre-existing issue, but can this lead to a system hang when the PHY
is unsupported?

Looking at rk_udphy_power_on(), if the requested mode is unsupported,
it returns 0 early without enabling the 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 this returns 0 instead of an error code, rk_udphy_dp_phy_power_on()
assumes the PHY is clocked and proceeds to call rk_udphy_dp_lane_enable():

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_dp_lane_enable(udphy, udphy->dp_lanes);
...
}

This results in accessing memory-mapped PHY registers via pma_regmap when the
corresponding PHY clocks are gated. Could this trigger a synchronous external
abort and a kernel panic?

Additionally, this same sequence appears in rk_udphy_dp_phy_power_off()
which calls rk_udphy_dp_lane_enable(udphy, 0) unconditionally without checking
if the PHY was successfully powered on:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_power_off() {
	mutex_lock(&udphy->mutex);
	rk_udphy_dp_lane_enable(udphy, 0);
	rk_udphy_power_off(udphy, UDPHY_MODE_DP);
...
}

Should rk_udphy_power_on() return an error like -EINVAL when the mode is not
supported, and should rk_udphy_dp_phy_power_off() avoid calling
rk_udphy_dp_lane_enable() if the PHY is not powered on?

>  
>  		if (udphy->mode & UDPHY_MODE_USB)
>  			rk_udphy_u3_port_disable(udphy, false);
> -		udphy->mode_change = false;
> -	} else if (udphy->mode_change) {
> +		udphy->phy_needs_reinit = false;
> +	} else if (udphy->phy_needs_reinit) {
>  		if (udphy->mode == UDPHY_MODE_DP)
>  			rk_udphy_u3_port_disable(udphy, true);
>  
>  		ret = rk_udphy_init(udphy);
>  		if (ret)
>  			return ret;
> -		udphy->mode_change = false;
> +		udphy->phy_needs_reinit = false;
>  	}
>  
>  	udphy->status |= mode;

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