Re: [PATCH v5 14/18] phy: rockchip: usbdp: Re-init the PHY on orientation change

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] rk_udphy_dp_phy_configure accesses shared state and modifies PHY registers without holding udphy->mutex, racing with orientation switches and PHY resets.
- [High] udphy->phy_needs_reinit is cleared before initialization but is not restored if rk_udphy_init fails, permanently breaking the PHY.
--

commit c7d9d38e9a690c6a3592fb9c0e4736966a879674
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Re-init the PHY on orientation change

Changing the cable orientation reconfigures the lane muxing, which requires
re-initializing the PHY. This patch sets the phy_needs_reinit flag when the
cable orientation is swapped to ensure the PHY is reset and reconfigured
appropriately.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index a3b4e2e0f5786..89a08267611c5 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
[ ... ]
> @@ -633,7 +634,10 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
>  		goto unlock_ret;
>  	}
>  
> -	udphy->flip = orien == TYPEC_ORIENTATION_REVERSE;
> +	if (udphy->flip != flipped)
> +		udphy->phy_needs_reinit = true;

[Severity: High]
This isn't a bug introduced by this patch, but setting
udphy->phy_needs_reinit here interacts with an error handling issue.

In drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_on(), the flag
is eagerly cleared before initialization:

rk_udphy_power_on() {
    ...
    } else if (udphy->phy_needs_reinit) {
        udphy->phy_needs_reinit = false;
        ...
        ret = rk_udphy_init(udphy);
        if (ret)
            return ret;
    ...
}

If rk_udphy_init() fails (for example, due to a retryable PLL lock timeout
returning -EPROBE_DEFER), the flag is not restored to true. Since the
hardware is held in reset on failure, will subsequent calls to
rk_udphy_power_on() skip reinitialization entirely and falsely report
success while leaving both USB and DP functionally dead?

> +
> +	udphy->flip = flipped;
>  	rk_udphy_set_typec_default_mapping(udphy);

[Severity: High]
This is a pre-existing issue, but can this state update race with DP PHY
configuration?

This function holds udphy->mutex while updating state like udphy->flip and
lane selections in rk_udphy_set_typec_default_mapping(). However,
rk_udphy_dp_phy_configure() is called by the generic PHY framework holding
only the DP-specific phy->mutex:

rk_udphy_dp_phy_configure() {
    struct rk_udphy *udphy = phy_get_drvdata(phy);
    // ... accesses udphy->dp_lane_sel without taking udphy->mutex
    ...
}

If a concurrent Type-C mode switch or USB PHY initialization holds
udphy->mutex and triggers rk_udphy_init(), it will assert all hardware
resets:

rk_udphy_init() {
    ...
    rk_udphy_reset_assert_all(udphy);
    ...
}

Because rk_udphy_dp_phy_configure() does not acquire udphy->mutex, can its
register writes interleave with a full hardware reset, leaving the DP PHY
unconfigured and causing link training failures?

>  	rk_udphy_usb_bvalid_enable(udphy, true);
>

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

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