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 <redacted>
---
v2: Unwind using a goto label, as per Simon Horman's request.
.../stmicro/stmmac/dwmac-intel-plat.c | 25 +++++++++++++------
1 file changed, 17 insertions(+), 8 deletions(-)
@@ -108,7 +108,12 @@ static int intel_eth_plat_probe(struct platform_device *pdev)if(IS_ERR(dwmac->tx_clk))returnPTR_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");+returnret;+}/* Check and configure TX clock rate */rate=clk_get_rate(dwmac->tx_clk);
@@ -119,7 +124,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev)if(ret){dev_err(&pdev->dev,"Failed to set tx_clk\n");-returnret;+gotoerr_tx_clk_disable;}}}
@@ -133,7 +138,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev)if(ret){dev_err(&pdev->dev,"Failed to set clk_ptp_ref\n");-returnret;+gotoerr_tx_clk_disable;}}}
@@ -149,12 +154,15 @@ 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);-returnret;-}+if(ret)+gotoerr_tx_clk_disable;return0;++err_tx_clk_disable:+if(dwmac->data->tx_clk_en)+clk_disable_unprepare(dwmac->tx_clk);+returnret;}staticvoidintel_eth_plat_remove(structplatform_device*pdev)
smatch warnings:
drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c:163 intel_eth_plat_probe() error: we previously assumed 'dwmac->data' could be null (see line 101)
There is a patch [1] targeted at net-next tree which removes the check. I
think there should be v2 posted soon.
As it's not the first time Smatch is pointing at this issue [2], is there
something to improve? I mean, posting the patches in form of a series or
explaining in commit message that the check is redundant and is a subject
for removal? Adding new redundant checks for the fix-patch would not be
good..
What would be the most appropriate way?
[1]: https://lore.kernel.org/netdev/20240930183926.2112546-1-mordan@ispras.ru/
[2]: https://lore.kernel.org/netdev/20241003111811.GJ1310185@kernel.org/
--
Thanks,
Fedor
From: Dan Carpenter <hidden> Date: 2024-11-11 10:39:59
On Mon, Nov 11, 2024 at 01:25:42PM +0300, Fedor Pchelkin wrote:
Hi,
On Mon, 11. Nov 12:39, Dan Carpenter wrote:
quoted
smatch warnings:
drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c:163 intel_eth_plat_probe() error: we previously assumed 'dwmac->data' could be null (see line 101)
There is a patch [1] targeted at net-next tree which removes the check. I
think there should be v2 posted soon.
As it's not the first time Smatch is pointing at this issue [2], is there
something to improve? I mean, posting the patches in form of a series or
explaining in commit message that the check is redundant and is a subject
for removal? Adding new redundant checks for the fix-patch would not be
good..
What would be the most appropriate way?
[1]: https://lore.kernel.org/netdev/20240930183926.2112546-1-mordan@ispras.ru/
[2]: https://lore.kernel.org/netdev/20241003111811.GJ1310185@kernel.org/
Once we remove the NULL check then the warning will go away.
I don't look at it like a big deal that both Simon and kbuild-bot reported the
same issue. Especially since he reported it against an earlier version of this
patch.
regards,
dan carpenter
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski [off-list ref]:
On Fri, 8 Nov 2024 20:33:34 +0300 you 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.
[...]