Re: [PATCH v14 11/38] phy: rockchip: usbdp: Support single-lane DP

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Prematurely caching `udphy->dp_lanes` based on Type-C state causes unintended DP physical lane enablement when DP mode is not active.
--

--- Patch [11]: [PATCH v14 11/38] phy: rockchip: usbdp: Support single-lane DP ---
commit 9992535ad6ec018fe7cd04829eeaaa211b9c6078
Author: Zhang Yubing <[email protected]>

phy: rockchip: usbdp: Support single-lane DP

Implement support for using just a single DisplayPort line.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 7540c897dde13..c4d62b234ea80 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c

[ ... ]

> @@ -1050,18 +1039,17 @@ static int rk_udphy_dp_phy_exit(struct phy *phy)
>  static int rk_udphy_dp_phy_power_on(struct phy *phy)
>  {
>  	struct rk_udphy *udphy = phy_get_drvdata(phy);
> -	int ret, dp_lanes;
> +	int ret;
>  
>  	mutex_lock(&udphy->mutex);
>  
> -	dp_lanes = rk_udphy_dplane_get(udphy);
> -	phy_set_bus_width(phy, dp_lanes);
> +	phy_set_bus_width(phy, udphy->dp_lanes);
>  
>  	ret = rk_udphy_power_on(udphy, UDPHY_MODE_DP);
>  	if (ret)
>  		goto unlock;
>  
> -	rk_udphy_dplane_enable(udphy, dp_lanes);
> +	rk_udphy_dplane_enable(udphy, udphy->dp_lanes);

[Severity: High]
Does this code inadvertently enable physical DP lanes when DP mode is not active?

By using the unconditionally cached udphy->dp_lanes instead of evaluating it
dynamically with rk_udphy_dplane_get(), could this be bypassing a safety check?

If HPD is not asserted or the mode is unsupported during rk_udphy_typec_mux_set(),
udphy->mode is not updated to include UDPHY_MODE_DP. Later, rk_udphy_power_on()
will return 0 because it ignores unsupported modes.

This would result in calling rk_udphy_dplane_enable(udphy, udphy->dp_lanes)
with a >0 lane count on a PHY that is not fully configured for DP, instead of
safely asserting reset as it did when the lane count evaluated to 0.

[ ... ]

> @@ -1341,6 +1329,7 @@ static int rk_udphy_typec_mux_set(struct typec_mux_dev *mux,
>  		udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
>  		udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
>  		mode = UDPHY_MODE_DP;
> +		udphy->dp_lanes = 4;
>  		break;
>  
>  	case TYPEC_DP_STATE_D:
> @@ -1357,6 +1346,7 @@ static int rk_udphy_typec_mux_set(struct typec_mux_dev *mux,
>  			udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
>  		}
>  		mode = UDPHY_MODE_DP_USB;
> +		udphy->dp_lanes = 2;
>  		break;
>  	}

[Severity: High]
Is it safe to unconditionally cache udphy->dp_lanes here before verifying HPD
assertion, knowing that it might desynchronize from the actual udphy->mode?

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

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