Re: [PATCH v6 2/4] phy: ti: pipe3: Fix clock resource leak on probe errors

[Manivannan Sadhasivam]

On Fri, Jun 19, 2026 at 11:02:12AM +0800, Hongling Zeng wrote:
> When devm_phy_create() or devm_of_phy_provider_register() fails,
> the refclk that was enabled earlier is not disabled, causing a
> resource leak.
> 
> Fix this by adding an error handling path to disable the clock
> when these functions fail.
> 
> Fixes: 234738ea3390 ("phy: ti-pipe3: move clk initialization to a separate function")
> Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> 
> ---
>  Change in v2:
>   -Add pm_runtime_disable() in error path (reported by Sashiko AI).
> ---
>  change in v3:
>   -Fix unbalanced clock disable by checking clk_prepare_enable()return value and
> setting sata_refclk_enabledonly on success.
>   -Fix error path teardown order to match ti_pipe3_remove()(disable clock before
> disabling runtime PM).
> ---
>  change in v4:
>   -Fix the patch version
> ---
> 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 (also fix incorrect return logic)
>  -SATA errata path in ti_pipe3_exit()
> ---
>  drivers/phy/ti/phy-ti-pipe3.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index b8c055893742..2d36fe4c4218 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
> @@ -834,21 +834,39 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>  	 */
>  	if (phy->mode == PIPE3_MODE_SATA) {
>  		if (!IS_ERR(phy->refclk)) {
> -			clk_prepare_enable(phy->refclk);
> +			ret = clk_prepare_enable(phy->refclk);
> +			if (ret) {
> +				dev_err(dev, "Failed to enable refclk %d\n", ret);
> +				goto err_pm_disable;
> +			}
>  			phy->sata_refclk_enabled = true;
>  		}
>  	}
>  
>  	generic_phy = devm_phy_create(dev, NULL, &ops);
> -	if (IS_ERR(generic_phy))
> -		return PTR_ERR(generic_phy);
> +	if (IS_ERR(generic_phy)) {
> +		ret = PTR_ERR(generic_phy);
> +		goto err_clk_disable;
> +	}
>  
>  	phy_set_drvdata(generic_phy, phy);
>  
>  	ti_pipe3_power_off(generic_phy);
>  
>  	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> -	return PTR_ERR_OR_ZERO(phy_provider);
> +	if (IS_ERR(phy_provider)) {
> +		ret = PTR_ERR(phy_provider);
> +		goto err_clk_disable;
> +	}
> +
> +	return 0;
> +
> +err_clk_disable:
> +	if (phy->sata_refclk_enabled && !IS_ERR(phy->refclk))

'!IS_ERR(phy->refclk)' check is redundant since the 'phy->sata_refclk_enabled'
flag is set only when refclk is valid. 

- Mani

-- 
மணிவண்ணன் சதாசிவம்