Re: [PATCH v5 07/18] phy: rockchip: usbdp: Support single-lane DP

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Incorrect USB orientation (`flip`) configuration when 1-lane DP is assigned to lane 1.

Pre-existing issues:
- [High] Writing to PMA registers while PHY clocks are disabled when DP mode is not supported.
- [High] Clock resources enabled during probe are leaked on probe error paths and upon driver unbind.
- [High] The DP PHY configuration lacks boundary checks against the physical lane count, leading to USB register corruption.
--

commit c46d73844b37e1f374324a737da2265a3fef0813
Author: Zhang Yubing <yubing.zhang@rock-chips.com>
Subject: phy: rockchip: usbdp: Support single-lane DP

This commit implements support for using a single DisplayPort line in the
Rockchip USBDP PHY driver.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index febc148a754e0..bf8394174294e 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
[ ... ]
> @@ -923,7 +910,8 @@ static int rk_udphy_parse_lane_mux_data(struct rk_udphy *udphy)
>  	}
>  
>  	udphy->mode = UDPHY_MODE_DP;
> -	if (num_lanes == 2) {
> +	udphy->dp_lanes = num_lanes;
> +	if (num_lanes == 1 || num_lanes == 2) {
>  		udphy->mode |= UDPHY_MODE_USB;
>  		udphy->flip = (udphy->lane_mux_sel[0] == PHY_LANE_MUX_DP);
>  	}

[Severity: Medium]
When configuring a single DP lane, does this correctly set the flip state if
the DP lane is assigned to lane 1 instead of lane 0?

For a 1-lane DP setup where rockchip,dp-lane-mux = <1>, lane 1 is occupied
by DP, so USB must be mapped to the lane 2 and 3 pair. This requires the
flip state to be true so the driver correctly polls lane 2 for USB RX CDR
lock.

However, in this scenario lane_mux_sel[0] will not be PHY_LANE_MUX_DP, making
flip evaluate to false. This might cause the driver to poll the unconnected
lane 0 for USB lock and time out. Should this condition check if either lane
0 or lane 1 is occupied by DP?

[ ... ]
> @@ -1074,18 +1062,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]
This is a pre-existing issue, but can this sequence crash the system if the
DP mode is not supported?

In rk_udphy_power_on(), if the mode does not include UDPHY_MODE_DP, it
prints an error message and returns 0 instead of an error code:

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, the execution falls through in
rk_udphy_dp_phy_power_on() and proceeds to call rk_udphy_dplane_enable().

This performs register operations on memory-mapped PHY registers. Accessing
these registers while the corresponding PHY clocks remain disabled can
result in a synchronous external abort and crash the kernel. Should
rk_udphy_power_on() return an error like -ENODEV to properly abort this
sequence?

[ ... ]
> @@ -1529,7 +1518,7 @@ static int rk_udphy_probe(struct platform_device *pdev)
>  		ret = PTR_ERR(udphy->phy_dp);
>  		return dev_err_probe(dev, ret, "failed to create DP phy\n");
>  	}
> -	phy_set_bus_width(udphy->phy_dp, rk_udphy_dplane_get(udphy));
> +	phy_set_bus_width(udphy->phy_dp, udphy->dp_lanes);

[Severity: High]
This is a pre-existing issue, but does the probe sequence leak clock
resources on error paths?

In rk_udphy_get_initial_status(), if the hardware is already in DP mode,
the clocks are enabled and intentionally left enabled:

rk_udphy_get_initial_status() {
    ...
    if (FIELD_GET(CMN_DP_LANE_MUX_ALL, value) && 
        FIELD_GET(CMN_DP_LANE_EN_ALL, value))
        udphy->status = UDPHY_MODE_DP;
    else
        rk_udphy_disable(udphy);
}

However, if subsequent operations in rk_udphy_probe() fail, the probe
function returns directly without calling clk_bulk_disable_unprepare():

rk_udphy_probe() {
    ...
    udphy->phy_u3 = devm_phy_create(dev, dev->of_node, 
                                    &rk_udphy_usb3_phy_ops);
    if (IS_ERR(udphy->phy_u3)) {
        ret = PTR_ERR(udphy->phy_u3);
        return dev_err_probe(dev, ret, "failed to create USB3 phy\n");
    }
    ...
}

Additionally, there appears to be no remove callback or devm action
registered to clean up these manually enabled clocks when the driver
unbinds.

[Severity: High]
This is also a pre-existing issue, but could a consumer request more lanes
than are physically configured in udphy->dp_lanes?

In rk_udphy_dp_phy_verify_lanes(), the driver validates that the requested
lane count is 1, 2, or 4, but it does not check if the requested lanes
exceed the physically configured capacity:

rk_udphy_dp_phy_verify_lanes() {
    ...
    switch (dp->lanes) {
    case 1:
    case 2:
    case 4:
        udphy->lanes = dp->lanes;
        break;
    ...
}

If a consumer requests 4 lanes on a 1-lane or 2-lane configuration,
rk_udphy_dp_phy_configure() will iterate up to the requested lane count,
reading from unconfigured indices:

rk_udphy_dp_phy_configure() {
    ...
    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);
    }
}

When writing voltage settings, this will overwrite the transceiver
registers for those unintended lanes. If one of those lanes is actively
assigned to USB, this could silently corrupt the active USB connection.

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