Re: [PATCH 1/2] dma-mapping: fix page attributes for dma_mmap_*
On 8/7/19 8:04 AM, Christoph Hellwig wrote: Actually it is typical modern Linux style to just provide a prototype and then use "if (IS_ENABLED(CONFIG_FOO))" to guard the call(s) to it. I see. Also, like Will mentioned earlier, the function name isn't entirely accurate anymore. I second the suggestion of using something like arch_dma_noncoherent_pgprot(). As mentioned I plan to remove arch_dma_mmap_pgprot for 5.4, so I'd rather avoid churn for the short period of time. Yeah, fair enough. As for your idea of defining pgprot_dmacoherent for all architectures as #ifndef pgprot_dmacoherent #define pgprot_dmacoherent pgprot_noncached #endif I think that the name here is kind of misleading too, since this definition will only be used when there is no support for proper DMA coherency. Do you have a suggestion for a better name? I'm pretty bad at naming, so just reusing the arm name seemed like a good way to avoid having to make naming decisions myself. Good question. Perhaps something like `pgprot_dmacoherent_fallback` would better convey that this is only used for devices that don't support DMA coherency? Or maybe `pgprot_dma_noncoherent`? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] dma-mapping: fix page attributes for dma_mmap_*
On Tue, Aug 06, 2019 at 09:39:06PM +0200, Shawn Anastasio wrote: >> -#ifdef CONFIG_ARCH_HAS_DMA_MMAP_PGPROT >> pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot, >> unsigned long attrs); >> -#else >> -# define arch_dma_mmap_pgprot(dev, prot, attrs) pgprot_noncached(prot) >> -#endif > > Nit, but maybe the prototype should still be ifdef'd here? It at least > could prevent a reader from incorrectly thinking that the function is > always present. Actually it is typical modern Linux style to just provide a prototype and then use "if (IS_ENABLED(CONFIG_FOO))" to guard the call(s) to it. > > Also, like Will mentioned earlier, the function name isn't entirely > accurate anymore. I second the suggestion of using something like > arch_dma_noncoherent_pgprot(). As mentioned I plan to remove arch_dma_mmap_pgprot for 5.4, so I'd rather avoid churn for the short period of time. > As for your idea of defining > pgprot_dmacoherent for all architectures as > > #ifndef pgprot_dmacoherent > #define pgprot_dmacoherent pgprot_noncached > #endif > > I think that the name here is kind of misleading too, since this > definition will only be used when there is no support for proper > DMA coherency. Do you have a suggestion for a better name? I'm pretty bad at naming, so just reusing the arm name seemed like a good way to avoid having to make naming decisions myself.
Re: [PATCH 1/2] dma-mapping: fix page attributes for dma_mmap_*
On 8/5/19 10:01 AM, Christoph Hellwig wrote: diff --git a/include/linux/dma-noncoherent.h b/include/linux/dma-noncoherent.h index 3813211a9aad..9ae5cee543c4 100644 --- a/include/linux/dma-noncoherent.h +++ b/include/linux/dma-noncoherent.h @@ -42,13 +42,8 @@ void arch_dma_free(struct device *dev, size_t size, void *cpu_addr, dma_addr_t dma_addr, unsigned long attrs); long arch_dma_coherent_to_pfn(struct device *dev, void *cpu_addr, dma_addr_t dma_addr); - -#ifdef CONFIG_ARCH_HAS_DMA_MMAP_PGPROT pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot, unsigned long attrs); -#else -# define arch_dma_mmap_pgprot(dev, prot, attrs)pgprot_noncached(prot) -#endif Nit, but maybe the prototype should still be ifdef'd here? It at least could prevent a reader from incorrectly thinking that the function is always present. Also, like Will mentioned earlier, the function name isn't entirely accurate anymore. I second the suggestion of using something like arch_dma_noncoherent_pgprot(). As for your idea of defining pgprot_dmacoherent for all architectures as #ifndef pgprot_dmacoherent #define pgprot_dmacoherent pgprot_noncached #endif I think that the name here is kind of misleading too, since this definition will only be used when there is no support for proper DMA coherency. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] dma-mapping: fix page attributes for dma_mmap_*
On Mon, Aug 05, 2019 at 11:01:44AM +0300, Christoph Hellwig wrote:
> All the way back to introducing dma_common_mmap we've defaulyed to mark
s/defaultyed/defaulted/
> the pages as uncached. But this is wrong for DMA coherent devices.
> Later on DMA_ATTR_WRITE_COMBINE also got incorrect treatment as that
> flag is only treated special on the alloc side for non-coherent devices.
>
> Introduce a new dma_pgprot helper that deals with the check for coherent
> devices so that only the remapping cases even reach arch_dma_mmap_pgprot
s/even/ever/
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 1d3f0b5a9940..bd2b039f43a6 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -14,9 +14,7 @@
> pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot,
> unsigned long attrs)
> {
> - if (!dev_is_dma_coherent(dev) || (attrs & DMA_ATTR_WRITE_COMBINE))
> - return pgprot_writecombine(prot);
> - return prot;
> + return pgprot_writecombine(prot);
> }
For arm64:
Acked-by: Catalin Marinas
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
