Re: [PATCH v13 06/35] phy: rockchip: usbdp: Drop seamless DP takeover

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

Pre-existing issues:
- [High] Unbalanced clock disable in the `rk_udphy_power_on()` error path during mode changes, leading to a potential clock reference count underflow.
- [High] Data race on DP PHY configuration arrays `dp_lane_sel` and `lane_mux_sel` without mutex protection.
--

--- Patch [6]: [PATCH v13 06/35] phy: rockchip: usbdp: Drop seamless DP takeover ---

commit 9eb0b683c1a1900efc12a73348d324147ff4047f
Author: Sebastian Reichel [off-list ref]
Subject: phy: rockchip: usbdp: Drop seamless DP takeover

This commit drops support for seamless DP takeover in the Rockchip USBDP
PHY driver. The feature was missing implementation in both DRM drivers
and bootloaders, and proper USB-C state machine handoff would require
significant work. The untested code supporting it is therefore removed.

Closes: https://lore.kernel.org/linux-phy/20260612164107.C7DB21F000E9@smtp.kernel.org/ (local)

[ ... ]

While reviewing this file, I noticed a couple of pre-existing issues in
other areas of the driver:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_on() {
[ ... ]
	} else if (udphy->mode_change) {
		if (udphy->mode == UDPHY_MODE_DP)
			rk_udphy_u3_port_disable(udphy, true);

		rk_udphy_disable(udphy);
		ret = rk_udphy_setup(udphy);
		if (ret)
			return ret;
		udphy->mode_change = false;
	}

	udphy->status |= mode;

	return 0;
}

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
rk_udphy_power_on() lead to an unbalanced clock disable during mode changes?

When transitioning modes, rk_udphy_power_on() disables the clocks via
rk_udphy_disable() to reset the state, and then calls rk_udphy_setup().

If rk_udphy_setup() fails, it returns early without updating udphy->status.
The software state retains the active mode, but the hardware clocks are
disabled.

When the consumer later calls rk_udphy_power_off(), it reads the active
status, clears it, and calls rk_udphy_disable() again.

Could this result in a double-disable and a potential clock reference count
underflow?


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];
			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]));
[ ... ]
}

[Severity: High]
This is a pre-existing issue, but is there a data race when accessing the
DP PHY configuration arrays?

rk_udphy_dp_phy_configure() is called by the PHY framework and reads
udphy->dp_lane_sel and udphy->lane_mux_sel without acquiring udphy->mutex.

Concurrently, type-C workqueues can write to these same arrays while holding
udphy->mutex:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_typec_mux_set() {
[ ... ]
	mutex_lock(&udphy->mutex);

	switch (state->mode) {
	case TYPEC_DP_STATE_C:
	case TYPEC_DP_STATE_E:
		udphy->lane_mux_sel[0] = PHY_LANE_MUX_DP;
		udphy->lane_mux_sel[1] = PHY_LANE_MUX_DP;
		udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
		udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
[ ... ]
}

If an asynchronous type-C event arrives while the display controller is
configuring the PHY for link training, can the arrays be read in a partially
updated state?

Could this result in an invalid DP lane configuration being applied to the
hardware, leading to DP alt mode failure or display corruption?

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

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