Re: [PATCH v5 4/4] phy: ti-pipe3: Fix clock leak in init error path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Resource leak in ti_pipe3_init() for SATA mode late-lock race.
- [High] Unbalanced clock lifecycle operations for SATA mode due to early return in ti_pipe3_exit().
--

commit d2822879f79b4be953a465847a8621060caa36ac
Author: Hongling Zeng <zenghongling@kylinos.cn>
Subject: phy: ti-pipe3: Fix clock leak in init error path

This commit fixes a clock reference count leak in the PCIe error path of
ti_pipe3_init(). It explicitly calls ti_pipe3_disable_clocks() when
regmap_update_bits() fails, ensuring proper cleanup since the PHY framework
won't invoke the .exit callback on init failure.

Link: https://lore.kernel.org/all/20260518023657.41852C2BCB0@smtp.kernel.org/

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index 9ec228c2a940c..4897e4ba2d7db 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
> @@ -518,6 +518,8 @@ static int ti_pipe3_init(struct phy *x)
>  		val = 0x96 << OMAP_CTRL_PCIE_PCS_DELAY_COUNT_SHIFT;
>  		ret = regmap_update_bits(phy->pcs_syscon, phy->pcie_pcs_reg,
>  					 PCIE_PCS_MASK, val);
> +		if (ret)
> +			ti_pipe3_disable_clocks(phy);
>  		return ret;
>  	}
>  

This problem wasn't introduced by this patch, but a similar regression
exists in the error handling path of ti_pipe3_init() just below this block.

When bringing the DPLL out of IDLE, if ti_pipe3_dpll_wait_lock() times out,
it sets ret to -EBUSY. Immediately after, PLL_STATUS is read:

	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
	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;

If the DPLL locked just after the timeout, the PLL_LOCK bit will be set.
For SATA mode, the function returns -EBUSY early without calling
ti_pipe3_disable_clocks().

Since the PHY framework won't invoke the exit callback on an init failure,
will this leak the clock reference counts?

Should the function either return 0 here to treat the late lock as a success,
or call ti_pipe3_disable_clocks() before returning the error?

Another regression that wasn't introduced by this patch exists in the driver's
lifecycle management for SATA mode.

In ti_pipe3_exit(), if the mode is SATA and dpll_reset_syscon is not present,
the function returns early to avoid powering down the SATA DPLL due to
Errata i783:

static int ti_pipe3_exit(struct phy *x)
{
	struct ti_pipe3 *phy = phy_get_drvdata(x);

	/* If dpll_reset_syscon is not present we wont power down SATA DPLL
	 * due to Errata i783
	 */
	if (phy->mode == PIPE3_MODE_SATA && !phy->dpll_reset_syscon)
		return 0;

However, it returns without calling ti_pipe3_disable_clocks().

Because ti_pipe3_init() unconditionally calls ti_pipe3_enable_clocks(),
could repeated initialization and exit cycles, such as during suspend and
resume or module reload, invoke clk_prepare_enable() without matching
clk_disable_unprepare() calls?

If the errata strictly requires keeping the clocks enabled, does the driver
need to track the initialization state to avoid double-enabling? Alternatively,
should it properly disable the clocks before returning?

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

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