| Message ID | 20241108173334.2973603-1-mordan@ispras.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net,v2] : stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines | expand |
Hi Vitalii,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Vitalii-Mordan/stmmac-dwmac-intel-plat-fix-call-balance-of-tx_clk-handling-routines/20241109-013647
base: net/main
patch link: https://lore.kernel.org/r/20241108173334.2973603-1-mordan%40ispras.ru
patch subject: [PATCH net v2]: stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines
config: arm-randconfig-r071-20241110 (https://download.01.org/0day-ci/archive/20241111/202411110911.fxtHBKSw-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202411110911.fxtHBKSw-lkp@intel.com/
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)
vim +163 drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 75 static int intel_eth_plat_probe(struct platform_device *pdev)
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 76 {
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 77 struct plat_stmmacenet_data *plat_dat;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 78 struct stmmac_resources stmmac_res;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 79 struct intel_dwmac *dwmac;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 80 unsigned long rate;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 81 int ret;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 82
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 83 ret = stmmac_get_platform_resources(pdev, &stmmac_res);
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 84 if (ret)
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 85 return ret;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 86
abea8fd5e801a6 Jisheng Zhang 2023-09-16 87 plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 88 if (IS_ERR(plat_dat)) {
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 89 dev_err(&pdev->dev, "dt configuration failed\n");
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 90 return PTR_ERR(plat_dat);
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 91 }
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 92
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 93 dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
abea8fd5e801a6 Jisheng Zhang 2023-09-16 94 if (!dwmac)
abea8fd5e801a6 Jisheng Zhang 2023-09-16 95 return -ENOMEM;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 96
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 97 dwmac->dev = &pdev->dev;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 98 dwmac->tx_clk = NULL;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 99
b0377116decdee Rob Herring 2023-10-09 100 dwmac->data = device_get_match_data(&pdev->dev);
b0377116decdee Rob Herring 2023-10-09 @101 if (dwmac->data) {
^^^^^^^^^^^
Check for NULL
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 102 if (dwmac->data->fix_mac_speed)
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 103 plat_dat->fix_mac_speed = dwmac->data->fix_mac_speed;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 104
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 105 /* Enable TX clock */
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 106 if (dwmac->data->tx_clk_en) {
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 107 dwmac->tx_clk = devm_clk_get(&pdev->dev, "tx_clk");
abea8fd5e801a6 Jisheng Zhang 2023-09-16 108 if (IS_ERR(dwmac->tx_clk))
abea8fd5e801a6 Jisheng Zhang 2023-09-16 109 return PTR_ERR(dwmac->tx_clk);
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 110
bd8cfad17c9530 Vitalii Mordan 2024-11-08 111 ret = clk_prepare_enable(dwmac->tx_clk);
bd8cfad17c9530 Vitalii Mordan 2024-11-08 112 if (ret) {
bd8cfad17c9530 Vitalii Mordan 2024-11-08 113 dev_err(&pdev->dev,
bd8cfad17c9530 Vitalii Mordan 2024-11-08 114 "Failed to enable tx_clk\n");
bd8cfad17c9530 Vitalii Mordan 2024-11-08 115 return ret;
bd8cfad17c9530 Vitalii Mordan 2024-11-08 116 }
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 117
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 118 /* Check and configure TX clock rate */
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 119 rate = clk_get_rate(dwmac->tx_clk);
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 120 if (dwmac->data->tx_clk_rate &&
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 121 rate != dwmac->data->tx_clk_rate) {
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 122 rate = dwmac->data->tx_clk_rate;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 123 ret = clk_set_rate(dwmac->tx_clk, rate);
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 124 if (ret) {
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 125 dev_err(&pdev->dev,
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 126 "Failed to set tx_clk\n");
bd8cfad17c9530 Vitalii Mordan 2024-11-08 127 goto err_tx_clk_disable;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 128 }
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 129 }
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 130 }
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 131
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 132 /* Check and configure PTP ref clock rate */
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 133 rate = clk_get_rate(plat_dat->clk_ptp_ref);
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 134 if (dwmac->data->ptp_ref_clk_rate &&
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 135 rate != dwmac->data->ptp_ref_clk_rate) {
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 136 rate = dwmac->data->ptp_ref_clk_rate;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 137 ret = clk_set_rate(plat_dat->clk_ptp_ref, rate);
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 138 if (ret) {
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 139 dev_err(&pdev->dev,
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 140 "Failed to set clk_ptp_ref\n");
bd8cfad17c9530 Vitalii Mordan 2024-11-08 141 goto err_tx_clk_disable;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 142 }
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 143 }
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 144 }
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 145
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 146 plat_dat->bsp_priv = dwmac;
b4c5f83ae3f3e2 Rusaimi Amira Ruslan 2020-09-28 147 plat_dat->eee_usecs_rate = plat_dat->clk_ptp_rate;
b4c5f83ae3f3e2 Rusaimi Amira Ruslan 2020-09-28 148
b4c5f83ae3f3e2 Rusaimi Amira Ruslan 2020-09-28 149 if (plat_dat->eee_usecs_rate > 0) {
b4c5f83ae3f3e2 Rusaimi Amira Ruslan 2020-09-28 150 u32 tx_lpi_usec;
b4c5f83ae3f3e2 Rusaimi Amira Ruslan 2020-09-28 151
b4c5f83ae3f3e2 Rusaimi Amira Ruslan 2020-09-28 152 tx_lpi_usec = (plat_dat->eee_usecs_rate / 1000000) - 1;
b4c5f83ae3f3e2 Rusaimi Amira Ruslan 2020-09-28 153 writel(tx_lpi_usec, stmmac_res.addr + GMAC_1US_TIC_COUNTER);
b4c5f83ae3f3e2 Rusaimi Amira Ruslan 2020-09-28 154 }
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 155
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 156 ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
bd8cfad17c9530 Vitalii Mordan 2024-11-08 157 if (ret)
bd8cfad17c9530 Vitalii Mordan 2024-11-08 158 goto err_tx_clk_disable;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 159
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 160 return 0;
bd8cfad17c9530 Vitalii Mordan 2024-11-08 161
bd8cfad17c9530 Vitalii Mordan 2024-11-08 162 err_tx_clk_disable:
bd8cfad17c9530 Vitalii Mordan 2024-11-08 @163 if (dwmac->data->tx_clk_en)
^^^^^^^^^^^^^
Unchecked dereference
bd8cfad17c9530 Vitalii Mordan 2024-11-08 164 clk_disable_unprepare(dwmac->tx_clk);
bd8cfad17c9530 Vitalii Mordan 2024-11-08 165 return ret;
9efc9b2b04c74e Rusaimi Amira Ruslan 2020-08-26 166 }
Hi, On Mon, 11. Nov 12:39, Dan Carpenter wrote: > 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
On Mon, Nov 11, 2024 at 01:25:42PM +0300, Fedor Pchelkin wrote: > Hi, > > On Mon, 11. Nov 12:39, Dan Carpenter wrote: > > 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 <kuba@kernel.org>: 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. > > [...] Here is the summary with links: - [net,v2] : stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines https://git.kernel.org/netdev/net/c/5b366eae7193 You are awesome, thank you!
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c index d68f0c4e7835..9739bc9867c5 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); @@ -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"); - return ret; + goto err_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"); - return ret; + goto err_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); - return ret; - } + if (ret) + goto err_tx_clk_disable; return 0; + +err_tx_clk_disable: + if (dwmac->data->tx_clk_en) + clk_disable_unprepare(dwmac->tx_clk); + return ret; } static void intel_eth_plat_remove(struct platform_device *pdev) @@ -162,7 +170,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> --- 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(-)