Re: [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines

[Simon Horman <horms@kernel.org>]

On Mon, Sep 30, 2024 at 09:37:15PM +0300, Vitalii Mordan wrote:
> If the clock dwmac->tx_clk was not enabled in intel_eth_plat_probe,
> it should not be disabled in any path.
> 
> Conversely, if it was enabled in intel_eth_plat_probe, it must be disabled
> in all error paths to ensure proper cleanup.
> 
> Found by Linux Verification Center (linuxtesting.org) with Klever.
> 
> Fixes: 9efc9b2b04c7 ("net: stmmac: Add dwmac-intel-plat for GBE driver")
> Signed-off-by: Vitalii Mordan <mordan@ispras.ru>
> ---
>  .../ethernet/stmicro/stmmac/dwmac-intel-plat.c   | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> index d68f0c4e7835..2a2893f2f2a8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> @@ -108,7 +108,12 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>  			if (IS_ERR(dwmac->tx_clk))
>  				return PTR_ERR(dwmac->tx_clk);
>  
> -			clk_prepare_enable(dwmac->tx_clk);
> +			ret = clk_prepare_enable(dwmac->tx_clk);
> +			if (ret) {
> +				dev_err(&pdev->dev,
> +					"Failed to enable tx_clk\n");
> +				return ret;
> +			}
>  
>  			/* Check and configure TX clock rate */
>  			rate = clk_get_rate(dwmac->tx_clk);
> @@ -117,6 +122,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>  				rate = dwmac->data->tx_clk_rate;
>  				ret = clk_set_rate(dwmac->tx_clk, rate);
>  				if (ret) {
> +					clk_disable_unprepare(dwmac->tx_clk);
>  					dev_err(&pdev->dev,
>  						"Failed to set tx_clk\n");
>  					return ret;

Hi Vitalii,

I think that unwinding using a goto label would be more idiomatic here
and in the following changes to intel_eth_plat_probe().

> @@ -131,6 +137,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>  			rate = dwmac->data->ptp_ref_clk_rate;
>  			ret = clk_set_rate(plat_dat->clk_ptp_ref, rate);
>  			if (ret) {
> +				if (dwmac->data->tx_clk_en)
> +					clk_disable_unprepare(dwmac->tx_clk);
>  				dev_err(&pdev->dev,
>  					"Failed to set clk_ptp_ref\n");
>  				return ret;
> @@ -150,7 +158,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev)
>  
>  	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
>  	if (ret) {
> -		clk_disable_unprepare(dwmac->tx_clk);
> +		if (dwmac->data->tx_clk_en)
> +			clk_disable_unprepare(dwmac->tx_clk);

Smatch warns that dwmac->data may be NULL here.

>  		return ret;
>  	}
>  
> @@ -162,7 +171,8 @@ static void intel_eth_plat_remove(struct platform_device *pdev)
>  	struct intel_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
>  
>  	stmmac_pltfr_remove(pdev);
> -	clk_disable_unprepare(dwmac->tx_clk);
> +	if (dwmac->data->tx_clk_en)

And I wonder if it can be NULL here too.

> +		clk_disable_unprepare(dwmac->tx_clk);
>  }
>  
>  static struct platform_driver intel_eth_plat_driver = {
> -- 
> 2.25.1
> 
>