[PATCH v7 0/2] PCI: dwc: fix two MSI issues

[PATCH v7 0/2] PCI: dwc: fix two MSI issues

[Jisheng Zhang]

Fix two MSI issues. One to skip PCIE_MSI_INTR0* programming if MSI is
disabled, another to use an address in the driver data for MSI address,
to fix the MSI page leakage during suspend/resume.

Since v6:
  - Keep the IS_ENABLED(CONFIG_PCI_MSI) check in dw_pcie_msi_init().

Since v5:
  - rebase on pci/dwc branch
  - add Acked-by tag

Since v4:
  - fix pci-dra7xx.c

Since v3:
  - add Acked-by tag
  - change patch2 commit msg to make it clear
  - map the MSI msg with dma_map_single_attrs() for some platforms
    which either has separate addrs for dma and phy or has mem access
    limitation for the PCIe.

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: Fix MSI page leakage in suspend/resume

 drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
 .../pci/controller/dwc/pcie-designware-host.c | 35 ++++++++++---------
 drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
 3 files changed, 37 insertions(+), 18 deletions(-)

-- 
2.28.0

[PATCH v7 0/2] PCI: dwc: fix two MSI issues

[Jisheng Zhang]

Fix two MSI issues. One to skip PCIE_MSI_INTR0* programming if MSI is
disabled, another to use an address in the driver data for MSI address,
to fix the MSI page leakage during suspend/resume.

Since v6:
  - Keep the IS_ENABLED(CONFIG_PCI_MSI) check in dw_pcie_msi_init().

Since v5:
  - rebase on pci/dwc branch
  - add Acked-by tag

Since v4:
  - fix pci-dra7xx.c

Since v3:
  - add Acked-by tag
  - change patch2 commit msg to make it clear
  - map the MSI msg with dma_map_single_attrs() for some platforms
    which either has separate addrs for dma and phy or has mem access
    limitation for the PCIe.

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: Fix MSI page leakage in suspend/resume

 drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
 .../pci/controller/dwc/pcie-designware-host.c | 35 ++++++++++---------
 drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
 3 files changed, 37 insertions(+), 18 deletions(-)

-- 
2.28.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled

[Jisheng Zhang]

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

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Reviewed-by: Rob Herring <robh@kernel.org>
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 317ff512f8df..d3e9ea11ce9e 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -528,7 +528,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

[PATCH v7 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled

[Jisheng Zhang]

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

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Reviewed-by: Rob Herring <robh@kernel.org>
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 317ff512f8df..d3e9ea11ce9e 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -528,7 +528,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[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
msi page will be leaked.

As pointed out by Rob and Ard, there's no need to allocate a page for
the MSI address, we could use an address in the driver data.

To avoid map the MSI msg again during resume, we move the map MSI msg
from dw_pcie_msi_init() to dw_pcie_host_init().

Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
 .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
 drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
 3 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
index 8f0b6d644e4b..6d012d2b1e90 100644
--- a/drivers/pci/controller/dwc/pci-dra7xx.c
+++ b/drivers/pci/controller/dwc/pci-dra7xx.c
@@ -466,7 +466,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;
 
@@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
+					   sizeof(pp->msi_msg),
+					   DMA_FROM_DEVICE,
+					   DMA_ATTR_SKIP_CPU_SYNC);
+	ret = dma_mapping_error(dev, pp->msi_data);
+	if (ret) {
+		dev_err(dev, "Failed to map MSI data\n");
+		pp->msi_data = 0;
+		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 d3e9ea11ce9e..d02c7e74738d 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -266,30 +266,23 @@ 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);
+	if (pp->msi_data) {
+		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+		struct device *dev = pci->dev;
+
+		dma_unmap_single_attrs(dev, pp->msi_data, sizeof(pp->msi_msg),
+				       DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+	}
 }
 
 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;
+	u64 msi_target = (u64)pp->msi_data;
 
 	if (!IS_ENABLED(CONFIG_PCI_MSI))
 		return;
 
-	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;
-
 	/* Program the msi_data */
 	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
 	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
@@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
+						      sizeof(pp->msi_msg),
+						      DMA_FROM_DEVICE,
+						      DMA_ATTR_SKIP_CPU_SYNC);
+			if (dma_mapping_error(pci->dev, pp->msi_data)) {
+				dev_err(pci->dev, "Failed to map MSI data\n");
+				pp->msi_data = 0;
+				goto err_free_msi;
+			}
 		} else {
 			ret = pp->ops->msi_host_init(pp);
 			if (ret < 0)
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 97c7063b9e89..9d2f511f13fa 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -190,8 +190,8 @@ struct pcie_port {
 	int			msi_irq;
 	struct irq_domain	*irq_domain;
 	struct irq_domain	*msi_domain;
+	u16			msi_msg;
 	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

[PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[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
msi page will be leaked.

As pointed out by Rob and Ard, there's no need to allocate a page for
the MSI address, we could use an address in the driver data.

To avoid map the MSI msg again during resume, we move the map MSI msg
from dw_pcie_msi_init() to dw_pcie_host_init().

Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
 .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
 drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
 3 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
index 8f0b6d644e4b..6d012d2b1e90 100644
--- a/drivers/pci/controller/dwc/pci-dra7xx.c
+++ b/drivers/pci/controller/dwc/pci-dra7xx.c
@@ -466,7 +466,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;
 
@@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
+					   sizeof(pp->msi_msg),
+					   DMA_FROM_DEVICE,
+					   DMA_ATTR_SKIP_CPU_SYNC);
+	ret = dma_mapping_error(dev, pp->msi_data);
+	if (ret) {
+		dev_err(dev, "Failed to map MSI data\n");
+		pp->msi_data = 0;
+		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 d3e9ea11ce9e..d02c7e74738d 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -266,30 +266,23 @@ 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);
+	if (pp->msi_data) {
+		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+		struct device *dev = pci->dev;
+
+		dma_unmap_single_attrs(dev, pp->msi_data, sizeof(pp->msi_msg),
+				       DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+	}
 }
 
 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;
+	u64 msi_target = (u64)pp->msi_data;
 
 	if (!IS_ENABLED(CONFIG_PCI_MSI))
 		return;
 
-	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;
-
 	/* Program the msi_data */
 	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
 	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
@@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
+						      sizeof(pp->msi_msg),
+						      DMA_FROM_DEVICE,
+						      DMA_ATTR_SKIP_CPU_SYNC);
+			if (dma_mapping_error(pci->dev, pp->msi_data)) {
+				dev_err(pci->dev, "Failed to map MSI data\n");
+				pp->msi_data = 0;
+				goto err_free_msi;
+			}
 		} else {
 			ret = pp->ops->msi_host_init(pp);
 			if (ret < 0)
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 97c7063b9e89..9d2f511f13fa 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -190,8 +190,8 @@ struct pcie_port {
 	int			msi_irq;
 	struct irq_domain	*irq_domain;
 	struct irq_domain	*msi_domain;
+	u16			msi_msg;
 	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Robin Murphy]

On 2020-10-09 08:55, Jisheng Zhang 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
> msi page will be leaked.
> 
> As pointed out by Rob and Ard, there's no need to allocate a page for
> the MSI address, we could use an address in the driver data.
> 
> To avoid map the MSI msg again during resume, we move the map MSI msg
> from dw_pcie_msi_init() to dw_pcie_host_init().

You should move the unmap there as well. As soon as you know what the 
relevant address would be if you *were* to do DMA to this location, then 
the exercise is complete. Leaving it mapped for the lifetime of the 
device in order to do not-DMA to it seems questionable (and represents 
technically incorrect API usage without at least a sync_for_cpu call 
before any other access to the data).

Another point of note is that using streaming DMA mappings at all is a 
bit fragile (regardless of this change). If the host controller itself 
has a limited DMA mask relative to physical memory (which integrators 
still seem to keep doing...) then you could end up punching your MSI 
hole right in the middle of the SWIOTLB bounce buffer, where it's then 
almost *guaranteed* to interfere with real DMA :(

If no DWC users have that problem and the current code is working well 
enough, then I see little reason not to make this partucular change to 
tidy up the implementation, just bear in mind that there's always the 
possibility of having to come back and change it yet again in future to 
make it more robust. I had it in mind that this trick was done with a 
coherent DMA allocation, which would be safe from addressing problems 
but would need to be kept around for the lifetime of the device, but 
maybe that was a different driver :/

Robin.

> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>   drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
>   .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
>   drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>   3 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> index 8f0b6d644e4b..6d012d2b1e90 100644
> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> @@ -466,7 +466,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;
>   
> @@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
> +					   sizeof(pp->msi_msg),
> +					   DMA_FROM_DEVICE,
> +					   DMA_ATTR_SKIP_CPU_SYNC);
> +	ret = dma_mapping_error(dev, pp->msi_data);
> +	if (ret) {
> +		dev_err(dev, "Failed to map MSI data\n");
> +		pp->msi_data = 0;
> +		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 d3e9ea11ce9e..d02c7e74738d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -266,30 +266,23 @@ 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);
> +	if (pp->msi_data) {
> +		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +		struct device *dev = pci->dev;
> +
> +		dma_unmap_single_attrs(dev, pp->msi_data, sizeof(pp->msi_msg),
> +				       DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> +	}
>   }
>   
>   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;
> +	u64 msi_target = (u64)pp->msi_data;
>   
>   	if (!IS_ENABLED(CONFIG_PCI_MSI))
>   		return;
>   
> -	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;
> -
>   	/* Program the msi_data */
>   	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
>   	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
> @@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
> +						      sizeof(pp->msi_msg),
> +						      DMA_FROM_DEVICE,
> +						      DMA_ATTR_SKIP_CPU_SYNC);
> +			if (dma_mapping_error(pci->dev, pp->msi_data)) {
> +				dev_err(pci->dev, "Failed to map MSI data\n");
> +				pp->msi_data = 0;
> +				goto err_free_msi;
> +			}
>   		} else {
>   			ret = pp->ops->msi_host_init(pp);
>   			if (ret < 0)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 97c7063b9e89..9d2f511f13fa 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -190,8 +190,8 @@ struct pcie_port {
>   	int			msi_irq;
>   	struct irq_domain	*irq_domain;
>   	struct irq_domain	*msi_domain;
> +	u16			msi_msg;
>   	dma_addr_t		msi_data;
> -	struct page		*msi_page;
>   	struct irq_chip		*msi_irq_chip;
>   	u32			num_vectors;
>   	u32			irq_mask[MAX_MSI_CTRLS];
> 

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Robin Murphy]

On 2020-10-09 08:55, Jisheng Zhang 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
> msi page will be leaked.
> 
> As pointed out by Rob and Ard, there's no need to allocate a page for
> the MSI address, we could use an address in the driver data.
> 
> To avoid map the MSI msg again during resume, we move the map MSI msg
> from dw_pcie_msi_init() to dw_pcie_host_init().

You should move the unmap there as well. As soon as you know what the 
relevant address would be if you *were* to do DMA to this location, then 
the exercise is complete. Leaving it mapped for the lifetime of the 
device in order to do not-DMA to it seems questionable (and represents 
technically incorrect API usage without at least a sync_for_cpu call 
before any other access to the data).

Another point of note is that using streaming DMA mappings at all is a 
bit fragile (regardless of this change). If the host controller itself 
has a limited DMA mask relative to physical memory (which integrators 
still seem to keep doing...) then you could end up punching your MSI 
hole right in the middle of the SWIOTLB bounce buffer, where it's then 
almost *guaranteed* to interfere with real DMA :(

If no DWC users have that problem and the current code is working well 
enough, then I see little reason not to make this partucular change to 
tidy up the implementation, just bear in mind that there's always the 
possibility of having to come back and change it yet again in future to 
make it more robust. I had it in mind that this trick was done with a 
coherent DMA allocation, which would be safe from addressing problems 
but would need to be kept around for the lifetime of the device, but 
maybe that was a different driver :/

Robin.

> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>   drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
>   .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
>   drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>   3 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> index 8f0b6d644e4b..6d012d2b1e90 100644
> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> @@ -466,7 +466,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;
>   
> @@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
> +					   sizeof(pp->msi_msg),
> +					   DMA_FROM_DEVICE,
> +					   DMA_ATTR_SKIP_CPU_SYNC);
> +	ret = dma_mapping_error(dev, pp->msi_data);
> +	if (ret) {
> +		dev_err(dev, "Failed to map MSI data\n");
> +		pp->msi_data = 0;
> +		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 d3e9ea11ce9e..d02c7e74738d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -266,30 +266,23 @@ 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);
> +	if (pp->msi_data) {
> +		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +		struct device *dev = pci->dev;
> +
> +		dma_unmap_single_attrs(dev, pp->msi_data, sizeof(pp->msi_msg),
> +				       DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> +	}
>   }
>   
>   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;
> +	u64 msi_target = (u64)pp->msi_data;
>   
>   	if (!IS_ENABLED(CONFIG_PCI_MSI))
>   		return;
>   
> -	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;
> -
>   	/* Program the msi_data */
>   	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
>   	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
> @@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
> +						      sizeof(pp->msi_msg),
> +						      DMA_FROM_DEVICE,
> +						      DMA_ATTR_SKIP_CPU_SYNC);
> +			if (dma_mapping_error(pci->dev, pp->msi_data)) {
> +				dev_err(pci->dev, "Failed to map MSI data\n");
> +				pp->msi_data = 0;
> +				goto err_free_msi;
> +			}
>   		} else {
>   			ret = pp->ops->msi_host_init(pp);
>   			if (ret < 0)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 97c7063b9e89..9d2f511f13fa 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -190,8 +190,8 @@ struct pcie_port {
>   	int			msi_irq;
>   	struct irq_domain	*irq_domain;
>   	struct irq_domain	*msi_domain;
> +	u16			msi_msg;
>   	dma_addr_t		msi_data;
> -	struct page		*msi_page;
>   	struct irq_chip		*msi_irq_chip;
>   	u32			num_vectors;
>   	u32			irq_mask[MAX_MSI_CTRLS];
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Vidya Sagar]

On 10/12/2020 5:07 PM, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2020-10-09 08:55, Jisheng Zhang 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
>> msi page will be leaked.
>>
>> As pointed out by Rob and Ard, there's no need to allocate a page for
>> the MSI address, we could use an address in the driver data.
>>
>> To avoid map the MSI msg again during resume, we move the map MSI msg
>> from dw_pcie_msi_init() to dw_pcie_host_init().
> 
> You should move the unmap there as well. As soon as you know what the
> relevant address would be if you *were* to do DMA to this location, then
> the exercise is complete. Leaving it mapped for the lifetime of the
> device in order to do not-DMA to it seems questionable (and represents
> technically incorrect API usage without at least a sync_for_cpu call
> before any other access to the data).
> 
> Another point of note is that using streaming DMA mappings at all is a
> bit fragile (regardless of this change). If the host controller itself
> has a limited DMA mask relative to physical memory (which integrators
> still seem to keep doing...) then you could end up punching your MSI
> hole right in the middle of the SWIOTLB bounce buffer, where it's then
> almost *guaranteed* to interfere with real DMA :(
Agree with Robin. Since the MSI page is going to be locked till 
shutdown/reboot, wouldn't it make sense to use dma_alloc_coherent() API?
Also, shouldn't we call dma_set_mask() to limit the address to only 
32-bits so as to enable MSI for even those legacy PCIe devices with only 
32-bit MSI capability?

- Vidya Sagar

> 
> If no DWC users have that problem and the current code is working well
> enough, then I see little reason not to make this partucular change to
> tidy up the implementation, just bear in mind that there's always the
> possibility of having to come back and change it yet again in future to
> make it more robust. I had it in mind that this trick was done with a
> coherent DMA allocation, which would be safe from addressing problems
> but would need to be kept around for the lifetime of the device, but
> maybe that was a different driver :/
> 
> Robin.
> 
>> Suggested-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>>   drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
>>   .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
>>   drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>>   3 files changed, 36 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c 
>> b/drivers/pci/controller/dwc/pci-dra7xx.c
>> index 8f0b6d644e4b..6d012d2b1e90 100644
>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
>> @@ -466,7 +466,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;
>>
>> @@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
>> +                                        sizeof(pp->msi_msg),
>> +                                        DMA_FROM_DEVICE,
>> +                                        DMA_ATTR_SKIP_CPU_SYNC);
>> +     ret = dma_mapping_error(dev, pp->msi_data);
>> +     if (ret) {
>> +             dev_err(dev, "Failed to map MSI data\n");
>> +             pp->msi_data = 0;
>> +             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 d3e9ea11ce9e..d02c7e74738d 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -266,30 +266,23 @@ 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);
>> +     if (pp->msi_data) {
>> +             struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +             struct device *dev = pci->dev;
>> +
>> +             dma_unmap_single_attrs(dev, pp->msi_data, 
>> sizeof(pp->msi_msg),
>> +                                    DMA_FROM_DEVICE, 
>> DMA_ATTR_SKIP_CPU_SYNC);
>> +     }
>>   }
>>
>>   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;
>> +     u64 msi_target = (u64)pp->msi_data;
>>
>>       if (!IS_ENABLED(CONFIG_PCI_MSI))
>>               return;
>>
>> -     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;
>> -
>>       /* Program the msi_data */
>>       dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, 
>> lower_32_bits(msi_target));
>>       dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, 
>> upper_32_bits(msi_target));
>> @@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, 
>> &pp->msi_msg,
>> +                                                   sizeof(pp->msi_msg),
>> +                                                   DMA_FROM_DEVICE,
>> +                                                   
>> DMA_ATTR_SKIP_CPU_SYNC);
>> +                     if (dma_mapping_error(pci->dev, pp->msi_data)) {
>> +                             dev_err(pci->dev, "Failed to map MSI 
>> data\n");
>> +                             pp->msi_data = 0;
>> +                             goto err_free_msi;
>> +                     }
>>               } else {
>>                       ret = pp->ops->msi_host_init(pp);
>>                       if (ret < 0)
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h 
>> b/drivers/pci/controller/dwc/pcie-designware.h
>> index 97c7063b9e89..9d2f511f13fa 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -190,8 +190,8 @@ struct pcie_port {
>>       int                     msi_irq;
>>       struct irq_domain       *irq_domain;
>>       struct irq_domain       *msi_domain;
>> +     u16                     msi_msg;
>>       dma_addr_t              msi_data;
>> -     struct page             *msi_page;
>>       struct irq_chip         *msi_irq_chip;
>>       u32                     num_vectors;
>>       u32                     irq_mask[MAX_MSI_CTRLS];
>>

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Vidya Sagar]

On 10/12/2020 5:07 PM, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2020-10-09 08:55, Jisheng Zhang 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
>> msi page will be leaked.
>>
>> As pointed out by Rob and Ard, there's no need to allocate a page for
>> the MSI address, we could use an address in the driver data.
>>
>> To avoid map the MSI msg again during resume, we move the map MSI msg
>> from dw_pcie_msi_init() to dw_pcie_host_init().
> 
> You should move the unmap there as well. As soon as you know what the
> relevant address would be if you *were* to do DMA to this location, then
> the exercise is complete. Leaving it mapped for the lifetime of the
> device in order to do not-DMA to it seems questionable (and represents
> technically incorrect API usage without at least a sync_for_cpu call
> before any other access to the data).
> 
> Another point of note is that using streaming DMA mappings at all is a
> bit fragile (regardless of this change). If the host controller itself
> has a limited DMA mask relative to physical memory (which integrators
> still seem to keep doing...) then you could end up punching your MSI
> hole right in the middle of the SWIOTLB bounce buffer, where it's then
> almost *guaranteed* to interfere with real DMA :(
Agree with Robin. Since the MSI page is going to be locked till 
shutdown/reboot, wouldn't it make sense to use dma_alloc_coherent() API?
Also, shouldn't we call dma_set_mask() to limit the address to only 
32-bits so as to enable MSI for even those legacy PCIe devices with only 
32-bit MSI capability?

- Vidya Sagar

> 
> If no DWC users have that problem and the current code is working well
> enough, then I see little reason not to make this partucular change to
> tidy up the implementation, just bear in mind that there's always the
> possibility of having to come back and change it yet again in future to
> make it more robust. I had it in mind that this trick was done with a
> coherent DMA allocation, which would be safe from addressing problems
> but would need to be kept around for the lifetime of the device, but
> maybe that was a different driver :/
> 
> Robin.
> 
>> Suggested-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>>   drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
>>   .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
>>   drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>>   3 files changed, 36 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c 
>> b/drivers/pci/controller/dwc/pci-dra7xx.c
>> index 8f0b6d644e4b..6d012d2b1e90 100644
>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
>> @@ -466,7 +466,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;
>>
>> @@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
>> +                                        sizeof(pp->msi_msg),
>> +                                        DMA_FROM_DEVICE,
>> +                                        DMA_ATTR_SKIP_CPU_SYNC);
>> +     ret = dma_mapping_error(dev, pp->msi_data);
>> +     if (ret) {
>> +             dev_err(dev, "Failed to map MSI data\n");
>> +             pp->msi_data = 0;
>> +             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 d3e9ea11ce9e..d02c7e74738d 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -266,30 +266,23 @@ 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);
>> +     if (pp->msi_data) {
>> +             struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +             struct device *dev = pci->dev;
>> +
>> +             dma_unmap_single_attrs(dev, pp->msi_data, 
>> sizeof(pp->msi_msg),
>> +                                    DMA_FROM_DEVICE, 
>> DMA_ATTR_SKIP_CPU_SYNC);
>> +     }
>>   }
>>
>>   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;
>> +     u64 msi_target = (u64)pp->msi_data;
>>
>>       if (!IS_ENABLED(CONFIG_PCI_MSI))
>>               return;
>>
>> -     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;
>> -
>>       /* Program the msi_data */
>>       dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, 
>> lower_32_bits(msi_target));
>>       dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, 
>> upper_32_bits(msi_target));
>> @@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, 
>> &pp->msi_msg,
>> +                                                   sizeof(pp->msi_msg),
>> +                                                   DMA_FROM_DEVICE,
>> +                                                   
>> DMA_ATTR_SKIP_CPU_SYNC);
>> +                     if (dma_mapping_error(pci->dev, pp->msi_data)) {
>> +                             dev_err(pci->dev, "Failed to map MSI 
>> data\n");
>> +                             pp->msi_data = 0;
>> +                             goto err_free_msi;
>> +                     }
>>               } else {
>>                       ret = pp->ops->msi_host_init(pp);
>>                       if (ret < 0)
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h 
>> b/drivers/pci/controller/dwc/pcie-designware.h
>> index 97c7063b9e89..9d2f511f13fa 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -190,8 +190,8 @@ struct pcie_port {
>>       int                     msi_irq;
>>       struct irq_domain       *irq_domain;
>>       struct irq_domain       *msi_domain;
>> +     u16                     msi_msg;
>>       dma_addr_t              msi_data;
>> -     struct page             *msi_page;
>>       struct irq_chip         *msi_irq_chip;
>>       u32                     num_vectors;
>>       u32                     irq_mask[MAX_MSI_CTRLS];
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Rob Herring]

On Mon, Oct 12, 2020 at 6:37 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-10-09 08:55, Jisheng Zhang 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
> > msi page will be leaked.
> >
> > As pointed out by Rob and Ard, there's no need to allocate a page for
> > the MSI address, we could use an address in the driver data.
> >
> > To avoid map the MSI msg again during resume, we move the map MSI msg
> > from dw_pcie_msi_init() to dw_pcie_host_init().
>
> You should move the unmap there as well. As soon as you know what the
> relevant address would be if you *were* to do DMA to this location, then
> the exercise is complete. Leaving it mapped for the lifetime of the
> device in order to do not-DMA to it seems questionable (and represents
> technically incorrect API usage without at least a sync_for_cpu call
> before any other access to the data).
>
> Another point of note is that using streaming DMA mappings at all is a
> bit fragile (regardless of this change). If the host controller itself
> has a limited DMA mask relative to physical memory (which integrators
> still seem to keep doing...) then you could end up punching your MSI
> hole right in the middle of the SWIOTLB bounce buffer, where it's then
> almost *guaranteed* to interfere with real DMA :(

Couldn't that happen with the current code too? alloc_page() isn't
guaranteed to be DMA'able, right?

> If no DWC users have that problem and the current code is working well
> enough, then I see little reason not to make this partucular change to
> tidy up the implementation, just bear in mind that there's always the
> possibility of having to come back and change it yet again in future to
> make it more robust. I had it in mind that this trick was done with a
> coherent DMA allocation, which would be safe from addressing problems
> but would need to be kept around for the lifetime of the device, but
> maybe that was a different driver :/

Well, we're wasting 4K or 64K of memory and then leaking it is the
main reason to change it.

We just need any address that's not memory which PCI could access. We
could possibly just take the end of (outbound) PCI memory space. Note
that the DWC driver never sets up inbound translations, so it's all
1:1 mapping (though upstream could have some translation).

Rob

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Rob Herring]

On Mon, Oct 12, 2020 at 6:37 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-10-09 08:55, Jisheng Zhang 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
> > msi page will be leaked.
> >
> > As pointed out by Rob and Ard, there's no need to allocate a page for
> > the MSI address, we could use an address in the driver data.
> >
> > To avoid map the MSI msg again during resume, we move the map MSI msg
> > from dw_pcie_msi_init() to dw_pcie_host_init().
>
> You should move the unmap there as well. As soon as you know what the
> relevant address would be if you *were* to do DMA to this location, then
> the exercise is complete. Leaving it mapped for the lifetime of the
> device in order to do not-DMA to it seems questionable (and represents
> technically incorrect API usage without at least a sync_for_cpu call
> before any other access to the data).
>
> Another point of note is that using streaming DMA mappings at all is a
> bit fragile (regardless of this change). If the host controller itself
> has a limited DMA mask relative to physical memory (which integrators
> still seem to keep doing...) then you could end up punching your MSI
> hole right in the middle of the SWIOTLB bounce buffer, where it's then
> almost *guaranteed* to interfere with real DMA :(

Couldn't that happen with the current code too? alloc_page() isn't
guaranteed to be DMA'able, right?

> If no DWC users have that problem and the current code is working well
> enough, then I see little reason not to make this partucular change to
> tidy up the implementation, just bear in mind that there's always the
> possibility of having to come back and change it yet again in future to
> make it more robust. I had it in mind that this trick was done with a
> coherent DMA allocation, which would be safe from addressing problems
> but would need to be kept around for the lifetime of the device, but
> maybe that was a different driver :/

Well, we're wasting 4K or 64K of memory and then leaking it is the
main reason to change it.

We just need any address that's not memory which PCI could access. We
could possibly just take the end of (outbound) PCI memory space. Note
that the DWC driver never sets up inbound translations, so it's all
1:1 mapping (though upstream could have some translation).

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Robin Murphy]

On 2020-10-14 15:15, Rob Herring wrote:
> On Mon, Oct 12, 2020 at 6:37 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-10-09 08:55, Jisheng Zhang 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
>>> msi page will be leaked.
>>>
>>> As pointed out by Rob and Ard, there's no need to allocate a page for
>>> the MSI address, we could use an address in the driver data.
>>>
>>> To avoid map the MSI msg again during resume, we move the map MSI msg
>>> from dw_pcie_msi_init() to dw_pcie_host_init().
>>
>> You should move the unmap there as well. As soon as you know what the
>> relevant address would be if you *were* to do DMA to this location, then
>> the exercise is complete. Leaving it mapped for the lifetime of the
>> device in order to do not-DMA to it seems questionable (and represents
>> technically incorrect API usage without at least a sync_for_cpu call
>> before any other access to the data).
>>
>> Another point of note is that using streaming DMA mappings at all is a
>> bit fragile (regardless of this change). If the host controller itself
>> has a limited DMA mask relative to physical memory (which integrators
>> still seem to keep doing...) then you could end up punching your MSI
>> hole right in the middle of the SWIOTLB bounce buffer, where it's then
>> almost *guaranteed* to interfere with real DMA :(
> 
> Couldn't that happen with the current code too? alloc_page() isn't
> guaranteed to be DMA'able, right?

Indeed that's what I meant by "regardless of this change".

>> If no DWC users have that problem and the current code is working well
>> enough, then I see little reason not to make this partucular change to
>> tidy up the implementation, just bear in mind that there's always the
>> possibility of having to come back and change it yet again in future to
>> make it more robust. I had it in mind that this trick was done with a
>> coherent DMA allocation, which would be safe from addressing problems
>> but would need to be kept around for the lifetime of the device, but
>> maybe that was a different driver :/
> 
> Well, we're wasting 4K or 64K of memory and then leaking it is the
> main reason to change it.
> 
> We just need any address that's not memory which PCI could access. We
> could possibly just take the end of (outbound) PCI memory space. Note
> that the DWC driver never sets up inbound translations, so it's all
> 1:1 mapping (though upstream could have some translation).

Right, this patch is undeniably a better implementation of the existing 
approach, I just felt it worth pointing out that that approach itself 
has fundamental flaws which may or may not be relevant to some current 
and/or future users. I know for a fact that there are platforms which 
cripple their PCIe host bridge to 32-bit physical addressing but support 
having RAM above that; I don't *think* any of the ones I know of are 
using the dw_pcie driver, but hey, how much do I know? ;)

Robin.

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Robin Murphy]

On 2020-10-14 15:15, Rob Herring wrote:
> On Mon, Oct 12, 2020 at 6:37 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-10-09 08:55, Jisheng Zhang 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
>>> msi page will be leaked.
>>>
>>> As pointed out by Rob and Ard, there's no need to allocate a page for
>>> the MSI address, we could use an address in the driver data.
>>>
>>> To avoid map the MSI msg again during resume, we move the map MSI msg
>>> from dw_pcie_msi_init() to dw_pcie_host_init().
>>
>> You should move the unmap there as well. As soon as you know what the
>> relevant address would be if you *were* to do DMA to this location, then
>> the exercise is complete. Leaving it mapped for the lifetime of the
>> device in order to do not-DMA to it seems questionable (and represents
>> technically incorrect API usage without at least a sync_for_cpu call
>> before any other access to the data).
>>
>> Another point of note is that using streaming DMA mappings at all is a
>> bit fragile (regardless of this change). If the host controller itself
>> has a limited DMA mask relative to physical memory (which integrators
>> still seem to keep doing...) then you could end up punching your MSI
>> hole right in the middle of the SWIOTLB bounce buffer, where it's then
>> almost *guaranteed* to interfere with real DMA :(
> 
> Couldn't that happen with the current code too? alloc_page() isn't
> guaranteed to be DMA'able, right?

Indeed that's what I meant by "regardless of this change".

>> If no DWC users have that problem and the current code is working well
>> enough, then I see little reason not to make this partucular change to
>> tidy up the implementation, just bear in mind that there's always the
>> possibility of having to come back and change it yet again in future to
>> make it more robust. I had it in mind that this trick was done with a
>> coherent DMA allocation, which would be safe from addressing problems
>> but would need to be kept around for the lifetime of the device, but
>> maybe that was a different driver :/
> 
> Well, we're wasting 4K or 64K of memory and then leaking it is the
> main reason to change it.
> 
> We just need any address that's not memory which PCI could access. We
> could possibly just take the end of (outbound) PCI memory space. Note
> that the DWC driver never sets up inbound translations, so it's all
> 1:1 mapping (though upstream could have some translation).

Right, this patch is undeniably a better implementation of the existing 
approach, I just felt it worth pointing out that that approach itself 
has fundamental flaws which may or may not be relevant to some current 
and/or future users. I know for a fact that there are platforms which 
cripple their PCIe host bridge to 32-bit physical addressing but support 
having RAM above that; I don't *think* any of the ones I know of are 
using the dw_pcie driver, but hey, how much do I know? ;)

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Ard Biesheuvel]

On Mon, 12 Oct 2020 at 13:38, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-10-09 08:55, Jisheng Zhang 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
> > msi page will be leaked.
> >
> > As pointed out by Rob and Ard, there's no need to allocate a page for
> > the MSI address, we could use an address in the driver data.
> >
> > To avoid map the MSI msg again during resume, we move the map MSI msg
> > from dw_pcie_msi_init() to dw_pcie_host_init().
>
> You should move the unmap there as well. As soon as you know what the
> relevant address would be if you *were* to do DMA to this location, then
> the exercise is complete. Leaving it mapped for the lifetime of the
> device in order to do not-DMA to it seems questionable (and represents
> technically incorrect API usage without at least a sync_for_cpu call
> before any other access to the data).
>
> Another point of note is that using streaming DMA mappings at all is a
> bit fragile (regardless of this change). If the host controller itself
> has a limited DMA mask relative to physical memory (which integrators
> still seem to keep doing...) then you could end up punching your MSI
> hole right in the middle of the SWIOTLB bounce buffer, where it's then
> almost *guaranteed* to interfere with real DMA :(
>

Wouldn't it be the unmap you are suggesting that would create this
problem? If the bounce buffer is never released, the fake MSI doorbell
address can never conflict with any other DMA mappings.


> If no DWC users have that problem and the current code is working well
> enough, then I see little reason not to make this partucular change to
> tidy up the implementation, just bear in mind that there's always the
> possibility of having to come back and change it yet again in future to
> make it more robust. I had it in mind that this trick was done with a
> coherent DMA allocation, which would be safe from addressing problems
> but would need to be kept around for the lifetime of the device, but
> maybe that was a different driver :/
>
> Robin.
>
> > Suggested-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >   drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
> >   .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
> >   drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
> >   3 files changed, 36 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> > index 8f0b6d644e4b..6d012d2b1e90 100644
> > --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> > @@ -466,7 +466,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;
> >
> > @@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
> > +                                        sizeof(pp->msi_msg),
> > +                                        DMA_FROM_DEVICE,
> > +                                        DMA_ATTR_SKIP_CPU_SYNC);
> > +     ret = dma_mapping_error(dev, pp->msi_data);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to map MSI data\n");
> > +             pp->msi_data = 0;
> > +             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 d3e9ea11ce9e..d02c7e74738d 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -266,30 +266,23 @@ 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);
> > +     if (pp->msi_data) {
> > +             struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +             struct device *dev = pci->dev;
> > +
> > +             dma_unmap_single_attrs(dev, pp->msi_data, sizeof(pp->msi_msg),
> > +                                    DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> > +     }
> >   }
> >
> >   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;
> > +     u64 msi_target = (u64)pp->msi_data;
> >
> >       if (!IS_ENABLED(CONFIG_PCI_MSI))
> >               return;
> >
> > -     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;
> > -
> >       /* Program the msi_data */
> >       dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
> >       dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
> > @@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
> > +                                                   sizeof(pp->msi_msg),
> > +                                                   DMA_FROM_DEVICE,
> > +                                                   DMA_ATTR_SKIP_CPU_SYNC);
> > +                     if (dma_mapping_error(pci->dev, pp->msi_data)) {
> > +                             dev_err(pci->dev, "Failed to map MSI data\n");
> > +                             pp->msi_data = 0;
> > +                             goto err_free_msi;
> > +                     }
> >               } else {
> >                       ret = pp->ops->msi_host_init(pp);
> >                       if (ret < 0)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 97c7063b9e89..9d2f511f13fa 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -190,8 +190,8 @@ struct pcie_port {
> >       int                     msi_irq;
> >       struct irq_domain       *irq_domain;
> >       struct irq_domain       *msi_domain;
> > +     u16                     msi_msg;
> >       dma_addr_t              msi_data;
> > -     struct page             *msi_page;
> >       struct irq_chip         *msi_irq_chip;
> >       u32                     num_vectors;
> >       u32                     irq_mask[MAX_MSI_CTRLS];
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Ard Biesheuvel]

On Mon, 12 Oct 2020 at 13:38, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-10-09 08:55, Jisheng Zhang 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
> > msi page will be leaked.
> >
> > As pointed out by Rob and Ard, there's no need to allocate a page for
> > the MSI address, we could use an address in the driver data.
> >
> > To avoid map the MSI msg again during resume, we move the map MSI msg
> > from dw_pcie_msi_init() to dw_pcie_host_init().
>
> You should move the unmap there as well. As soon as you know what the
> relevant address would be if you *were* to do DMA to this location, then
> the exercise is complete. Leaving it mapped for the lifetime of the
> device in order to do not-DMA to it seems questionable (and represents
> technically incorrect API usage without at least a sync_for_cpu call
> before any other access to the data).
>
> Another point of note is that using streaming DMA mappings at all is a
> bit fragile (regardless of this change). If the host controller itself
> has a limited DMA mask relative to physical memory (which integrators
> still seem to keep doing...) then you could end up punching your MSI
> hole right in the middle of the SWIOTLB bounce buffer, where it's then
> almost *guaranteed* to interfere with real DMA :(
>

Wouldn't it be the unmap you are suggesting that would create this
problem? If the bounce buffer is never released, the fake MSI doorbell
address can never conflict with any other DMA mappings.


> If no DWC users have that problem and the current code is working well
> enough, then I see little reason not to make this partucular change to
> tidy up the implementation, just bear in mind that there's always the
> possibility of having to come back and change it yet again in future to
> make it more robust. I had it in mind that this trick was done with a
> coherent DMA allocation, which would be safe from addressing problems
> but would need to be kept around for the lifetime of the device, but
> maybe that was a different driver :/
>
> Robin.
>
> > Suggested-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >   drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
> >   .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
> >   drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
> >   3 files changed, 36 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> > index 8f0b6d644e4b..6d012d2b1e90 100644
> > --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> > @@ -466,7 +466,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;
> >
> > @@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
> > +                                        sizeof(pp->msi_msg),
> > +                                        DMA_FROM_DEVICE,
> > +                                        DMA_ATTR_SKIP_CPU_SYNC);
> > +     ret = dma_mapping_error(dev, pp->msi_data);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to map MSI data\n");
> > +             pp->msi_data = 0;
> > +             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 d3e9ea11ce9e..d02c7e74738d 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -266,30 +266,23 @@ 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);
> > +     if (pp->msi_data) {
> > +             struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +             struct device *dev = pci->dev;
> > +
> > +             dma_unmap_single_attrs(dev, pp->msi_data, sizeof(pp->msi_msg),
> > +                                    DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> > +     }
> >   }
> >
> >   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;
> > +     u64 msi_target = (u64)pp->msi_data;
> >
> >       if (!IS_ENABLED(CONFIG_PCI_MSI))
> >               return;
> >
> > -     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;
> > -
> >       /* Program the msi_data */
> >       dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
> >       dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
> > @@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
> > +                                                   sizeof(pp->msi_msg),
> > +                                                   DMA_FROM_DEVICE,
> > +                                                   DMA_ATTR_SKIP_CPU_SYNC);
> > +                     if (dma_mapping_error(pci->dev, pp->msi_data)) {
> > +                             dev_err(pci->dev, "Failed to map MSI data\n");
> > +                             pp->msi_data = 0;
> > +                             goto err_free_msi;
> > +                     }
> >               } else {
> >                       ret = pp->ops->msi_host_init(pp);
> >                       if (ret < 0)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 97c7063b9e89..9d2f511f13fa 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -190,8 +190,8 @@ struct pcie_port {
> >       int                     msi_irq;
> >       struct irq_domain       *irq_domain;
> >       struct irq_domain       *msi_domain;
> > +     u16                     msi_msg;
> >       dma_addr_t              msi_data;
> > -     struct page             *msi_page;
> >       struct irq_chip         *msi_irq_chip;
> >       u32                     num_vectors;
> >       u32                     irq_mask[MAX_MSI_CTRLS];
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Robin Murphy]

On 2020-10-14 17:52, Ard Biesheuvel wrote:
> On Mon, 12 Oct 2020 at 13:38, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-10-09 08:55, Jisheng Zhang 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
>>> msi page will be leaked.
>>>
>>> As pointed out by Rob and Ard, there's no need to allocate a page for
>>> the MSI address, we could use an address in the driver data.
>>>
>>> To avoid map the MSI msg again during resume, we move the map MSI msg
>>> from dw_pcie_msi_init() to dw_pcie_host_init().
>>
>> You should move the unmap there as well. As soon as you know what the
>> relevant address would be if you *were* to do DMA to this location, then
>> the exercise is complete. Leaving it mapped for the lifetime of the
>> device in order to do not-DMA to it seems questionable (and represents
>> technically incorrect API usage without at least a sync_for_cpu call
>> before any other access to the data).
>>
>> Another point of note is that using streaming DMA mappings at all is a
>> bit fragile (regardless of this change). If the host controller itself
>> has a limited DMA mask relative to physical memory (which integrators
>> still seem to keep doing...) then you could end up punching your MSI
>> hole right in the middle of the SWIOTLB bounce buffer, where it's then
>> almost *guaranteed* to interfere with real DMA :(
>>
> 
> Wouldn't it be the unmap you are suggesting that would create this
> problem? If the bounce buffer is never released, the fake MSI doorbell
> address can never conflict with any other DMA mappings.

True, it's just that hogging a SWIOTLB slot for effectively the entire 
lifetime of the system is such an anathema that it didn't even cross my 
mind...

Ugh, so I guess the patch as it stands does actually work out more 
robust than I gave it credit for, even if the reason is horrid... oh well :)

Robin.

>> If no DWC users have that problem and the current code is working well
>> enough, then I see little reason not to make this partucular change to
>> tidy up the implementation, just bear in mind that there's always the
>> possibility of having to come back and change it yet again in future to
>> make it more robust. I had it in mind that this trick was done with a
>> coherent DMA allocation, which would be safe from addressing problems
>> but would need to be kept around for the lifetime of the device, but
>> maybe that was a different driver :/
>>
>> Robin.
>>
>>> Suggested-by: Rob Herring <robh@kernel.org>
>>> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>> ---
>>>    drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
>>>    .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
>>>    drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>>>    3 files changed, 36 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
>>> index 8f0b6d644e4b..6d012d2b1e90 100644
>>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
>>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
>>> @@ -466,7 +466,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;
>>>
>>> @@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
>>> +                                        sizeof(pp->msi_msg),
>>> +                                        DMA_FROM_DEVICE,
>>> +                                        DMA_ATTR_SKIP_CPU_SYNC);
>>> +     ret = dma_mapping_error(dev, pp->msi_data);
>>> +     if (ret) {
>>> +             dev_err(dev, "Failed to map MSI data\n");
>>> +             pp->msi_data = 0;
>>> +             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 d3e9ea11ce9e..d02c7e74738d 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> @@ -266,30 +266,23 @@ 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);
>>> +     if (pp->msi_data) {
>>> +             struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +             struct device *dev = pci->dev;
>>> +
>>> +             dma_unmap_single_attrs(dev, pp->msi_data, sizeof(pp->msi_msg),
>>> +                                    DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
>>> +     }
>>>    }
>>>
>>>    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;
>>> +     u64 msi_target = (u64)pp->msi_data;
>>>
>>>        if (!IS_ENABLED(CONFIG_PCI_MSI))
>>>                return;
>>>
>>> -     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;
>>> -
>>>        /* Program the msi_data */
>>>        dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
>>>        dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
>>> @@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
>>> +                                                   sizeof(pp->msi_msg),
>>> +                                                   DMA_FROM_DEVICE,
>>> +                                                   DMA_ATTR_SKIP_CPU_SYNC);
>>> +                     if (dma_mapping_error(pci->dev, pp->msi_data)) {
>>> +                             dev_err(pci->dev, "Failed to map MSI data\n");
>>> +                             pp->msi_data = 0;
>>> +                             goto err_free_msi;
>>> +                     }
>>>                } else {
>>>                        ret = pp->ops->msi_host_init(pp);
>>>                        if (ret < 0)
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>>> index 97c7063b9e89..9d2f511f13fa 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>> @@ -190,8 +190,8 @@ struct pcie_port {
>>>        int                     msi_irq;
>>>        struct irq_domain       *irq_domain;
>>>        struct irq_domain       *msi_domain;
>>> +     u16                     msi_msg;
>>>        dma_addr_t              msi_data;
>>> -     struct page             *msi_page;
>>>        struct irq_chip         *msi_irq_chip;
>>>        u32                     num_vectors;
>>>        u32                     irq_mask[MAX_MSI_CTRLS];
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume

[Robin Murphy]

On 2020-10-14 17:52, Ard Biesheuvel wrote:
> On Mon, 12 Oct 2020 at 13:38, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-10-09 08:55, Jisheng Zhang 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
>>> msi page will be leaked.
>>>
>>> As pointed out by Rob and Ard, there's no need to allocate a page for
>>> the MSI address, we could use an address in the driver data.
>>>
>>> To avoid map the MSI msg again during resume, we move the map MSI msg
>>> from dw_pcie_msi_init() to dw_pcie_host_init().
>>
>> You should move the unmap there as well. As soon as you know what the
>> relevant address would be if you *were* to do DMA to this location, then
>> the exercise is complete. Leaving it mapped for the lifetime of the
>> device in order to do not-DMA to it seems questionable (and represents
>> technically incorrect API usage without at least a sync_for_cpu call
>> before any other access to the data).
>>
>> Another point of note is that using streaming DMA mappings at all is a
>> bit fragile (regardless of this change). If the host controller itself
>> has a limited DMA mask relative to physical memory (which integrators
>> still seem to keep doing...) then you could end up punching your MSI
>> hole right in the middle of the SWIOTLB bounce buffer, where it's then
>> almost *guaranteed* to interfere with real DMA :(
>>
> 
> Wouldn't it be the unmap you are suggesting that would create this
> problem? If the bounce buffer is never released, the fake MSI doorbell
> address can never conflict with any other DMA mappings.

True, it's just that hogging a SWIOTLB slot for effectively the entire 
lifetime of the system is such an anathema that it didn't even cross my 
mind...

Ugh, so I guess the patch as it stands does actually work out more 
robust than I gave it credit for, even if the reason is horrid... oh well :)

Robin.

>> If no DWC users have that problem and the current code is working well
>> enough, then I see little reason not to make this partucular change to
>> tidy up the implementation, just bear in mind that there's always the
>> possibility of having to come back and change it yet again in future to
>> make it more robust. I had it in mind that this trick was done with a
>> coherent DMA allocation, which would be safe from addressing problems
>> but would need to be kept around for the lifetime of the device, but
>> maybe that was a different driver :/
>>
>> Robin.
>>
>>> Suggested-by: Rob Herring <robh@kernel.org>
>>> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>> ---
>>>    drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
>>>    .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++---------
>>>    drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>>>    3 files changed, 36 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
>>> index 8f0b6d644e4b..6d012d2b1e90 100644
>>> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
>>> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
>>> @@ -466,7 +466,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;
>>>
>>> @@ -482,7 +484,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_data = dma_map_single_attrs(dev, &pp->msi_msg,
>>> +                                        sizeof(pp->msi_msg),
>>> +                                        DMA_FROM_DEVICE,
>>> +                                        DMA_ATTR_SKIP_CPU_SYNC);
>>> +     ret = dma_mapping_error(dev, pp->msi_data);
>>> +     if (ret) {
>>> +             dev_err(dev, "Failed to map MSI data\n");
>>> +             pp->msi_data = 0;
>>> +             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 d3e9ea11ce9e..d02c7e74738d 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> @@ -266,30 +266,23 @@ 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);
>>> +     if (pp->msi_data) {
>>> +             struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +             struct device *dev = pci->dev;
>>> +
>>> +             dma_unmap_single_attrs(dev, pp->msi_data, sizeof(pp->msi_msg),
>>> +                                    DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
>>> +     }
>>>    }
>>>
>>>    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;
>>> +     u64 msi_target = (u64)pp->msi_data;
>>>
>>>        if (!IS_ENABLED(CONFIG_PCI_MSI))
>>>                return;
>>>
>>> -     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;
>>> -
>>>        /* Program the msi_data */
>>>        dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
>>>        dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
>>> @@ -394,6 +387,16 @@ 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_data = dma_map_single_attrs(pci->dev, &pp->msi_msg,
>>> +                                                   sizeof(pp->msi_msg),
>>> +                                                   DMA_FROM_DEVICE,
>>> +                                                   DMA_ATTR_SKIP_CPU_SYNC);
>>> +                     if (dma_mapping_error(pci->dev, pp->msi_data)) {
>>> +                             dev_err(pci->dev, "Failed to map MSI data\n");
>>> +                             pp->msi_data = 0;
>>> +                             goto err_free_msi;
>>> +                     }
>>>                } else {
>>>                        ret = pp->ops->msi_host_init(pp);
>>>                        if (ret < 0)
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>>> index 97c7063b9e89..9d2f511f13fa 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>> @@ -190,8 +190,8 @@ struct pcie_port {
>>>        int                     msi_irq;
>>>        struct irq_domain       *irq_domain;
>>>        struct irq_domain       *msi_domain;
>>> +     u16                     msi_msg;
>>>        dma_addr_t              msi_data;
>>> -     struct page             *msi_page;
>>>        struct irq_chip         *msi_irq_chip;
>>>        u32                     num_vectors;
>>>        u32                     irq_mask[MAX_MSI_CTRLS];
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 0/2] PCI: dwc: fix two MSI issues

[Lorenzo Pieralisi]

On Fri, Oct 09, 2020 at 03:53:11PM +0800, Jisheng Zhang wrote:
> Fix two MSI issues. One to skip PCIE_MSI_INTR0* programming if MSI is
> disabled, another to use an address in the driver data for MSI address,
> to fix the MSI page leakage during suspend/resume.
> 
> Since v6:
>   - Keep the IS_ENABLED(CONFIG_PCI_MSI) check in dw_pcie_msi_init().
> 
> Since v5:
>   - rebase on pci/dwc branch
>   - add Acked-by tag
> 
> Since v4:
>   - fix pci-dra7xx.c
> 
> Since v3:
>   - add Acked-by tag
>   - change patch2 commit msg to make it clear
>   - map the MSI msg with dma_map_single_attrs() for some platforms
>     which either has separate addrs for dma and phy or has mem access
>     limitation for the PCIe.
> 
> 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: Fix MSI page leakage in suspend/resume
> 
>  drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
>  .../pci/controller/dwc/pcie-designware-host.c | 35 ++++++++++---------
>  drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>  3 files changed, 37 insertions(+), 18 deletions(-)

Applied to pci/dwc, thanks.

Lorenzo

Re: [PATCH v7 0/2] PCI: dwc: fix two MSI issues

[Lorenzo Pieralisi]

On Fri, Oct 09, 2020 at 03:53:11PM +0800, Jisheng Zhang wrote:
> Fix two MSI issues. One to skip PCIE_MSI_INTR0* programming if MSI is
> disabled, another to use an address in the driver data for MSI address,
> to fix the MSI page leakage during suspend/resume.
> 
> Since v6:
>   - Keep the IS_ENABLED(CONFIG_PCI_MSI) check in dw_pcie_msi_init().
> 
> Since v5:
>   - rebase on pci/dwc branch
>   - add Acked-by tag
> 
> Since v4:
>   - fix pci-dra7xx.c
> 
> Since v3:
>   - add Acked-by tag
>   - change patch2 commit msg to make it clear
>   - map the MSI msg with dma_map_single_attrs() for some platforms
>     which either has separate addrs for dma and phy or has mem access
>     limitation for the PCIe.
> 
> 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: Fix MSI page leakage in suspend/resume
> 
>  drivers/pci/controller/dwc/pci-dra7xx.c       | 18 +++++++++-
>  .../pci/controller/dwc/pcie-designware-host.c | 35 ++++++++++---------
>  drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>  3 files changed, 37 insertions(+), 18 deletions(-)

Applied to pci/dwc, thanks.

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel