| Message ID | 1553004121-24606-1-git-send-email-vidyas@nvidia.com |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [V2] PCI: tegra: Use the DMA-API to get the MSI address | expand |
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 >
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 >
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 >> >
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 > >> > > >
[+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 >
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 >>
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 >>>
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);