[PATCH v2] pci: controller: pci-ftpci100: Release the clock resources

[PATCH v2] pci: controller: pci-ftpci100: Release the clock resources

[Junyan Ye]

Smatch reported:
1. drivers/pci/controller/pci-ftpci100.c:526 faraday_pci_probe()
warn: 'clk' from clk_prepare_enable() not released on lines:
442,451,462,478,512,517.
2. drivers/pci/controller/pci-ftpci100.c:526 faraday_pci_probe()
warn: 'p->bus_clk' from clk_prepare_enable() not released on lines:
451,462,478,512,517.

The clock resource is obtained by the devm_clk_get function. The
clk_prepare_enable function then makes the clock resource ready for use,
notifying the system that the clock resource should be run. After that,
the clock resource should be released when it is no longer needed. The
corresponding function is clk_disable_unprepare. However, while doing
some error handling in the faraday_pci_probe function, the
clk_disable_unprepare function is not called to release the clk and
p->bus_clk resources.

Fix this warning by changing the devm_clk_get function to
devm_clk_get_enabled, which is equivalent to
devm_clk_get() + clk_prepare_enable(). And with the
devm_clk_get_enabled function, the clock will automatically be
disabled, unprepared and freed when the device is unbound from the bus.

Fixes: b3c433efb8a3 ("PCI: faraday: Fix wrong pointer passed to PTR_ERR()")
Fixes: 2eeb02b28579 ("PCI: faraday: Add clock handling")
Fixes: 783a862563f7 ("PCI: faraday: Use pci_parse_request_of_pci_ranges()")
Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
Fixes: f1e8bd21e39e ("PCI: faraday: Convert IRQ masking to raw PCI config accessors")
Signed-off-by: Junyan Ye <yejunyan@hust.edu.cn>
Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
---
v1 -> v2: Switch from clk_disable_unprepare() to devm_clk_get_enabled() to release the clock.
This issue is found by static analyzer.

 drivers/pci/controller/pci-ftpci100.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pci-ftpci100.c b/drivers/pci/controller/pci-ftpci100.c
index ecd3009df586..6e7981d2ed5e 100644
--- a/drivers/pci/controller/pci-ftpci100.c
+++ b/drivers/pci/controller/pci-ftpci100.c
@@ -429,22 +429,12 @@ static int faraday_pci_probe(struct platform_device *pdev)
 	p->dev = dev;
 
 	/* Retrieve and enable optional clocks */
-	clk = devm_clk_get(dev, "PCLK");
+	clk = devm_clk_get_enabled(dev, "PCLK");
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "could not prepare PCLK\n");
-		return ret;
-	}
-	p->bus_clk = devm_clk_get(dev, "PCICLK");
+	p->bus_clk = devm_clk_get_enabled(dev, "PCICLK");
 	if (IS_ERR(p->bus_clk))
 		return PTR_ERR(p->bus_clk);
-	ret = clk_prepare_enable(p->bus_clk);
-	if (ret) {
-		dev_err(dev, "could not prepare PCICLK\n");
-		return ret;
-	}
 
 	p->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(p->base))
-- 
2.25.1

Re: [PATCH v2] pci: controller: pci-ftpci100: Release the clock resources

[Linus Walleij]

On Sun, Apr 23, 2023 at 7:34 AM Junyan Ye <yejunyan@hust.edu.cn> wrote:

> Smatch reported:
> 1. drivers/pci/controller/pci-ftpci100.c:526 faraday_pci_probe()
> warn: 'clk' from clk_prepare_enable() not released on lines:
> 442,451,462,478,512,517.
> 2. drivers/pci/controller/pci-ftpci100.c:526 faraday_pci_probe()
> warn: 'p->bus_clk' from clk_prepare_enable() not released on lines:
> 451,462,478,512,517.
>
> The clock resource is obtained by the devm_clk_get function. The
> clk_prepare_enable function then makes the clock resource ready for use,
> notifying the system that the clock resource should be run. After that,
> the clock resource should be released when it is no longer needed. The
> corresponding function is clk_disable_unprepare. However, while doing
> some error handling in the faraday_pci_probe function, the
> clk_disable_unprepare function is not called to release the clk and
> p->bus_clk resources.
>
> Fix this warning by changing the devm_clk_get function to
> devm_clk_get_enabled, which is equivalent to
> devm_clk_get() + clk_prepare_enable(). And with the
> devm_clk_get_enabled function, the clock will automatically be
> disabled, unprepared and freed when the device is unbound from the bus.
>
> Fixes: b3c433efb8a3 ("PCI: faraday: Fix wrong pointer passed to PTR_ERR()")
> Fixes: 2eeb02b28579 ("PCI: faraday: Add clock handling")
> Fixes: 783a862563f7 ("PCI: faraday: Use pci_parse_request_of_pci_ranges()")
> Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
> Fixes: f1e8bd21e39e ("PCI: faraday: Convert IRQ masking to raw PCI config accessors")
> Signed-off-by: Junyan Ye <yejunyan@hust.edu.cn>
> Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
> ---
> v1 -> v2: Switch from clk_disable_unprepare() to devm_clk_get_enabled() to release the clock.
> This issue is found by static analyzer.

Neat fix, saves codelines!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2] pci: controller: pci-ftpci100: Release the clock resources

[Bjorn Helgaas]

On Sun, Apr 23, 2023 at 01:32:07PM +0800, Junyan Ye wrote:
> Smatch reported:
> 1. drivers/pci/controller/pci-ftpci100.c:526 faraday_pci_probe()
> warn: 'clk' from clk_prepare_enable() not released on lines:
> 442,451,462,478,512,517.
> 2. drivers/pci/controller/pci-ftpci100.c:526 faraday_pci_probe()
> warn: 'p->bus_clk' from clk_prepare_enable() not released on lines:
> 451,462,478,512,517.

If/when you repost this, please:

  - Rebase to v6.4-rc1 (when it's available).

  - Look at subject line history for this file and match it.

  - Include the smatch warnings exactly above (not wrapped to fit in
    75 columns).  This is to make it easier to search for the text.

  - Add "()" after function names below to make it obvious they are
    functions.  You can also omit "function", e.g., "... is obtained
    by devm_clk_get()."

> The clock resource is obtained by the devm_clk_get function. The
> clk_prepare_enable function then makes the clock resource ready for use,
> notifying the system that the clock resource should be run. After that,
> the clock resource should be released when it is no longer needed. The
> corresponding function is clk_disable_unprepare. However, while doing
> some error handling in the faraday_pci_probe function, the
> clk_disable_unprepare function is not called to release the clk and
> p->bus_clk resources.

I don't know the clk terminology.  Does "... clock resource should be
run" mean the clock should be "running" or "enabled"?
include/linux/clk.h only uses "running" twice but has many instances
of "enable", so maybe that would be better.  Or maybe it's enough to
say:

  clk_prepare_enable() then makes the clock resource ready for use.

and omit the rest of that sentence?

Looks like a nice fix, thank you!

Bjorn