[4/4] PCI: rcar: Teardown MSI setup if rcar_pcie_enable() fails
diff mbox series

Message ID 20180521131123.2009-4-marek.vasut+renesas@gmail.com
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • [1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()
Related show

Commit Message

Marek Vasut May 21, 2018, 1:11 p.m. UTC
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(-)

Comments

Simon Horman May 22, 2018, 9:16 a.m. UTC | #1
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>
Geert Uytterhoeven May 22, 2018, 6:36 p.m. UTC | #2
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
Marek Vasut May 22, 2018, 9:53 p.m. UTC | #3
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 ?
Geert Uytterhoeven May 23, 2018, 6:18 a.m. UTC | #4
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
Marek Vasut May 23, 2018, 10:38 a.m. UTC | #5
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

Patch
diff mbox series

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