linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] PCI: v3: fix missing clk_disable_unprepare() on error in v3_pci_probe
@ 2020-11-03  7:33 Qinglang Miao
  2020-11-03 14:16 ` Rob Herring
  0 siblings, 1 reply; 4+ messages in thread
From: Qinglang Miao @ 2020-11-03  7:33 UTC (permalink / raw)
  To: Linus Walleij, Lorenzo Pieralisi, Rob Herring, Bjorn Helgaas
  Cc: linux-pci, linux-kernel, Qinglang Miao

Fix the missing clk_disable_unprepare() before return
from v3_pci_probe() in the error handling case.

Moving the clock enable later to avoid some fixes.

Fixes: 6e0832fa432e (" PCI: Collect all native drivers under drivers/pci/controller/")
Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com>
---
 drivers/pci/controller/pci-v3-semi.c | 40 ++++++++++++++++------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/pci-v3-semi.c b/drivers/pci/controller/pci-v3-semi.c
index 154a53986..90520555b 100644
--- a/drivers/pci/controller/pci-v3-semi.c
+++ b/drivers/pci/controller/pci-v3-semi.c
@@ -725,18 +725,6 @@ static int v3_pci_probe(struct platform_device *pdev)
 	host->sysdata = v3;
 	v3->dev = dev;
 
-	/* Get and enable host clock */
-	clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(clk)) {
-		dev_err(dev, "clock not found\n");
-		return PTR_ERR(clk);
-	}
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "unable to enable clock\n");
-		return ret;
-	}
-
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	v3->base = devm_ioremap_resource(dev, regs);
 	if (IS_ERR(v3->base))
@@ -761,17 +749,31 @@ static int v3_pci_probe(struct platform_device *pdev)
 	if (IS_ERR(v3->config_base))
 		return PTR_ERR(v3->config_base);
 
+	/* Get and enable host clock */
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(dev, "clock not found\n");
+		return PTR_ERR(clk);
+	}
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		dev_err(dev, "unable to enable clock\n");
+		return ret;
+	}
+
 	/* Get and request error IRQ resource */
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
+	if (irq < 0) {
+		clk_disable_unprepare(clk);
 		return irq;
-
+	}
 	ret = devm_request_irq(dev, irq, v3_irq, 0,
 			"PCIv3 error", v3);
 	if (ret < 0) {
 		dev_err(dev,
 			"unable to request PCIv3 error IRQ %d (%d)\n",
 			irq, ret);
+		clk_disable_unprepare(clk);
 		return ret;
 	}
 
@@ -814,13 +816,15 @@ static int v3_pci_probe(struct platform_device *pdev)
 		ret = v3_pci_setup_resource(v3, host, win);
 		if (ret) {
 			dev_err(dev, "error setting up resources\n");
+			clk_disable_unprepare(clk);
 			return ret;
 		}
 	}
 	ret = v3_pci_parse_map_dma_ranges(v3, np);
-	if (ret)
+	if (ret) {
+		clk_disable_unprepare(clk);
 		return ret;
-
+	}
 	/*
 	 * Disable PCI to host IO cycles, enable I/O buffers @3.3V,
 	 * set AD_LOW0 to 1 if one of the LB_MAP registers choose
@@ -862,8 +866,10 @@ static int v3_pci_probe(struct platform_device *pdev)
 	/* Special Integrator initialization */
 	if (of_device_is_compatible(np, "arm,integrator-ap-pci")) {
 		ret = v3_integrator_init(v3);
-		if (ret)
+		if (ret) {
+			clk_disable_unprepare(clk);
 			return ret;
+		}
 	}
 
 	/* Post-init: enable PCI memory and invalidate (master already on) */
-- 
2.23.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] PCI: v3: fix missing clk_disable_unprepare() on error in v3_pci_probe
  2020-11-03  7:33 [PATCH v2] PCI: v3: fix missing clk_disable_unprepare() on error in v3_pci_probe Qinglang Miao
@ 2020-11-03 14:16 ` Rob Herring
  2020-11-06  3:08   ` Qinglang Miao
  2020-11-06  3:39   ` Qinglang Miao
  0 siblings, 2 replies; 4+ messages in thread
From: Rob Herring @ 2020-11-03 14:16 UTC (permalink / raw)
  To: Qinglang Miao
  Cc: Linus Walleij, Lorenzo Pieralisi, Bjorn Helgaas, PCI, linux-kernel

On Tue, Nov 3, 2020 at 1:28 AM Qinglang Miao <miaoqinglang@huawei.com> wrote:
>
> Fix the missing clk_disable_unprepare() before return
> from v3_pci_probe() in the error handling case.
>
> Moving the clock enable later to avoid some fixes.
>
> Fixes: 6e0832fa432e (" PCI: Collect all native drivers under drivers/pci/controller/")

I don't think this commit caused the problem.

> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com>
> ---
>  drivers/pci/controller/pci-v3-semi.c | 40 ++++++++++++++++------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-v3-semi.c b/drivers/pci/controller/pci-v3-semi.c
> index 154a53986..90520555b 100644
> --- a/drivers/pci/controller/pci-v3-semi.c
> +++ b/drivers/pci/controller/pci-v3-semi.c
> @@ -725,18 +725,6 @@ static int v3_pci_probe(struct platform_device *pdev)
>         host->sysdata = v3;
>         v3->dev = dev;
>
> -       /* Get and enable host clock */
> -       clk = devm_clk_get(dev, NULL);
> -       if (IS_ERR(clk)) {
> -               dev_err(dev, "clock not found\n");
> -               return PTR_ERR(clk);
> -       }
> -       ret = clk_prepare_enable(clk);
> -       if (ret) {
> -               dev_err(dev, "unable to enable clock\n");
> -               return ret;
> -       }
> -
>         regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         v3->base = devm_ioremap_resource(dev, regs);
>         if (IS_ERR(v3->base))
> @@ -761,17 +749,31 @@ static int v3_pci_probe(struct platform_device *pdev)
>         if (IS_ERR(v3->config_base))
>                 return PTR_ERR(v3->config_base);
>
> +       /* Get and enable host clock */
> +       clk = devm_clk_get(dev, NULL);
> +       if (IS_ERR(clk)) {
> +               dev_err(dev, "clock not found\n");
> +               return PTR_ERR(clk);
> +       }
> +       ret = clk_prepare_enable(clk);
> +       if (ret) {
> +               dev_err(dev, "unable to enable clock\n");
> +               return ret;
> +       }
> +
>         /* Get and request error IRQ resource */
>         irq = platform_get_irq(pdev, 0);
> -       if (irq < 0)
> +       if (irq < 0) {
> +               clk_disable_unprepare(clk);
>                 return irq;
> -
> +       }
>         ret = devm_request_irq(dev, irq, v3_irq, 0,
>                         "PCIv3 error", v3);
>         if (ret < 0) {
>                 dev_err(dev,
>                         "unable to request PCIv3 error IRQ %d (%d)\n",
>                         irq, ret);
> +               clk_disable_unprepare(clk);
>                 return ret;
>         }
>
> @@ -814,13 +816,15 @@ static int v3_pci_probe(struct platform_device *pdev)
>                 ret = v3_pci_setup_resource(v3, host, win);
>                 if (ret) {
>                         dev_err(dev, "error setting up resources\n");
> +                       clk_disable_unprepare(clk);
>                         return ret;
>                 }
>         }
>         ret = v3_pci_parse_map_dma_ranges(v3, np);
> -       if (ret)
> +       if (ret) {
> +               clk_disable_unprepare(clk);
>                 return ret;
> -
> +       }
>         /*
>          * Disable PCI to host IO cycles, enable I/O buffers @3.3V,
>          * set AD_LOW0 to 1 if one of the LB_MAP registers choose
> @@ -862,8 +866,10 @@ static int v3_pci_probe(struct platform_device *pdev)
>         /* Special Integrator initialization */
>         if (of_device_is_compatible(np, "arm,integrator-ap-pci")) {
>                 ret = v3_integrator_init(v3);
> -               if (ret)
> +               if (ret) {
> +                       clk_disable_unprepare(clk);

You should make all these a goto and just have one clk_disable_unprepare() call.

You are still missing error handling after pci_host_probe().

>                         return ret;
> +               }
>         }
>
>         /* Post-init: enable PCI memory and invalidate (master already on) */
> --
> 2.23.0
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] PCI: v3: fix missing clk_disable_unprepare() on error in v3_pci_probe
  2020-11-03 14:16 ` Rob Herring
@ 2020-11-06  3:08   ` Qinglang Miao
  2020-11-06  3:39   ` Qinglang Miao
  1 sibling, 0 replies; 4+ messages in thread
From: Qinglang Miao @ 2020-11-06  3:08 UTC (permalink / raw)
  To: Rob Herring
  Cc: Linus Walleij, Lorenzo Pieralisi, Bjorn Helgaas, PCI, linux-kernel



在 2020/11/3 22:16, Rob Herring 写道:
> On Tue, Nov 3, 2020 at 1:28 AM Qinglang Miao <miaoqinglang@huawei.com> wrote:
>>
>> Fix the missing clk_disable_unprepare() before return
>> from v3_pci_probe() in the error handling case.
>>
>> Moving the clock enable later to avoid some fixes.
>>
>> Fixes: 6e0832fa432e (" PCI: Collect all native drivers under drivers/pci/controller/")
> 
> I don't think this commit caused the problem.
> 
>> Suggested-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com>
>> ---
>>   drivers/pci/controller/pci-v3-semi.c | 40 ++++++++++++++++------------
>>   1 file changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-v3-semi.c b/drivers/pci/controller/pci-v3-semi.c
>> index 154a53986..90520555b 100644
>> --- a/drivers/pci/controller/pci-v3-semi.c
>> +++ b/drivers/pci/controller/pci-v3-semi.c
>> @@ -725,18 +725,6 @@ static int v3_pci_probe(struct platform_device *pdev)
>>          host->sysdata = v3;
>>          v3->dev = dev;
>>
>> -       /* Get and enable host clock */
>> -       clk = devm_clk_get(dev, NULL);
>> -       if (IS_ERR(clk)) {
>> -               dev_err(dev, "clock not found\n");
>> -               return PTR_ERR(clk);
>> -       }
>> -       ret = clk_prepare_enable(clk);
>> -       if (ret) {
>> -               dev_err(dev, "unable to enable clock\n");
>> -               return ret;
>> -       }
>> -
>>          regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>          v3->base = devm_ioremap_resource(dev, regs);
>>          if (IS_ERR(v3->base))
>> @@ -761,17 +749,31 @@ static int v3_pci_probe(struct platform_device *pdev)
>>          if (IS_ERR(v3->config_base))
>>                  return PTR_ERR(v3->config_base);
>>
>> +       /* Get and enable host clock */
>> +       clk = devm_clk_get(dev, NULL);
>> +       if (IS_ERR(clk)) {
>> +               dev_err(dev, "clock not found\n");
>> +               return PTR_ERR(clk);
>> +       }
>> +       ret = clk_prepare_enable(clk);
>> +       if (ret) {
>> +               dev_err(dev, "unable to enable clock\n");
>> +               return ret;
>> +       }
>> +
>>          /* Get and request error IRQ resource */
>>          irq = platform_get_irq(pdev, 0);
>> -       if (irq < 0)
>> +       if (irq < 0) {
>> +               clk_disable_unprepare(clk);
>>                  return irq;
>> -
>> +       }
>>          ret = devm_request_irq(dev, irq, v3_irq, 0,
>>                          "PCIv3 error", v3);
>>          if (ret < 0) {
>>                  dev_err(dev,
>>                          "unable to request PCIv3 error IRQ %d (%d)\n",
>>                          irq, ret);
>> +               clk_disable_unprepare(clk);
>>                  return ret;
>>          }
>>
>> @@ -814,13 +816,15 @@ static int v3_pci_probe(struct platform_device *pdev)
>>                  ret = v3_pci_setup_resource(v3, host, win);
>>                  if (ret) {
>>                          dev_err(dev, "error setting up resources\n");
>> +                       clk_disable_unprepare(clk);
>>                          return ret;
>>                  }
>>          }
>>          ret = v3_pci_parse_map_dma_ranges(v3, np);
>> -       if (ret)
>> +       if (ret) {
>> +               clk_disable_unprepare(clk);
>>                  return ret;
>> -
>> +       }
>>          /*
>>           * Disable PCI to host IO cycles, enable I/O buffers @3.3V,
>>           * set AD_LOW0 to 1 if one of the LB_MAP registers choose
>> @@ -862,8 +866,10 @@ static int v3_pci_probe(struct platform_device *pdev)
>>          /* Special Integrator initialization */
>>          if (of_device_is_compatible(np, "arm,integrator-ap-pci")) {
>>                  ret = v3_integrator_init(v3);
>> -               if (ret)
>> +               if (ret) {
>> +                       clk_disable_unprepare(clk);
> 
> You should make all these a goto and just have one clk_disable_unprepare() call.
> 
> You are still missing error handling after pci_host_probe().
Hi Rob,

I've sent a v3 patch towards this bug.

Thanks a lot for your advice.
> 
>>                          return ret;
>> +               }
>>          }
>>
>>          /* Post-init: enable PCI memory and invalidate (master already on) */
>> --
>> 2.23.0
>>
> .
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] PCI: v3: fix missing clk_disable_unprepare() on error in v3_pci_probe
  2020-11-03 14:16 ` Rob Herring
  2020-11-06  3:08   ` Qinglang Miao
@ 2020-11-06  3:39   ` Qinglang Miao
  1 sibling, 0 replies; 4+ messages in thread
From: Qinglang Miao @ 2020-11-06  3:39 UTC (permalink / raw)
  To: Rob Herring
  Cc: Linus Walleij, Lorenzo Pieralisi, Bjorn Helgaas, PCI, linux-kernel



在 2020/11/3 22:16, Rob Herring 写道:
> On Tue, Nov 3, 2020 at 1:28 AM Qinglang Miao <miaoqinglang@huawei.com> wrote:
>>
>> Fix the missing clk_disable_unprepare() before return
>> from v3_pci_probe() in the error handling case.
>>
>> Moving the clock enable later to avoid some fixes.
>>
>> Fixes: 6e0832fa432e (" PCI: Collect all native drivers under drivers/pci/controller/")
> 
> I don't think this commit caused the problem.
> 
>> Suggested-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com>
>> ---
>>   drivers/pci/controller/pci-v3-semi.c | 40 ++++++++++++++++------------
>>   1 file changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-v3-semi.c b/drivers/pci/controller/pci-v3-semi.c
>> index 154a53986..90520555b 100644
>> --- a/drivers/pci/controller/pci-v3-semi.c
>> +++ b/drivers/pci/controller/pci-v3-semi.c
>> @@ -725,18 +725,6 @@ static int v3_pci_probe(struct platform_device *pdev)
>>          host->sysdata = v3;
>>          v3->dev = dev;
>>
>> -       /* Get and enable host clock */
>> -       clk = devm_clk_get(dev, NULL);
>> -       if (IS_ERR(clk)) {
>> -               dev_err(dev, "clock not found\n");
>> -               return PTR_ERR(clk);
>> -       }
>> -       ret = clk_prepare_enable(clk);
>> -       if (ret) {
>> -               dev_err(dev, "unable to enable clock\n");
>> -               return ret;
>> -       }
>> -
>>          regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>          v3->base = devm_ioremap_resource(dev, regs);
>>          if (IS_ERR(v3->base))
>> @@ -761,17 +749,31 @@ static int v3_pci_probe(struct platform_device *pdev)
>>          if (IS_ERR(v3->config_base))
>>                  return PTR_ERR(v3->config_base);
>>
>> +       /* Get and enable host clock */
>> +       clk = devm_clk_get(dev, NULL);
>> +       if (IS_ERR(clk)) {
>> +               dev_err(dev, "clock not found\n");
>> +               return PTR_ERR(clk);
>> +       }
>> +       ret = clk_prepare_enable(clk);
>> +       if (ret) {
>> +               dev_err(dev, "unable to enable clock\n");
>> +               return ret;
>> +       }
>> +
>>          /* Get and request error IRQ resource */
>>          irq = platform_get_irq(pdev, 0);
>> -       if (irq < 0)
>> +       if (irq < 0) {
>> +               clk_disable_unprepare(clk);
>>                  return irq;
>> -
>> +       }
>>          ret = devm_request_irq(dev, irq, v3_irq, 0,
>>                          "PCIv3 error", v3);
>>          if (ret < 0) {
>>                  dev_err(dev,
>>                          "unable to request PCIv3 error IRQ %d (%d)\n",
>>                          irq, ret);
>> +               clk_disable_unprepare(clk);
>>                  return ret;
>>          }
>>
>> @@ -814,13 +816,15 @@ static int v3_pci_probe(struct platform_device *pdev)
>>                  ret = v3_pci_setup_resource(v3, host, win);
>>                  if (ret) {
>>                          dev_err(dev, "error setting up resources\n");
>> +                       clk_disable_unprepare(clk);
>>                          return ret;
>>                  }
>>          }
>>          ret = v3_pci_parse_map_dma_ranges(v3, np);
>> -       if (ret)
>> +       if (ret) {
>> +               clk_disable_unprepare(clk);
>>                  return ret;
>> -
>> +       }
>>          /*
>>           * Disable PCI to host IO cycles, enable I/O buffers @3.3V,
>>           * set AD_LOW0 to 1 if one of the LB_MAP registers choose
>> @@ -862,8 +866,10 @@ static int v3_pci_probe(struct platform_device *pdev)
>>          /* Special Integrator initialization */
>>          if (of_device_is_compatible(np, "arm,integrator-ap-pci")) {
>>                  ret = v3_integrator_init(v3);
>> -               if (ret)
>> +               if (ret) {
>> +                       clk_disable_unprepare(clk);
> 
> You should make all these a goto and just have one clk_disable_unprepare() call.
> 
> You are still missing error handling after pci_host_probe().

I made a mistake on author name on v3 so resent a v4 on this one.

Sorry about that.
> 
>>                          return ret;
>> +               }
>>          }
>>
>>          /* Post-init: enable PCI memory and invalidate (master already on) */
>> --
>> 2.23.0
>>
> .
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-11-06  3:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03  7:33 [PATCH v2] PCI: v3: fix missing clk_disable_unprepare() on error in v3_pci_probe Qinglang Miao
2020-11-03 14:16 ` Rob Herring
2020-11-06  3:08   ` Qinglang Miao
2020-11-06  3:39   ` Qinglang Miao

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).