| Message ID | 20201103073338.144465-1-miaoqinglang@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PCI: v3: fix missing clk_disable_unprepare() on error in v3_pci_probe | expand |
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 >
在 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 >> > . >
在 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 >> > . >
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) */
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(-)