Re: [PATCH v14 07/38] phy: rockchip: usbdp: Keep clocks running on PHY re-init

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

Pre-existing issues:
- [High] The `rk_udphy_dp_phy_configure` function accesses shared state and performs hardware register operations without holding the driver-level `udphy->mutex`.
--

--- Patch [7]: [PATCH v14 07/38] phy: rockchip: usbdp: Keep clocks running on PHY re-init ---
commit d5dccbf2706084c2aa72c0b8c94ff4bf16bfe46f
Author: Sebastian Reichel <[email protected]>

phy: rockchip: usbdp: Keep clocks running on PHY re-init

This commit avoids disabling and re-enabling clocks during PHY mode changes.
It directly calls rk_udphy_init() instead of rk_udphy_disable() and
rk_udphy_setup() to prevent unnecessary delays.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 6cb9f6b4dbf6d..e3f5a26c876a5 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -987,8 +987,7 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  		if (udphy->mode == UDPHY_MODE_DP)
>  			rk_udphy_u3_port_disable(udphy, true);
>  
> -		rk_udphy_disable(udphy);
> -		ret = rk_udphy_setup(udphy);
> +		ret = rk_udphy_init(udphy);
>  		if (ret)
>  			return ret;
>  		udphy->mode_change = false;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file I
noticed a potential race condition in rk_udphy_dp_phy_configure().

Can this corrupt hardware state during a concurrent Type-C event?
Looking at rk_udphy_dp_phy_configure(), it accesses shared state without
holding the udphy->mutex:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
	struct rk_udphy *udphy = phy_get_drvdata(phy);
	...
		for (i = 0; i < udphy->lanes; i++) {
			lane = udphy->dp_lane_sel[i];
			switch (udphy->link_rate) {
			case 1620:
			case 2700:
				regmap_update_bits(udphy->pma_regmap,
						   TRSV_ANA_TX_CLK_OFFSET_N(lane),
						   LN_ANA_TX_SER_TXCLK_INV,
						   FIELD_PREP(LN_ANA_TX_SER_TXCLK_INV,
						   udphy->lane_mux_sel[lane]));
	...
}

The Type-C framework invokes rk_udphy_orien_sw_set() or
rk_udphy_typec_mux_set() asynchronously. These functions acquire
udphy->mutex and overwrite the udphy->dp_lane_sel and udphy->lane_mux_sel
arrays, and can trigger a PHY re-initialization.

Concurrently, the DRM DP framework can invoke rk_udphy_dp_phy_configure(),
which reads these arrays and modifies the PHY hardware pma_regmap locklessly.

If a Type-C device is plugged in while the DP framework is performing link
training, could writing configuration to the incorrect PHY lanes due to a
partially updated lane mapping array break DisplayPort link training or
interleave register writes with a concurrent PHY reset?

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

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