| Message ID | 20180521131123.2009-4-marek.vasut+renesas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, May 21, 2018 at 03:11:23PM +0200, Marek Vasut wrote: > If the rcar_pcie_enable() fails and MSIs are enabled, the setup done in > rcar_pcie_enable_msi() is never undone. Add a function to tear down the > MSI setup by disabling the MSI handling in the PCIe block, deallocating > the pages requested for the MSIs and zapping the IRQ mapping. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > --- > drivers/pci/host/pcie-rcar.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Hi Marek, On Mon, May 21, 2018 at 3:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > If the rcar_pcie_enable() fails and MSIs are enabled, the setup done in > rcar_pcie_enable_msi() is never undone. Add a function to tear down the > MSI setup by disabling the MSI handling in the PCIe block, deallocating > the pages requested for the MSIs and zapping the IRQ mapping. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -900,6 +900,28 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) > return err; > } > > +static void rcar_pcie_teardown_msi(struct rcar_pcie *pcie) > +{ > + struct rcar_msi *msi = &pcie->msi; > + int irq, i; > + > + /* Disable all MSI interrupts */ > + rcar_pci_write_reg(pcie, 0, PCIEMSIIER); > + > + /* Disable address decoding of the MSI interrupt, MSIFE */ > + rcar_pci_write_reg(pcie, 0, PCIEMSIALR); > + > + free_pages(msi->pages, 0); > + > + for (i = 0; i < INT_PCI_MSI_NR; i++) { > + irq = irq_find_mapping(msi->domain, i); > + if (irq > 0) > + irq_dispose_mapping(irq); > + } Note that similar calls to irq_dispose_mapping() should be added to the failure path of rcar_pcie_enable_msi(), too. > + > + irq_domain_remove(msi->domain); > +} Gr{oetje,eeting}s, Geert
On 05/22/2018 08:36 PM, Geert Uytterhoeven wrote: > Hi Marek, > > On Mon, May 21, 2018 at 3:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> If the rcar_pcie_enable() fails and MSIs are enabled, the setup done in >> rcar_pcie_enable_msi() is never undone. Add a function to tear down the >> MSI setup by disabling the MSI handling in the PCIe block, deallocating >> the pages requested for the MSIs and zapping the IRQ mapping. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > >> --- a/drivers/pci/host/pcie-rcar.c >> +++ b/drivers/pci/host/pcie-rcar.c >> @@ -900,6 +900,28 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) >> return err; >> } >> >> +static void rcar_pcie_teardown_msi(struct rcar_pcie *pcie) >> +{ >> + struct rcar_msi *msi = &pcie->msi; >> + int irq, i; >> + >> + /* Disable all MSI interrupts */ >> + rcar_pci_write_reg(pcie, 0, PCIEMSIIER); >> + >> + /* Disable address decoding of the MSI interrupt, MSIFE */ >> + rcar_pci_write_reg(pcie, 0, PCIEMSIALR); >> + >> + free_pages(msi->pages, 0); >> + >> + for (i = 0; i < INT_PCI_MSI_NR; i++) { >> + irq = irq_find_mapping(msi->domain, i); >> + if (irq > 0) >> + irq_dispose_mapping(irq); >> + } > > Note that similar calls to irq_dispose_mapping() should be added to the > failure path of rcar_pcie_enable_msi(), too. There are no failures happening in rcar_pcie_enable_msi() after the mapping is created, just memory writes, so no. Did I miss something there ?
Hi Marek, On Tue, May 22, 2018 at 11:53 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > On 05/22/2018 08:36 PM, Geert Uytterhoeven wrote: >> On Mon, May 21, 2018 at 3:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >>> --- a/drivers/pci/host/pcie-rcar.c >>> +++ b/drivers/pci/host/pcie-rcar.c >>> @@ -900,6 +900,28 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) >>> return err; >>> } >>> >>> +static void rcar_pcie_teardown_msi(struct rcar_pcie *pcie) >>> +{ >>> + struct rcar_msi *msi = &pcie->msi; >>> + int irq, i; >>> + >>> + /* Disable all MSI interrupts */ >>> + rcar_pci_write_reg(pcie, 0, PCIEMSIIER); >>> + >>> + /* Disable address decoding of the MSI interrupt, MSIFE */ >>> + rcar_pci_write_reg(pcie, 0, PCIEMSIALR); >>> + >>> + free_pages(msi->pages, 0); >>> + >>> + for (i = 0; i < INT_PCI_MSI_NR; i++) { >>> + irq = irq_find_mapping(msi->domain, i); >>> + if (irq > 0) >>> + irq_dispose_mapping(irq); >>> + } >> >> Note that similar calls to irq_dispose_mapping() should be added to the >> failure path of rcar_pcie_enable_msi(), too. > > There are no failures happening in rcar_pcie_enable_msi() after the > mapping is created, just memory writes, so no. Did I miss something there ? In my copy, there are two calls to devm_request_irq(). If they fail, the IRQ domain is removed, but the mappings are never disposed of. Gr{oetje,eeting}s, Geert
On 05/23/2018 08:18 AM, Geert Uytterhoeven wrote: > Hi Marek, > > On Tue, May 22, 2018 at 11:53 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> On 05/22/2018 08:36 PM, Geert Uytterhoeven wrote: >>> On Mon, May 21, 2018 at 3:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >>>> --- a/drivers/pci/host/pcie-rcar.c >>>> +++ b/drivers/pci/host/pcie-rcar.c >>>> @@ -900,6 +900,28 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) >>>> return err; >>>> } >>>> >>>> +static void rcar_pcie_teardown_msi(struct rcar_pcie *pcie) >>>> +{ >>>> + struct rcar_msi *msi = &pcie->msi; >>>> + int irq, i; >>>> + >>>> + /* Disable all MSI interrupts */ >>>> + rcar_pci_write_reg(pcie, 0, PCIEMSIIER); >>>> + >>>> + /* Disable address decoding of the MSI interrupt, MSIFE */ >>>> + rcar_pci_write_reg(pcie, 0, PCIEMSIALR); >>>> + >>>> + free_pages(msi->pages, 0); >>>> + >>>> + for (i = 0; i < INT_PCI_MSI_NR; i++) { >>>> + irq = irq_find_mapping(msi->domain, i); >>>> + if (irq > 0) >>>> + irq_dispose_mapping(irq); >>>> + } >>> >>> Note that similar calls to irq_dispose_mapping() should be added to the >>> failure path of rcar_pcie_enable_msi(), too. >> >> There are no failures happening in rcar_pcie_enable_msi() after the >> mapping is created, just memory writes, so no. Did I miss something there ? > > In my copy, there are two calls to devm_request_irq(). If they fail, the > IRQ domain is removed, but the mappings are never disposed of. Ah, true, I'll pull out a bit of the rcar_pcie_teardown_msi and call it in the failpath to remove the mapping. Thanks
diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index 3cc84f7e05f7..5c365f743df5 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -900,6 +900,28 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie) return err; } +static void rcar_pcie_teardown_msi(struct rcar_pcie *pcie) +{ + struct rcar_msi *msi = &pcie->msi; + int irq, i; + + /* Disable all MSI interrupts */ + rcar_pci_write_reg(pcie, 0, PCIEMSIIER); + + /* Disable address decoding of the MSI interrupt, MSIFE */ + rcar_pci_write_reg(pcie, 0, PCIEMSIALR); + + free_pages(msi->pages, 0); + + for (i = 0; i < INT_PCI_MSI_NR; i++) { + irq = irq_find_mapping(msi->domain, i); + if (irq > 0) + irq_dispose_mapping(irq); + } + + irq_domain_remove(msi->domain); +} + static int rcar_pcie_get_resources(struct rcar_pcie *pcie) { struct device *dev = pcie->dev; @@ -1156,10 +1178,14 @@ static int rcar_pcie_probe(struct platform_device *pdev) err = rcar_pcie_enable(pcie); if (err) - goto err_clk_disable; + goto err_msi_teardown; return 0; +err_msi_teardown: + if (IS_ENABLED(CONFIG_PCI_MSI)) + rcar_pcie_teardown_msi(pcie); + err_clk_disable: clk_disable_unprepare(pcie->bus_clk);
If the rcar_pcie_enable() fails and MSIs are enabled, the setup done in rcar_pcie_enable_msi() is never undone. Add a function to tear down the MSI setup by disabling the MSI handling in the PCIe block, deallocating the pages requested for the MSIs and zapping the IRQ mapping. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Phil Edworthy <phil.edworthy@renesas.com> Cc: Simon Horman <horms+renesas@verge.net.au> Cc: Wolfram Sang <wsa@the-dreams.de> Cc: linux-renesas-soc@vger.kernel.org --- drivers/pci/host/pcie-rcar.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)