Re: [PATCH v7 24/27] 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:

New issues:
- [High] The `typec_mux_set` early return condition is a tautology, causing the function to always return 0 and ignore all events.
- [High] The `usb_in_use` flag fails to record the USB controller's intent if `phy_init` is called while a DP-only adapter is plugged in.
- [High] The fallback USB power-on logic in `rk_udphy_typec_mux_set` will never trigger due to mode transition interference from orientation switch callbacks.

Pre-existing issues:
- [High] The `UDPHY_MODE_USB` state bit is leaked in `udphy->status` if the USB controller unbinds/exits while the PHY is in DP-only mode.
--

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

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

When a USB-C adapter mapping all Superspeed lanes to DP is plugged in,
USB support is disabled in the PHY. This patch tracks USB usage and
powers on the USB PHY in the Type-C mux set callback to ensure USB
functionality is restored when switching to a USB-capable adapter.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 6f3184011e5f2..1821418375968 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
[ ... ]
> @@ -1293,7 +1299,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;
> +
> +	return 0;
>  }

[Severity: High]
If the USB controller probes and calls rk_udphy_usb3_phy_init() while a
DP-only adapter is physically connected, udphy->mode will lack
UDPHY_MODE_USB. The early return right before this will skip setting
udphy->usb_in_use = true.

Will this cause the fallback logic introduced below to fail when later
switched to a USB-capable adapter?

>  static int rk_udphy_usb3_phy_exit(struct phy *phy)
> @@ -1302,6 +1314,8 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
>  
>  	guard(mutex)(&udphy->mutex);
>  
> +	udphy->usb_in_use = false;
> +
>  	/* DP only or high-speed */
>  	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
>  		return 0;
>  
>  	rk_udphy_power_off(udphy, UDPHY_MODE_USB);

[Severity: High]
This is a pre-existing issue, but does this early return bypass state
cleanup if the USB controller unbinds while the PHY is in DP-only mode?

If udphy->mode lacks UDPHY_MODE_USB, rk_udphy_power_off() is skipped, which
permanently leaves UDPHY_MODE_USB set in udphy->status and leaks hardware
clocks and resets.

> @@ -1321,6 +1335,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 Safe State, USB State or DP AltMode,
>  	 * because the mode field is being reused, e.g. state->mode == 4 could
>  	 * be either TYPEC_MODE_USB4 or TYPEC_DP_STATE_C.
>  	 */
>  	if (state->mode >= TYPEC_STATE_MODAL || !state->alt ||
>  	    state->alt->svid != USB_TYPEC_DP_SID)
>  		return 0;

[Severity: High]
Is this early return condition a tautology that drops all events?

For Safe and USB states, state->alt is NULL, so !state->alt evaluates to
true. For DP AltMode states, state->mode >= TYPEC_STATE_MODAL evaluates to
true. Since these use logical OR, the function will always return 0.

>  
>  	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) &&
> +	    udphy->usb_in_use && !udphy->hs)
> +		return rk_udphy_power_on(udphy, UDPHY_MODE_USB);
> +
>  	return 0;
>  }

[Severity: High]
Will this fallback logic ever execute during a replug?

When swapping a DP-only adapter for a USB-capable one, the orientation switch
callback rk_udphy_orien_sw_set() is typically invoked first. This sets the
mode to UDPHY_MODE_DP_USB.

By the time rk_udphy_typec_mux_set() is called here, udphy->mode already
includes UDPHY_MODE_USB, so old_mode will have it as well. As a result,
!(old_mode & UDPHY_MODE_USB) will evaluate to false and bypass this logic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-rockchip-usbdp-cleanup-v7-0-38eb3cf654fd@collabora.com?part=24

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