Re: [PATCH v13 03/35] phy: rockchip: usbdp: Do not lose USB3 PHY status

[sashiko-bot@kernel.org]

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

New issues:
- [High] State desynchronization on PHY setup failure causes double clock disable.
- [High] USB status bit permanently sticks when transitioning to DP-only mode, causing a permanent hardware power leak.

Pre-existing issues:
- [High] USB3 port remains permanently disabled after Type-C transitions from DP-only to DP+USB.
- [High] Clock resource leak on probe failure and module unload if the bootloader left the PHY enabled.
- [High] Data race in `rk_udphy_dp_phy_configure` leading to corrupted DP lane configuration.
--

--- Patch [3]: [PATCH v13 03/35] phy: rockchip: usbdp: Do not lose USB3 PHY status ---
commit d46c4c27785a2f2be7f57da59562db8997835bf3
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Do not lose USB3 PHY status

This patch modifies the Rockchip USB/DP combo PHY driver to prevent the status
variable from being cleared during a DP mode change. This ensures that the
tracking information for USB3 is not lost when re-initializing the PHY.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index a5ffdf7a6e76b..12dfdd470a782 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1008,7 +1008,6 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  			rk_udphy_u3_port_disable(udphy, false);
>  		udphy->mode_change = false;
>  	} else if (udphy->mode_change) {
> -		udphy->status = UDPHY_MODE_NONE;

[Severity: High]
If rk_udphy_setup() fails in the subsequent step (e.g., due to a PLL lock
timeout), could this cause a state desynchronization?

By no longer clearing the status here, the previous active flags remain if the
setup fails:

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;
    ...
}

When the consumer unwinds the failure and calls phy_power_off(), the driver
will see the non-zero status, clear the bit, and invoke rk_udphy_disable() a
second time on already disabled clocks. Will this cause the clock enable
refcount to underflow and generate a kernel warning?

[Severity: High]
Additionally, does removing this clearance cause a permanent hardware power
leak when transitioning to a DP-only mode?

If the Type-C port negotiates a DP-only mode, udphy->mode changes to
UDPHY_MODE_DP, but the UDPHY_MODE_USB bit now persists in udphy->status from
prior initialization.

Later, if the USB core suspends or unbinds, rk_udphy_usb3_phy_exit() is called:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_usb3_phy_exit() {
    ...
	/* DP only or high-speed */
	if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
		goto unlock;

	rk_udphy_power_off(udphy, UDPHY_MODE_USB);
    ...
}

Because udphy->mode is UDPHY_MODE_DP, it skips the call to
rk_udphy_power_off(), meaning the UDPHY_MODE_USB bit is never cleared.
When the DP consumer eventually powers off, rk_udphy_disable() is skipped
because status remains non-zero, leaving the clocks permanently enabled.

>  		if (udphy->mode == UDPHY_MODE_DP)
>  			rk_udphy_u3_port_disable(udphy, true);

[Severity: High]
This is a pre-existing issue, but does this block fail to explicitly re-enable
the USB3 port when transitioning to a mode that requires it?

When transitioning from a DP-only mode to a DP+USB combo mode, udphy->mode
changes to UDPHY_MODE_DP_USB and udphy->mode_change is set to true.

During rk_udphy_power_on(), the code previously disabled the U3 port when
entering DP-only mode. However, when transitioning back to DP+USB, the code
skips the initial UDPHY_MODE_NONE block and enters the
else if (udphy->mode_change) block, which entirely lacks a call to explicitly
re-enable the port via rk_udphy_u3_port_disable(udphy, false).

Will the GRF register retain the disabled state indefinitely, silently
breaking USB3 functionality?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver leak clock
resources if the bootloader left the PHY enabled and probe fails?

In rk_udphy_get_initial_status(), if it detects the PHY was already enabled
by the bootloader, it intentionally skips disabling the clocks:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_get_initial_status() {
    ...
	regmap_read(udphy->pma_regmap, CMN_LANE_MUX_AND_EN_OFFSET, &value);
	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);
    ...
}

Since there is no devm cleanup action registered for these manually enabled
clocks, if a subsequent probe step like devm_phy_create() fails, probe
returns an error without disabling them. Furthermore, since the platform
driver lacks a .remove callback, will the module leak these clocks if it is
cleanly unloaded?

[Severity: High]
This is a pre-existing issue, but is there a data race in
rk_udphy_dp_phy_configure() that could lead to corrupted DP lane configuration?

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

The udphy->dp_lane_sel and udphy->lane_mux_sel arrays are read without
holding udphy->mutex.

Concurrently, if a Type-C orientation flip occurs, the TCPM worker thread
calls rk_udphy_orien_sw_set(), which mutates these exact arrays under
udphy->mutex.

If the DP driver performs link training concurrently with an orientation flip,
could it read partially updated lane arrays and write invalid voltage swing
configurations to incorrect lane registers on the PHY?

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