Re: [PATCH v13 21/35] phy: rockchip: usbdp: Properly handle TYPEC_STATE_SAFE and TYPEC_STATE_USB

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete hardware reconfiguration when exiting DP AltMode leaves USB3 permanently broken.
- [High] Resource leak due to checking `udphy->mode` instead of `udphy->status` during PHY exit and power off.
--

--- Patch [21]: [PATCH v13 21/35] phy: rockchip: usbdp: Properly handle TYPEC_STATE_SAFE and TYPEC_STATE_USB ---
commit 00cf3afa847e665c47f5fa7b9e7cf40b3d920a4f
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Properly handle TYPEC_STATE_SAFE and TYPEC_STATE_USB

This commit updates the Rockchip USB/DP Combo PHY driver to handle Type-C
safe and USB states. This is intended to ensure that the muxing is properly
updated when exiting DP AltMode.

Closes: https://sashiko.dev/#/message/20260619155020.CC7361F000E9%40smtp.kernel.org

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 4e54020d17551..0399cbf96e195 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1293,17 +1293,26 @@ static const struct phy_ops rk_udphy_usb3_phy_ops = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +static bool rk_udphy_is_supported_mode(struct typec_mux_state *state)
> +{
> +	/* Handle Safe State and USB State */
> +	if (state->mode < TYPEC_STATE_MODAL)
> +		return true;
> +
> +	/* Handle DP AltMode */
> +	if (state->alt && state->alt->svid == USB_TYPEC_DP_SID)
> +		return true;
> +
> +	return false;
> +}
> +
>  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);
>  
> -	/*
> -	 * Ignore mux events not involving 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->alt || state->alt->svid != USB_TYPEC_DP_SID)
> +	/* Ignore mux events not involving USB or DP */
> +	if (!rk_udphy_is_supported_mode(state))
>  		return 0;
>  
>  	mutex_lock(&udphy->mutex);

[Severity: High]
This isn't a bug introduced by this patch, but does this fully resolve the
problem when transitioning back to USB-only mode?

When the Type-C framework exits DP AltMode (entering TYPEC_STATE_SAFE or
TYPEC_STATE_USB), rk_udphy_typec_mux_set() updates udphy->mode and sets
udphy->phy_needs_reinit = true. However, it appears the hardware is never
actually reconfigured to reflect this.

The hardware is only reconfigured inside rk_udphy_power_on(), which checks
phy_needs_reinit. Because the USB subsystem already powered on the USB PHY
before DP was connected, it holds a power reference and will not call
phy_power_on() again due to a Type-C mux change.

Furthermore, when the DP driver calls phy_power_off() during AltMode exit,
rk_udphy_power_off() clears the UDPHY_MODE_DP flag from udphy->status but does
not check phy_needs_reinit or trigger hardware reconfiguration.

Does this leave the hardware PHY stuck in 4-lane DP mode with the U3 port
disabled, breaking USB3 functionality after unplugging the DP cable?


[Severity: High]
This is also a pre-existing issue, but could the way udphy->mode is updated
during these mux transitions lead to a resource leak?

If rk_udphy_typec_mux_set() switches udphy->mode to UDPHY_MODE_DP (4-lane DP)
while the USB3 PHY is actively in use, udphy->mode loses the UDPHY_MODE_USB
bit.

Later, if the USB3 driver is unbound or the system suspends,
rk_udphy_usb3_phy_exit() is called:

rk_udphy_usb3_phy_exit() {
    ...
    if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
        goto unlock;
    ...
}

Because the check uses udphy->mode instead of udphy->status, it returns early
and skips rk_udphy_power_off(). This leaves UDPHY_MODE_USB permanently set in
udphy->status.

Additionally, rk_udphy_power_off() also incorrectly validates against
udphy->mode instead of udphy->status:

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

Because udphy->status never returns to 0 (UDPHY_MODE_NONE), rk_udphy_disable()
is never called. Does this leave the PHY fully powered indefinitely, preventing
the system from entering low-power suspend states?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-rockchip-usbdp-cleanup-v13-0-6cb3e769d4c5@collabora.com?part=21