Re: [PATCH v14 06/38] phy: rockchip: usbdp: Drop seamless DP takeover

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] If PHY setup fails during a mode change, the driver returns an error but leaves the software state `udphy->status` incorrectly indicating that the PHY is still powered on, leading to an unbalanced clock disable later.
- [High] The driver no longer asserts the PHY resets during probe, leaving the hardware in an unknown, potentially active state inherited from the bootloader. If the bootloader left the PHY enabled, the CCF will cut its clocks during late initcall, which can cause a bus lockup.
--

--- Patch [6]: [PATCH v14 06/38] phy: rockchip: usbdp: Drop seamless DP takeover ---
commit 50b49f9a40304fe270c1d3495217384507476b1d
Author: Sebastian Reichel <[email protected]>
Subject: phy: rockchip: usbdp: Drop seamless DP takeover

This commit removes seamless DisplayPort takeover functionality from the
rockchip usbdp driver, as it is incomplete and untestable. It removes the
initial status checking and reset deassertion logic during probe.

Closes: https://lore.kernel.org/linux-phy/[email protected]/

> 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;
> -

[Severity: High]
Since rk_udphy_get_initial_status() is removed, the driver no longer manages
the initial bootloader state of the PHY. If the bootloader left the PHY
enabled, could the clock framework's late initcall disable the clocks while
the hardware is still active?

Should a forceful rk_udphy_reset_assert_all(udphy) be added here to ensure
the PHY is cleanly suspended and prevent a bus lockup when unused clocks
are disabled?

>  	mutex_init(&udphy->mutex);
>  	platform_set_drvdata(pdev, udphy);

[ ... ]

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

[Severity: High]
If rk_udphy_setup() fails during a mode change, does returning here leave
udphy->status incorrectly indicating the PHY is still powered on?

If the status is not cleared, could a subsequent teardown via
rk_udphy_power_off() observe the stale udphy->status and call
rk_udphy_disable() again, resulting in an unbalanced clock disable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy