Re: [PATCH v5 17/18] 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:
- [Medium] The return value of `rk_udphy_power_on()` is silently ignored during Type-C mux state transitions.
- [Medium] The `usb_in_use` flag is permanently left as `true` if `rk_udphy_power_on()` fails during USB PHY initialization.

Pre-existing issues:
- [High] The `UDPHY_MODE_USB` bit in `udphy->status` is permanently leaked if `rk_udphy_usb3_phy_exit()` is called while the Type-C mux is currently in DP-only mode.
- [High] Failing to reinitialize the PHY after a transient error masks subsequent failures and reports false success.
--

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

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

When a USB-C adapter, which maps all Superspeed lanes to DP is plugged
in, the USB support is disabled in the PHY. When the adapter is
unplugged and a different adapter with USB functionality is plugged in
afterwards, USB functionality is not restored as the USB controller
keeps the PHY enabled for the entire time.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c

[ ... ]

> @@ -1015,6 +1016,9 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  		ret = rk_udphy_init(udphy);
>  		if (ret)
>  			return ret;

[Severity: High]
This is a pre-existing issue, but if rk_udphy_init() fails here, 
udphy->phy_needs_reinit was already set to false earlier in the block.

Will subsequent calls to rk_udphy_power_on() skip initialization and 
falsely return success while the hardware remains broken? 

Should phy_needs_reinit be restored to true on the error path?

> +
> +		if (udphy->mode & UDPHY_MODE_USB)
> +			rk_udphy_u3_port_disable(udphy, false);
>  	}
>  
>  	udphy->status |= mode;

[ ... ]

> @@ -1278,6 +1282,15 @@ static int rk_udphy_usb3_phy_init(struct phy *phy)
>  
>  	guard(mutex)(&udphy->mutex);
>  
> +	udphy->usb_in_use = true;
> +
>  	/* DP only or high-speed, disable U3 port */
>  	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
>  		rk_udphy_u3_port_disable(udphy, true);
>  		return 0;
>  	}
>  
>  	ret = rk_udphy_power_on(udphy, UDPHY_MODE_USB);
>  
>  	return ret;
>  }

[Severity: Medium]
If rk_udphy_power_on() fails and returns an error, usb_in_use remains true. 

Will this cause rk_udphy_typec_mux_set() to incorrectly attempt to power on 
the USB PHY later, despite the USB controller not actively using it?

Should the flag be reverted to false on the error path?

> @@ -1295,6 +1301,13 @@ 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;

[Severity: High]
This isn't a bug introduced by this patch, but if the Type-C mode 
transitions to DP-only, udphy->mode loses the UDPHY_MODE_USB bit. 

When rk_udphy_usb3_phy_exit() is subsequently called, it checks 
!(udphy->mode & UDPHY_MODE_USB) and returns 0 early, skipping 
rk_udphy_power_off(). 

Does this result in a permanent leak of the PHY's clocks and resets, since 
udphy->status never clears the UDPHY_MODE_USB bit?

>  
>  	rk_udphy_power_off(udphy, UDPHY_MODE_USB);
>  
>  	return 0;
>  }

[ ... ]

> @@ -1325,8 +1334,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) &&
> +	    udphy->usb_in_use && !udphy->hs)
> +		rk_udphy_power_on(udphy, UDPHY_MODE_USB);

[Severity: Medium]
Does this code discard the return value of rk_udphy_power_on()? 

If initialization fails, the error is ignored and the driver reports success 
for the Type-C mux state transition, which could leave the USB functionality 
silently broken. 

Should this check the return value and at least log or propagate the error?

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-rockchip-usbdp-cleanup-v5-0-efc83069869f@collabora.com?part=17

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