* [PATCH v3] pci: controller: pci-ftpci100: Release the clock resources
@ 2023-05-08 4:36 Junyan Ye
2023-05-08 6:12 ` Linus Walleij
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Junyan Ye @ 2023-05-08 4:36 UTC (permalink / raw)
To: Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Wei Yongjun, Linus Walleij, Andrew Murray
Cc: hust-os-kernel-patches, Junyan Ye, Dongliang Mu, linux-pci, linux-kernel
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 <dzm91@hust.edu.cn>
Signed-off-by: Junyan Ye <yejunyan@hust.edu.cn>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] pci: controller: pci-ftpci100: Release the clock resources
2023-05-08 4:36 [PATCH v3] pci: controller: pci-ftpci100: Release the clock resources Junyan Ye
@ 2023-05-08 6:12 ` Linus Walleij
2023-05-08 6:59 ` Christophe JAILLET
2023-05-29 8:24 ` Lorenzo Pieralisi
2 siblings, 0 replies; 5+ messages in thread
From: Linus Walleij @ 2023-05-08 6:12 UTC (permalink / raw)
To: Junyan Ye
Cc: Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Wei Yongjun, Andrew Murray,
hust-os-kernel-patches, Dongliang Mu, linux-pci, linux-kernel
On Mon, May 8, 2023 at 6:41 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 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 <dzm91@hust.edu.cn>
> Signed-off-by: Junyan Ye <yejunyan@hust.edu.cn>
> ---
> v2 -> v3: Rebase to v6.4-rc1 and modify commit message.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
It's a fine fix since it saves lots of code.
I would merge it as a non-urgent fix.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] pci: controller: pci-ftpci100: Release the clock resources
2023-05-08 4:36 [PATCH v3] pci: controller: pci-ftpci100: Release the clock resources Junyan Ye
2023-05-08 6:12 ` Linus Walleij
@ 2023-05-08 6:59 ` Christophe JAILLET
2023-05-08 12:38 ` Linus Walleij
2023-05-29 8:24 ` Lorenzo Pieralisi
2 siblings, 1 reply; 5+ messages in thread
From: Christophe JAILLET @ 2023-05-08 6:59 UTC (permalink / raw)
To: Junyan Ye, Lorenzo Pieralisi, Krzysztof Wilczyński,
Rob Herring, Bjorn Helgaas, Wei Yongjun, Linus Walleij,
Andrew Murray
Cc: hust-os-kernel-patches, Dongliang Mu, linux-pci, linux-kernel
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 <dzm91@hust.edu.cn>
> Signed-off-by: Junyan Ye <yejunyan@hust.edu.cn>
> ---
> 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))
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] pci: controller: pci-ftpci100: Release the clock resources
2023-05-08 6:59 ` Christophe JAILLET
@ 2023-05-08 12:38 ` Linus Walleij
0 siblings, 0 replies; 5+ messages in thread
From: Linus Walleij @ 2023-05-08 12:38 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Junyan Ye, Lorenzo Pieralisi, Krzysztof Wilczyński,
Rob Herring, Bjorn Helgaas, Wei Yongjun, Andrew Murray,
hust-os-kernel-patches, Dongliang Mu, linux-pci, linux-kernel
On Mon, May 8, 2023 at 8:59 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] pci: controller: pci-ftpci100: Release the clock resources
2023-05-08 4:36 [PATCH v3] pci: controller: pci-ftpci100: Release the clock resources Junyan Ye
2023-05-08 6:12 ` Linus Walleij
2023-05-08 6:59 ` Christophe JAILLET
@ 2023-05-29 8:24 ` Lorenzo Pieralisi
2 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Pieralisi @ 2023-05-29 8:24 UTC (permalink / raw)
To: Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Wei Yongjun, Linus Walleij, Andrew Murray, Junyan Ye
Cc: Lorenzo Pieralisi, hust-os-kernel-patches, Dongliang Mu,
linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-05-29 8:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-08 4:36 [PATCH v3] pci: controller: pci-ftpci100: Release the clock resources Junyan Ye
2023-05-08 6:12 ` Linus Walleij
2023-05-08 6:59 ` Christophe JAILLET
2023-05-08 12:38 ` Linus Walleij
2023-05-29 8:24 ` Lorenzo Pieralisi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).