Re: [PATCH v7 1/4] phy: ti-pipe3: Fix ignored clock enable return value in init

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Clock enable count leak on race condition error path during SATA lock check in ti_pipe3_init()
--

commit 6767d03245555c6eb67e9613e47d287d39988b18
Author: Hongling Zeng <zenghongling@kylinos.cn>

phy: ti-pipe3: Fix ignored clock enable return value in init

ti_pipe3_init() ignores the return value of ti_pipe3_enable_clocks(),
which can lead to unclocked hardware access and unbalanced clock disables.
Fix this by checking the return value and returning early if it fails.

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index b5543b5c674c8..0e3bc92187338 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
> @@ -498,9 +498,12 @@ static int ti_pipe3_init(struct phy *x)
>  {
>  	struct ti_pipe3 *phy = phy_get_drvdata(x);
>  	u32 val;
> -	int ret = 0;
> +	int ret;
> +
> +	ret = ti_pipe3_enable_clocks(phy);
> +	if (ret)
> +		return ret;
>  
> -	ti_pipe3_enable_clocks(phy);
>  	/*
>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

[Severity: High]
This is a pre-existing issue, but further down in this function, does this
leak the clock enable count on a race condition error path?

If ti_pipe3_dpll_wait_lock() fails due to a timeout (returning -EBUSY), but
the DPLL locks immediately after, (val & PLL_LOCK) will evaluate to true.

For SATA mode, it then returns the stale -EBUSY error without disabling the
clocks:

drivers/phy/ti/phy-ti-pipe3.c:ti_pipe3_init() {
    ...
	if (val & PLL_IDLE) {
		val &= ~PLL_IDLE;
		ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
		ret = ti_pipe3_dpll_wait_lock(phy);
	}

	/* SATA has issues if re-programmed when locked */
	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
	if ((val & PLL_LOCK) && phy->mode == PIPE3_MODE_SATA)
		return ret;
    ...
}

While this requires specific hardware timings or delays (like high load or
preemption) allowing the timeout to expire just before the lock succeeds, it
remains a structural leak on this failure path.

Should this failure path also call ti_pipe3_disable_clocks() before returning
an error, similar to the other error paths in this function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909113433.1287454-1-zenghongling@kylinos.cn?part=1

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