Re: [PATCH v11 21/36] 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] Exiting DP AltMode fails to physically restore USB lane muxing if the USB controller remains logically active.
- [High] `rk_udphy_usb3_phy_exit` leaks the USB PHY power state if the Type-C mux mode was changed to DP-only before the USB controller calls `phy_exit`.
--

--- Patch [21]: [PATCH v11 21/36] phy: rockchip: usbdp: Properly handle TYPEC_STATE_SAFE and TYPEC_STATE_USB ---
commit 8663a5e7459552707b3ef82190885d5f5b02aa6b
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Properly handle TYPEC_STATE_SAFE and TYPEC_STATE_USB

This commit adds support for TYPEC_STATE_SAFE and TYPEC_STATE_USB Type-C
state events to ensure muxing is properly updated when exiting DP AltMode.
It fixes an issue in the rockchip usbdp combo phy driver.

Fixes: 2f70bbddeb45 ("phy: rockchip: add usbdp combo phy driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
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,
>  };

[Severity: High]
This is a pre-existing issue, but I am wondering if rk_udphy_usb3_phy_exit()
leaks the USB PHY power state when the Type-C mux mode is changed to DP-only
before the USB controller calls phy_exit()?

If the Type-C mux switches to 4-lane DP AltMode, udphy->mode becomes
UDPHY_MODE_DP. If the USB controller then suspends or is unbound, it calls
rk_udphy_usb3_phy_exit(), which returns early:

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);
    ...
}

This skips rk_udphy_power_off(), leaving UDPHY_MODE_USB set in udphy->status.
Since status never reaches UDPHY_MODE_NONE when DP later powers off,
rk_udphy_disable() is never called. Will the PHY clocks and resets remain
permanently active when they should be suspended?

>  
> +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 is a pre-existing issue, but can the PHY get permanently stuck in 4-lane
DP mode when exiting DP AltMode if the USB controller remains logically active?

When rk_udphy_typec_mux_set() handles the transition to TYPEC_STATE_SAFE
or TYPEC_STATE_USB, it updates the software mode and sets phy_needs_reinit
to true via rk_udphy_set_typec_state() later in this function:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_typec_mux_set() {
    ...
    rk_udphy_set_typec_state(udphy, state->mode);
    ...
}

However, the hardware muxing is only updated in rk_udphy_power_on():

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_on() {
    ...
    } else if (udphy->phy_needs_reinit) {
        if (udphy->mode == UDPHY_MODE_DP)
            rk_udphy_u3_port_disable(udphy, true);

        ret = rk_udphy_init(udphy);
    ...
}

If the USB controller (e.g. dwc3) is already active, it will not call
phy_power_on() again. This causes the hardware re-initialization to be missed,
leaving the PHY physically in 4-lane DP mode. Is it possible for USB 3.x
to become completely unresponsive in this state, even though the Type-C
subsystem correctly returned to USB mode?

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