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

[Shawn Anastasio]

On 7/17/19 9:59 PM, Alexey Kardashevskiy wrote:
> 
> 
> On 18/07/2019 09:54, Shawn Anastasio wrote:
>> The refactor of powerpc DMA functions in commit 6666cc17d780
>> ("powerpc/dma: remove dma_nommu_mmap_coherent") incorrectly
>> changes the way DMA mappings are handled on powerpc.
>> Since this change, all mapped pages are marked as cache-inhibited
>> through the default implementation of arch_dma_mmap_pgprot.
>> This differs from the previous behavior of only marking pages
>> in noncoherent mappings as cache-inhibited and has resulted in
>> sporadic system crashes in certain hardware configurations and
>> workloads (see Bugzilla).
>>
>> This commit restores the previous correct behavior by providing
>> an implementation of arch_dma_mmap_pgprot that only marks
>> pages in noncoherent mappings as cache-inhibited. As this behavior
>> should be universal for all powerpc platforms a new file,
>> dma-generic.c, was created to store it.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204145
>> Fixes: 6666cc17d780 ("powerpc/dma: remove dma_nommu_mmap_coherent")
>> Signed-off-by: Shawn Anastasio <shawn at anastas.io>
> 
> 
> Is this the default one?
> 
> include/linux/dma-noncoherent.h
> # define arch_dma_mmap_pgprot(dev, prot, attrs) pgprot_noncached(prot)

Yep, that's the one.

> Out of curiosity - do not we want to fix this one for everyone?

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.

> Either way, the patch is correct. I'm glad to know it was not my " 
> powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59" 
> which broke it :)

Yeah, turns out it was just bad luck that I happened to run into these
crashes right after deciding to try out your patch :)

> Reviewed-by: Alexey Kardashevskiy <aik at ozlabs.ru>

Thanks!

> 
> 
> 
>> ---
>>   arch/powerpc/Kconfig             |  1 +
>>   arch/powerpc/kernel/Makefile     |  3 ++-
>>   arch/powerpc/kernel/dma-common.c | 17 +++++++++++++++++
>>   3 files changed, 20 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/powerpc/kernel/dma-common.c
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index d8dcd8820369..77f6ebf97113 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -121,6 +121,7 @@ config PPC
>>       select ARCH_32BIT_OFF_T if PPC32
>>       select ARCH_HAS_DEBUG_VIRTUAL
>>       select ARCH_HAS_DEVMEM_IS_ALLOWED
>> +    select ARCH_HAS_DMA_MMAP_PGPROT
>>       select ARCH_HAS_ELF_RANDOMIZE
>>       select ARCH_HAS_FORTIFY_SOURCE
>>       select ARCH_HAS_GCOV_PROFILE_ALL
>> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
>> index 56dfa7a2a6f2..ea0c69236789 100644
>> --- a/arch/powerpc/kernel/Makefile
>> +++ b/arch/powerpc/kernel/Makefile
>> @@ -49,7 +49,8 @@ obj-y                := cputable.o ptrace.o 
>> syscalls.o \
>>                      signal.o sysfs.o cacheinfo.o time.o \
>>                      prom.o traps.o setup-common.o \
>>                      udbg.o misc.o io.o misc_$(BITS).o \
>> -                   of_platform.o prom_parse.o
>> +                   of_platform.o prom_parse.o \
>> +                   dma-common.o
>>   obj-$(CONFIG_PPC64)        += setup_64.o sys_ppc32.o \
>>                      signal_64.o ptrace32.o \
>>                      paca.o nvram_64.o firmware.o
>> diff --git a/arch/powerpc/kernel/dma-common.c 
>> b/arch/powerpc/kernel/dma-common.c
>> new file mode 100644
>> index 000000000000..5a15f99f4199
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/dma-common.c
>> @@ -0,0 +1,17 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Contains common dma routines for all powerpc platforms.
>> + *
>> + * Copyright (C) 2019 Shawn Anastasio (shawn at anastas.io)
>> + */
>> +
>> +#include <linux/mm.h>
>> +#include <linux/dma-noncoherent.h>
>> +
>> +pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot,
>> +        unsigned long attrs)
>> +{
>> +    if (!dev_is_dma_coherent(dev))
>> +        return pgprot_noncached(prot);
>> +    return prot;
>> +}
>>
>