Re: [PATCH v2] phy: rockchip: snps-pcie3: Raise the SRAM init timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] phy: rockchip: misleading log and bitwise OR of error codes
--

commit 3bcb5033387e2e5fffc56e29d4197fecdb178968
Author: Shawn Lin <shawn.lin@rock-chips.com>

phy: rockchip: snps-pcie3: Raise the SRAM init timeout

The SRAM initialization of the PCIe3 PHY may take longer than the 500us
currently allowed. This commit raises the timeout to 20ms and uses a non-zero
sleep interval to yield the CPU instead of tight-looping.

> diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> index 4e8ffd173096a..69bda8b2255d1 100644
> --- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> +++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c

[ ... ]

> @@ -187,11 +189,11 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
>  	ret = regmap_read_poll_timeout(priv->phy_grf,
>  				       RK3588_PCIE3PHY_GRF_PHY0_STATUS1,
>  				       reg, RK3588_SRAM_INIT_DONE(reg),
> -				       0, 500);
> +				       1000, RK_PCIE_SRAM_INIT_TIMEOUT);
>  	ret |= regmap_read_poll_timeout(priv->phy_grf,
>  					RK3588_PCIE3PHY_GRF_PHY1_STATUS1,
>  					reg, RK3588_SRAM_INIT_DONE(reg),
> -					0, 500);
> +					1000, RK_PCIE_SRAM_INIT_TIMEOUT);
>  	if (ret)
>  		dev_err(&priv->phy->dev, "lock failed 0x%x, check input refclk and power supply\n",
>  			reg);

[Severity: Low]
Does this sequence safely handle errors when the first poll times out but the
second one succeeds?

Looking at rockchip_p3phy_rk3588_init(), if the first
regmap_read_poll_timeout() fails, it will return an error code and leave reg
at its failing value. However, the bitwise OR assignment executes the second
poll regardless of the first poll's outcome.

If the second poll succeeds, reg is overwritten with a successful state.
The bitwise OR will combine the error code with the second poll's return
value, which might corrupt the error code returned to the caller.

Additionally, the error check evaluates if the combined ret is non-zero,
triggering the error log. The log will print the potentially successful reg
value from the second lane, which could mislead developers investigating lock
failures.

Should the second poll be skipped if the first one fails, or should separate
variables be used to track the status of each lane?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1792051843-93245-1-git-send-email-shawn.lin@rock-chips.com?part=1

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