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

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

New issues:
- [High] Removing the clearing of `udphy->status` causes unbalanced clock disables (double disable) and state desynchronization if `rk_udphy_setup()` fails during a mode change.

Pre-existing issues:
- [High] During a mode change to DP+USB, the driver physically disables the PHY clocks without first isolating the active USB controller, causing system crashes.
--

--- Patch [3]: [PATCH v14 03/38] phy: rockchip: usbdp: Do not lose USB3 PHY status ---
commit a08162faff57c7960dad8d0bf118c557b9fa9db9
Author: Sebastian Reichel <[email protected]>

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

By default (i.e. without manually enabling runtime PM) DWC3 requests the
USB3 PHY once and keeps it enabled all the time. When DisplayPort is
being requested later on, a mode change is needed. This re-initializes
the PHY. During re-initialization the status variable has incorrectly
been cleared, which means the tracking information for USB3 is lost.

> 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]
Does removing this clearing of the status cause issues if rk_udphy_setup()
fails during a mode change?

If initialization fails, rk_udphy_power_on() returns an error while the
hardware is left disabled by the preceding rk_udphy_disable(). Because the
software status is no longer cleared, it retains its prior active mode.

Later, when the USB consumer eventually powers off the PHY,
rk_udphy_power_off() observes the stale active status, drops to
UDPHY_MODE_NONE, and issues a redundant rk_udphy_disable(). This
double-disables the clocks.

Could we instead clear udphy->status only in the error path of
rk_udphy_setup()?

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

[Severity: High]
This is a pre-existing issue, but does this conditional isolation lead to
system crashes when the mode is UDPHY_MODE_DP_USB?

During a mode change to DP+USB, the PHY clocks are physically disabled via
rk_udphy_disable() shortly after this check. By only isolating the USB port
when the mode is exactly UDPHY_MODE_DP, a transition to UDPHY_MODE_DP_USB
skips the isolation.

Disabling the PHY clocks while the DWC3 controller is still actively running
and accessing its registers can cause a synchronous external abort. Should
this isolation also happen for UDPHY_MODE_DP_USB?

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy