Re: [PATCH v14 02/38] phy: rockchip: usbdp: Update mode_change after error handling

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

Pre-existing issues:
- [High] The PHY status tracking drops the active mode state when handling a mode change, potentially causing premature PHY shutdown for active clients.
- [High] Transitioning to a combo mode (DP+USB) fails to re-enable the USB3 port because `rk_udphy_u3_port_disable(udphy, false)` is never called in the mode_change path.
- [High] Returning success (0) for an unsupported mode in `rk_udphy_power_on()` bypasses PHY clock enablement, leading to a kernel panic when the caller performs unclocked register accesses.
- [High] Type-C mux handler allows 4-lane DP states on 2-lane hardware, using uninitialized lane mappings and corrupting hardware configuration.
--

--- Patch [2]: [PATCH v14 02/38] phy: rockchip: usbdp: Update mode_change after error handling ---
commit 44520eeef5c577fb420ec25d39b4f335cf37c1a1
Author: Sebastian Reichel <[email protected]>

phy: rockchip: usbdp: Update mode_change after error handling

If rk_udphy_init() or rk_udphy_setup() fails, the reinit will not be
tried again. Fix this by only updating the variable after all potential
errors have been handled.

Fixes: 2f70bbddeb45 ("phy: rockchip: add usbdp combo phy driver")
Reported-by: Sashiko <[email protected]>
Closes: https://lore.kernel.org/linux-phy/[email protected]/
Tested-by: Igor Paunovic <[email protected]> # Orange Pi 5 Plus

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index fba35510d88ce..a5ffdf7a6e76b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1000,15 +1000,14 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)

[Severity: High]
This is a pre-existing issue, but returning 0 (success) for an unsupported
mode bypasses PHY clock enablement.

If rk_udphy_power_on() is called with a mode not currently present in
udphy->mode (for example, DP forced by DRM when the PHY is configured for
USB-only), it prints an info message and returns 0:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_on() {
	if (!(udphy->mode & mode)) {
		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
		return 0;
	}
        ...
}

Because it returns success instead of an error code like -EOPNOTSUPP, the
caller assumes the PHY and its clocks are fully enabled.

Will this cause the caller to proceed with operations like
rk_udphy_dplane_enable() and trigger an SError when writing to the unclocked
pma_regmap?

>  	}
>  
>  	if (udphy->status == UDPHY_MODE_NONE) {
> -		udphy->mode_change = false;
>  		ret = rk_udphy_setup(udphy);
>  		if (ret)
>  			return ret;
>  
>  		if (udphy->mode & UDPHY_MODE_USB)
>  			rk_udphy_u3_port_disable(udphy, false);
> -	} else if (udphy->mode_change) {
>  		udphy->mode_change = false;
> +	} else if (udphy->mode_change) {
>  		udphy->status = UDPHY_MODE_NONE;

[Severity: High]
This isn't a bug introduced by this patch, but does resetting the status to
UDPHY_MODE_NONE here lose track of other active clients?

When a Type-C event triggers a mode change while both DP and USB clients are
active (status == UDPHY_MODE_DP | UDPHY_MODE_USB), one client like DP might
call rk_udphy_power_on().

This branch resets udphy->status to UDPHY_MODE_NONE and reinitializes the
PHY. At the end of the function, it sets udphy->status |= mode (which is
only UDPHY_MODE_DP), permanently losing the UDPHY_MODE_USB bit.

If the DP client later powers off, the status becomes UDPHY_MODE_NONE, and
the PHY is physically disabled.

Could this cause a kernel panic (SError) if the DWC3 USB controller is still
active and tries to access the unclocked/unpowered PHY?

[Severity: High]
This is a pre-existing issue, but this mode_change branch seems to omit
re-enabling the U3 port.

If the system boots or engages DP-only mode, rk_udphy_u3_port_disable(udphy,
true) disables the USB3 port.  When a Type-C event requests Combo mode
(UDPHY_MODE_DP_USB), rk_udphy_power_on() executes this mode_change block.

Unlike the initial boot path which checks if (udphy->mode & UDPHY_MODE_USB),
this branch doesn't appear to re-enable the U3 port.

Does this leave the USB3 port disabled in hardware, causing USB 3.0 data
transfers to fail silently when transitioning to a combo mode?

>  		if (udphy->mode == UDPHY_MODE_DP)
>  			rk_udphy_u3_port_disable(udphy, true);
>  
> @@ -1017,6 +1016,7 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  		ret = rk_udphy_setup(udphy);
>  		if (ret)
>  			return ret;
> +		udphy->mode_change = false;
>  	}
>  
>  	udphy->status |= mode;

[Severity: High]
This is a pre-existing issue, but does the Type-C mux handler allow 4-lane
DP states on 2-lane hardware?

If the hardware has only 2 lanes (specified via rockchip,dp-lane-mux in DT),
rk_udphy_parse_lane_mux_data() initializes udphy->dp_lane_sel[0] and [1],
leaving [2] and [3] as 0.

However, if an unsupported 4-lane Alt Mode request is accepted, it sets
mode = UDPHY_MODE_DP.  When the PHY powers on, rk_udphy_dplane_select()
computes shifts using the uninitialized elements:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dplane_select() {
    ...
	case UDPHY_MODE_DP:
		value |= 2 << udphy->dp_lane_sel[2] * 2;
		value |= 3 << udphy->dp_lane_sel[3] * 2;
		fallthrough;
    ...
}

Because they are both 0, it computes 2 << 0 and 3 << 0.

Could this corrupt the lane selection bits for lane 0 and write an invalid
bitmask to the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy