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
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 <[email protected]>
Signed-off-by: Jisheng Zhang <[email protected]>
---
.../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
+Niklas
On Fri, Sep 25, 2020 at 2:39 AM Jisheng Zhang
<[email protected]> 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 <[email protected]>
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> .../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 <[email protected]>
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
>
On Fri, 25 Sep 2020 09:33:54 -0600 Rob Herring wrote:
>
> +Niklas
>
> On Fri, Sep 25, 2020 at 2:39 AM Jisheng Zhang
> <[email protected]> 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 <[email protected]>
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > ---
> > .../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,