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

[Hongling Zeng]

在 2026年09月09日 15:58, Manivannan Sadhasivam 写道:
> On Fri, Jun 19, 2026 at 11:02:14AM +0800, Hongling Zeng wrote:
> > When regmap_update_bits() fails in ti_pipe3_init() for PCIe mode,
> > the function returns the error without calling ti_pipe3_disable_clocks().
> > This leaves the clocks permanently enabled since the PHY framework won't
> > invoke the .exit callback on init failure.
> >
> > Fix this by adding proper clock cleanup in the PCIe error path, consistent
> > with how the DPLL program error path handles cleanup.
> >
> > Fixes: 234738ea3390 ("phy: ti-pipe3: move clk initialization to a separate function")
> > Reported-by: Sashiko AI <sashiko@xxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/20260518023657.41852C2BCB0@xxxxxxxxxxxxxxx/
> > Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> >
> > ---
> > Change in v5:
> >    -Add Fix ignored clock enable return value in init patch
> > ---
> > Change in v6:
> >   -Fix all clock leak paths comprehensively:
> >   -PCIe syscon update failure path
> >   -SATA DPLL lock check path
> >   -SATA errata path in ti_pipe3_exit()
> > ---
> >   drivers/phy/ti/phy-ti-pipe3.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> > index 9ec228c2a940..860058f31594 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;
> >   	}
> >   
> > @@ -531,8 +533,9 @@ static int ti_pipe3_init(struct phy *x)
> >   
> >   	/* 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 ((val & PLL_LOCK) && phy->mode == PIPE3_MODE_SATA) {
> > +		return 0;
> Why are you returning 0 here and below?
>
> - Mani
Hi Mani,

Thanks for pointing this out.

You're right. Changing the SATA PLL-locked path from `return ret` to
`return 0` was an unintended change and is not related to the clock leak
fix. I'll restore the original code in the next version:

    if ((val & PLL_LOCK) && phy->mode == PIPE3_MODE_SATA)
        return ret;

Thanks,
Hongling