Re: [PATCH v9 35/38] phy: rockchip: usbdp: Simplify power state handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Dynamic PHY re-initialization destroys active DP lane state, silently breaking the active display link.
- [High] State leak on initialization failure leaves the driver desynchronized from the PHY subsystem.
- [High] High-speed only configurations fully power on unused SuperSpeed PHY hardware, wasting power and risking timeouts.
- [High] The PHY fails to re-initialize when the Type-C mux changes between 2-lane DP and 4-lane DP while only DP is actively powered on.
- [High] Initialization failure during a mode change leaves the software state permanently desynchronized from the hardware.
- [Medium] PHY_NOTIFY_POST_RESET is sent to the DWC3 driver unconditionally, even when powering down the PHY.
--

commit e22432b71115cadf57aac2173cacace2579279d3
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
Subject: phy: rockchip: usbdp: Simplify power state handling

This commit introduces a sw_mode field to track software-requested PHY modes
independently from hardware capabilities. It centralizes power state
management and re-initialization into a single update function.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index a19b048fb39ca..51aac07ef9fe9 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c

[ ... ]

> @@ -1023,66 +1015,61 @@ static int rk_udphy_parse_dt(struct rk_udphy *udphy)
>  	return rk_udphy_reset_init(udphy, dev);
>  }
>  
> -static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
> +static int rk_udphy_update_power_state(struct rk_udphy *udphy)
>  {
> +	u8 target_mode = udphy->hw_mode & udphy->sw_mode;
>  	int ret;
>  
> -	if (!(udphy->hw_mode & mode)) {
> -		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
> +	if (!udphy->phy_needs_reinit && udphy->status == target_mode)
>  		return 0;

[Severity: High]
Does this early return prevent the PHY from re-initializing when the Type-C
mux changes between 2-lane and 4-lane DP?

If the Type-C driver changes the mux configuration so hw_mode changes from
UDPHY_MODE_DP_USB to UDPHY_MODE_DP, but only DP is currently requested
(sw_mode == UDPHY_MODE_DP), target_mode will evaluate to UDPHY_MODE_DP in
both cases.

Because target_mode acts as a bitmask, the lane count change in hw_mode is
masked out, causing this check to succeed and entirely skip the required
re-initialization.

[ ... ]

> +	if (udphy->status == UDPHY_MODE_NONE) {
> +		/* Power up (incl. clocks) */
>  		ret = rk_udphy_setup(udphy);
> -		if (ret)
> +		if (ret) {
> +			phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);
>  			return ret;
> -
> -		if (!udphy->hs && udphy->hw_mode & UDPHY_MODE_USB)
> -			rk_udphy_u3_port_disable(udphy, false);
> -		udphy->phy_needs_reinit = false;
> -
> -		phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);
> -	} else if (udphy->phy_needs_reinit) {
> -		phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_PRE_RESET);
> -
> -		rk_udphy_u3_port_disable(udphy, true);
> -		udelay(10);
> -
> +		}
> +	} else if (target_mode == UDPHY_MODE_NONE) {
> +		/* Power down (incl. clocks) */
> +		rk_udphy_disable(udphy);
> +	} else {
> +		/* Mode change => re-init */
>  		ret = rk_udphy_init(udphy);

[Severity: Critical]
Does dynamically re-initializing the PHY here destroy the active DP lane
state and silently break the display link?

When a concurrent mode change occurs, such as the DWC3 controller waking
from runtime suspend and requesting UDPHY_MODE_USB, rk_udphy_init is called
to re-initialize the PHY.

During initialization, rk_udphy_init unconditionally asserts resets and
writes 0 to CMN_DP_LANE_EN_ALL. However, rk_udphy_dp_lane_enable is
exclusively called during rk_udphy_dp_phy_power_on.

Since the DP lanes are never restored after this re-initialization, will
any USB runtime PM transition drop the display link without notifying the
DRM stack?

>  		if (ret) {
>  			phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);
>  			return ret;
>  		}

[Severity: High]
If rk_udphy_init fails, does this leave the software state permanently
desynchronized from the hardware?

If a failure occurs during a mode change, such as a PLL lock timeout,
rk_udphy_init jumps to its assert_resets error label, leaving all PHY
hardware resets asserted.

Since this error path returns without clearing udphy->status, the software
state will believe the previous mode is actively running while the hardware
is dead. If the subsystem retries or reverts to the old mode,
udphy->status == target_mode will erroneously succeed, trapping the PHY
in a broken state.

[ ... ]

> +	/* Ensure USB3 support is enabled when supported and requested */
> +	if (!udphy->hs && target_mode & UDPHY_MODE_USB)
> +		rk_udphy_u3_port_disable(udphy, false);
>  
> -	udphy->status |= mode;
> +	/*
> +	 * Inform DWC3, that we are done with the reset, so that it can deassert
> +	 * its PIPE reset line.
> +	 */
> +	phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);

[Severity: Medium]
Is it safe to unconditionally send PHY_NOTIFY_POST_RESET to the DWC3 driver
here, even when target_mode == UDPHY_MODE_NONE?

Since this notification is now sent after the power-down branch calls
rk_udphy_disable, the DWC3 driver may deassert its internal reset line
and resume PIPE communication. Doing this when the PHY is completely powered
off and its clocks are disabled could lead to internal controller state
corruption or SErrors.

[ ... ]

> @@ -1091,9 +1078,11 @@ static int rk_udphy_dp_phy_power_on(struct phy *phy)
>  	int ret;
>  
>  	scoped_guard(mutex, &udphy->mutex) {
> +		udphy->sw_mode |= UDPHY_MODE_DP;
> +
>  		phy_set_bus_width(phy, udphy->dp_lanes);
>  
> -		ret = rk_udphy_power_on(udphy, UDPHY_MODE_DP);
> +		ret = rk_udphy_update_power_state(udphy);
>  		if (ret)
>  			return ret;

[Severity: High]
If rk_udphy_update_power_state fails, does this leak the UDPHY_MODE_DP bit
in sw_mode?

The bit is added before the fallible update call and is never reverted on
error. The PHY framework considers the initialization failed and will never
call phy_power_off to clear the state. Future operations will then
incorrectly include this leaked mode in target_mode.

[ ... ]

> @@ -1329,13 +1320,9 @@ static int rk_udphy_usb3_phy_init(struct phy *phy)
>  
>  	guard(mutex)(&udphy->mutex);
>  
> -	/* DP only or high-speed, disable U3 port */
> -	if (!(udphy->hw_mode & UDPHY_MODE_USB) || udphy->hs) {
> -		rk_udphy_u3_port_disable(udphy, true);
> -		return 0;
> -	}
> +	udphy->sw_mode |= UDPHY_MODE_USB;
>  
> -	return rk_udphy_power_on(udphy, UDPHY_MODE_USB);
> +	return rk_udphy_update_power_state(udphy);
>  }

[Severity: High]
By removing the early return for high-speed only configurations (udphy->hs),
does this force full SuperSpeed initialization unnecessarily?

Without the early return, UDPHY_MODE_USB is unconditionally set in sw_mode.
This forces rk_udphy_update_power_state to power up the PMA/PCS and wait
for the SuperSpeed LCPLL to lock, which wastes power on unconnected or
unsupported hardware and may cause LCPLL lock timeouts.

[Severity: High]
Similar to the DP power on routine, if rk_udphy_update_power_state fails
here, does this leak the UDPHY_MODE_USB bit in sw_mode?

The bit is set before the update call and is never reverted on error,
desynchronizing the driver from the PHY subsystem.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-rockchip-usbdp-cleanup-v9-0-e31efbb62d2e@collabora.com?part=35