Re: [PATCH v12 02/36] phy: rockchip: usbdp: Update mode_change after error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Asynchronous PHY mode changes while USB is active cause fatal SErrors (kernel panics) because PHY clocks are disabled without notifying the active DWC3 controller.
- [High] `rk_udphy_power_on()` incorrectly drops active consumer tracking in `udphy->status` during a mode change, potentially causing the PHY to be disabled while still in use.
- [High] The U3 (USB3) port is not re-enabled when transitioning from DP-only mode to DP+USB mode, causing USB3 to remain broken.
- [High] `rk_udphy_clk_init()` shadows the error code from `devm_clk_bulk_get_all()`, breaking deferred probing.
- [High] The driver leaks clock enable counts and hardware reset states on probe error paths if the initial hardware status is active.
- [High] The `rk_udphy_dp_phy_configure` function accesses shared arrays and modifies hardware registers without acquiring `udphy->mutex`, leading to race conditions.
- [High] Non-DP Alternate Mode events corrupt the PHY lane mux configuration, silently breaking USB3 functionality.
- [High] DisplayPort HPD (Hot Plug Detect) events received before the DP PHY is initialized are permanently lost, preventing display detection.
--

--- Patch [2]: [PATCH v12 02/36] phy: rockchip: usbdp: Update mode_change after error handling ---
commit 44a3e651748d53a497e7bb21061bf3cb86d67fbd
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Update mode_change after error handling

If rk_udphy_init() or rk_udphy_setup() fails, the reinit will not be
tried again. Fix this by only updating the variable after all potential
errors have been handled.

Link: 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
> index fba35510d88ce..a5ffdf7a6e76b 100644
> --- 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: High]
This isn't a bug introduced by this patch, but does this mode change block
neglect to conditionally re-enable the U3 port for USB modes?

If the PHY is active in DP-only mode and transitions to DP+USB, this
else if (udphy->mode_change) block is taken. Unlike the initialization
block, it fails to evaluate if (udphy->mode & UDPHY_MODE_USB) and call
rk_udphy_u3_port_disable(udphy, false).

Does this cause USB 3.0 to fail to initialize when plugged into an already
active DisplayPort?

>  		udphy->status = UDPHY_MODE_NONE;

[Severity: High]
This is a pre-existing issue, but does this forcefully erase the tracking
of all currently active consumers without transferring them to the new
state?

If DP is active, udphy->status holds UDPHY_MODE_DP. When a USB device is
plugged in, rk_udphy_power_on is called for UDPHY_MODE_USB with mode_change
true. The code clears udphy->status to UDPHY_MODE_NONE here.

Does this permanently lose the previously active UDPHY_MODE_DP bit? When
USB is later unplugged, power_off clears the USB bit, dropping status to 0,
which might prematurely disable the PHY despite DP still needing it.

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

[Severity: Critical]
This isn't a bug introduced by this patch, but does the asynchronous
disabling of PHY bulk clocks around here cause fatal SErrors?

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

If DWC3 is actively transferring USB data and a DisplayPort Alternate Mode
event arrives, rk_udphy_typec_mux_set triggers a mode change. The code
unconditionally calls rk_udphy_disable(udphy) which abruptly severs the
PHY clocks.

Since the DWC3 driver isn't notified, could its next register access hang
the system bus?

[ ... ]

> @@ -1017,6 +1016,7 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  		ret = rk_udphy_setup(udphy);
>  		if (ret)
>  			return ret;
> +		udphy->mode_change = false;
>  	}
>  
>  	udphy->status |= mode;

[Severity: High]
This is a pre-existing issue, but does setting the status bit here only
register the consumer that triggered the function call, leaving previous
consumers untracked?


There are a few other pre-existing issues in this file that might be worth
looking into:

[Severity: High]
This isn't a bug introduced by this patch, but does this logic shadow the
error code from devm_clk_bulk_get_all and break deferred probing?

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_clk_init() {
    ...
    if (udphy->num_clks < 1)
        return -ENODEV;
    ...
}

If devm_clk_bulk_get_all returns -EPROBE_DEFER when clock providers aren't
ready yet, this explicitly checks if udphy->num_clks < 1 and returns a
hardcoded -ENODEV. Will this cause the PHY driver to fail to probe
permanently if clocks are not immediately available during early boot?


[Severity: High]
This is a pre-existing issue, but does the driver leak clock enable counts
and hardware reset states on probe error paths if the initial hardware
status is active?

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_probe() {
    ...
    udphy->phy_u3 = devm_phy_create(dev, dev->of_node, &rk_udphy_usb3_phy_ops);
    if (IS_ERR(udphy->phy_u3)) {
        ret = PTR_ERR(udphy->phy_u3);
        return dev_err_probe(dev, ret, "failed to create USB3 phy\n");
    }
    ...
}

rk_udphy_get_initial_status calls clk_bulk_prepare_enable. If it detects
active DP hardware, it leaves the clocks enabled and sets udphy->status to
UDPHY_MODE_DP.

If any subsequent step in rk_udphy_probe fails, the driver returns the error
without disabling the clocks. Without a devm-action registered to clean this
up, does this clock enable refcount leak prevent the system from ever
powering down the clock?


[Severity: High]
This isn't a bug introduced by this patch, but does rk_udphy_dp_phy_configure
access shared arrays and modify hardware registers without acquiring
udphy->mutex, leading to race conditions?

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
    ...
    lane = udphy->dp_lane_sel[i];
    ...
}

This reads a shared array that is actively modified by Type-C event
handlers. At the same time, Type-C orientation switch callbacks like
rk_udphy_typec_mux_set or USB initialization can modify these exact same
structures or registers.

Could this concurrent access corrupt hardware registers or result in
inconsistent lane mappings?


[Severity: High]
This is a pre-existing issue, but do non-DP Alternate Mode events corrupt
the PHY lane mux configuration and silently break USB3 functionality?

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_typec_mux_set() {
    ...
    switch (state->mode) {
    case TYPEC_DP_STATE_C:
    case TYPEC_DP_STATE_E:
        udphy->lane_mux_sel[0] = PHY_LANE_MUX_DP;
    ...
}

This executes the switch block and overwrites udphy->lane_mux_sel before
verifying state->alt->svid == USB_TYPEC_DP_SID.

Because TYPEC_DP_STATE_C is defined as TYPEC_STATE_MODAL, any other
Alternate Mode that transitions into its modal state will inadvertently
match this case. Will this erroneously route all PHY lanes to DisplayPort
if a non-DP Type-C Alternate Mode device is attached?


[Severity: High]
This isn't a bug introduced by this patch, but are DisplayPort HPD events
received before the DP PHY is initialized permanently lost, preventing
display detection?

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_init() {
    ...
    udphy->dp_in_use = true;
    ...
}

rk_udphy_dp_hpd_event_trigger caches the HPD state in
udphy->dp_sink_hpd_cfg if udphy->dp_in_use is false. Later, when the DP
DRM driver calls rk_udphy_dp_phy_init, udphy->dp_in_use is set to true.

However, the function fails to write the cached HPD configuration out to
the hardware registers. Does this result in an attached DisplayPort monitor
not being detected by the system?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-rockchip-usbdp-cleanup-v12-0-8b41a9a9bef0@collabora.com?part=2