[PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Jisheng Zhang]

Currently, dw_pcie_msi_init() allocates and maps page for msi, then
program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
may lose power during suspend-to-RAM, so when we resume, we want to
redo the latter but not the former. If designware based driver (for
example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
previous msi page will be leaked.

Move the allocate and map msi page from dw_pcie_msi_init() to
dw_pcie_host_init() to fix this problem.

Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 drivers/pci/controller/dwc/pci-dra7xx.c       | 18 ++++++++++++-
 .../pci/controller/dwc/pcie-designware-host.c | 27 +++++++++----------
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
index dc387724cf08..4301cf844a4c 100644
--- a/drivers/pci/controller/dwc/pci-dra7xx.c
+++ b/drivers/pci/controller/dwc/pci-dra7xx.c
@@ -490,7 +490,9 @@ static struct irq_chip dra7xx_pci_msi_bottom_irq_chip = {
 static int dra7xx_pcie_msi_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct device *dev = pci->dev;
 	u32 ctrl, num_ctrls;
+	int ret;
 
 	pp->msi_irq_chip = &dra7xx_pci_msi_bottom_irq_chip;
 
@@ -506,7 +508,21 @@ static int dra7xx_pcie_msi_host_init(struct pcie_port *pp)
 				    ~0);
 	}
 
-	return dw_pcie_allocate_domains(pp);
+	ret = dw_pcie_allocate_domains(pp);
+	if (ret)
+		return ret;
+
+	pp->msi_page = alloc_page(GFP_KERNEL);
+	pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE,
+				    DMA_FROM_DEVICE);
+	ret = dma_mapping_error(dev, pp->msi_data);
+	if (ret) {
+		dev_err(dev, "Failed to map MSI data\n");
+		__free_page(pp->msi_page);
+		pp->msi_page = NULL;
+		dw_pcie_free_msi(pp);
+	}
+	return ret;
 }
 
 static const struct dw_pcie_host_ops dra7xx_pcie_host_ops = {
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 9dafecba347f..c23ba64f64fe 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -294,20 +294,7 @@ void dw_pcie_free_msi(struct pcie_port *pp)
 
 void dw_pcie_msi_init(struct pcie_port *pp)
 {
-	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	struct device *dev = pci->dev;
-	u64 msi_target;
-
-	pp->msi_page = alloc_page(GFP_KERNEL);
-	pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE,
-				    DMA_FROM_DEVICE);
-	if (dma_mapping_error(dev, pp->msi_data)) {
-		dev_err(dev, "Failed to map MSI data\n");
-		__free_page(pp->msi_page);
-		pp->msi_page = NULL;
-		return;
-	}
-	msi_target = (u64)pp->msi_data;
+	u64 msi_target = (u64)pp->msi_data;
 
 	/* Program the msi_data */
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
@@ -440,6 +427,18 @@ int dw_pcie_host_init(struct pcie_port *pp)
 				irq_set_chained_handler_and_data(pp->msi_irq,
 							    dw_chained_msi_isr,
 							    pp);
+
+			pp->msi_page = alloc_page(GFP_KERNEL);
+			pp->msi_data = dma_map_page(pci->dev, pp->msi_page,
+						    0, PAGE_SIZE,
+						    DMA_FROM_DEVICE);
+			ret = dma_mapping_error(pci->dev, pp->msi_data);
+			if (ret) {
+				dev_err(pci->dev, "Failed to map MSI data\n");
+				__free_page(pp->msi_page);
+				pp->msi_page = NULL;
+				goto err_free_msi;
+			}
 		} else {
 			ret = pp->ops->msi_host_init(pp);
 			if (ret < 0)
-- 
2.28.0

Re: [PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Rob Herring]

On Wed, Sep 23, 2020 at 12:27 AM Jisheng Zhang
<Jisheng.Zhang@synaptics.com> wrote:
>
> Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> may lose power during suspend-to-RAM, so when we resume, we want to
> redo the latter but not the former. If designware based driver (for
> example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
> previous msi page will be leaked.

It's worse than this. I think there's also error paths too leaking the
page. Also, there's never a dma_unmap_page call which should happen
before freeing.

> Move the allocate and map msi page from dw_pcie_msi_init() to
> dw_pcie_host_init() to fix this problem.
>
> Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> ---
>  drivers/pci/controller/dwc/pci-dra7xx.c       | 18 ++++++++++++-
>  .../pci/controller/dwc/pcie-designware-host.c | 27 +++++++++----------
>  2 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> index dc387724cf08..4301cf844a4c 100644
> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> @@ -490,7 +490,9 @@ static struct irq_chip dra7xx_pci_msi_bottom_irq_chip = {
>  static int dra7xx_pcie_msi_host_init(struct pcie_port *pp)
>  {
>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +       struct device *dev = pci->dev;
>         u32 ctrl, num_ctrls;
> +       int ret;
>
>         pp->msi_irq_chip = &dra7xx_pci_msi_bottom_irq_chip;
>
> @@ -506,7 +508,21 @@ static int dra7xx_pcie_msi_host_init(struct pcie_port *pp)
>                                     ~0);
>         }
>
> -       return dw_pcie_allocate_domains(pp);
> +       ret = dw_pcie_allocate_domains(pp);
> +       if (ret)
> +               return ret;
> +
> +       pp->msi_page = alloc_page(GFP_KERNEL);
> +       pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE,
> +                                   DMA_FROM_DEVICE);
> +       ret = dma_mapping_error(dev, pp->msi_data);
> +       if (ret) {
> +               dev_err(dev, "Failed to map MSI data\n");
> +               __free_page(pp->msi_page);
> +               pp->msi_page = NULL;
> +               dw_pcie_free_msi(pp);
> +       }

I don't like having 2 copies of the same thing. Also, doesn't keystone
need this too?

The other thing is .msi_host_init() is abused by having an empty
function to disable MSI support. We should have a flag instead to
enable/disable MSI support and then we can key off of that in the
common code.

> +       return ret;
>  }
>
>  static const struct dw_pcie_host_ops dra7xx_pcie_host_ops = {
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 9dafecba347f..c23ba64f64fe 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -294,20 +294,7 @@ void dw_pcie_free_msi(struct pcie_port *pp)
>
>  void dw_pcie_msi_init(struct pcie_port *pp)

Might be good to rename this function with exactly what it does.
There's too many 'init' and 'setup' functions...

>  {
> -       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> -       struct device *dev = pci->dev;
> -       u64 msi_target;
> -
> -       pp->msi_page = alloc_page(GFP_KERNEL);
> -       pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE,
> -                                   DMA_FROM_DEVICE);
> -       if (dma_mapping_error(dev, pp->msi_data)) {
> -               dev_err(dev, "Failed to map MSI data\n");
> -               __free_page(pp->msi_page);
> -               pp->msi_page = NULL;
> -               return;
> -       }
> -       msi_target = (u64)pp->msi_data;
> +       u64 msi_target = (u64)pp->msi_data;
>
>         /* Program the msi_data */
>         dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> @@ -440,6 +427,18 @@ int dw_pcie_host_init(struct pcie_port *pp)
>                                 irq_set_chained_handler_and_data(pp->msi_irq,
>                                                             dw_chained_msi_isr,
>                                                             pp);
> +
> +                       pp->msi_page = alloc_page(GFP_KERNEL);
> +                       pp->msi_data = dma_map_page(pci->dev, pp->msi_page,
> +                                                   0, PAGE_SIZE,
> +                                                   DMA_FROM_DEVICE);
> +                       ret = dma_mapping_error(pci->dev, pp->msi_data);
> +                       if (ret) {
> +                               dev_err(pci->dev, "Failed to map MSI data\n");
> +                               __free_page(pp->msi_page);
> +                               pp->msi_page = NULL;
> +                               goto err_free_msi;
> +                       }
>                 } else {
>                         ret = pp->ops->msi_host_init(pp);
>                         if (ret < 0)
> --
> 2.28.0
>

Re: [PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Jisheng Zhang]

Hi Rob,

On Wed, 23 Sep 2020 10:41:45 -0600 Rob Herring wrote:

> 
> On Wed, Sep 23, 2020 at 12:27 AM Jisheng Zhang
> <Jisheng.Zhang@synaptics.com> wrote:
> >
> > Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> > may lose power during suspend-to-RAM, so when we resume, we want to
> > redo the latter but not the former. If designware based driver (for
> > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
> > previous msi page will be leaked.  
> 
> It's worse than this. I think there's also error paths too leaking the

I think you mean the leaking in pcie-tegra194.c's error path. Synaptics
SoC pcie driver(not mainlined) needs to call dw_pcie_msi_init() in resume
path, pcie-tegra194.c shares the same problem, so I mentioned it in the commit
msg, but the patch isn't targeting to fix all the leaking issues in
pcie-tegra194.c. This patch at least fix one of the issue. 

> page. Also, there's never a dma_unmap_page call which should happen
> before freeing.

Thanks for pointing it out. I will add it in v2

> 
> > Move the allocate and map msi page from dw_pcie_msi_init() to
> > dw_pcie_host_init() to fix this problem.
> >
> > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > ---
> >  drivers/pci/controller/dwc/pci-dra7xx.c       | 18 ++++++++++++-
> >  .../pci/controller/dwc/pcie-designware-host.c | 27 +++++++++----------
> >  2 files changed, 30 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> > index dc387724cf08..4301cf844a4c 100644
> > --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> > @@ -490,7 +490,9 @@ static struct irq_chip dra7xx_pci_msi_bottom_irq_chip = {
> >  static int dra7xx_pcie_msi_host_init(struct pcie_port *pp)
> >  {
> >         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +       struct device *dev = pci->dev;
> >         u32 ctrl, num_ctrls;
> > +       int ret;
> >
> >         pp->msi_irq_chip = &dra7xx_pci_msi_bottom_irq_chip;
> >
> > @@ -506,7 +508,21 @@ static int dra7xx_pcie_msi_host_init(struct pcie_port *pp)
> >                                     ~0);
> >         }
> >
> > -       return dw_pcie_allocate_domains(pp);
> > +       ret = dw_pcie_allocate_domains(pp);
> > +       if (ret)
> > +               return ret;
> > +
> > +       pp->msi_page = alloc_page(GFP_KERNEL);
> > +       pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE,
> > +                                   DMA_FROM_DEVICE);
> > +       ret = dma_mapping_error(dev, pp->msi_data);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to map MSI data\n");
> > +               __free_page(pp->msi_page);
> > +               pp->msi_page = NULL;
> > +               dw_pcie_free_msi(pp);
> > +       }  
> 
> I don't like having 2 copies of the same thing. Also, doesn't keystone
> need this too?

what about introduce dw_pcie_msi_alloc() to do this?

IIUC, keystone doesn't need this.

> 
> The other thing is .msi_host_init() is abused by having an empty
> function to disable MSI support. We should have a flag instead to
> enable/disable MSI support and then we can key off of that in the
> common code.

FWICT, the .msi_host_init() is to init soc's own msi support rather
than init the DWC's integrated MSI module. So the usage is correct.

> 
> > +       return ret;
> >  }
> >
> >  static const struct dw_pcie_host_ops dra7xx_pcie_host_ops = {
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 9dafecba347f..c23ba64f64fe 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -294,20 +294,7 @@ void dw_pcie_free_msi(struct pcie_port *pp)
> >
> >  void dw_pcie_msi_init(struct pcie_port *pp)  
> 
> Might be good to rename this function with exactly what it does.
> There's too many 'init' and 'setup' functions...

If we move the msi page allocation out of dw_pcie_msi_init(), then
it only initializes the integrated MSI.

Thanks

> 
> >  {
> > -       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > -       struct device *dev = pci->dev;
> > -       u64 msi_target;
> > -
> > -       pp->msi_page = alloc_page(GFP_KERNEL);
> > -       pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE,
> > -                                   DMA_FROM_DEVICE);
> > -       if (dma_mapping_error(dev, pp->msi_data)) {
> > -               dev_err(dev, "Failed to map MSI data\n");
> > -               __free_page(pp->msi_page);
> > -               pp->msi_page = NULL;
> > -               return;
> > -       }
> > -       msi_target = (u64)pp->msi_data;
> > +       u64 msi_target = (u64)pp->msi_data;
> >
> >         /* Program the msi_data */
> >         dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> > @@ -440,6 +427,18 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >                                 irq_set_chained_handler_and_data(pp->msi_irq,
> >                                                             dw_chained_msi_isr,
> >                                                             pp);
> > +
> > +                       pp->msi_page = alloc_page(GFP_KERNEL);
> > +                       pp->msi_data = dma_map_page(pci->dev, pp->msi_page,
> > +                                                   0, PAGE_SIZE,
> > +                                                   DMA_FROM_DEVICE);
> > +                       ret = dma_mapping_error(pci->dev, pp->msi_data);
> > +                       if (ret) {
> > +                               dev_err(pci->dev, "Failed to map MSI data\n");
> > +                               __free_page(pp->msi_page);
> > +                               pp->msi_page = NULL;
> > +                               goto err_free_msi;
> > +                       }
> >                 } else {
> >                         ret = pp->ops->msi_host_init(pp);
> >                         if (ret < 0)
> > --
> > 2.28.0
> >  

Re: [PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Ard Biesheuvel]

On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
>
> Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> may lose power during suspend-to-RAM, so when we resume, we want to
> redo the latter but not the former. If designware based driver (for
> example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
> previous msi page will be leaked.
>
> Move the allocate and map msi page from dw_pcie_msi_init() to
> dw_pcie_host_init() to fix this problem.
>
> Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>

Why do you allocate a page for this in the first place? Isn't
PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that
never gets forwarded across to the CPU side of the host bridge, and
triggers a SPI instead, which gets handled by reading
PCIE_MSI_INTR0_STATUS ?

Couldn't you just map the zero page instead?


> ---
>  drivers/pci/controller/dwc/pci-dra7xx.c       | 18 ++++++++++++-
>  .../pci/controller/dwc/pcie-designware-host.c | 27 +++++++++----------
>  2 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> index dc387724cf08..4301cf844a4c 100644
> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> @@ -490,7 +490,9 @@ static struct irq_chip dra7xx_pci_msi_bottom_irq_chip = {
>  static int dra7xx_pcie_msi_host_init(struct pcie_port *pp)
>  {
>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +       struct device *dev = pci->dev;
>         u32 ctrl, num_ctrls;
> +       int ret;
>
>         pp->msi_irq_chip = &dra7xx_pci_msi_bottom_irq_chip;
>
> @@ -506,7 +508,21 @@ static int dra7xx_pcie_msi_host_init(struct pcie_port *pp)
>                                     ~0);
>         }
>
> -       return dw_pcie_allocate_domains(pp);
> +       ret = dw_pcie_allocate_domains(pp);
> +       if (ret)
> +               return ret;
> +
> +       pp->msi_page = alloc_page(GFP_KERNEL);
> +       pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE,
> +                                   DMA_FROM_DEVICE);
> +       ret = dma_mapping_error(dev, pp->msi_data);
> +       if (ret) {
> +               dev_err(dev, "Failed to map MSI data\n");
> +               __free_page(pp->msi_page);
> +               pp->msi_page = NULL;
> +               dw_pcie_free_msi(pp);
> +       }
> +       return ret;
>  }
>
>  static const struct dw_pcie_host_ops dra7xx_pcie_host_ops = {
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 9dafecba347f..c23ba64f64fe 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -294,20 +294,7 @@ void dw_pcie_free_msi(struct pcie_port *pp)
>
>  void dw_pcie_msi_init(struct pcie_port *pp)
>  {
> -       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> -       struct device *dev = pci->dev;
> -       u64 msi_target;
> -
> -       pp->msi_page = alloc_page(GFP_KERNEL);
> -       pp->msi_data = dma_map_page(dev, pp->msi_page, 0, PAGE_SIZE,
> -                                   DMA_FROM_DEVICE);
> -       if (dma_mapping_error(dev, pp->msi_data)) {
> -               dev_err(dev, "Failed to map MSI data\n");
> -               __free_page(pp->msi_page);
> -               pp->msi_page = NULL;
> -               return;
> -       }
> -       msi_target = (u64)pp->msi_data;
> +       u64 msi_target = (u64)pp->msi_data;
>
>         /* Program the msi_data */
>         dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> @@ -440,6 +427,18 @@ int dw_pcie_host_init(struct pcie_port *pp)
>                                 irq_set_chained_handler_and_data(pp->msi_irq,
>                                                             dw_chained_msi_isr,
>                                                             pp);
> +
> +                       pp->msi_page = alloc_page(GFP_KERNEL);
> +                       pp->msi_data = dma_map_page(pci->dev, pp->msi_page,
> +                                                   0, PAGE_SIZE,
> +                                                   DMA_FROM_DEVICE);
> +                       ret = dma_mapping_error(pci->dev, pp->msi_data);
> +                       if (ret) {
> +                               dev_err(pci->dev, "Failed to map MSI data\n");
> +                               __free_page(pp->msi_page);
> +                               pp->msi_page = NULL;
> +                               goto err_free_msi;
> +                       }
>                 } else {
>                         ret = pp->ops->msi_host_init(pp);
>                         if (ret < 0)
> --
> 2.28.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Rob Herring]

On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> >
> > Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> > may lose power during suspend-to-RAM, so when we resume, we want to
> > redo the latter but not the former. If designware based driver (for
> > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
> > previous msi page will be leaked.
> >
> > Move the allocate and map msi page from dw_pcie_msi_init() to
> > dw_pcie_host_init() to fix this problem.
> >
> > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
>
> Why do you allocate a page for this in the first place? Isn't
> PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that
> never gets forwarded across to the CPU side of the host bridge, and
> triggers a SPI instead, which gets handled by reading
> PCIE_MSI_INTR0_STATUS ?

My question too after digging into this some more. I've asked the
question on the thread that further complicated all this changing from
virt_to_phys() to dma_map_page()[1].

> Couldn't you just map the zero page instead?

Why a page even? You could use PCIE_MSI_ADDR_LO address itself even.
Or just an address in the driver data which is what some other drivers
do.

Rob

[1] https://lore.kernel.org/linux-pci/20200923231846.GA1499246@bogus/

Re: [PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Ard Biesheuvel]

On Thu, 24 Sep 2020 at 15:28, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> > >
> > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> > > may lose power during suspend-to-RAM, so when we resume, we want to
> > > redo the latter but not the former. If designware based driver (for
> > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
> > > previous msi page will be leaked.
> > >
> > > Move the allocate and map msi page from dw_pcie_msi_init() to
> > > dw_pcie_host_init() to fix this problem.
> > >
> > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> >
> > Why do you allocate a page for this in the first place? Isn't
> > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that
> > never gets forwarded across to the CPU side of the host bridge, and
> > triggers a SPI instead, which gets handled by reading
> > PCIE_MSI_INTR0_STATUS ?
>
> My question too after digging into this some more. I've asked the
> question on the thread that further complicated all this changing from
> virt_to_phys() to dma_map_page()[1].
>
> > Couldn't you just map the zero page instead?
>
> Why a page even? You could use PCIE_MSI_ADDR_LO address itself even.
> Or just an address in the driver data which is what some other drivers
> do.
>

PCIE_MSI_ADDR_LO itself could collide with an actual DRAM address if
any translation is applied on inbound transactions. Using an actual
DRAM address avoids that.

Re: [PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Ard Biesheuvel]

On Thu, 24 Sep 2020 at 15:45, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 24 Sep 2020 at 15:28, Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> > > >
> > > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> > > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> > > > may lose power during suspend-to-RAM, so when we resume, we want to
> > > > redo the latter but not the former. If designware based driver (for
> > > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
> > > > previous msi page will be leaked.
> > > >
> > > > Move the allocate and map msi page from dw_pcie_msi_init() to
> > > > dw_pcie_host_init() to fix this problem.
> > > >
> > > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > >
> > > Why do you allocate a page for this in the first place? Isn't
> > > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that
> > > never gets forwarded across to the CPU side of the host bridge, and
> > > triggers a SPI instead, which gets handled by reading
> > > PCIE_MSI_INTR0_STATUS ?
> >
> > My question too after digging into this some more. I've asked the
> > question on the thread that further complicated all this changing from
> > virt_to_phys() to dma_map_page()[1].
> >
> > > Couldn't you just map the zero page instead?
> >
> > Why a page even? You could use PCIE_MSI_ADDR_LO address itself even.
> > Or just an address in the driver data which is what some other drivers
> > do.
> >
>
> PCIE_MSI_ADDR_LO itself could collide with an actual DRAM address if
> any translation is applied on inbound transactions. Using an actual
> DRAM address avoids that.

... although the MSI doorbell register on the GIC, for instance, needs
to be DMA addressable as well, of course.

Re: [PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Rob Herring]

On Thu, Sep 24, 2020 at 8:33 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 24 Sep 2020 at 15:45, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 24 Sep 2020 at 15:28, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> > > > >
> > > > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> > > > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> > > > > may lose power during suspend-to-RAM, so when we resume, we want to
> > > > > redo the latter but not the former. If designware based driver (for
> > > > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
> > > > > previous msi page will be leaked.
> > > > >
> > > > > Move the allocate and map msi page from dw_pcie_msi_init() to
> > > > > dw_pcie_host_init() to fix this problem.
> > > > >
> > > > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> > > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > > >
> > > > Why do you allocate a page for this in the first place? Isn't
> > > > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that
> > > > never gets forwarded across to the CPU side of the host bridge, and
> > > > triggers a SPI instead, which gets handled by reading
> > > > PCIE_MSI_INTR0_STATUS ?
> > >
> > > My question too after digging into this some more. I've asked the
> > > question on the thread that further complicated all this changing from
> > > virt_to_phys() to dma_map_page()[1].
> > >
> > > > Couldn't you just map the zero page instead?
> > >
> > > Why a page even? You could use PCIE_MSI_ADDR_LO address itself even.
> > > Or just an address in the driver data which is what some other drivers
> > > do.
> > >
> >
> > PCIE_MSI_ADDR_LO itself could collide with an actual DRAM address if
> > any translation is applied on inbound transactions. Using an actual
> > DRAM address avoids that.

Good point, and the inbound windows could be less than all DRAM if
there's any restrictions. However, the DWC doesn't do any inbound
setup (at least for platforms with iATU), so I guess the default is
all traffic is forwarded.

> ... although the MSI doorbell register on the GIC, for instance, needs
> to be DMA addressable as well, of course.

There's at least one DWC driver that has its own doorbell register
too. I suppose there's a way for the host driver to retrieve the GIC
address if configuration is needed. Doesn't seem to be needed on DWC
given the above.

Rob

Re: [PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Jisheng Zhang]

On Thu, 24 Sep 2020 07:28:25 -0600
Rob Herring <robh@kernel.org> wrote:

> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:  
> > >
> > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> > > may lose power during suspend-to-RAM, so when we resume, we want to
> > > redo the latter but not the former. If designware based driver (for
> > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
> > > previous msi page will be leaked.
> > >
> > > Move the allocate and map msi page from dw_pcie_msi_init() to
> > > dw_pcie_host_init() to fix this problem.
> > >
> > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>  
> >
> > Why do you allocate a page for this in the first place? Isn't
> > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that
> > never gets forwarded across to the CPU side of the host bridge, and
> > triggers a SPI instead, which gets handled by reading
> > PCIE_MSI_INTR0_STATUS ?  
> 
> My question too after digging into this some more. I've asked the
> question on the thread that further complicated all this changing from
> virt_to_phys() to dma_map_page()[1].
> 
> > Couldn't you just map the zero page instead?  
> 
> Why a page even? You could use PCIE_MSI_ADDR_LO address itself even.
> Or just an address in the driver data which is what some other drivers
> do.
> 

Thank Ard and Rob. I have sent out V3 (https://lkml.org/lkml/2020/9/25/272)
which uses an address in the driver data for MSI address.


Hi Niklas,

can you please try v3 to see whether it solves your problem as well?

Thanks