Re: [PATCH 1/2] dma-mapping: fix page attributes for dma_mmap_*

From: Christoph Hellwig <hch@lst•de>
To: Shawn Anastasio <shawn@anastas•io>
Cc: Gavin Li <git@thegavinli•com>,
	linux-kernel@vger•kernel.org,
	Michael Ellerman <mpe@ellerman•id.au>,
	linuxppc-dev@lists•ozlabs.org,
	Russell King <linux@armlinux•org.uk>,
	linux-mips@vger•kernel.org, iommu@lists•linux-foundation.org,
	Paul Burton <paul.burton@mips•com>,
	Catalin Marinas <catalin.marinas@arm•com>,
	James Hogan <jhogan@kernel•org>, Will Deacon <will@kernel•org>,
	Christoph Hellwig <hch@lst•de>,
	linux-arm-kernel@lists•infradead.org,
	Robin Murphy <robin.murphy@arm•com>
Subject: Re: [PATCH 1/2] dma-mapping: fix page attributes for dma_mmap_*
Date: Wed, 7 Aug 2019 08:04:32 +0200	[thread overview]
Message-ID: <20190807060432.GD6627@lst.de> (raw)
In-Reply-To: <7df95ffb-6df3-b118-284c-ee32cad81199@anastas.io>

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel