Re: [PATCH v5 02/18] 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:

New issues:
- [High] State desynchronization on `rk_udphy_init` failure causes subsequent power-on retries to falsely return success without re-initializing the hardware, leading to dead PHY or bus hangs.

Pre-existing issues:
- [High] Subsystem power-off requests permanently leak `udphy->status` bits if the Type-C mode has physically changed, causing a perpetual resource leak of the PHY clocks.
--

commit 93e296ece1bf03449fcdb7d0485cb77da80dd9b7
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

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

This commit fixes an issue where the USB3 tracking information is lost
when the DisplayPort is re-initialized. It does this by preventing the
status variable from being cleared during mode changes in the PHY driver.

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

[Severity: High]
If rk_udphy_init() fails (for example, returning -EPROBE_DEFER after a lock
timeout), will this lead to a permanent state desynchronization?

By removing the clearing of udphy->status, if the initialization fails,
udphy->mode_change is already set to false but udphy->status remains
non-zero.

If the subsystem later retries rk_udphy_power_on(), it will see
udphy->status != UDPHY_MODE_NONE and udphy->mode_change == false. Could
this cause it to bypass all initialization, blindly update the status, and
falsely return success while the PHY hardware remains disabled and in reset?

[Severity: High]
This is a pre-existing issue, but removing the forced clearing of
udphy->status here exposes a potential permanent resource leak of the PHY
clocks.

Looking at rk_udphy_power_off():

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

and rk_udphy_usb3_phy_exit():

	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
		return 0;

If the Type-C multiplexer switches from DP_USB to a DP_ONLY mode,
udphy->mode loses the USB bit. If the USB controller then unbinds and calls
phy_exit() while in DP_ONLY mode, the early returns above prevent clearing
the UDPHY_MODE_USB bit from udphy->status.

When DP eventually powers off, will udphy->status remain non-zero due to
this orphaned USB bit, preventing rk_udphy_disable() from ever executing
and leaking the PHY clocks and power?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-rockchip-usbdp-cleanup-v5-0-efc83069869f@collabora.com?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy