On 8/4/23 18:44, Suzuki K Poulose wrote:
On 03/08/2023 09:27, Anshuman Khandual wrote:
Hello Dan,
On 7/29/23 12:50, Dan Carpenter wrote:
Hello Anshuman Khandual,
The patch 73d779a03a76: "coresight: etm4x: Change etm4_platform_driver driver for MMIO devices" from Jul 10, 2023 (linux-next), leads to the following Smatch static checker warning:
drivers/hwtracing/coresight/coresight-etm4x-core.c:2272 etm4_remove_platform_dev() error: we previously assumed 'drvdata' could be null (see line 2268)
drivers/hwtracing/coresight/coresight-etm4x-core.c 2264 static int __exit etm4_remove_platform_dev(struct platform_device *pdev) 2265 { 2266 struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev); 2267 2268 if (drvdata)
This check can probably be removed, right?
etm4_remove_dev(drvdata) cannot be called if drvdata is NULL. But there might be an implicit assumption that 'drvdata' could never be NULL in this path.
2269 etm4_remove_dev(drvdata); 2270 pm_runtime_disable(&pdev->dev); 2271 --> 2272 if (drvdata->pclk)
Not checked here
We could either have the following change which ensures that 'drvdata' is checked again before the clock is handled.
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 703b6fcbb6a5..9d186af81ea0 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -2267,11 +2267,13 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev) if (drvdata) etm4_remove_dev(drvdata); - pm_runtime_disable(&pdev->dev); - if (drvdata->pclk) - clk_put(drvdata->pclk); + pm_runtime_disable(&pdev->dev); + if (drvdata) { + if (drvdata->pclk && !IS_ERR(drvdata->pclk)) + clk_put(drvdata->pclk); + } return 0; } OR remove the first drvdata check as suggested.
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 703b6fcbb6a5..6e1ffddbd4be 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -2265,11 +2265,9 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev) { struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev); - if (drvdata) - etm4_remove_dev(drvdata); + etm4_remove_dev(drvdata); pm_runtime_disable(&pdev->dev);
- if (drvdata->pclk) + if (drvdata->pclk && !IS_ERR(drvdata->pclk) clk_put(drvdata->pclk); return 0;
2273 clk_put(drvdata->pclk); 2274 2275 return 0; 2276 }
Also adding Suzuki and James for their input.
Could we not do :
pm_runtime_disable(&pdev->dev); if (drvdata) { etm4_remove_dev(drvdata); if (drvdata->pclk) clk_put(drvdata->pclk); }
return 0;
Could pm_runtime_disable() be called before etm4_remove_dev() ? I was bit cautious, otherwise we could definitely change as suggested above.