Re: [PATCH v13 02/35] phy: rockchip: usbdp: Update mode_change after error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] State tracking corruption in `rk_udphy_power_on` drops active consumer modes.
- [High] USB3 port remains disabled in system controller when switching from DP-only to DP+USB mode.
- [High] Dynamically disabling PHY clocks without USB controller coordination causes SErrors.
- [High] Type-C disconnects (TYPEC_STATE_SAFE) or USB-only transitions are ignored, stranding PHY state.
- [High] Missing mutex protection in `rk_udphy_dp_phy_configure` allows concurrent access to shared PHY state.
- [Medium] Probe error paths and driver unbind leak clock references.
- [Medium] U3 port remains enabled in system controller when initializing into DP-only mode.
--

--- Patch [2]: [PATCH v13 02/35] phy: rockchip: usbdp: Update mode_change after error handling ---
commit 5e684d6b433ae738bd5f378d7294d9e76a2199d9
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Update mode_change after error handling

This commit delays resetting the mode_change flag in rk_udphy_power_on()
until after all potential errors are handled. This ensures that if
rk_udphy_setup() fails, the reinitialization attempt won't be silently
skipped on subsequent tries.

Closes: https://lore.kernel.org/linux-phy/20260612163835.8D5471F000E9@smtp.kernel.org/

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1000,15 +1000,14 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)

[ ... ]

>  	if (udphy->status == UDPHY_MODE_NONE) {
> -		udphy->mode_change = false;
>  		ret = rk_udphy_setup(udphy);
>  		if (ret)
>  			return ret;
>  
>  		if (udphy->mode & UDPHY_MODE_USB)
>  			rk_udphy_u3_port_disable(udphy, false);
> -	} else if (udphy->mode_change) {
>  		udphy->mode_change = false;
> +	} else if (udphy->mode_change) {

[Severity: Medium]
This is a pre-existing issue, but does this block leave the U3 port enabled
when initializing into DP-only mode?

If the PHY is completely powered off and the Type-C mux selects DP-only mode,
the DP consumer calls rk_udphy_power_on().

While this block handles initialization, it lacks the logic to call
rk_udphy_u3_port_disable(udphy, true). If the USB consumer is not bound or
initialized yet, does the GRF register for the U3 port retain its previous
state, potentially causing unexpected behavior or minor power drain?

>  		udphy->status = UDPHY_MODE_NONE;

[Severity: High]
This is a pre-existing issue, but does this aggressively clear the state and
drop active consumer modes?

If both DP and USB consumers are active, and a Type-C event triggers a mode
change, the first consumer to call rk_udphy_power_on() executes this block.
By forcefully overwriting udphy->status = UDPHY_MODE_NONE before
re-initializing, and only appending the caller's mode at the end
(udphy->status |= mode), isn't the other active consumer's state flag
permanently lost?

If that first consumer later calls rk_udphy_power_off(), wouldn't the status
evaluate to 0 and completely disable the PHY, crashing the other consumer
which is still running?

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

[ ... ]

>  		rk_udphy_disable(udphy);

[Severity: High]
This isn't a bug introduced by this patch, but can dynamically disabling PHY
clocks without coordinating with the USB controller cause SErrors?

When a Type-C mode change occurs while the USB controller (DWC3) is active,
this block calls rk_udphy_disable(), which immediately cuts the PHY clocks.

Because there is no coordination (like phy_notify_reset) to quiesce DWC3 or
reroute its clock via rk_udphy_u3_port_disable(udphy, true) before the
disable call, will any active DWC3 register access instantly cause an SError
kernel panic?

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

[Severity: High]
This is a pre-existing issue, but does this branch leave the USB 3.0 port
disabled?

When the Type-C port switches from DP-only to DP+USB mode, the USB3 port was
previously disabled via GRF registers. This branch resets the PHY and clears
mode_change, but it never explicitly calls rk_udphy_u3_port_disable(udphy,
false) to re-enable it.

Since the USB consumer's initialization relies on rk_udphy_power_on() to
handle this, will any dynamic transition from DP-only to DP+USB via cable
flip or replug leave the USB3 port broken?


[Severity: High]
This isn't a bug introduced by this patch, but are Type-C disconnects being
ignored in rk_udphy_typec_mux_set()?

Looking at rk_udphy_typec_mux_set(), the mode update is improperly gated
behind an alt-mode check:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_typec_mux_set() {
    ...
    if (state->alt && state->alt->svid == USB_TYPEC_DP_SID) {
        struct typec_displayport_data *data = state->data;

        if (!data) {
    ...
}

On Type-C cable unplug or a transition to USB-only mode, state->mode is
TYPEC_STATE_SAFE or TYPEC_STATE_USB, so state->alt is NULL. Since the mode
update logic is strictly guarded by the SVID check, it gets completely
bypassed.

Does this leave udphy->mode indefinitely retaining the old DP state, breaking
subsequent USB connections and preventing proper PHY power down?


[Severity: High]
This is a pre-existing issue, but is there missing mutex protection in
rk_udphy_dp_phy_configure()?

This function accesses and mutates shared state without acquiring
udphy->mutex:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
    ...
    if (dp->set_rate) {
        udphy->link_rate = dp->link_rate;
    ...
}

Since rk_udphy_dp_phy_configure() is invoked by the DP subsystem during link
training, it reads shared state like udphy->lane_mux_sel and modifies
udphy->bw and udphy->link_rate.

Meanwhile, an asynchronous Type-C event running rk_udphy_typec_mux_set()
modifies these exact arrays while holding the mutex. Can this race condition
lead to reading torn lane mappings and programming incorrect hardware
registers?


[Severity: Medium]
This isn't a bug introduced by this patch, but do the probe error paths and
driver unbind leak clock references?

In rk_udphy_probe(), clocks can be enabled if the bootloader left the PHY
active:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_probe() {
    ...
    ret = rk_udphy_get_initial_status(udphy);
    if (ret)
        return ret;

    mutex_init(&udphy->mutex);
    platform_set_drvdata(pdev, udphy);
    ...
}

If a subsequent step in the probe fails (such as devm_phy_create), or if the
driver is unbound, there appears to be no corresponding
clk_bulk_disable_unprepare() execution or .remove callback to release these
clocks. Does this cause a clock reference count leak?

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