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

[Robin Murphy <robin.murphy@xxxxxxx>]

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@xxxxxxxxxx>
> > Reviewed-by: Thierry Reding <treding@xxxxxxxxxx>
> > Acked-by: Thierry Reding <treding@xxxxxxxxxx>
> > ---
> > 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