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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Removing the clearance of `udphy->status` introduces unbalanced clock disables and repeated hardware teardowns on the `rk_udphy_setup()` error path.

Pre-existing issues:
- [High] Missing mutex acquisition in `rk_udphy_dp_phy_configure` leads to data races on shared driver state.
- [High] Incomplete handling of mode transitions causes DWC3 SErrors and permanently breaks the USB3 data stream.
--

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

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

By default (i.e. without manually enabling runtime PM) 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. This re-initializes
the PHY. During re-initialization the status variable has incorrectly
been cleared, which means the tracking information for USB3 is lost.

This is not an immediate problem, since the DP side keeps the PHY
enabled. But once DP is toggled off, the whole PHY will be disabled.
This is a problem, because the USB side still needs it powered.

Fix things by not clearing the status flags.

> 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) {

[Severity: High]
This is a pre-existing issue, but I have a question about how the mode
transitions are handled here in rk_udphy_power_on().

When Type-C negotiation changes the mode to UDPHY_MODE_DP_USB, the driver
skips calling rk_udphy_u3_port_disable(udphy, true). It then cuts the PHY
clocks via rk_udphy_disable() while the DWC3 controller might still be
active. Can this cause System Errors (SErrors) from the DWC3 controller?

Additionally, if the old mode was UDPHY_MODE_DP, the USB3 port is disabled
but never re-enabled after rk_udphy_setup() succeeds. Does this leave the
USB3 data stream permanently broken in the GRF registers?

> -		udphy->status = UDPHY_MODE_NONE;
>  		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;

[Severity: High]
Since this clearance is removed, what happens if rk_udphy_setup() fails
immediately after this block?

If rk_udphy_setup() returns an error, the function exits without updating
udphy->status to reflect that the hardware was actually disabled by
rk_udphy_disable() just above.

Could this lead to unbalanced clock disables and repeated resets later when
phy_power_off() or phy_power_on() is called, since the driver still assumes
the PHY is active?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does rk_udphy_dp_phy_configure() need to
hold udphy->mutex when accessing shared driver state?

Looking at drivers/phy/rockchip/phy-rockchip-usbdp.c:

rk_udphy_dp_phy_configure() {
	struct rk_udphy *udphy = phy_get_drvdata(phy);
	...
	if (dp->set_voltages) {
		for (i = 0; i < udphy->lanes; i++) {
			lane = udphy->dp_lane_sel[i];
...

This function reads udphy->dp_lane_sel, udphy->lanes, and
udphy->lane_mux_sel without acquiring the mutex.

Meanwhile, if a Type-C orientation flip occurs, rk_udphy_orien_sw_set()
acquires the mutex and mutates this exact state:

rk_udphy_orien_sw_set() {
	...
	mutex_lock(&udphy->mutex);
	...
	udphy->flip = orien == TYPEC_ORIENTATION_REVERSE;
	rk_udphy_set_typec_default_mapping(udphy);
...

Can this data race cause voltages to be programmed to the wrong PHY lanes,
leading to DP link training failures if a cable is flipped during active
DisplayPort use?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-rockchip-usbdp-cleanup-v12-0-8b41a9a9bef0@collabora.com?part=3