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 devm_clk_get(), and then
clk_prepare_enable() makes the clock resource ready for use. After that,
clk_disable_unprepare() should be called to release the clock resource
when it is no longer needed. However, while doing some error handling
in faraday_pci_probe(), clk_disable_unprepare() is not called to release
clk and p->bus_clk before returning. These return lines are exactly 442,
451, 462, 478, 512, 517.
Fix this warning by replacing devm_clk_get() with devm_clk_get_enabled(),
which is equivalent to devm_clk_get() + clk_prepare_enable(). And with
devm_clk_get_enabled(), 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")
Reviewed-by: Dongliang Mu <[email protected]>
Signed-off-by: Junyan Ye <[email protected]>
---
v2 -> v3: Rebase to v6.4-rc1 and modify commit message.
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
On Mon, May 8, 2023 at 6:41 AM Junyan Ye <[email protected]> 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 devm_clk_get(), and then
> clk_prepare_enable() makes the clock resource ready for use. After that,
> clk_disable_unprepare() should be called to release the clock resource
> when it is no longer needed. However, while doing some error handling
> in faraday_pci_probe(), clk_disable_unprepare() is not called to release
> clk and p->bus_clk before returning. These return lines are exactly 442,
> 451, 462, 478, 512, 517.
>
> Fix this warning by replacing devm_clk_get() with devm_clk_get_enabled(),
> which is equivalent to devm_clk_get() + clk_prepare_enable(). And with
> devm_clk_get_enabled(), 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")
> Reviewed-by: Dongliang Mu <[email protected]>
> Signed-off-by: Junyan Ye <[email protected]>
> ---
> v2 -> v3: Rebase to v6.4-rc1 and modify commit message.
Reviewed-by: Linus Walleij <[email protected]>
It's a fine fix since it saves lots of code.
I would merge it as a non-urgent fix.
Yours,
Linus Walleij
Le 08/05/2023 à 06:36, Junyan Ye a écrit :
> 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 devm_clk_get(), and then
> clk_prepare_enable() makes the clock resource ready for use. After that,
> clk_disable_unprepare() should be called to release the clock resource
> when it is no longer needed. However, while doing some error handling
> in faraday_pci_probe(), clk_disable_unprepare() is not called to release
> clk and p->bus_clk before returning. These return lines are exactly 442,
> 451, 462, 478, 512, 517.
>
> Fix this warning by replacing devm_clk_get() with devm_clk_get_enabled(),
> which is equivalent to devm_clk_get() + clk_prepare_enable(). And with
> devm_clk_get_enabled(), 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")
> Reviewed-by: Dongliang Mu <[email protected]>
> Signed-off-by: Junyan Ye <[email protected]>
> ---
> v2 -> v3: Rebase to v6.4-rc1 and modify commit message.
> 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 */
Hi,
completely unrelated to your patch, but this comments state "optional".
The code below seems to make both clocks mandatory.
Moreover, a few lines later, we have:
if (!IS_ERR(p->bus_clk)) {
which seems to say that bus_clk is optional.
This was introduced by 2eeb02b28579.
Just a guess, but either the comment should be updated, or the code
modified.
Just my 2c,
CJ
> - 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))
On Mon, May 8, 2023 at 8:59 AM Christophe JAILLET
<[email protected]> wrote:
> completely unrelated to your patch, but this comments state "optional".
> The code below seems to make both clocks mandatory.
>
> Moreover, a few lines later, we have:
> if (!IS_ERR(p->bus_clk)) {
> which seems to say that bus_clk is optional.
>
> This was introduced by 2eeb02b28579.
>
> Just a guess, but either the comment should be updated, or the code
> modified.
It's fine to make the clocks mandatory, because all Gemini
systems provide these clocks.
But that is good to mention in the commit message as well.
Yours,
Linus Walleij
On Mon, 08 May 2023 12:36:41 +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.
>
> The clock resource is obtained by devm_clk_get(), and then
> clk_prepare_enable() makes the clock resource ready for use. After that,
> clk_disable_unprepare() should be called to release the clock resource
> when it is no longer needed. However, while doing some error handling
> in faraday_pci_probe(), clk_disable_unprepare() is not called to release
> clk and p->bus_clk before returning. These return lines are exactly 442,
> 451, 462, 478, 512, 517.
>
> [...]
Applied to pci/ftpci100, thanks!
[1/1] pci: controller: pci-ftpci100: Release the clock resources
https://git.kernel.org/pci/pci/c/c60738de85f4
Thanks,
Lorenzo