Re: [PATCH v5 03/18] phy: rockchip: usbdp: Keep clocks running on PHY re-init

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing clock disable on probe failure and driver unbind if the bootloader left the PHY enabled.
- [High] Premature consumption of the re-initialization flag leads to false success reporting on subsequent retries if PHY initialization fails.
- [High] Initialization failure of a new mode silently breaks the already active mode in the combo PHY.
- [High] Shadowing of error codes from devm_clk_bulk_get_all() breaks deferred probing.
- [High] Missing mutex_lock(&udphy->mutex) in rk_udphy_dp_phy_configure() allows data races and hardware state corruption.
--

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

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

This commit avoids unnecessarily disabling and re-enabling clocks when
re-initializing the PHY during a mode change. Since rk_udphy_init()
asserts the reset lines anyway, keeping the clocks running simplifies
the flow and avoids delaying the process.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 744cc7c642f49..98562a888b42a 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1012,8 +1012,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);

[Severity: High]
This is a pre-existing issue, but by consuming udphy->phy_needs_reinit earlier
in rk_udphy_power_on(), could this falsely report success on subsequent
retries if rk_udphy_init() fails?

When transitioning to a combo mode, udphy->phy_needs_reinit is cleared before
rk_udphy_init() is called:

drivers/phy/rockchip/phy-rockchip-usbdp.c: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() returns an error, the flag remains false. On the next retry
by the framework, it might skip hardware initialization, incorrectly add the
new mode to udphy->status, and return success despite the PHY not functioning.

>  		if (ret)
>  			return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but is there a leak of the clock enable count
during probe error paths or driver unbind if the bootloader left the PHY
enabled?

In rk_udphy_get_initial_status(), clocks are prepared and enabled:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_get_initial_status() {
    ...
	ret = clk_bulk_prepare_enable(udphy->num_clks, udphy->clks);
    ...
}

If the PHY was left enabled by the bootloader, udphy->status is set to
UDPHY_MODE_DP and clocks are intentionally kept running. However, the driver
does not appear to register a devm cleanup action for these clocks. If
subsequent steps in rk_udphy_probe() fail, or if the driver is unbound, the
clock enable count is never decremented.

[Severity: High]
This is a pre-existing issue, but would an initialization failure of a new
mode in rk_udphy_init() silently break the already active mode?

If a mode change occurs (e.g., turning on DisplayPort while USB is already
active) and rk_udphy_init() fails, the error path asserts all reset lines:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_init() {
    ...
assert_resets:
	rk_udphy_reset_assert_all(udphy);
	return ret;
}

This forcefully asserts reset lines for the entire combo PHY, breaking the
already active USB PHY without notifying the USB subsystem.

[Severity: High]
This is a pre-existing issue, but by replacing the error code in
rk_udphy_clk_init(), could deferred probing be broken?

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_clk_init() {
    ...
	udphy->num_clks = devm_clk_bulk_get_all(dev, &udphy->clks);
	if (udphy->num_clks < 1)
		return -ENODEV;
    ...
}

If devm_clk_bulk_get_all() returns -EPROBE_DEFER, it is replaced with -ENODEV,
permanently failing the driver probe instead of allowing it to defer.

[Severity: High]
This is a pre-existing issue, but without acquiring udphy->mutex in
rk_udphy_dp_phy_configure(), are there potential data races or hardware state
corruption?

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

The function modifies shared driver state (udphy->dp_lane_sel,
udphy->lane_mux_sel, udphy->bw, udphy->link_rate) and writes to shared PHY
hardware registers. Without holding the mutex, this can race with Type-C
callbacks or concurrent USB PHY operations.

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

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