Re: [PATCH v12 27/36] phy: rockchip: usbdp: Handle rk_udphy_reset_deassert errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] If PHY re-initialization fails during a mode change, the U3 port is left permanently disabled and in reset, but the software state incorrectly remains active.
--

--- Patch [27]: [PATCH v12 27/36] phy: rockchip: usbdp: Handle rk_udphy_reset_deassert errors ---
commit 00e32198a00a3780b193e546b8f16d2218cefb74
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

phy: rockchip: usbdp: Handle rk_udphy_reset_deassert errors

This commit modifies the initialization sequence to handle errors returned
by reset deassertion, avoiding a theoretical SError.

Fixes: 2f70bbddeb45 ("phy: rockchip: add usbdp combo phy driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/message/20260626211151.2332F1F000E9%40smtp.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
> @@ -803,8 +803,12 @@ static int rk_udphy_init(struct rk_udphy *udphy)
>  	/* Step 1: power on pma and deassert apb rstn */
>  	rk_udphy_grfreg_write(udphy->udphygrf, &cfg->grfcfg.low_pwrn, true);
>  
> -	rk_udphy_reset_deassert(udphy, "pma_apb");
> -	rk_udphy_reset_deassert(udphy, "pcs_apb");
> +	ret = rk_udphy_reset_deassert(udphy, "pma_apb");
> +	if (ret)
> +		goto assert_resets;

[Severity: Medium]
This is a pre-existing issue, but returning an error here propagates up to
rk_udphy_power_on(). Does this leave the U3 port in a permanently disabled
state?

When USB3 is active and a new mode is enabled, rk_udphy_power_on() triggers
re-initialization. It disables the U3 port and calls rk_udphy_init(). If
rk_udphy_init() fails and returns an error here, the caller propagates it
immediately:

rk_udphy_power_on() {
        ...
        ret = rk_udphy_init(udphy);
        if (ret)
                return ret;

        if (udphy->mode & UDPHY_MODE_USB)
                rk_udphy_u3_port_disable(udphy, false);
        ...
}

If this path is taken, the U3 port is left permanently disabled and in reset,
but the software state (udphy->status) incorrectly remains active. The USB3
controller is unaware that the PHY has been forced into reset. 

Could this result in a silently dropped USB3 connection, potentially causing
I/O errors for active storage devices? 

While a previous patch in the series mitigates the SError by safely routing
the PIPE clock to UTMI, the state inconsistency and dead USB connection seem
to remain.

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