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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4256 bytes --]

On Tue, Mar 12, 2019 at 06:33:48PM +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 is slightly confusing because there's no dedicated DMA pool that is
reserved for PCIe. At least not upstream. That said, if there were such
a pool, then, yes, we'd want the allocation to be from that pool, so I
think this is okay as-is.

> 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

                                                         endpoints

> only 32-bit MSI target address and those that support 64-bit MSI target
> address anyway work with 32-bit MSI target address

Fullstop at the end here.

> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> Earlier, a different patch was sent with the subject
> "PCI: tegra: Do not allocate MSI target memory"
> ( http://patchwork.ozlabs.org/patch/1049550/ )
> to address the same issue, but since I'm going to use a different subject for
> this patch, I'm sending it as a different patch instead of a different version
> to the previous patch
> 
>  drivers/pci/controller/pci-tegra.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index f4f53d092e00..55a1626ddb69 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -231,8 +231,8 @@ struct tegra_msi {
>  	struct msi_controller chip;
>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>  	struct irq_domain *domain;
> -	unsigned long pages;
>  	struct mutex lock;
> +	void *virt;
>  	u64 phys;

I think this now needs to be dma_addr_t.

>  	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,29 @@ 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;
> +	}
> +
> +	err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));

This is not technically correct because the controller supports a much
wider address space (48 bits I think), so perhaps add a comment above
the line to explain why you're restricting it to 32 bits here.

> +	if (err < 0) {
> +		dev_err(dev, "failed to set dma coherent mask: %d\n", err);

"dma" -> "DMA", please.

> +		goto free_irq;
> +	}
> +
> +	msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
> +	if (!msi->virt) {
> +		dev_err(dev, "failed to alloc dma mem for MSI\n");

I'm not sure this needs an error message. The system should already warn
if this happens. It does so at least for regular memory allocations. But
if you want to keep it, please spell out "allocate" and "memory". Also:
"dma" -> "DMA".

> +		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 +1604,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);

With the above issues fixed, this is:

Reviewed-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]