Re: [PATCH v11 03/36] phy: rockchip: usbdp: Do not lose USB3 PHY status

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Preserving the USB status bit during a mode change to 4-lane DP prevents the PHY from ever powering off, breaking system PM.
- [Medium] Removing the unconditional clear of `udphy->status` introduces a state desynchronization and unbalanced clock disable bug if `rk_udphy_setup()` fails during a mode change.
--

--- Patch [3]: [PATCH v11 03/36] phy: rockchip: usbdp: Do not lose USB3 PHY status ---
commit 183c8937bbbbcb2c04198e33a8cf6f50111a49a2
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Do not lose USB3 PHY status

By default DWC3 requests the USB3 PHY once and keeps it enabled all the time.
When DisplayPort is being requested later on, a mode change is needed which
re-initializes the PHY. During re-initialization the status variable has
incorrectly been cleared, which means the tracking information for USB3 is lost.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index a5ffdf7a6e76b..12dfdd470a782 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1008,7 +1008,6 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  			rk_udphy_u3_port_disable(udphy, false);
>  		udphy->mode_change = false;
>  	} else if (udphy->mode_change) {
> -		udphy->status = UDPHY_MODE_NONE;

[Severity: Medium]
If rk_udphy_setup() fails during a mode change, could this lead to an unbalanced
clock disable?

rk_udphy_power_on() calls rk_udphy_disable(), but if rk_udphy_setup() fails:

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

it returns early without clearing udphy->status since the unconditional
reset to UDPHY_MODE_NONE was removed.

Later, if rk_udphy_power_off() is called to unwind, it clears the old mode bit
from udphy->status, reaches UDPHY_MODE_NONE, and calls rk_udphy_disable() a
second time on already-disabled clocks.

[Severity: High]
Does preserving the USB status bit during a mode change to 4-lane DP prevent
the PHY from powering off?

When Type-C negotiates 4-lane DisplayPort, udphy->mode is updated exclusively
to UDPHY_MODE_DP.

Because the reset is removed here, rk_udphy_power_on() preserves the
UDPHY_MODE_USB bit in udphy->status when it updates the mode:

	udphy->status |= mode;

When the USB subsystem suspends and calls rk_udphy_usb3_phy_exit(), it
performs an early bailout:

static int rk_udphy_usb3_phy_exit(struct phy *phy)
{
	...
	/* DP only or high-speed */
	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
		goto unlock;

This leaves the USB bit permanently trapped in udphy->status.

Finally, when DP is powered off, udphy->status resolves to UDPHY_MODE_USB,
which skips the physical rk_udphy_disable() call and leaves the PHY running.

>  		if (udphy->mode == UDPHY_MODE_DP)
>  			rk_udphy_u3_port_disable(udphy, true);
>

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