Re: [PATCH v13 12/35] phy: rockchip: usbdp: Limit DP lane count to muxed lanes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing locking in `rk_udphy_dp_phy_configure` causes a TOCTOU race and state desynchronization against Type-C events, bypassing the newly added lane bounds check.
--

--- Patch [12]: [PATCH v13 12/35] phy: rockchip: usbdp: Limit DP lane count to muxed lanes ---
commit 51fd699581be62fd9931337e9c0dc6e4f2302b10
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Limit DP lane count to muxed lanes
    
In theory the DP controller could request 4 lanes when the PHY is
restricted to 2 lanes as the other half is used by USB3.

With the current user (DW-DP) this cannot happen, but as the check is
cheap and users might change in the future protect things accordingly.
Not doing so would corrupt USB3 usage by the following code configuring
the voltages.

Fixes: 2f70bbddeb45 ("phy: rockchip: add usbdp combo phy driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-phy/20260612165546.98E1F1F000E9@smtp.kernel.org/
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index c4d62b234ea80..b172ce6a722ee 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1099,6 +1099,9 @@ static int rk_udphy_dp_phy_verify_link_rate(struct rk_udphy *udphy,
>  static int rk_udphy_dp_phy_verify_lanes(struct rk_udphy *udphy,
>  					struct phy_configure_opts_dp *dp)
>  {
> +	if (dp->lanes > udphy->dp_lanes)
> +		return -EINVAL;
> +

[Severity: High]
This isn't a bug introduced by this patch, but can a race condition still
bypass this new check and corrupt USB3 lanes?

If a Type-C event occurs and rk_udphy_typec_mux_set() reduces udphy->dp_lanes
from 4 to 2 while updating udphy->lane_mux_sel for USB3, it does so under
udphy->mutex. However, rk_udphy_dp_phy_configure() lacks mutex protection and
operates on a cached udphy->lanes value.

If the DRM driver subsequently calls rk_udphy_dp_phy_configure() to update
voltages (with set_voltages = 1 and set_lanes = 0), this verify function is
skipped entirely:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
    ...
	if (dp->set_lanes) {
		ret = rk_udphy_dp_phy_verify_lanes(udphy, dp);
		if (ret)
			return ret;
	}
    ...
}

Because the check is skipped, the configuration loop iterates using the stale
cached udphy->lanes == 4:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
    ...
	if (dp->set_voltages) {
		for (i = 0; i < udphy->lanes; i++) {
			lane = udphy->dp_lane_sel[i];
			...
			rk_udphy_dp_set_voltage(udphy, udphy->bw, dp->voltage[i],
						dp->pre[i], lane);
		}
	}
    ...
}

Could this overwrite the TRSV registers of physical lanes now assigned to
USB3, corrupting active USB3 data streams? Should rk_udphy_dp_phy_configure()
hold udphy->mutex to prevent state desynchronization?

>  	switch (dp->lanes) {
>  	case 1:
>  	case 2:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-rockchip-usbdp-cleanup-v13-0-6cb3e769d4c5@collabora.com?part=12

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