Re: [PATCH] powerpc/dma: Fix invalid DMA mmap behavior

From: Shawn Anastasio <shawn@anastas.io>
To: Christoph Hellwig <hch@lst.de>, Oliver O'Halloran <oohall@gmail.com>
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>,
	Sam Bobroff <sbobroff@linux.ibm.com>,
	iommu@lists.linux-foundation.org,
	linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
	Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCH] powerpc/dma: Fix invalid DMA mmap behavior
Date: Thu, 18 Jul 2019 14:46:00 -0500	[thread overview]
Message-ID: <2da4fafe-93f2-4bf1-62e1-180a3ac800fa@anastas.io> (raw)
In-Reply-To: <20190718095200.GA25744@lst.de>

On 7/18/19 4:52 AM, Christoph Hellwig wrote:
> On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote:
>> On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote:
>>>> Other than m68k, mips, and arm64, everybody else that doesn't have
>>>> ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
>>>> I assume this behavior is acceptable on those architectures.
>>>
>>> It might be acceptable, but there's no reason to use pgport_noncached
>>> if the platform supports cache-coherent DMA.
>>>
>>> Christoph (+cc) made the change so maybe he saw something we're missing.
>>
>> I always found the forcing of noncached access even for coherent
>> devices a little odd, but this was inherited from the previous
>> implementation, which surprised me a bit as the different attributes
>> are usually problematic even on x86.  Let me dig into the history a
>> bit more, but I suspect the righ fix is to default to cached mappings
>> for coherent devices.
> 
> Ok, some history:
> 
> The generic dma mmap implementation, which we are effectively still
> using today was added by:
> 
> commit 64ccc9c033c6089b2d426dad3c56477ab066c999
> Author: Marek Szyprowski <m.szyprowski@samsung.com>
> Date:   Thu Jun 14 13:03:04 2012 +0200
> 
>      common: dma-mapping: add support for generic dma_mmap_* calls
> 
> and unconditionally uses pgprot_noncached in dma_common_mmap, which is
> then used as the fallback by dma_mmap_attrs if no ->mmap method is
> present.  At that point we already had the powerpc implementation
> that only uses pgprot_noncached for non-coherent mappings, and
> the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE
> is set and otherwise pgprot_dmacoherent, which seems to be uncached.
> Arm did support coherent platforms at that time, but they might have
> been an afterthought and not handled properly.
> 
> So it migt have been that we were all wrong for that time and might
> have to fix it up.

Personally, I'm not a huge fan of an implicit default for something
inherently architecture-dependent like this at all. What I'd like to
see is a mechanism that forces architecture code to explicitly
opt in to the default pgprot settings if they don't provide an
implementation of arch_dma_mmap_pgprot. This could perhaps be done
by reversing ARCH_HAS_DMA_MMAP_PGPROT to something like
ARCH_USE_DEFAULT_DMA_MMAP_PGPROT.

This way as more systems are moved to use the common mmap code instead
of their ops->mmap, the people doing the refactoring have to make an
explicit decision about the pgprot settings to use. Such a configuration
would have likely prevented this situation with powerpc from happening.

That being said, if the default behavior doesn't make sense in the
general case it should probably be fixed as well.

Curious to hear some thoughts on this.