[PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[Marek Vasut]

The function name is just too confusing, rename it, no functional change.
Rename the function to rcar_pcie_alloc_and_parse_pci_resource_list() as
it's matching failpath function is pci_free_resource_list() so the names
align much better and the new name also describes what the function does
much better.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index e403c5206b24..dbc80e457f95 100644
--- a/drivers/pci/host/pcie-rcar.c
+++ b/drivers/pci/host/pcie-rcar.c
@@ -1051,7 +1051,7 @@ static const struct of_device_id rcar_pcie_of_match[] = {
 	{},
 };
 
-static int rcar_pcie_parse_request_of_pci_ranges(struct rcar_pcie *pci)
+static int rcar_pcie_alloc_and_parse_pci_resource_list(struct rcar_pcie *pci)
 {
 	int err;
 	struct device *dev = pci->dev;
@@ -1108,7 +1108,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
 
 	INIT_LIST_HEAD(&pcie->resources);
 
-	err = rcar_pcie_parse_request_of_pci_ranges(pcie);
+	err = rcar_pcie_alloc_and_parse_pci_resource_list(pcie);
 	if (err)
 		goto err_free_bridge;
 
-- 
2.16.2

[PATCH 2/4] PCI: rcar: Pull bus clock enable/disable from rcar_pcie_get_resources()

[Marek Vasut]

The rcar_pcie_get_resources() is another misnomer with a side effect.
The function does not only get resources, but also enables/disables bus
clock. This is forgotten in the probe() function though and if anything
in probe() fails after rcar_pcie_get_resources() is called, the bus
clock are never disabled.

This patch pulls the clock handling out of the rcar_pcie_get_resources()
and enables clock after all the resources were requested. Moreover, this
patch also always disables the clock in case of failure.

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 | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index dbc80e457f95..eac4b71d9c60 100644
--- a/drivers/pci/host/pcie-rcar.c
+++ b/drivers/pci/host/pcie-rcar.c
@@ -919,32 +919,22 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie)
 		dev_err(dev, "cannot get pcie bus clock\n");
 		return PTR_ERR(pcie->bus_clk);
 	}
-	err = clk_prepare_enable(pcie->bus_clk);
-	if (err)
-		return err;
 
 	i = irq_of_parse_and_map(dev->of_node, 0);
 	if (!i) {
 		dev_err(dev, "cannot get platform resources for msi interrupt\n");
-		err = -ENOENT;
-		goto err_map_reg;
+		return -ENOENT;
 	}
 	pcie->msi.irq1 = i;
 
 	i = irq_of_parse_and_map(dev->of_node, 1);
 	if (!i) {
 		dev_err(dev, "cannot get platform resources for msi interrupt\n");
-		err = -ENOENT;
-		goto err_map_reg;
+		return -ENOENT;
 	}
 	pcie->msi.irq2 = i;
 
 	return 0;
-
-err_map_reg:
-	clk_disable_unprepare(pcie->bus_clk);
-
-	return err;
 }
 
 static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
@@ -1125,9 +1115,15 @@ static int rcar_pcie_probe(struct platform_device *pdev)
 		goto err_pm_put;
 	}
 
+	err = clk_prepare_enable(pcie->bus_clk);
+	if (err) {
+		dev_err(dev, "failed to enable bus clock: %d\n", err);
+		goto err_pm_put;
+	}
+
 	err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node);
 	if (err)
-		goto err_pm_put;
+		goto err_clk_disable;
 
 	/* Failure to get a link might just be that no cards are inserted */
 	hw_init_fn = of_device_get_match_data(dev);
@@ -1135,7 +1131,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
 	if (err) {
 		dev_info(dev, "PCIe link down\n");
 		err = -ENODEV;
-		goto err_pm_put;
+		goto err_clk_disable;
 	}
 
 	data = rcar_pci_read_reg(pcie, MACSR);
@@ -1147,16 +1143,19 @@ static int rcar_pcie_probe(struct platform_device *pdev)
 			dev_err(dev,
 				"failed to enable MSI support: %d\n",
 				err);
-			goto err_pm_put;
+			goto err_clk_disable;
 		}
 	}
 
 	err = rcar_pcie_enable(pcie);
 	if (err)
-		goto err_pm_put;
+		goto err_clk_disable;
 
 	return 0;
 
+err_clk_disable:
+	clk_disable_unprepare(pcie->bus_clk);
+
 err_pm_put:
 	pm_runtime_put(dev);
 
-- 
2.16.2

[PATCH 3/4] PCI: rcar: Add missing irq_dispose_mapping() into failpath

[Marek Vasut]

The rcar_pcie_get_resources() is another misnomer with a side effect.
The function does not only get resources, but also maps MSI IRQs via
irq_of_parse_and_map(). In case anything fails afterward, the IRQ
mapping must be disposed through irq_dispose_mapping() which is not
done.

This patch handles irq_of_parse_and_map() failures in by disposing
of the mapping in rcar_pcie_get_resources() as well as in probe.

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 | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index eac4b71d9c60..3cc84f7e05f7 100644
--- a/drivers/pci/host/pcie-rcar.c
+++ b/drivers/pci/host/pcie-rcar.c
@@ -923,18 +923,25 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie)
 	i = irq_of_parse_and_map(dev->of_node, 0);
 	if (!i) {
 		dev_err(dev, "cannot get platform resources for msi interrupt\n");
-		return -ENOENT;
+		err = -ENOENT;
+		goto err_irq1;
 	}
 	pcie->msi.irq1 = i;
 
 	i = irq_of_parse_and_map(dev->of_node, 1);
 	if (!i) {
 		dev_err(dev, "cannot get platform resources for msi interrupt\n");
-		return -ENOENT;
+		err = -ENOENT;
+		goto err_irq2;
 	}
 	pcie->msi.irq2 = i;
 
 	return 0;
+
+err_irq2:
+	irq_dispose_mapping(pcie->msi.irq1);
+err_irq1:
+	return err;
 }
 
 static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
@@ -1118,7 +1125,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
 	err = clk_prepare_enable(pcie->bus_clk);
 	if (err) {
 		dev_err(dev, "failed to enable bus clock: %d\n", err);
-		goto err_pm_put;
+		goto err_unmap_msi_irqs;
 	}
 
 	err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node);
@@ -1156,6 +1163,10 @@ static int rcar_pcie_probe(struct platform_device *pdev)
 err_clk_disable:
 	clk_disable_unprepare(pcie->bus_clk);
 
+err_unmap_msi_irqs:
+	irq_dispose_mapping(pcie->msi.irq2);
+	irq_dispose_mapping(pcie->msi.irq1);
+
 err_pm_put:
 	pm_runtime_put(dev);
 
-- 
2.16.2

[PATCH 4/4] PCI: rcar: Teardown MSI setup if rcar_pcie_enable() fails

[Marek Vasut]

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

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

Re: [PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[Simon Horman]

On Mon, May 21, 2018 at 03:11:20PM +0200, Marek Vasut wrote:
> The function name is just too confusing, rename it, no functional change.
> Rename the function to rcar_pcie_alloc_and_parse_pci_resource_list() as
> it's matching failpath function is pci_free_resource_list() so the names
> align much better and the new name also describes what the function does
> much better.
> 
> 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

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH 2/4] PCI: rcar: Pull bus clock enable/disable from rcar_pcie_get_resources()

[Simon Horman]

On Mon, May 21, 2018 at 03:11:21PM +0200, Marek Vasut wrote:
> The rcar_pcie_get_resources() is another misnomer with a side effect.
> The function does not only get resources, but also enables/disables bus
> clock. This is forgotten in the probe() function though and if anything
> in probe() fails after rcar_pcie_get_resources() is called, the bus
> clock are never disabled.
> 
> This patch pulls the clock handling out of the rcar_pcie_get_resources()
> and enables clock after all the resources were requested. Moreover, this
> patch also always disables the clock in case of failure.
> 
> 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

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH 3/4] PCI: rcar: Add missing irq_dispose_mapping() into failpath

[Simon Horman]

On Mon, May 21, 2018 at 03:11:22PM +0200, Marek Vasut wrote:
> The rcar_pcie_get_resources() is another misnomer with a side effect.
> The function does not only get resources, but also maps MSI IRQs via
> irq_of_parse_and_map(). In case anything fails afterward, the IRQ
> mapping must be disposed through irq_dispose_mapping() which is not
> done.
> 
> This patch handles irq_of_parse_and_map() failures in by disposing
> of the mapping in rcar_pcie_get_resources() as well as in probe.
> 
> 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 | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH 4/4] PCI: rcar: Teardown MSI setup if rcar_pcie_enable() fails

[Simon Horman]

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>

Re: [PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[Geert Uytterhoeven]

On Mon, May 21, 2018 at 3:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> The function name is just too confusing, rename it, no functional change.
> Rename the function to rcar_pcie_alloc_and_parse_pci_resource_list() as
> it's matching failpath function is pci_free_resource_list() so the names
> align much better and the new name also describes what the function does
> much better.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] PCI: rcar: Pull bus clock enable/disable from rcar_pcie_get_resources()

[Geert Uytterhoeven]

Hi Marek,

On Mon, May 21, 2018 at 3:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> The rcar_pcie_get_resources() is another misnomer with a side effect.
> The function does not only get resources, but also enables/disables bus
> clock. This is forgotten in the probe() function though and if anything
> in probe() fails after rcar_pcie_get_resources() is called, the bus
> clock are never disabled.
>
> This patch pulls the clock handling out of the rcar_pcie_get_resources()
> and enables clock after all the resources were requested. Moreover, this
> patch also always disables the clock in case of failure.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Thanks for your patch!

Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] PCI: rcar: Add missing irq_dispose_mapping() into failpath

[Geert Uytterhoeven]

Hi Marek,

On Mon, May 21, 2018 at 3:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> The rcar_pcie_get_resources() is another misnomer with a side effect.
> The function does not only get resources, but also maps MSI IRQs via
> irq_of_parse_and_map(). In case anything fails afterward, the IRQ
> mapping must be disposed through irq_dispose_mapping() which is not
> done.
>
> This patch handles irq_of_parse_and_map() failures in by disposing
> of the mapping in rcar_pcie_get_resources() as well as in probe.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -923,18 +923,25 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie)
>         i = irq_of_parse_and_map(dev->of_node, 0);
>         if (!i) {
>                 dev_err(dev, "cannot get platform resources for msi interrupt\n");
> -               return -ENOENT;
> +               err = -ENOENT;
> +               goto err_irq1;

You could have kept the return here.

>         }
>         pcie->msi.irq1 = i;
>
>         i = irq_of_parse_and_map(dev->of_node, 1);
>         if (!i) {
>                 dev_err(dev, "cannot get platform resources for msi interrupt\n");
> -               return -ENOENT;
> +               err = -ENOENT;
> +               goto err_irq2;
>         }
>         pcie->msi.irq2 = i;
>
>         return 0;
> +
> +err_irq2:
> +       irq_dispose_mapping(pcie->msi.irq1);
> +err_irq1:
> +       return err;
>  }
>
>  static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] PCI: rcar: Teardown MSI setup if rcar_pcie_enable() fails

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] PCI: rcar: Add missing irq_dispose_mapping() into failpath

[Marek Vasut]

On 05/22/2018 05:18 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Mon, May 21, 2018 at 3:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> The rcar_pcie_get_resources() is another misnomer with a side effect.
>> The function does not only get resources, but also maps MSI IRQs via
>> irq_of_parse_and_map(). In case anything fails afterward, the IRQ
>> mapping must be disposed through irq_dispose_mapping() which is not
>> done.
>>
>> This patch handles irq_of_parse_and_map() failures in by disposing
>> of the mapping in rcar_pcie_get_resources() as well as in probe.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
>> --- a/drivers/pci/host/pcie-rcar.c
>> +++ b/drivers/pci/host/pcie-rcar.c
>> @@ -923,18 +923,25 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie)
>>         i = irq_of_parse_and_map(dev->of_node, 0);
>>         if (!i) {
>>                 dev_err(dev, "cannot get platform resources for msi interrupt\n");
>> -               return -ENOENT;
>> +               err = -ENOENT;
>> +               goto err_irq1;
> 
> You could have kept the return here.

I like the symmetry ;-)

[...]

-- 
Best regards,
Marek Vasut

Re: [PATCH 4/4] PCI: rcar: Teardown MSI setup if rcar_pcie_enable() fails

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut

Re: [PATCH 4/4] PCI: rcar: Teardown MSI setup if rcar_pcie_enable() fails

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] PCI: rcar: Teardown MSI setup if rcar_pcie_enable() fails

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut

Re: [PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[Lorenzo Pieralisi]

On Mon, May 21, 2018 at 03:11:20PM +0200, Marek Vasut wrote:
> The function name is just too confusing, rename it, no functional change.
> Rename the function to rcar_pcie_alloc_and_parse_pci_resource_list() as
> it's matching failpath function is pci_free_resource_list() so the names
> align much better and the new name also describes what the function does
> much better.
> 
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Can you rebase this series against my pci/rcar branch please ?

I will merge it then, thanks.

Lorenzo

> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index e403c5206b24..dbc80e457f95 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -1051,7 +1051,7 @@ static const struct of_device_id rcar_pcie_of_match[] = {
>  	{},
>  };
>  
> -static int rcar_pcie_parse_request_of_pci_ranges(struct rcar_pcie *pci)
> +static int rcar_pcie_alloc_and_parse_pci_resource_list(struct rcar_pcie *pci)
>  {
>  	int err;
>  	struct device *dev = pci->dev;
> @@ -1108,7 +1108,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>  
>  	INIT_LIST_HEAD(&pcie->resources);
>  
> -	err = rcar_pcie_parse_request_of_pci_ranges(pcie);
> +	err = rcar_pcie_alloc_and_parse_pci_resource_list(pcie);
>  	if (err)
>  		goto err_free_bridge;
>  
> -- 
> 2.16.2
> 

Re: [PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[Marek Vasut]

On 05/23/2018 06:17 PM, Lorenzo Pieralisi wrote:
> On Mon, May 21, 2018 at 03:11:20PM +0200, Marek Vasut wrote:
>> The function name is just too confusing, rename it, no functional change.
>> Rename the function to rcar_pcie_alloc_and_parse_pci_resource_list() as
>> it's matching failpath function is pci_free_resource_list() so the names
>> align much better and the new name also describes what the function does
>> much better.
>>
>> 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 | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Can you rebase this series against my pci/rcar branch please ?
> 
> I will merge it then, thanks.

Where is that tree/branch located ?

It applies fine on current next 20180517, is there a problem ?

-- 
Best regards,
Marek Vasut

Re: [PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[Bjorn Helgaas]

On Wed, May 23, 2018 at 07:05:06PM +0200, Marek Vasut wrote:
> On 05/23/2018 06:17 PM, Lorenzo Pieralisi wrote:
> > On Mon, May 21, 2018 at 03:11:20PM +0200, Marek Vasut wrote:
> >> The function name is just too confusing, rename it, no functional change.
> >> Rename the function to rcar_pcie_alloc_and_parse_pci_resource_list() as
> >> it's matching failpath function is pci_free_resource_list() so the names
> >> align much better and the new name also describes what the function does
> >> much better.
> >>
> >> 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 | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Can you rebase this series against my pci/rcar branch please ?
> > 
> > I will merge it then, thanks.
> 
> Where is that tree/branch located ?
> 
> It applies fine on current next 20180517, is there a problem ?

https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/log/?h=pci/rcar

I don't think next 20180517 includes Lorenzo's pci/rcar branch, so
there might be conflicts.  I think Stephen is on vacation until next
week, so there isn't a newer -next tree yet.

Re: [PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[Marek Vasut]

On 05/23/2018 11:56 PM, Bjorn Helgaas wrote:
> On Wed, May 23, 2018 at 07:05:06PM +0200, Marek Vasut wrote:
>> On 05/23/2018 06:17 PM, Lorenzo Pieralisi wrote:
>>> On Mon, May 21, 2018 at 03:11:20PM +0200, Marek Vasut wrote:
>>>> The function name is just too confusing, rename it, no functional change.
>>>> Rename the function to rcar_pcie_alloc_and_parse_pci_resource_list() as
>>>> it's matching failpath function is pci_free_resource_list() so the names
>>>> align much better and the new name also describes what the function does
>>>> much better.
>>>>
>>>> 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 | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> Can you rebase this series against my pci/rcar branch please ?
>>>
>>> I will merge it then, thanks.
>>
>> Where is that tree/branch located ?
>>
>> It applies fine on current next 20180517, is there a problem ?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/log/?h=pci/rcar

OK, 1/4 is dropped, the remaining patches are resubmitted. I added one
more (6/6) since the phy patches in that tree added a bug into the fail
path, so I fixed that too.

> I don't think next 20180517 includes Lorenzo's pci/rcar branch, so
> there might be conflicts.  I think Stephen is on vacation until next
> week, so there isn't a newer -next tree yet.

OK

-- 
Best regards,
Marek Vasut

Re: [PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[Lorenzo Pieralisi]

On Thu, May 24, 2018 at 09:24:27AM +0200, Marek Vasut wrote:
> On 05/23/2018 11:56 PM, Bjorn Helgaas wrote:
> > On Wed, May 23, 2018 at 07:05:06PM +0200, Marek Vasut wrote:
> >> On 05/23/2018 06:17 PM, Lorenzo Pieralisi wrote:
> >>> On Mon, May 21, 2018 at 03:11:20PM +0200, Marek Vasut wrote:
> >>>> The function name is just too confusing, rename it, no functional change.
> >>>> Rename the function to rcar_pcie_alloc_and_parse_pci_resource_list() as
> >>>> it's matching failpath function is pci_free_resource_list() so the names
> >>>> align much better and the new name also describes what the function does
> >>>> much better.
> >>>>
> >>>> 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 | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> Can you rebase this series against my pci/rcar branch please ?
> >>>
> >>> I will merge it then, thanks.
> >>
> >> Where is that tree/branch located ?
> >>
> >> It applies fine on current next 20180517, is there a problem ?
> > https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/log/?h=pci/rcar
> 
> OK, 1/4 is dropped, the remaining patches are resubmitted. I added one
> more (6/6) since the phy patches in that tree added a bug into the fail
> path, so I fixed that too.

Please CC me if you want me to merge your code, I monitor linux-pci
but that would help me, thanks.

You did not add the review tags you received to the new series (also a
cover letter - however minimal - and a version number would help),
please add them and post a v2 so that I can merge them, I can do that
for you but it is good practice for the future.

Thanks,
Lorenzo

> > I don't think next 20180517 includes Lorenzo's pci/rcar branch, so
> > there might be conflicts.  I think Stephen is on vacation until next
> > week, so there isn't a newer -next tree yet.
> 
> OK
> 
> -- 
> Best regards,
> Marek Vasut

Re: [PATCH 1/4] PCI: rcar: Rename rcar_pcie_parse_request_of_pci_ranges()

[Marek Vasut]

On 05/24/2018 03:50 PM, Lorenzo Pieralisi wrote:
> On Thu, May 24, 2018 at 09:24:27AM +0200, Marek Vasut wrote:
>> On 05/23/2018 11:56 PM, Bjorn Helgaas wrote:
>>> On Wed, May 23, 2018 at 07:05:06PM +0200, Marek Vasut wrote:
>>>> On 05/23/2018 06:17 PM, Lorenzo Pieralisi wrote:
>>>>> On Mon, May 21, 2018 at 03:11:20PM +0200, Marek Vasut wrote:
>>>>>> The function name is just too confusing, rename it, no functional change.
>>>>>> Rename the function to rcar_pcie_alloc_and_parse_pci_resource_list() as
>>>>>> it's matching failpath function is pci_free_resource_list() so the names
>>>>>> align much better and the new name also describes what the function does
>>>>>> much better.
>>>>>>
>>>>>> 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 | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> Can you rebase this series against my pci/rcar branch please ?
>>>>>
>>>>> I will merge it then, thanks.
>>>>
>>>> Where is that tree/branch located ?
>>>>
>>>> It applies fine on current next 20180517, is there a problem ?
>>> https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/log/?h=pci/rcar
>>
>> OK, 1/4 is dropped, the remaining patches are resubmitted. I added one
>> more (6/6) since the phy patches in that tree added a bug into the fail
>> path, so I fixed that too.
> 
> Please CC me if you want me to merge your code, I monitor linux-pci
> but that would help me, thanks.

Right

> You did not add the review tags you received to the new series

Because I had to rebase the series on top of your tree which contained
patches from two months ago which I was not told about which triggered
changes in this series, so of course I had to scrub the tags.

> (also a
> cover letter - however minimal - and a version number would help),
> please add them and post a v2 so that I can merge them, I can do that
> for you but it is good practice for the future.

Sure, I clearly need that practice.

-- 
Best regards,
Marek Vasut