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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Potential clock leak and state corruption if PHY re-initialization fails during a mode change.

Pre-existing issues:
- [High] The `rk_udphy_dp_phy_configure` callback modifies the shared `pma_regmap` and reads shared lane mapping arrays without holding the driver-specific `udphy->mutex`.
- [High] The USB3 (U3) port is not re-enabled in the GRF registers if the PHY dynamically transitions from a DP-only mode to a DP+USB mode while powered on.
--

--- Patch [7]: [PATCH v13 07/35] phy: rockchip: usbdp: Keep clocks running on PHY re-init ---
commit 0ed1125cb68d99c18b6937c983323c7c388ecb47
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

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

When a mode change is required, rk_udphy_power_on() originally disabled
clocks before re-enabling them. This commit updates it to call
rk_udphy_init() directly, keeping clocks running during PHY
re-initialization.

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

> 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);
>  

[Severity: High]
This is a pre-existing issue, but does this code fail to unmask the USB3
port during dynamic mode changes?

If the PHY transitions dynamically from a DP-only mode to a DP+USB mode
while powered on, this block handles the mode change but does not call
rk_udphy_u3_port_disable(udphy, false) to re-enable the U3 port in the GRF
registers. This seems to prevent USB3 devices from enumerating after the
transition.

> -		rk_udphy_disable(udphy);
> -		ret = rk_udphy_setup(udphy);
> +		ret = rk_udphy_init(udphy);
>  		if (ret)
>  			return ret;

[Severity: High]
If rk_udphy_init() fails, does this regression leak the clock resources?

Prior to this patch, if rk_udphy_setup() failed, rk_udphy_disable() had
already powered off the PHY. Now, if rk_udphy_init() fails (e.g., due to a
PLL lock timeout), it returns the error without disabling the clocks.

Since udphy->status was already set to UDPHY_MODE_NONE earlier in this
block, the teardown path will see a 0 status:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_power_off() {
    if (!udphy->status)
        return;
    ...
}

and return early without calling rk_udphy_disable(), permanently leaking
the clocks.

>  		udphy->mode_change = false;

[Severity: High]
This is a pre-existing issue, but does rk_udphy_dp_phy_configure() need to
hold the udphy->mutex?

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;

    if (dp->set_rate) {
    ...
}

Unlike other PHY operations in this driver, rk_udphy_dp_phy_configure() does
not acquire udphy->mutex. Since the generic PHY framework serializes
phy_configure on a per-instance basis, this could run concurrently with
operations on the USB3 PHY instance (like rk_udphy_usb3_phy_init) or Type-C
callbacks.

This can result in unprotected accesses to shared resources, such as
pma_regmap or dp_lane_sel, and potentially cause synchronous external aborts
if registers are accessed while the hardware block is concurrently put into
reset by rk_udphy_init().

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

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