| Message ID | 20240930183715.2112075-1-mordan@ispras.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines | expand |
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 > >
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@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. 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@ispras.ru/ 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 > > > >
+ 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@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. > > 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@ispras.ru/ > > 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 > > > > > > >
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; @@ -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); 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) + clk_disable_unprepare(dwmac->tx_clk); } static struct platform_driver intel_eth_plat_driver = {
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(-)