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

[Simon Horman]

+ Andrew Lunn

On Thu, Oct 03, 2024 at 03:55:35PM +0300, Fedor Pchelkin wrote:
> 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().

Thanks, I hadn't correlated that patch with this one.
I agree that it address my comment about needing
to check wmac->data here and below.

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

I see that Andrew Lunn responded that a comment is appropriate,
rather than writing code to please the checkers. And, FWIIW,
I agree with his suggestion.

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