Re: [PATCH v11 02/36] phy: rockchip: usbdp: Update mode_change after error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] When changing the PHY mode from DP-only to a state that includes USB (e.g., DP+USB), the U3 port remains permanently disabled, breaking USB 3.0 functionality.
- [High] The PHY is left permanently enabled and clocks leak due to flawed mode-checking in the power off path during Type-C role switches.
- [High] Re-initializing the PHY during a mode change while the USB controller is active disables PHY clocks without re-routing the DWC3 clock, leading to a fatal SError on register access.
--

--- Patch [2]: [PATCH v11 02/36] phy: rockchip: usbdp: Update mode_change after error handling ---
commit 1bc42e532da1064edae3f14df8f37ade40a0072d
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Update mode_change after error handling

If rk_udphy_init() or rk_udphy_setup() fails, the reinit will not be
tried again. Fix this by only updating the variable after all potential
errors have been handled.

Link: https://lore.kernel.org/linux-phy/20260612163835.8D5471F000E9@smtp.kernel.org/

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index fba35510d88ce..a5ffdf7a6e76b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1000,15 +1000,14 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  	}
>  
>  	if (udphy->status == UDPHY_MODE_NONE) {
> -		udphy->mode_change = false;
>  		ret = rk_udphy_setup(udphy);
>  		if (ret)
>  			return ret;
>  
>  		if (udphy->mode & UDPHY_MODE_USB)
>  			rk_udphy_u3_port_disable(udphy, false);
> -	} else if (udphy->mode_change) {
>  		udphy->mode_change = false;
> +	} else if (udphy->mode_change) {
>  		udphy->status = UDPHY_MODE_NONE;
>  		if (udphy->mode == UDPHY_MODE_DP)
>  			rk_udphy_u3_port_disable(udphy, true);

[Severity: High]
This isn't a bug introduced by this patch, but does this lead to a fatal
SError when changing to a mode that includes USB?

If a mode change to DP+USB triggers rk_udphy_power_on(), the code skips calling
rk_udphy_u3_port_disable(udphy, true). It then unconditionally calls
rk_udphy_disable(udphy) below, which turns off the PHY clocks.

Because the USB3 port wasn't disabled to safely re-route the clock, and the
DWC3 driver isn't notified via phy_notify_reset(), the active DWC3 controller
could crash the system when it touches its registers.

>  
>  		rk_udphy_disable(udphy);
>  		ret = rk_udphy_setup(udphy);
>  		if (ret)
>  			return ret;
> +		udphy->mode_change = false;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does this skip re-enabling the
U3 port when the new mode requires USB?

When changing the PHY mode from DP-only to a state that includes USB (e.g.,
DP+USB), the U3 port remains permanently disabled. The mode change block
fails to call rk_udphy_u3_port_disable(udphy, false) to re-enable it for the
newly added USB 3.0 mode, leaving USB functionality broken until a power cycle.

[Severity: High]
This isn't a bug introduced by this patch, but can the PHY be left
permanently enabled during Type-C role switches?

When the Type-C mux changes mode from DP+USB to DP-only, udphy->mode loses
the UDPHY_MODE_USB bit immediately.

Later, when the USB core calls rk_udphy_usb3_phy_exit():

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_usb3_phy_exit() {
	...
	/* DP only or high-speed */
	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
		goto unlock;

	rk_udphy_power_off(udphy, UDPHY_MODE_USB);
	...
}

It sees the new mode and skips calling rk_udphy_power_off(). Even if it
were called, rk_udphy_power_off() also refuses to act if the requested
mode is absent from udphy->mode:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_off() {
	...
	if (!(udphy->mode & mode)) {
		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
		return;
	}
	...
}

Consequently, udphy->status incorrectly retains the USB bit, preventing
rk_udphy_disable() from ever powering down the PHY and leaking clocks.

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