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

[Fedor Pchelkin]

Hello,

On Thu, 03. Oct 12:18, Simon Horman wrote:
> 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@xxxxxxxxx>
> > ---
> >  .../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.

FWIW, there is a patch [1] targeted at net-next which removes the seemingly
redundant check for dwmac->data.

[1]: https://lore.kernel.org/netdev/20240930183926.2112546-1-mordan@xxxxxxxxx/

At the moment device_get_match_data() can't return NULL in probe function
of this driver - it gets the data from static const intel_eth_plat_match[]
table where every entry has defined non-NULL .data.

It's not expected (at least currently) that there would be any code changes
to the driver match table so it looks worthwhile to remove the check in
order to reduce additional complexity in error paths and
intel_eth_plat_remove().

That said, maybe it would be more safe now to rearrange the check to fail
at probe stage in case dwmac->data is NULL. Just not to confuse the static
analysis tools :)

Thanks!

> 
> >  		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
> > 
> >