[PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[Vidya Sagar]

Since the upstream MSI memory writes are generated by downstream devices,
it is logically correct to have MSI target memory coming from the DMA pool
reserved for PCIe than from the general memory pool reserved for CPU
access. This avoids PCIe DMA addresses coinciding with MSI target address
thereby raising unwanted MSI interrupts. This patch also enforces to limit
the MSI target address to 32-bits to make it work for PCIe endponits that
support only 32-bit MSI target address and those endpoints that support
64-bit MSI target address anyway work with 32-bit MSI target address.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
Reviewed-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
v2:
* changed 'phys' type to 'dma_addr_t' from 'u64'
* added a comment on why DMA mask is set to 32-bit
* replaced 'dma' with 'DMA'

 drivers/pci/controller/pci-tegra.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index f4f53d092e00..f8173a5e352d 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -231,9 +231,9 @@ struct tegra_msi {
 	struct msi_controller chip;
 	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
 	struct irq_domain *domain;
-	unsigned long pages;
 	struct mutex lock;
-	u64 phys;
+	void *virt;
+	dma_addr_t phys;
 	int irq;
 };
 
@@ -1536,7 +1536,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
 	err = platform_get_irq_byname(pdev, "msi");
 	if (err < 0) {
 		dev_err(dev, "failed to get IRQ: %d\n", err);
-		goto err;
+		goto free_irq_domain;
 	}
 
 	msi->irq = err;
@@ -1545,17 +1545,34 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
 			  tegra_msi_irq_chip.name, pcie);
 	if (err < 0) {
 		dev_err(dev, "failed to request IRQ: %d\n", err);
-		goto err;
+		goto free_irq_domain;
+	}
+
+	/* Though the PCIe controller can address >32-bit address space, to
+	 * facilitate endpoints that support only 32-bit MSI target address,
+	 * the mask is set to 32-bit to make sure that MSI target address is
+	 * always a 32-bit address
+	 */
+	err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
+	if (err < 0) {
+		dev_err(dev, "failed to set DMA coherent mask: %d\n", err);
+		goto free_irq;
+	}
+
+	msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
+	if (!msi->virt) {
+		dev_err(dev, "failed to allocate DMA memory for MSI\n");
+		err = -ENOMEM;
+		goto free_irq;
 	}
 
-	/* setup AFI/FPCI range */
-	msi->pages = __get_free_pages(GFP_KERNEL, 0);
-	msi->phys = virt_to_phys((void *)msi->pages);
 	host->msi = &msi->chip;
 
 	return 0;
 
-err:
+free_irq:
+	free_irq(msi->irq, pcie);
+free_irq_domain:
 	irq_domain_remove(msi->domain);
 	return err;
 }
@@ -1592,7 +1609,7 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
 	struct tegra_msi *msi = &pcie->msi;
 	unsigned int i, irq;
 
-	free_pages(msi->pages, 0);
+	dma_free_coherent(pcie->dev, PAGE_SIZE, msi->virt, msi->phys);
 
 	if (msi->irq > 0)
 		free_irq(msi->irq, pcie);
-- 
2.7.4

Re: [PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[vidya sagar]

Hi Bjorn/Lorenzo,
Can you please review this patch?
Thierry has reviewed it and I already took care of his comments.

Thanks,
Vidya Sagar

On Tue, Mar 19, 2019 at 7:33 PM Vidya Sagar <vidyas@nvidia.com> wrote:
>
> Since the upstream MSI memory writes are generated by downstream devices,
> it is logically correct to have MSI target memory coming from the DMA pool
> reserved for PCIe than from the general memory pool reserved for CPU
> access. This avoids PCIe DMA addresses coinciding with MSI target address
> thereby raising unwanted MSI interrupts. This patch also enforces to limit
> the MSI target address to 32-bits to make it work for PCIe endponits that
> support only 32-bit MSI target address and those endpoints that support
> 64-bit MSI target address anyway work with 32-bit MSI target address.
>
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---
> v2:
> * changed 'phys' type to 'dma_addr_t' from 'u64'
> * added a comment on why DMA mask is set to 32-bit
> * replaced 'dma' with 'DMA'
>
>  drivers/pci/controller/pci-tegra.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index f4f53d092e00..f8173a5e352d 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -231,9 +231,9 @@ struct tegra_msi {
>         struct msi_controller chip;
>         DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>         struct irq_domain *domain;
> -       unsigned long pages;
>         struct mutex lock;
> -       u64 phys;
> +       void *virt;
> +       dma_addr_t phys;
>         int irq;
>  };
>
> @@ -1536,7 +1536,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>         err = platform_get_irq_byname(pdev, "msi");
>         if (err < 0) {
>                 dev_err(dev, "failed to get IRQ: %d\n", err);
> -               goto err;
> +               goto free_irq_domain;
>         }
>
>         msi->irq = err;
> @@ -1545,17 +1545,34 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>                           tegra_msi_irq_chip.name, pcie);
>         if (err < 0) {
>                 dev_err(dev, "failed to request IRQ: %d\n", err);
> -               goto err;
> +               goto free_irq_domain;
> +       }
> +
> +       /* Though the PCIe controller can address >32-bit address space, to
> +        * facilitate endpoints that support only 32-bit MSI target address,
> +        * the mask is set to 32-bit to make sure that MSI target address is
> +        * always a 32-bit address
> +        */
> +       err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> +       if (err < 0) {
> +               dev_err(dev, "failed to set DMA coherent mask: %d\n", err);
> +               goto free_irq;
> +       }
> +
> +       msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
> +       if (!msi->virt) {
> +               dev_err(dev, "failed to allocate DMA memory for MSI\n");
> +               err = -ENOMEM;
> +               goto free_irq;
>         }
>
> -       /* setup AFI/FPCI range */
> -       msi->pages = __get_free_pages(GFP_KERNEL, 0);
> -       msi->phys = virt_to_phys((void *)msi->pages);
>         host->msi = &msi->chip;
>
>         return 0;
>
> -err:
> +free_irq:
> +       free_irq(msi->irq, pcie);
> +free_irq_domain:
>         irq_domain_remove(msi->domain);
>         return err;
>  }
> @@ -1592,7 +1609,7 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
>         struct tegra_msi *msi = &pcie->msi;
>         unsigned int i, irq;
>
> -       free_pages(msi->pages, 0);
> +       dma_free_coherent(pcie->dev, PAGE_SIZE, msi->virt, msi->phys);
>
>         if (msi->irq > 0)
>                 free_irq(msi->irq, pcie);
> --
> 2.7.4
>

Re: [PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[Vidya Sagar]

Hi Bjorn / Lorenzo,
Can you please review this patch?

Thanks,
Vidya Sagar

On 3/27/2019 4:29 PM, vidya sagar wrote:
> Hi Bjorn/Lorenzo,
> Can you please review this patch?
> Thierry has reviewed it and I already took care of his comments.
> 
> Thanks,
> Vidya Sagar
> 
> On Tue, Mar 19, 2019 at 7:33 PM Vidya Sagar <vidyas@nvidia.com <mailto:vidyas@nvidia.com>> wrote:
> 
>     Since the upstream MSI memory writes are generated by downstream devices,
>     it is logically correct to have MSI target memory coming from the DMA pool
>     reserved for PCIe than from the general memory pool reserved for CPU
>     access. This avoids PCIe DMA addresses coinciding with MSI target address
>     thereby raising unwanted MSI interrupts. This patch also enforces to limit
>     the MSI target address to 32-bits to make it work for PCIe endponits that
>     support only 32-bit MSI target address and those endpoints that support
>     64-bit MSI target address anyway work with 32-bit MSI target address.
> 
>     Signed-off-by: Vidya Sagar <vidyas@nvidia.com <mailto:vidyas@nvidia.com>>
>     Reviewed-by: Thierry Reding <treding@nvidia.com <mailto:treding@nvidia.com>>
>     Acked-by: Thierry Reding <treding@nvidia.com <mailto:treding@nvidia.com>>
>     ---
>     v2:
>     * changed 'phys' type to 'dma_addr_t' from 'u64'
>     * added a comment on why DMA mask is set to 32-bit
>     * replaced 'dma' with 'DMA'
> 
>       drivers/pci/controller/pci-tegra.c | 35 ++++++++++++++++++++++++++---------
>       1 file changed, 26 insertions(+), 9 deletions(-)
> 
>     diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>     index f4f53d092e00..f8173a5e352d 100644
>     --- a/drivers/pci/controller/pci-tegra.c
>     +++ b/drivers/pci/controller/pci-tegra.c
>     @@ -231,9 +231,9 @@ struct tegra_msi {
>              struct msi_controller chip;
>              DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>              struct irq_domain *domain;
>     -       unsigned long pages;
>              struct mutex lock;
>     -       u64 phys;
>     +       void *virt;
>     +       dma_addr_t phys;
>              int irq;
>       };
> 
>     @@ -1536,7 +1536,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>              err = platform_get_irq_byname(pdev, "msi");
>              if (err < 0) {
>                      dev_err(dev, "failed to get IRQ: %d\n", err);
>     -               goto err;
>     +               goto free_irq_domain;
>              }
> 
>              msi->irq = err;
>     @@ -1545,17 +1545,34 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>     tegra_msi_irq_chip.name <http://tegra_msi_irq_chip.name>, pcie);
>              if (err < 0) {
>                      dev_err(dev, "failed to request IRQ: %d\n", err);
>     -               goto err;
>     +               goto free_irq_domain;
>     +       }
>     +
>     +       /* Though the PCIe controller can address >32-bit address space, to
>     +        * facilitate endpoints that support only 32-bit MSI target address,
>     +        * the mask is set to 32-bit to make sure that MSI target address is
>     +        * always a 32-bit address
>     +        */
>     +       err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>     +       if (err < 0) {
>     +               dev_err(dev, "failed to set DMA coherent mask: %d\n", err);
>     +               goto free_irq;
>     +       }
>     +
>     +       msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
>     +       if (!msi->virt) {
>     +               dev_err(dev, "failed to allocate DMA memory for MSI\n");
>     +               err = -ENOMEM;
>     +               goto free_irq;
>              }
> 
>     -       /* setup AFI/FPCI range */
>     -       msi->pages = __get_free_pages(GFP_KERNEL, 0);
>     -       msi->phys = virt_to_phys((void *)msi->pages);
>              host->msi = &msi->chip;
> 
>              return 0;
> 
>     -err:
>     +free_irq:
>     +       free_irq(msi->irq, pcie);
>     +free_irq_domain:
>              irq_domain_remove(msi->domain);
>              return err;
>       }
>     @@ -1592,7 +1609,7 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
>              struct tegra_msi *msi = &pcie->msi;
>              unsigned int i, irq;
> 
>     -       free_pages(msi->pages, 0);
>     +       dma_free_coherent(pcie->dev, PAGE_SIZE, msi->virt, msi->phys);
> 
>              if (msi->irq > 0)
>                      free_irq(msi->irq, pcie);
>     -- 
>     2.7.4
> 

Re: [PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[Vidya Sagar]

On 4/1/2019 11:13 AM, Vidya Sagar wrote:

Hi Bjorn / Lorenzo,
Apologies for reminding you again. Could you please review this patch?

Thanks,
Vidya Sagar

> Hi Bjorn / Lorenzo,
> Can you please review this patch?
> 
> Thanks,
> Vidya Sagar
> 
> On 3/27/2019 4:29 PM, vidya sagar wrote:
>> Hi Bjorn/Lorenzo,
>> Can you please review this patch?
>> Thierry has reviewed it and I already took care of his comments.
>>
>> Thanks,
>> Vidya Sagar
>>
>> On Tue, Mar 19, 2019 at 7:33 PM Vidya Sagar <vidyas@nvidia.com <mailto:vidyas@nvidia.com>> wrote:
>>
>>     Since the upstream MSI memory writes are generated by downstream devices,
>>     it is logically correct to have MSI target memory coming from the DMA pool
>>     reserved for PCIe than from the general memory pool reserved for CPU
>>     access. This avoids PCIe DMA addresses coinciding with MSI target address
>>     thereby raising unwanted MSI interrupts. This patch also enforces to limit
>>     the MSI target address to 32-bits to make it work for PCIe endponits that
>>     support only 32-bit MSI target address and those endpoints that support
>>     64-bit MSI target address anyway work with 32-bit MSI target address.
>>
>>     Signed-off-by: Vidya Sagar <vidyas@nvidia.com <mailto:vidyas@nvidia.com>>
>>     Reviewed-by: Thierry Reding <treding@nvidia.com <mailto:treding@nvidia.com>>
>>     Acked-by: Thierry Reding <treding@nvidia.com <mailto:treding@nvidia.com>>
>>     ---
>>     v2:
>>     * changed 'phys' type to 'dma_addr_t' from 'u64'
>>     * added a comment on why DMA mask is set to 32-bit
>>     * replaced 'dma' with 'DMA'
>>
>>       drivers/pci/controller/pci-tegra.c | 35 ++++++++++++++++++++++++++---------
>>       1 file changed, 26 insertions(+), 9 deletions(-)
>>
>>     diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>>     index f4f53d092e00..f8173a5e352d 100644
>>     --- a/drivers/pci/controller/pci-tegra.c
>>     +++ b/drivers/pci/controller/pci-tegra.c
>>     @@ -231,9 +231,9 @@ struct tegra_msi {
>>              struct msi_controller chip;
>>              DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>>              struct irq_domain *domain;
>>     -       unsigned long pages;
>>              struct mutex lock;
>>     -       u64 phys;
>>     +       void *virt;
>>     +       dma_addr_t phys;
>>              int irq;
>>       };
>>
>>     @@ -1536,7 +1536,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>>              err = platform_get_irq_byname(pdev, "msi");
>>              if (err < 0) {
>>                      dev_err(dev, "failed to get IRQ: %d\n", err);
>>     -               goto err;
>>     +               goto free_irq_domain;
>>              }
>>
>>              msi->irq = err;
>>     @@ -1545,17 +1545,34 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>>     tegra_msi_irq_chip.name <http://tegra_msi_irq_chip.name>, pcie);
>>              if (err < 0) {
>>                      dev_err(dev, "failed to request IRQ: %d\n", err);
>>     -               goto err;
>>     +               goto free_irq_domain;
>>     +       }
>>     +
>>     +       /* Though the PCIe controller can address >32-bit address space, to
>>     +        * facilitate endpoints that support only 32-bit MSI target address,
>>     +        * the mask is set to 32-bit to make sure that MSI target address is
>>     +        * always a 32-bit address
>>     +        */
>>     +       err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>     +       if (err < 0) {
>>     +               dev_err(dev, "failed to set DMA coherent mask: %d\n", err);
>>     +               goto free_irq;
>>     +       }
>>     +
>>     +       msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
>>     +       if (!msi->virt) {
>>     +               dev_err(dev, "failed to allocate DMA memory for MSI\n");
>>     +               err = -ENOMEM;
>>     +               goto free_irq;
>>              }
>>
>>     -       /* setup AFI/FPCI range */
>>     -       msi->pages = __get_free_pages(GFP_KERNEL, 0);
>>     -       msi->phys = virt_to_phys((void *)msi->pages);
>>              host->msi = &msi->chip;
>>
>>              return 0;
>>
>>     -err:
>>     +free_irq:
>>     +       free_irq(msi->irq, pcie);
>>     +free_irq_domain:
>>              irq_domain_remove(msi->domain);
>>              return err;
>>       }
>>     @@ -1592,7 +1609,7 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
>>              struct tegra_msi *msi = &pcie->msi;
>>              unsigned int i, irq;
>>
>>     -       free_pages(msi->pages, 0);
>>     +       dma_free_coherent(pcie->dev, PAGE_SIZE, msi->virt, msi->phys);
>>
>>              if (msi->irq > 0)
>>                      free_irq(msi->irq, pcie);
>>     --     2.7.4
>>
> 

Re: [PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[Lorenzo Pieralisi]

On Fri, Apr 05, 2019 at 03:00:36AM +0530, Vidya Sagar wrote:
> On 4/1/2019 11:13 AM, Vidya Sagar wrote:
> 
> Hi Bjorn / Lorenzo,
> Apologies for reminding you again. Could you please review this patch?

It is in the patchwork queue - I will get to it, thank you for your
patience.

Lorenzo

> Thanks,
> Vidya Sagar
> 
> >Hi Bjorn / Lorenzo,
> >Can you please review this patch?
> >
> >Thanks,
> >Vidya Sagar
> >
> >On 3/27/2019 4:29 PM, vidya sagar wrote:
> >>Hi Bjorn/Lorenzo,
> >>Can you please review this patch?
> >>Thierry has reviewed it and I already took care of his comments.
> >>
> >>Thanks,
> >>Vidya Sagar
> >>
> >>On Tue, Mar 19, 2019 at 7:33 PM Vidya Sagar <vidyas@nvidia.com <mailto:vidyas@nvidia.com>> wrote:
> >>
> >>    Since the upstream MSI memory writes are generated by downstream devices,
> >>    it is logically correct to have MSI target memory coming from the DMA pool
> >>    reserved for PCIe than from the general memory pool reserved for CPU
> >>    access. This avoids PCIe DMA addresses coinciding with MSI target address
> >>    thereby raising unwanted MSI interrupts. This patch also enforces to limit
> >>    the MSI target address to 32-bits to make it work for PCIe endponits that
> >>    support only 32-bit MSI target address and those endpoints that support
> >>    64-bit MSI target address anyway work with 32-bit MSI target address.
> >>
> >>    Signed-off-by: Vidya Sagar <vidyas@nvidia.com <mailto:vidyas@nvidia.com>>
> >>    Reviewed-by: Thierry Reding <treding@nvidia.com <mailto:treding@nvidia.com>>
> >>    Acked-by: Thierry Reding <treding@nvidia.com <mailto:treding@nvidia.com>>
> >>    ---
> >>    v2:
> >>    * changed 'phys' type to 'dma_addr_t' from 'u64'
> >>    * added a comment on why DMA mask is set to 32-bit
> >>    * replaced 'dma' with 'DMA'
> >>
> >>      drivers/pci/controller/pci-tegra.c | 35 ++++++++++++++++++++++++++---------
> >>      1 file changed, 26 insertions(+), 9 deletions(-)
> >>
> >>    diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> >>    index f4f53d092e00..f8173a5e352d 100644
> >>    --- a/drivers/pci/controller/pci-tegra.c
> >>    +++ b/drivers/pci/controller/pci-tegra.c
> >>    @@ -231,9 +231,9 @@ struct tegra_msi {
> >>             struct msi_controller chip;
> >>             DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> >>             struct irq_domain *domain;
> >>    -       unsigned long pages;
> >>             struct mutex lock;
> >>    -       u64 phys;
> >>    +       void *virt;
> >>    +       dma_addr_t phys;
> >>             int irq;
> >>      };
> >>
> >>    @@ -1536,7 +1536,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
> >>             err = platform_get_irq_byname(pdev, "msi");
> >>             if (err < 0) {
> >>                     dev_err(dev, "failed to get IRQ: %d\n", err);
> >>    -               goto err;
> >>    +               goto free_irq_domain;
> >>             }
> >>
> >>             msi->irq = err;
> >>    @@ -1545,17 +1545,34 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
> >>    tegra_msi_irq_chip.name <http://tegra_msi_irq_chip.name>, pcie);
> >>             if (err < 0) {
> >>                     dev_err(dev, "failed to request IRQ: %d\n", err);
> >>    -               goto err;
> >>    +               goto free_irq_domain;
> >>    +       }
> >>    +
> >>    +       /* Though the PCIe controller can address >32-bit address space, to
> >>    +        * facilitate endpoints that support only 32-bit MSI target address,
> >>    +        * the mask is set to 32-bit to make sure that MSI target address is
> >>    +        * always a 32-bit address
> >>    +        */
> >>    +       err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> >>    +       if (err < 0) {
> >>    +               dev_err(dev, "failed to set DMA coherent mask: %d\n", err);
> >>    +               goto free_irq;
> >>    +       }
> >>    +
> >>    +       msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
> >>    +       if (!msi->virt) {
> >>    +               dev_err(dev, "failed to allocate DMA memory for MSI\n");
> >>    +               err = -ENOMEM;
> >>    +               goto free_irq;
> >>             }
> >>
> >>    -       /* setup AFI/FPCI range */
> >>    -       msi->pages = __get_free_pages(GFP_KERNEL, 0);
> >>    -       msi->phys = virt_to_phys((void *)msi->pages);
> >>             host->msi = &msi->chip;
> >>
> >>             return 0;
> >>
> >>    -err:
> >>    +free_irq:
> >>    +       free_irq(msi->irq, pcie);
> >>    +free_irq_domain:
> >>             irq_domain_remove(msi->domain);
> >>             return err;
> >>      }
> >>    @@ -1592,7 +1609,7 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
> >>             struct tegra_msi *msi = &pcie->msi;
> >>             unsigned int i, irq;
> >>
> >>    -       free_pages(msi->pages, 0);
> >>    +       dma_free_coherent(pcie->dev, PAGE_SIZE, msi->virt, msi->phys);
> >>
> >>             if (msi->irq > 0)
> >>                     free_irq(msi->irq, pcie);
> >>    --     2.7.4
> >>
> >
> 

Re: [PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[Lorenzo Pieralisi]

[+Robin]

I would like Robin to have a look before merging this patch so
that we agree that's the expected approach.

Lorenzo

On Tue, Mar 19, 2019 at 07:32:01PM +0530, Vidya Sagar wrote:
> Since the upstream MSI memory writes are generated by downstream devices,
> it is logically correct to have MSI target memory coming from the DMA pool
> reserved for PCIe than from the general memory pool reserved for CPU
> access. This avoids PCIe DMA addresses coinciding with MSI target address
> thereby raising unwanted MSI interrupts. This patch also enforces to limit
> the MSI target address to 32-bits to make it work for PCIe endponits that
> support only 32-bit MSI target address and those endpoints that support
> 64-bit MSI target address anyway work with 32-bit MSI target address.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---
> v2:
> * changed 'phys' type to 'dma_addr_t' from 'u64'
> * added a comment on why DMA mask is set to 32-bit
> * replaced 'dma' with 'DMA'
> 
>  drivers/pci/controller/pci-tegra.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index f4f53d092e00..f8173a5e352d 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -231,9 +231,9 @@ struct tegra_msi {
>  	struct msi_controller chip;
>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>  	struct irq_domain *domain;
> -	unsigned long pages;
>  	struct mutex lock;
> -	u64 phys;
> +	void *virt;
> +	dma_addr_t phys;
>  	int irq;
>  };
>  
> @@ -1536,7 +1536,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>  	err = platform_get_irq_byname(pdev, "msi");
>  	if (err < 0) {
>  		dev_err(dev, "failed to get IRQ: %d\n", err);
> -		goto err;
> +		goto free_irq_domain;
>  	}
>  
>  	msi->irq = err;
> @@ -1545,17 +1545,34 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>  			  tegra_msi_irq_chip.name, pcie);
>  	if (err < 0) {
>  		dev_err(dev, "failed to request IRQ: %d\n", err);
> -		goto err;
> +		goto free_irq_domain;
> +	}
> +
> +	/* Though the PCIe controller can address >32-bit address space, to
> +	 * facilitate endpoints that support only 32-bit MSI target address,
> +	 * the mask is set to 32-bit to make sure that MSI target address is
> +	 * always a 32-bit address
> +	 */
> +	err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> +	if (err < 0) {
> +		dev_err(dev, "failed to set DMA coherent mask: %d\n", err);
> +		goto free_irq;
> +	}
> +
> +	msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
> +	if (!msi->virt) {
> +		dev_err(dev, "failed to allocate DMA memory for MSI\n");
> +		err = -ENOMEM;
> +		goto free_irq;
>  	}
>  
> -	/* setup AFI/FPCI range */
> -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> -	msi->phys = virt_to_phys((void *)msi->pages);
>  	host->msi = &msi->chip;
>  
>  	return 0;
>  
> -err:
> +free_irq:
> +	free_irq(msi->irq, pcie);
> +free_irq_domain:
>  	irq_domain_remove(msi->domain);
>  	return err;
>  }
> @@ -1592,7 +1609,7 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
>  	struct tegra_msi *msi = &pcie->msi;
>  	unsigned int i, irq;
>  
> -	free_pages(msi->pages, 0);
> +	dma_free_coherent(pcie->dev, PAGE_SIZE, msi->virt, msi->phys);
>  
>  	if (msi->irq > 0)
>  		free_irq(msi->irq, pcie);
> -- 
> 2.7.4
> 

Re: [PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[Robin Murphy]

On 11/04/2019 10:40, Lorenzo Pieralisi wrote:
> [+Robin]
> 
> I would like Robin to have a look before merging this patch so
> that we agree that's the expected approach.

It's a bit crazy, but I guess it's not really any worse than the 
existing implementation. As long the comments and commit message clearly 
document that this is just trickery to reserve a 32-bit DMA address 
(which AFAICS they more or less do) I don't think I have a significant 
objection.

One suggestion I would make is using dma_alloc_attrs() with 
DMA_ATTR_NO_KERNEL_MAPPING, to make it clear that this is being set 
aside for 'special' device purposes and is not intended to be accessed 
as a regular buffer (plus I believe this is non-coherent, so that should 
also skip the small overhead of creating a non-cacheable remap).

Ultimately, it might make sense to have an API in the PCI core which can 
look at memblock/bridge windows/etc. to find a suitable non-overlapping 
address for this kind of root-complex-integrated doorbell without having 
to subvert the page allocator, as it seems to be a fairly common setup 
in 'embedded' IPs.

Robin.

> 
> Lorenzo
> 
> On Tue, Mar 19, 2019 at 07:32:01PM +0530, Vidya Sagar wrote:
>> Since the upstream MSI memory writes are generated by downstream devices,
>> it is logically correct to have MSI target memory coming from the DMA pool
>> reserved for PCIe than from the general memory pool reserved for CPU
>> access. This avoids PCIe DMA addresses coinciding with MSI target address
>> thereby raising unwanted MSI interrupts. This patch also enforces to limit
>> the MSI target address to 32-bits to make it work for PCIe endponits that
>> support only 32-bit MSI target address and those endpoints that support
>> 64-bit MSI target address anyway work with 32-bit MSI target address.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> Reviewed-by: Thierry Reding <treding@nvidia.com>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> ---
>> v2:
>> * changed 'phys' type to 'dma_addr_t' from 'u64'
>> * added a comment on why DMA mask is set to 32-bit
>> * replaced 'dma' with 'DMA'
>>
>>   drivers/pci/controller/pci-tegra.c | 35 ++++++++++++++++++++++++++---------
>>   1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>> index f4f53d092e00..f8173a5e352d 100644
>> --- a/drivers/pci/controller/pci-tegra.c
>> +++ b/drivers/pci/controller/pci-tegra.c
>> @@ -231,9 +231,9 @@ struct tegra_msi {
>>   	struct msi_controller chip;
>>   	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>>   	struct irq_domain *domain;
>> -	unsigned long pages;
>>   	struct mutex lock;
>> -	u64 phys;
>> +	void *virt;
>> +	dma_addr_t phys;
>>   	int irq;
>>   };
>>   
>> @@ -1536,7 +1536,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>>   	err = platform_get_irq_byname(pdev, "msi");
>>   	if (err < 0) {
>>   		dev_err(dev, "failed to get IRQ: %d\n", err);
>> -		goto err;
>> +		goto free_irq_domain;
>>   	}
>>   
>>   	msi->irq = err;
>> @@ -1545,17 +1545,34 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>>   			  tegra_msi_irq_chip.name, pcie);
>>   	if (err < 0) {
>>   		dev_err(dev, "failed to request IRQ: %d\n", err);
>> -		goto err;
>> +		goto free_irq_domain;
>> +	}
>> +
>> +	/* Though the PCIe controller can address >32-bit address space, to
>> +	 * facilitate endpoints that support only 32-bit MSI target address,
>> +	 * the mask is set to 32-bit to make sure that MSI target address is
>> +	 * always a 32-bit address
>> +	 */
>> +	err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>> +	if (err < 0) {
>> +		dev_err(dev, "failed to set DMA coherent mask: %d\n", err);
>> +		goto free_irq;
>> +	}
>> +
>> +	msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
>> +	if (!msi->virt) {
>> +		dev_err(dev, "failed to allocate DMA memory for MSI\n");
>> +		err = -ENOMEM;
>> +		goto free_irq;
>>   	}
>>   
>> -	/* setup AFI/FPCI range */
>> -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>> -	msi->phys = virt_to_phys((void *)msi->pages);
>>   	host->msi = &msi->chip;
>>   
>>   	return 0;
>>   
>> -err:
>> +free_irq:
>> +	free_irq(msi->irq, pcie);
>> +free_irq_domain:
>>   	irq_domain_remove(msi->domain);
>>   	return err;
>>   }
>> @@ -1592,7 +1609,7 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
>>   	struct tegra_msi *msi = &pcie->msi;
>>   	unsigned int i, irq;
>>   
>> -	free_pages(msi->pages, 0);
>> +	dma_free_coherent(pcie->dev, PAGE_SIZE, msi->virt, msi->phys);
>>   
>>   	if (msi->irq > 0)
>>   		free_irq(msi->irq, pcie);
>> -- 
>> 2.7.4
>>

Re: [PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[Vidya Sagar]

On 4/12/2019 8:30 PM, Robin Murphy wrote:
> On 11/04/2019 10:40, Lorenzo Pieralisi wrote:
>> [+Robin]
>>
>> I would like Robin to have a look before merging this patch so
>> that we agree that's the expected approach.
> 
> It's a bit crazy, but I guess it's not really any worse than the existing implementation. As long the comments and commit message clearly document that this is just trickery to reserve a 32-bit DMA address (which AFAICS they more or less do) I don't think I have a significant objection.
> 
> One suggestion I would make is using dma_alloc_attrs() with DMA_ATTR_NO_KERNEL_MAPPING, to make it clear that this is being set aside for 'special' device purposes and is not intended to be accessed as a regular buffer (plus I believe this is non-coherent, so that should also skip the small overhead of creating a non-cacheable remap).
Thanks Robin for your inputs. I'll take care of changing API from dma_alloc_coherent()/dma_free_coherent() to dma_alloc_attrs()/dma_free_attrs() in V3 patch.

> 
> Ultimately, it might make sense to have an API in the PCI core which can look at memblock/bridge windows/etc. to find a suitable non-overlapping address for this kind of root-complex-integrated doorbell without having to subvert the page allocator, as it seems to be a fairly common setup in 'embedded' IPs.
> 
> Robin.
> 
>>
>> Lorenzo
>>
>> On Tue, Mar 19, 2019 at 07:32:01PM +0530, Vidya Sagar wrote:
>>> Since the upstream MSI memory writes are generated by downstream devices,
>>> it is logically correct to have MSI target memory coming from the DMA pool
>>> reserved for PCIe than from the general memory pool reserved for CPU
>>> access. This avoids PCIe DMA addresses coinciding with MSI target address
>>> thereby raising unwanted MSI interrupts. This patch also enforces to limit
>>> the MSI target address to 32-bits to make it work for PCIe endponits that
>>> support only 32-bit MSI target address and those endpoints that support
>>> 64-bit MSI target address anyway work with 32-bit MSI target address.
>>>
>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>> Reviewed-by: Thierry Reding <treding@nvidia.com>
>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>> v2:
>>> * changed 'phys' type to 'dma_addr_t' from 'u64'
>>> * added a comment on why DMA mask is set to 32-bit
>>> * replaced 'dma' with 'DMA'
>>>
>>>   drivers/pci/controller/pci-tegra.c | 35 ++++++++++++++++++++++++++---------
>>>   1 file changed, 26 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>>> index f4f53d092e00..f8173a5e352d 100644
>>> --- a/drivers/pci/controller/pci-tegra.c
>>> +++ b/drivers/pci/controller/pci-tegra.c
>>> @@ -231,9 +231,9 @@ struct tegra_msi {
>>>       struct msi_controller chip;
>>>       DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>>>       struct irq_domain *domain;
>>> -    unsigned long pages;
>>>       struct mutex lock;
>>> -    u64 phys;
>>> +    void *virt;
>>> +    dma_addr_t phys;
>>>       int irq;
>>>   };
>>> @@ -1536,7 +1536,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>>>       err = platform_get_irq_byname(pdev, "msi");
>>>       if (err < 0) {
>>>           dev_err(dev, "failed to get IRQ: %d\n", err);
>>> -        goto err;
>>> +        goto free_irq_domain;
>>>       }
>>>       msi->irq = err;
>>> @@ -1545,17 +1545,34 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
>>>                 tegra_msi_irq_chip.name, pcie);
>>>       if (err < 0) {
>>>           dev_err(dev, "failed to request IRQ: %d\n", err);
>>> -        goto err;
>>> +        goto free_irq_domain;
>>> +    }
>>> +
>>> +    /* Though the PCIe controller can address >32-bit address space, to
>>> +     * facilitate endpoints that support only 32-bit MSI target address,
>>> +     * the mask is set to 32-bit to make sure that MSI target address is
>>> +     * always a 32-bit address
>>> +     */
>>> +    err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>> +    if (err < 0) {
>>> +        dev_err(dev, "failed to set DMA coherent mask: %d\n", err);
>>> +        goto free_irq;
>>> +    }
>>> +
>>> +    msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
>>> +    if (!msi->virt) {
>>> +        dev_err(dev, "failed to allocate DMA memory for MSI\n");
>>> +        err = -ENOMEM;
>>> +        goto free_irq;
>>>       }
>>> -    /* setup AFI/FPCI range */
>>> -    msi->pages = __get_free_pages(GFP_KERNEL, 0);
>>> -    msi->phys = virt_to_phys((void *)msi->pages);
>>>       host->msi = &msi->chip;
>>>       return 0;
>>> -err:
>>> +free_irq:
>>> +    free_irq(msi->irq, pcie);
>>> +free_irq_domain:
>>>       irq_domain_remove(msi->domain);
>>>       return err;
>>>   }
>>> @@ -1592,7 +1609,7 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
>>>       struct tegra_msi *msi = &pcie->msi;
>>>       unsigned int i, irq;
>>> -    free_pages(msi->pages, 0);
>>> +    dma_free_coherent(pcie->dev, PAGE_SIZE, msi->virt, msi->phys);
>>>       if (msi->irq > 0)
>>>           free_irq(msi->irq, pcie);
>>> -- 
>>> 2.7.4
>>>