Re: [PATCH v11 33/36] phy: rockchip: usbdp: Rename mode to hw_mode

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] TYPEC_STATE_USB and TYPEC_STATE_SAFE mux events incorrectly configure the PHY for DisplayPort instead of USB-only.
- [High] rk_udphy_power_off and rk_udphy_usb3_phy_exit incorrectly use hw_mode to bypass power-off, permanently leaking the PHY state and preventing rk_udphy_disable() from being called.
--

--- Patch [33]: [PATCH v11 33/36] phy: rockchip: usbdp: Rename mode to hw_mode ---
commit 6f61047a0ab430913024fc15e4a26038f67189e4
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Rename mode to hw_mode

Rename mode field to hw_mode to make clear that this is the modes
currently supported by the hardware, but not necessarily requested
by software. It is only set by either the USB-C state machine
or device-tree if the PHY is used in a fixed routing setup.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 3fb6eea2979b9..e2cd72643a7d7 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
[ ... ]
> @@ -616,12 +616,12 @@ static void rk_udphy_set_typec_state(struct rk_udphy *udphy, unsigned long state
>  			udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
>  			udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
>  		}
> -		mode = UDPHY_MODE_DP_USB;
> +		hw_mode = UDPHY_MODE_DP_USB;

[Severity: High]
This isn't a bug introduced by this patch, but since the switch statement lacks
explicit cases for TYPEC_STATE_USB and TYPEC_STATE_SAFE, do they fall through
to the default case here and incorrectly configure the PHY for DisplayPort
instead of USB-only?

If a standard USB device is connected or the port is in a safe/unattached state,
the Type-C subsystem fires a mux event with state->mode = TYPEC_STATE_USB (or
TYPEC_STATE_SAFE), and it seems they would erroneously be assigned 
UDPHY_MODE_DP_USB with two DisplayPort lanes instead of just UDPHY_MODE_USB.

>  		udphy->dp_lanes = 2;
>  		break;
>  	}
[ ... ]
> @@ -1033,7 +1033,7 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  
>  static void rk_udphy_power_off(struct rk_udphy *udphy, u8 mode)
>  {
> -	if (!(udphy->mode & mode)) {
> +	if (!(udphy->hw_mode & mode)) {

[Severity: High]
This is a pre-existing issue, but does using hw_mode here bypass the power-off
sequence and permanently leak the physical PHY state?

If a Type-C event changes the hardware mode to DP-only (hw_mode =
UDPHY_MODE_DP), the USB mode is dropped. If rk_udphy_power_off() is called
afterwards, this check will bypass physical shutdown since the requested mode
was already removed from hw_mode, preventing rk_udphy_disable() from being
called.

>  		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
>  		return;
>  	}
[ ... ]
> @@ -1307,7 +1307,7 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
>  	guard(mutex)(&udphy->mutex);
>  
>  	/* DP only or high-speed */
> -	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
> +	if (!(udphy->hw_mode & UDPHY_MODE_USB) || udphy->hs) {

[Severity: High]
This is a pre-existing issue, but this appears to have the same flaw as
rk_udphy_power_off() above.

When the USB consumer subsequently calls phy_exit(), and hw_mode has already
dropped the USB mode, this returns early without delegating to
rk_udphy_power_off(). If the PHY status reached UDPHY_MODE_NONE,
rk_udphy_disable() is never called, and the PHY remains physically powered on
indefinitely.

>  		udphy->status &= ~UDPHY_MODE_USB;
>  		return 0;
>  	}

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