PCI: dwc: fix two MSI issues
v3 cover PCI: dwc: fix two MSI issues
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(-)
Signature · 1 line
2.28.0
PATCH v3 1/2 PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled
If MSI is disabled, there's no need to program PCIE_MSI_INTR0_MASK and PCIE_MSI_INTR0_ENABLE registers.
--- drivers/pci/controller/dwc/pcie-designware-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-designware-host.c +1 -1
@@ -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 */
Signature · 1 line
2.28.0
Re: [PATCH v3 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled
Quoted 10 lines · Jisheng Zhang
If MSI is disabled, there's no need to program
PCIE_MSI_INTR0_MASKandPCIE_MSI_INTR0_ENABLEregisters.Fixes 7c5925afbc58 ("PCI: dwc: Move MSI IRQs allocation to IRQ domainshierarchical API")Signed-off-by Jisheng Zhang <[email protected]>Acked-by Gustavo Pimentel <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
PATCH v3 2/2 PCI: dwc: Use an address in the driver data for MSI address
In reply to Jisheng ZhangThere'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.
--- .../pci/controller/dwc/pcie-designware-host.c | 22 ++----------------- drivers/pci/controller/dwc/pcie-designware.h | 1 - 2 files changed, 2 insertions(+), 21 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +2 -20
@@ -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,
drivers/pci/controller/dwc/pcie-designware.h +0 -1
@@ -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 ];
Signature · 1 line
2.28.0
Re: [PATCH v3 2/2] PCI: dwc: Use an address in the driver data for MSI address
+Niklas
Quoted 66 lines · Jisheng Zhang
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.cfor example, it callsdw_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(-)drivers/pci/controller/dwc/pcie-designware-host.c +2
-20@@ -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,drivers/pci/controller/dwc/pcie-designware.h +0
-0
drivers/pci/controller/dwc/pcie-designware.h struct pcie_port { · line 197
Quoted 3 lines
drivers/pci/controller/dwc/pcie-designware.h +0
-0@@ -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,
Would be nice to hear from others that have touched this code as with Fixes it's going to get backported.
Rob
Quoted 7 lines
drivers/pci/controller/dwc/pcie-designware.h +0
-2-struct page *msi_page;struct irq_chip *msi_irq_chip;u32 num_vectors;u32 irq_mask[MAX_MSI_CTRLS ]; --2.28.0
Re: [PATCH v3 2/2] PCI: dwc: Use an address in the driver data for MSI address
Quoted 46 lines · On Fri, 25 Sep 2020 09:33:54 -0600 Rob Herring wrote:
+Niklas
Quoted 41 lines · Jisheng Zhang
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.cfor example, it callsdw_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(-)drivers/pci/controller/dwc/pcie-designware-host.c +1
-6@@ -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); }
Quoted 9 lines
drivers/pci/controller/dwc/pcie-designware-host.c void dw_pcie_free_msi(struct pcie_port *pp) · line 303
Quoted 9 lines
drivers/pci/controller/dwc/pcie-designware-host.c +0
-7void 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
Quoted 11 lines
Quoted 11 lines
drivers/pci/controller/dwc/pcie-designware-host.c +1
-7-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,