Re: [PATCH v13 18/35] 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] Unclocked register access leading to system hang when an unsupported PHY mode is requested.
--

--- Patch [18]: [PATCH v13 18/35] phy: rockchip: usbdp: Rename mode_change to phy_needs_reinit ---
commit fe40e70a3614f2ec7c81e0e8bd7039efe25202c3
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Rename mode_change to phy_needs_reinit

This commit renames the mode_change property to phy_needs_reinit to better
reflect that a re-initialization is required not just on mode changes, but
also on cable 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)
>  
>  		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) {

[Severity: High]
This is a pre-existing issue, but can returning 0 for unsupported modes
lead to unclocked register accesses and system hangs?

In rk_udphy_power_on(), if the requested mode is not supported by the
hardware, the function prints an info message and returns 0 (success)
instead of an error:

        if (!(udphy->mode & mode)) {
                dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
                return 0;
        }

This success return tricks the caller into proceeding to access unclocked
hardware:

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);
        ...
}

Because the mode was unsupported, the clocks are not enabled. Accessing
the registers in rk_udphy_dp_lane_enable() will then crash the system.

Similarly, rk_udphy_dp_phy_power_off() unconditionally accesses hardware
registers before checking if the mode was supported and powered on:

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);
        ...
}

Does it make sense to return an error code like -EOPNOTSUPP for
unsupported modes, and guard the register accesses in the power-off path?

>  		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/20260714-rockchip-usbdp-cleanup-v13-0-6cb3e769d4c5@collabora.com?part=18