Re: [PATCH v14 19/38] phy: rockchip: usbdp: Re-init the PHY on orientation change

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

Pre-existing issues:
- [High] Missing hardware re-initialization for active USB connections upon orientation swap.
- [High] Data race in `rk_udphy_dp_phy_configure` due to missing `udphy->mutex` acquisition.
--

--- Patch [19]: [PATCH v14 19/38] phy: rockchip: usbdp: Re-init the PHY on orientation change ---
commit c57df7f56eefba79468e8b70c0ccc01c090e116d
Author: Sebastian Reichel <[email protected]>

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

This commit addresses DisplayPort functionality breaking upon cable
re-plugging with swapped orientation. It tracks the orientation flip and
sets a flag to trigger PHY re-initialization.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index e44d19c9d1193..4b454798c4ed4 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c

[ ... ]

> @@ -631,7 +632,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 is a pre-existing issue, but does setting this flag actually
re-initialize the hardware synchronously if the USB PHY is already active?

If a pure USB 3.0 Type-C device is plugged in a flipped orientation, the
Type-C subsystem calls rk_udphy_orien_sw_set() which defers the state via
phy_needs_reinit.

Since USB controllers like DWC3 do not typically power-cycle the PHY on
orientation changes, won't the hardware remain in the previous orientation,
causing SuperSpeed USB connections to silently fail and fall back to USB 2.0?

> +
> +	udphy->flip = flipped;
>  	rk_udphy_set_typec_default_mapping(udphy);
>  	rk_udphy_usb_bvalid_enable(udphy, true);
>  

Additionally, while reviewing this file, I noticed another pre-existing
issue regarding concurrency:

[Severity: High]
This isn't a bug introduced by this patch, but does
rk_udphy_dp_phy_configure() race with Type-C events?

The DP controller driver calls phy_configure() which executes
rk_udphy_dp_phy_configure() holding only the generic phy->mutex:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
        struct rk_udphy *udphy = phy_get_drvdata(phy);
        ...
}

Concurrently, Type-C events like unplug or orientation swap trigger
rk_udphy_orien_sw_set() or rk_udphy_typec_mux_set(), which modify shared
state (udphy->dp_lane_sel, udphy->lane_mux_sel, and udphy->dp_lanes) while
holding udphy->mutex.

Because rk_udphy_dp_phy_configure() accesses and writes to this shared
hardware state without acquiring udphy->mutex, can the DisplayPort PHY be
programmed with inconsistent lane assignments if a Type-C unplug or
orientation event occurs during DisplayPort link training?

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

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