Re: [PATCH v11 06/36] phy: rockchip: usbdp: Drop seamless DP takeover

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] If PHY setup fails during a mode change, the error path leaves the state machine inconsistent, leading to an unbalanced clock disable when the PHY is later powered off.
--

--- Patch [6]: [PATCH v11 06/36] phy: rockchip: usbdp: Drop seamless DP takeover ---
commit 6e8a746d34fb8b93e5acd40e0d0766f740b5b995
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Drop seamless DP takeover
    
This commit removes the initial status checking code from the Rockchip USBDP
combo PHY driver, dropping support for seamless DP takeover. The feature is
currently unsupported by DRM drivers and bootloaders, and would require
complex TCPM state synchronization for USB-C alternate modes. Removing it
cleans up unused code until it can be properly implemented.

Closes: https://lore.kernel.org/linux-phy/20260612164107.C7DB21F000E9@smtp.kernel.org/

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 3fc8222fcaec2..6cb9f6b4dbf6d 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c

[ ... ]

> @@ -1495,10 +1468,6 @@ static int rk_udphy_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = rk_udphy_get_initial_status(udphy);
> -	if (ret)
> -		return ret;
> -
>  	mutex_init(&udphy->mutex);
>  	platform_set_drvdata(pdev, udphy);
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential state inconsistency
in the PHY mode change error path.

In drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_on():

	} else if (udphy->mode_change) {
		if (udphy->mode == UDPHY_MODE_DP)
			rk_udphy_u3_port_disable(udphy, true);

		rk_udphy_disable(udphy);
		ret = rk_udphy_setup(udphy);
		if (ret)
			return ret;

If rk_udphy_setup() fails (e.g. due to a PLL lock timeout), the function
returns an error directly without updating udphy->status.

Since the hardware has already been disabled by the preceding call to
rk_udphy_disable(), but udphy->status still reflects the previous active
state, could this lead to a double-disable of the bulk clocks when the
consumer later tears down the initial mode and calls phy_power_off()?

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