linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] PCI: dwc: fix two MSI issues
@ 2020-09-25  8:34 Jisheng Zhang
  2020-09-25  8:37 ` [PATCH v3 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled Jisheng Zhang
  2020-09-25  8:38 ` [PATCH v3 2/2] PCI: dwc: Use an address in the driver data for MSI address Jisheng Zhang
  0 siblings, 2 replies; 6+ messages in thread
From: Jisheng Zhang @ 2020-09-25  8:34 UTC (permalink / raw)
  To: Jingoo Han, Gustavo Pimentel, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, Ard Biesheuvel
  Cc: linux-pci, linux-kernel

Fix two MSI issues. One to skip PCIE_MSI_INTR0* programming is MSI is
disabled, another to use an address in the driver data for MSI address,
the side effect of this patch is fixing the MSI page leakage during
suspend/resume.

Since v2:
  - add Acked-by tag
  - use an address in the driver data for MSI address. Thank Ard and Rob
    for pointing out this correct direction.
  - Since the MSI page has gone, the leak issue doesn't exist anymore,
    remove unnecessary patches.
  - Remove dw_pcie_free_msi rename and the last patch. They could be
    targeted to next. So will send out patches in a separate series.

Since v1:
  - add proper error handling patches.
  - solve the msi page leakage by moving dw_pcie_msi_init() from each
    users to designware host

Jisheng Zhang (2):
  PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled
  PCI: dwc: Use an address in the driver data for MSI address

 .../pci/controller/dwc/pcie-designware-host.c | 24 +++----------------
 drivers/pci/controller/dwc/pcie-designware.h  |  1 -
 2 files changed, 3 insertions(+), 22 deletions(-)

-- 
2.28.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled
  2020-09-25  8:34 [PATCH v3 0/2] PCI: dwc: fix two MSI issues Jisheng Zhang
@ 2020-09-25  8:37 ` Jisheng Zhang
  2020-09-25 15:50   ` Rob Herring
  2020-09-25  8:38 ` [PATCH v3 2/2] PCI: dwc: Use an address in the driver data for MSI address Jisheng Zhang
  1 sibling, 1 reply; 6+ messages in thread
From: Jisheng Zhang @ 2020-09-25  8:37 UTC (permalink / raw)
  To: Jingoo Han, Gustavo Pimentel, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, Ard Biesheuvel
  Cc: linux-pci, linux-kernel

If MSI is disabled, there's no need to program PCIE_MSI_INTR0_MASK
and PCIE_MSI_INTR0_ENABLE registers.

Fixes: 7c5925afbc58 ("PCI: dwc: Move MSI IRQs allocation to IRQ domainshierarchical API")
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 9dafecba347f..f08f4d97f321 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -632,7 +632,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 
 	dw_pcie_setup(pci);
 
-	if (!pp->ops->msi_host_init) {
+	if (pci_msi_enabled() && !pp->ops->msi_host_init) {
 		num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
 
 		/* Initialize IRQ Status array */
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/2] PCI: dwc: Use an address in the driver data for MSI address
  2020-09-25  8:34 [PATCH v3 0/2] PCI: dwc: fix two MSI issues Jisheng Zhang
  2020-09-25  8:37 ` [PATCH v3 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled Jisheng Zhang
@ 2020-09-25  8:38 ` Jisheng Zhang
  2020-09-25 15:33   ` Rob Herring
  1 sibling, 1 reply; 6+ messages in thread
From: Jisheng Zhang @ 2020-09-25  8:38 UTC (permalink / raw)
  To: Jingoo Han, Gustavo Pimentel, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, Ard Biesheuvel
  Cc: linux-pci, linux-kernel

There's no need to allocate a page for the MSI address, we could use
an address in the driver data.

One side effect of this patch is fixing the MSI page leakage during
suspend/resume. Take the pcie-tegra194.c for example, it calls
dw_pcie_msi_init() in resume path, thus the previous MSI page is
leaked.

Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 .../pci/controller/dwc/pcie-designware-host.c | 22 ++-----------------
 drivers/pci/controller/dwc/pcie-designware.h  |  1 -
 2 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f08f4d97f321..bf25d783b5c5 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -126,9 +126,7 @@ static void dw_pci_setup_msi_msg(struct irq_data *d, struct msi_msg *msg)
 {
 	struct pcie_port *pp = irq_data_get_irq_chip_data(d);
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	u64 msi_target;
-
-	msi_target = (u64)pp->msi_data;
+	u64 msi_target = virt_to_phys(&pp->msi_data);
 
 	msg->address_lo = lower_32_bits(msi_target);
 	msg->address_hi = upper_32_bits(msi_target);
@@ -287,27 +285,11 @@ void dw_pcie_free_msi(struct pcie_port *pp)
 
 	irq_domain_remove(pp->msi_domain);
 	irq_domain_remove(pp->irq_domain);
-
-	if (pp->msi_page)
-		__free_page(pp->msi_page);
 }
 
 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 = virt_to_phys(&pp->msi_data);
 
 	/* Program the msi_data */
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index f911760dcc69..a88e15a09745 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -195,7 +195,6 @@ struct pcie_port {
 	struct irq_domain	*irq_domain;
 	struct irq_domain	*msi_domain;
 	dma_addr_t		msi_data;
-	struct page		*msi_page;
 	struct irq_chip		*msi_irq_chip;
 	u32			num_vectors;
 	u32			irq_mask[MAX_MSI_CTRLS];
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] PCI: dwc: Use an address in the driver data for MSI address
  2020-09-25  8:38 ` [PATCH v3 2/2] PCI: dwc: Use an address in the driver data for MSI address Jisheng Zhang
@ 2020-09-25 15:33   ` Rob Herring
  2020-09-27  9:23     ` Jisheng Zhang
  0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2020-09-25 15:33 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Jingoo Han, Gustavo Pimentel, Lorenzo Pieralisi, Bjorn Helgaas,
	Ard Biesheuvel, PCI, linux-kernel, Niklas Cassel

+Niklas

On Fri, Sep 25, 2020 at 2:39 AM Jisheng Zhang
<Jisheng.Zhang@synaptics.com> wrote:
>
> There's no need to allocate a page for the MSI address, we could use
> an address in the driver data.
>
> One side effect of this patch is fixing the MSI page leakage during
> suspend/resume. Take the pcie-tegra194.c for example, it calls
> dw_pcie_msi_init() in resume path, thus the previous MSI page is
> leaked.
>
> Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 22 ++-----------------
>  drivers/pci/controller/dwc/pcie-designware.h  |  1 -
>  2 files changed, 2 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f08f4d97f321..bf25d783b5c5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -126,9 +126,7 @@ static void dw_pci_setup_msi_msg(struct irq_data *d, struct msi_msg *msg)
>  {
>         struct pcie_port *pp = irq_data_get_irq_chip_data(d);
>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> -       u64 msi_target;
> -
> -       msi_target = (u64)pp->msi_data;
> +       u64 msi_target = virt_to_phys(&pp->msi_data);
>
>         msg->address_lo = lower_32_bits(msi_target);
>         msg->address_hi = upper_32_bits(msi_target);
> @@ -287,27 +285,11 @@ void dw_pcie_free_msi(struct pcie_port *pp)
>
>         irq_domain_remove(pp->msi_domain);
>         irq_domain_remove(pp->irq_domain);
> -
> -       if (pp->msi_page)
> -               __free_page(pp->msi_page);
>  }
>
>  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 = virt_to_phys(&pp->msi_data);
>
>         /* Program the msi_data */
>         dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index f911760dcc69..a88e15a09745 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -195,7 +195,6 @@ struct pcie_port {
>         struct irq_domain       *irq_domain;
>         struct irq_domain       *msi_domain;
>         dma_addr_t              msi_data;

You should probably change the type here. u16 I guess since that's the
MSI data size.

With that,

Reviewed-by: Rob Herring <robh@kernel.org>

Would be nice to hear from others that have touched this code as with
Fixes it's going to get backported.

Rob


> -       struct page             *msi_page;
>         struct irq_chip         *msi_irq_chip;
>         u32                     num_vectors;
>         u32                     irq_mask[MAX_MSI_CTRLS];
> --
> 2.28.0
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled
  2020-09-25  8:37 ` [PATCH v3 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled Jisheng Zhang
@ 2020-09-25 15:50   ` Rob Herring
  0 siblings, 0 replies; 6+ messages in thread
From: Rob Herring @ 2020-09-25 15:50 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Jingoo Han, Gustavo Pimentel, Lorenzo Pieralisi, Bjorn Helgaas,
	Ard Biesheuvel, PCI, linux-kernel

On Fri, Sep 25, 2020 at 2:39 AM Jisheng Zhang
<Jisheng.Zhang@synaptics.com> wrote:
>
> If MSI is disabled, there's no need to program PCIE_MSI_INTR0_MASK
> and PCIE_MSI_INTR0_ENABLE registers.
>
> Fixes: 7c5925afbc58 ("PCI: dwc: Move MSI IRQs allocation to IRQ domainshierarchical API")
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] PCI: dwc: Use an address in the driver data for MSI address
  2020-09-25 15:33   ` Rob Herring
@ 2020-09-27  9:23     ` Jisheng Zhang
  0 siblings, 0 replies; 6+ messages in thread
From: Jisheng Zhang @ 2020-09-27  9:23 UTC (permalink / raw)
  To: Rob Herring
  Cc: Jingoo Han, Gustavo Pimentel, Lorenzo Pieralisi, Bjorn Helgaas,
	Ard Biesheuvel, PCI, linux-kernel, Niklas Cassel

On Fri, 25 Sep 2020 09:33:54 -0600 Rob Herring wrote:


> 
> +Niklas
> 
> On Fri, Sep 25, 2020 at 2:39 AM Jisheng Zhang
> <Jisheng.Zhang@synaptics.com> wrote:
> >
> > There's no need to allocate a page for the MSI address, we could use
> > an address in the driver data.
> >
> > One side effect of this patch is fixing the MSI page leakage during
> > suspend/resume. Take the pcie-tegra194.c for example, it calls
> > dw_pcie_msi_init() in resume path, thus the previous MSI page is
> > leaked.
> >
> > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> > Suggested-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > ---
> >  .../pci/controller/dwc/pcie-designware-host.c | 22 ++-----------------
> >  drivers/pci/controller/dwc/pcie-designware.h  |  1 -
> >  2 files changed, 2 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index f08f4d97f321..bf25d783b5c5 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -126,9 +126,7 @@ static void dw_pci_setup_msi_msg(struct irq_data *d, struct msi_msg *msg)
> >  {
> >         struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> >         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > -       u64 msi_target;
> > -
> > -       msi_target = (u64)pp->msi_data;
> > +       u64 msi_target = virt_to_phys(&pp->msi_data);
> >
> >         msg->address_lo = lower_32_bits(msi_target);
> >         msg->address_hi = upper_32_bits(msi_target);
> > @@ -287,27 +285,11 @@ void dw_pcie_free_msi(struct pcie_port *pp)
> >
> >         irq_domain_remove(pp->msi_domain);
> >         irq_domain_remove(pp->irq_domain);
> > -
> > -       if (pp->msi_page)
> > -               __free_page(pp->msi_page);
> >  }
> >
> >  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);

I checked commit 111111a72e677ff13 ("PCI: dwc: Use the DMA-API to get the MSI
address") again, I think we need dma_map_single() here, either due to 

The phy address is different with dma address on some systems

or

All memory isn't accessible from PCIe RC on some systems

dma_map_single() could work on all systems. But this leaves a problem: how
to avoid the map again during resume? A simple solution would be:

move the dma_map_single out of dw_pcie_msi_init() as V1 does, sure,
pci-dra7xx.c needs to call dma_map_single() itself.

Is this acceptable?

Thanks

> > -       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 = virt_to_phys(&pp->msi_data);
> >
> >         /* Program the msi_data */
> >         dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-09-27  9:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-25  8:34 [PATCH v3 0/2] PCI: dwc: fix two MSI issues Jisheng Zhang
2020-09-25  8:37 ` [PATCH v3 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled Jisheng Zhang
2020-09-25 15:50   ` Rob Herring
2020-09-25  8:38 ` [PATCH v3 2/2] PCI: dwc: Use an address in the driver data for MSI address Jisheng Zhang
2020-09-25 15:33   ` Rob Herring
2020-09-27  9:23     ` Jisheng Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).