Re: [PATCH v6 21/23] phy: rockchip: usbdp: Support going from DP-only mode to USB mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `usb_in_use` flag is bypassed if `phy_init` is called while the PHY is in a DP-only mode, permanently breaking USB functionality for subsequent adapter swaps.
- [High] The fix fails to restore USB functionality when transitioning from a DP-only adapter to a pure USB Type-C device.
- [High] The `!(old_mode & UDPHY_MODE_USB)` check in `rk_udphy_typec_mux_set()` erroneously fails because `rk_udphy_orien_sw_set()` preemptively modifies the mode during unplug/replug.
- [Medium] If `rk_udphy_power_on()` fails during a type-C mux transition, `udphy->mode` remains permanently updated, preventing future retries from restoring USB power.
--

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

phy: rockchip: usbdp: Support going from DP-only mode to USB mode

This patch adds support for restoring USB functionality when a DP-only
adapter is unplugged and replaced with an adapter supporting USB, working
around the USB controller keeping the PHY enabled.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 81aae3bc57476..7f26b74cb5151 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
[ ... ]
> @@ -1287,7 +1293,13 @@ static int rk_udphy_usb3_phy_init(struct phy *phy)
>  		return 0;
>  	}
>  
> -	return rk_udphy_power_on(udphy, UDPHY_MODE_USB);
> +	ret = rk_udphy_power_on(udphy, UDPHY_MODE_USB);
> +	if (ret)
> +		return ret;
> +
> +	udphy->usb_in_use = true;

[Severity: High]
Does this early return logic introduce a regression that prevents the flag
from being set if the system boots with a DP-only adapter?

If phy_init is called when the current mode lacks USB, it takes the early
return visible in the context above and skips executing
udphy->usb_in_use = true.

When a DP+USB adapter is later plugged in, udphy->usb_in_use will be false,
and the newly added restoration logic in rk_udphy_typec_mux_set() will
silently skip powering on the USB PHY.

> +
> +	return 0;
>  }
[ ... ]
> @@ -1315,6 +1329,7 @@ static int rk_udphy_typec_mux_set(struct typec_mux_dev *mux,
>  				  struct typec_mux_state *state)
>  {
>  	struct rk_udphy *udphy = typec_mux_get_drvdata(mux);
> +	u8 old_mode;
>  
>  	/*
>  	 * Ignore mux events not involving DP AltMode, because

[Severity: High]
Is there a regression here where pure USB adapters bypass this restoration
logic entirely?

The code just below this in rk_udphy_typec_mux_set() aborts for non-DP
events:

	if (!state->alt || state->alt->svid != USB_TYPEC_DP_SID)
		return 0;

If a pure USB Type-C device is connected after a DP-only adapter,
rk_udphy_orien_sw_set() updates the default mapping to UDPHY_MODE_DP_USB,
but rk_udphy_typec_mux_set() is never triggered. Since the USB controller
keeps the PHY logically enabled without calling phy_init again, the USB PHY
would remain powered off.

> @@ -1326,8 +1341,20 @@ static int rk_udphy_typec_mux_set(struct typec_mux_dev *mux,
>  
>  	guard(mutex)(&udphy->mutex);
>  
> +	old_mode = udphy->mode;
> +
>  	rk_udphy_set_typec_state(udphy, state->mode);
>  
> +	/*
> +	 * If the new mode includes USB but the old one didn't (e.g. leaving
> +	 * DP-only), and the USB PHY was already initialized by the USB
> +	 * controller, we need to power on the USB side now since no
> +	 * subsequent phy_init call will come from the controller.
> +	 */
> +	if ((udphy->mode & UDPHY_MODE_USB) && !(old_mode & UDPHY_MODE_USB) &&

[Severity: High]
Does this introduce a regression where old_mode already includes
UDPHY_MODE_USB during an unplug/replug sequence?

When unplugging a DP-only adapter and plugging in a DP+USB adapter,
rk_udphy_orien_sw_set() is called first and unconditionally sets
udphy->mode to UDPHY_MODE_DP_USB.

When rk_udphy_typec_mux_set() is called moments later, old_mode = udphy->mode
will capture UDPHY_MODE_DP_USB.  The check !(old_mode & UDPHY_MODE_USB)
would then evaluate to false, causing the USB PHY restoration to be skipped.

> +	    udphy->usb_in_use && !udphy->hs)
> +		return rk_udphy_power_on(udphy, UDPHY_MODE_USB);

[Severity: Medium]
If rk_udphy_power_on() fails here, does it leave udphy->mode permanently
updated without a rollback?

rk_udphy_set_typec_state() modifies udphy->mode before this power-on
attempt.  If rk_udphy_power_on() returns an error, the mode retains the USB
bit. If the Type-C subsystem retries the switch later, old_mode will now
include the USB bit, which would permanently prevent future retries from
passing the restoration check.

> +
>  	return 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619-rockchip-usbdp-cleanup-v6-0-3bb1f54b3f35@collabora.com?part=21