Re: [PATCH v17 4/7] phy: rockchip: usbdp: Fix power state handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] phy: rockchip: usbdp: skipped reset handler and port disable on
  USB3 exit
- [Medium] phy: rockchip: usbdp: clock and power leak when udphy->hs is
  true
- [Low] phy: rockchip: usbdp: state desync on error in
  rk_udphy_update_power_state()
--

commit 336f724e3ffaaeb893f1fa7b7dc4865cc0240d95
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Fix power state handling

This commit replaces rk_udphy_power_on() and rk_udphy_power_off() with a
single rk_udphy_update_power_state() helper that derives the target PHY
state from udphy->hw_mode and udphy->sw_mode.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index de8f01bd04672..fc3c4104dcd7e 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c

[ ... ]

> @@ -1029,70 +1029,93 @@ void rk_udphy_register_reset_handler(struct phy *phy,
>  }
>  EXPORT_SYMBOL_GPL(rk_udphy_register_reset_handler);
>  
> -static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
> +static int rk_udphy_update_power_state(struct rk_udphy *udphy)
>  {
> +	bool usb3_port_enable;
> +	u8 target_mode;
>  	int ret;
>  
> -	if (!(udphy->hw_mode & mode)) {
> -		dev_info(udphy->dev, "mode 0x%02x is not support\n", mode);
> +	/*
> +	 * Initialize PHY mode according to the hardware setup (either described
> +	 * in DT or negotiated via the Type-C controller) instead of requesting
> +	 * only the needed PHY side, because that would break the USB/DP data
> +	 * streams when the other PHY is being requested. This is not an issue
> +	 * during the Type-C negotiation as that happens during the hotplug phase
> +	 * and not during normal operation. Also disable everything if the
> +	 * software has not requested anything, as there shouldn't be any active
> +	 * data streams in that case.
> +	 */
> +	target_mode = udphy->hw_mode;
> +	if (udphy->sw_mode == UDPHY_MODE_NONE)
> +		target_mode = UDPHY_MODE_NONE;
> +
> +	usb3_port_enable = !udphy->hs && (target_mode & UDPHY_MODE_USB);
> +
> +	if (!udphy->phy_needs_reinit && udphy->status == target_mode) {
> +		if (udphy->sw_mode & UDPHY_MODE_USB)
> +			rk_udphy_u3_port_disable(udphy, !usb3_port_enable);
>  		return 0;
>  	}
>  
> -	if (udphy->status == UDPHY_MODE_NONE) {
> -		rk_udphy_run_reset_handler(udphy, true);
> +	/* Avoid to re-init disabled PHY */
> +	if (udphy->status == target_mode && target_mode == UDPHY_MODE_NONE)
> +		return 0;
> +
> +	/*
> +	 * Inform DWC3 driver, that we are about to reset the PHY, so that it can
> +	 * assert its PIPE reset lines and avoid DWC3 getting into a buggy state.
> +	 * This is intentionally done for a PHY disable, since that also changes
> +	 * the clocks routed to the PHY.
> +	 */
> +	rk_udphy_run_reset_handler(udphy, true);
>  
> +	/*
> +	 * Disable USB3 port, which among other things re-routes a DWC3 clock to
> +	 * avoid SErrors when the DWC3 registers are accessed while the PHY is
> +	 * disabled. This is only done, when the DWC3 is running as the accessed
> +	 * GRF registers and in PD_USB.
> +	 */
> +	if (udphy->sw_mode & UDPHY_MODE_USB) {
>  		rk_udphy_u3_port_disable(udphy, true);
>  		udelay(10);
> +	}
>  
> +	if (udphy->status == UDPHY_MODE_NONE) {
> +		/* Power up (incl. clocks) */
>  		ret = rk_udphy_setup(udphy);
>  		if (ret) {
>  			rk_udphy_run_reset_handler(udphy, false);
>  			return ret;
>  		}
> -
> -		if (!udphy->hs && udphy->hw_mode & UDPHY_MODE_USB)
> -			rk_udphy_u3_port_disable(udphy, false);
> -		udphy->phy_needs_reinit = false;
> -
> -		rk_udphy_run_reset_handler(udphy, false);
> -	} else if (udphy->phy_needs_reinit) {
> -		rk_udphy_run_reset_handler(udphy, true);
> -
> -		rk_udphy_u3_port_disable(udphy, true);
> -		udelay(10);
> -
> +	} else if (target_mode == UDPHY_MODE_NONE) {
> +		/* Power down (incl. clocks) */
> +		rk_udphy_disable(udphy);

[Severity: High]
Does clearing UDPHY_MODE_USB from udphy->sw_mode in rk_udphy_usb3_phy_exit()
before calling rk_udphy_update_power_state() skip both
rk_udphy_run_reset_handler() and rk_udphy_u3_port_disable(udphy, true) on
USB3 teardown and PHY disable?

rk_udphy_run_reset_handler() returns early when !(udphy->sw_mode &
UDPHY_MODE_USB), and target_mode only becomes UDPHY_MODE_NONE when
udphy->sw_mode == UDPHY_MODE_NONE.  When rk_udphy_usb3_phy_exit() runs, or
whenever rk_udphy_disable() is reached, UDPHY_MODE_USB is already clear in
udphy->sw_mode, so the DWC3 reset callback is never invoked and the USB3
port is left enabled in usbgrf without re-routing the DWC3 PIPE clock,
which can trigger SErrors if DWC3 registers are accessed while the PHY is
disabled.

> +	} else {
> +		/* Mode change => re-init */
> +		ret = rk_udphy_init(udphy);
> +		if (ret) {
> +			rk_udphy_run_reset_handler(udphy, false);
> +			return ret;
> +		}
>  	}

[ ... ]

> @@ -1135,9 +1158,7 @@ static int rk_udphy_dp_phy_power_off(struct phy *phy)
>  	udphy->sw_mode &= ~UDPHY_MODE_DP;
>  
>  	rk_udphy_dp_lane_enable(udphy, 0);
> -	rk_udphy_power_off(udphy, UDPHY_MODE_DP);
> -
> -	return 0;
> +	return rk_udphy_update_power_state(udphy);
>  }

[Severity: Low]
If a pending re-init (udphy->phy_needs_reinit) fails inside
rk_udphy_init() when rk_udphy_dp_phy_power_off() or
rk_udphy_usb3_phy_exit() calls rk_udphy_update_power_state(), can state
become desynchronized?

On failure, rk_udphy_init() asserts all PHY resets while leaving the
clocks enabled, the USB3 port disabled, and udphy->status set to the old
mode.  Propagating that error causes phy_power_off() or phy_exit() to keep
phy->power_count or phy->init_count elevated even though the caller has
already cleared its bit in udphy->sw_mode.

> @@ -1349,12 +1370,12 @@ static int rk_udphy_usb3_phy_init(struct phy *phy)
>  	udphy->sw_mode |= UDPHY_MODE_USB;
>  
>  	/* DP only or high-speed, disable U3 port */
> -	if (!(udphy->hw_mode & UDPHY_MODE_USB) || udphy->hs) {
> +	if (udphy->hs) {
>  		rk_udphy_u3_port_disable(udphy, true);
>  		return 0;
>  	}
>  
> -	ret = rk_udphy_power_on(udphy, UDPHY_MODE_USB);
> +	ret = rk_udphy_update_power_state(udphy);
>  	if (ret) {
>  		udphy->sw_mode &= ~UDPHY_MODE_USB;
>  		return ret;
> @@ -1371,13 +1392,10 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
>  
>  	udphy->sw_mode &= ~UDPHY_MODE_USB;
>  
> -	/* DP only or high-speed */
> -	if (!(udphy->hw_mode & UDPHY_MODE_USB) || udphy->hs)
> +	if (udphy->hs)
>  		return 0;
>  
> -	rk_udphy_power_off(udphy, UDPHY_MODE_USB);
> -
> -	return 0;
> +	return rk_udphy_update_power_state(udphy);
>  }

[Severity: Medium]
When udphy->hs is true, does setting UDPHY_MODE_USB in udphy->sw_mode and
returning early from rk_udphy_usb3_phy_init() and rk_udphy_usb3_phy_exit()
leak the PHY clocks and leave the PHY powered on after DisplayPort is used?

If rk_udphy_usb3_phy_init() runs first with udphy->hs set, udphy->sw_mode
holds UDPHY_MODE_USB while udphy->status stays UDPHY_MODE_NONE.  Powering
DisplayPort on via rk_udphy_dp_phy_power_on() then enables udphy->clks in
rk_udphy_setup(), but rk_udphy_dp_phy_power_off() sees udphy->sw_mode ==
UDPHY_MODE_USB and keeps target_mode at udphy->hw_mode instead of disabling
the PHY.  A subsequent rk_udphy_usb3_phy_exit() clears UDPHY_MODE_USB and
returns early at if (udphy->hs) without calling
rk_udphy_update_power_state(), so rk_udphy_disable() is never called.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-b4-rockchip-usbdp-cleanup-part3-v17-0-1af5f566408e@collabora.com?part=4

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