Re: [PATCH 1/3] dma-direct: unify the dma_capable definitions

[Robin Murphy <robin.murphy@arm.com>]

On 19/11/2019 10:26 am, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> On 19.11.2019 10:44, Krzysztof Kozlowski wrote:
>> On Tue, 19 Nov 2019 at 17:27, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>> Hi Christoph,
>>>
>>> On 13.11.2019 08:35, Christoph Hellwig wrote:
>>>> Currently each architectures that wants to override dma_to_phys and
>>>> phys_to_dma also has to provide dma_capable.  But there isn't really
>>>> any good reason for that.  powerpc and mips just have copies of the
>>>> generic one minus the latests fix, and the arm one was the inspiration
>>>> for said fix, but misses the bus_dma_mask handling.
>>>> Make all architectures use the generic version instead.
>>>>
>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> This patch breaks DMAengine PL330 driver on Samsung Exynos SoCs:
>>
>> Thanks Marek for bisecting it. I wonder whether it is also the cause
>> for boot failures I see on NXP Vybrid VF50 SoC (NXP/Freescale
>> fsl-edma) since few days:
>>
>> [ 2.853428] fsl-edma 40018000.dma-controller: overflow 0x40027007+1 of
>> DMA mask ffffffff bus mask 0
>> [ 2.862566] ------------[ cut here ]------------
>> [ 2.867273] WARNING: CPU: 0 PID: 1 at
>> /home/buildbot/worker/builddir_yocto/build/build/tmp/work-shared/col-vf50-proceq-mainline-next/kernel-source/kernel/dma/direct.c:35
>> report_addr+0xc0/0xfc
>> [ 2.884380] Modules linked in:
>> [ 2.887486] CPU: 0 PID: 1 Comm: swapper Tainted: G W
>> 5.4.0-rc7-next-20191118-g519ead8f6a32 #1
>> [ 2.897364] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
>> [ 2.903892] [<8010ddfc>] (unwind_backtrace) from [<8010b4b8>]
>> (show_stack+0x10/0x14)
>> [ 2.911712] [<8010b4b8>] (show_stack) from [<8011b08c>] (__warn+0xd4/0xec)
>> [ 2.918653] [<8011b08c>] (__warn) from [<8011b154>]
>> (warn_slowpath_fmt+0xb0/0xb8)
>> [ 2.926218] [<8011b154>] (warn_slowpath_fmt) from [<80155f7c>]
>> (report_addr+0xc0/0xfc)
>> [ 2.934221] [<80155f7c>] (report_addr) from [<801561f0>]
>> (dma_direct_map_resource+0x98/0xa4)
>> [ 2.942744] [<801561f0>] (dma_direct_map_resource) from [<8041d5d4>]
>> (fsl_edma_prep_slave_dma+0x12c/0x150)
>> [ 2.952475] [<8041d5d4>] (fsl_edma_prep_slave_dma) from [<8041d8cc>]
>> (fsl_edma_prep_dma_cyclic+0x30/0x21c)
>> [ 2.962213] [<8041d8cc>] (fsl_edma_prep_dma_cyclic) from [<80452e10>]
>> (lpuart_rx_dma_startup+0x188/0x36c)
>> [ 2.971871] [<80452e10>] (lpuart_rx_dma_startup) from [<80453058>]
>> (lpuart_startup+0x64/0x78)
>> [ 2.980477] [<80453058>] (lpuart_startup) from [<8044e924>]
>> (uart_startup.part.7+0x110/0x23c)
>> [ 2.989080] [<8044e924>] (uart_startup.part.7) from [<8044eaa0>]
>> (uart_port_activate+0x50/0x7c)
>> [ 2.997857] [<8044eaa0>] (uart_port_activate) from [<80438dc0>]
>> (tty_port_open+0x74/0xc0)
>> [ 3.006111] [<80438dc0>] (tty_port_open) from [<8044be30>] (uart_open+0x18/0x20)
>> [ 3.013588] [<8044be30>] (uart_open) from [<80431b4c>] (tty_open+0x108/0x428)
>> [ 3.020794] [<80431b4c>] (tty_open) from [<801edb48>] (chrdev_open+0xac/0x164)
>> [ 3.028098] [<801edb48>] (chrdev_open) from [<801e55c8>]
>> (do_dentry_open+0x22c/0x3e4)
>> [ 3.036010] [<801e55c8>] (do_dentry_open) from [<801f72a8>]
>> (path_openat+0x4a4/0xf78)
>> [ 3.043912] [<801f72a8>] (path_openat) from [<801f8d34>]
>> (do_filp_open+0x70/0xdc)
>> [ 3.051472] [<801f8d34>] (do_filp_open) from [<801e6998>]
>> (do_sys_open+0x128/0x1f4)
>> [ 3.059217] [<801e6998>] (do_sys_open) from [<80a00ee0>]
>> (kernel_init_freeable+0x150/0x1c4)
>> [ 3.067658] [<80a00ee0>] (kernel_init_freeable) from [<8068e208>]
>> (kernel_init+0x8/0x110)
>> [ 3.075917] [<8068e208>] (kernel_init) from [<801010e8>]
>> (ret_from_fork+0x14/0x2c)
>> [ 3.083539] Exception stack(0x86843fb0 to 0x86843ff8)
>> [ 3.088631] 3fa0: 00000000 00000000 00000000 00000000
>> [ 3.096866] 3fc0: 00000000 00000000 00000000 00000000 00000000
>> 00000000 00000000 00000000
>> [ 3.105103] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> [ 3.111752] ---[ end trace 6fb699802256a309 ]---
>> [    3.116423] fsl-lpuart 40027000.serial: Cannot prepare cyclic DMA
>> [    3.192968] VFS: Mounted root (nfs4 filesystem) on device 0:13.
>> [    3.201432] devtmpfs: mounted
>> [    3.210985] Freeing unused kernel memory: 1024K
>> [    3.217854] Run /sbin/init as init process
>> [    4.643355] systemd[1]: System time before build time, advancing clock.
>> [    4.774106] random: systemd: uninitialized urandom read (16 bytes read)
>> [    4.838361] systemd[1]: systemd 232 running in system mode. (-PAM
>> -AUDIT -SELINUX -IMA -APPARMOR -SMACK +SYSVINIT +UTMP -LIBCRYPTSETUP
>> -GCRYPT -GNUTLS +ACL +XZ -LZ4 -SECCOMP +BLKID -ELFUTILS +KMOD -IDN)
>> [    4.858997] systemd[1]: Detected architecture arm.
>> [    4.873438] fsl-lpuart 40027000.serial: Cannot prepare TX slave DMA!
>> [    4.880138] fsl-lpuart 40027000.serial: Cannot prepare TX slave DMA!
>> [    4.886585] fsl-lpuart 40027000.serial: Cannot prepare TX slave DMA!
>> [    4.893124] fsl-lpuart 40027000.serial: Cannot prepare TX slave DMA!
>> [    4.899679] fsl-lpuart 40027000.serial: Cannot prepare TX slave DMA!
>> [    4.906110] fsl-lpuart 40027000.serial: Cannot prepare TX slave DMA!
>> [    4.912616] fsl-lpuart 40027000.serial: Cannot prepare TX slave DMA!
>>
>> Although maybe that's just the fsl-edma problem?
> 
> This is the same issue.
> 
> A quick looks at the dma-direct code revealed that the
> dma_direct_map_resource() is broken after this unification, because it
> calls dma_direct_possible(), which then calls dma_capable(). The generic
> dma_capable() from include/linux/dma-direct.h checks if the provided
> address is in RAM memory range, what in the case of
> dma_direct_map_resource() is not true.
> 
> A quick fix for this issue is to inline dma_direct_possible() with a
> modified dma_capable() directly to the dma_direct_map_resource()
> function (without the min_low_pfn check):
> 
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 077876ae5c74..b6dd6a0b338b 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -411,8 +411,10 @@ dma_addr_t dma_direct_map_resource(struct device
> *dev, phys_addr_t paddr,
>                   size_t size, enum dma_data_direction dir, unsigned long
> attrs)
>    {
>           dma_addr_t dma_addr = paddr;
> +       dma_addr_t end = dma_addr + size - 1;
> 
> -       if (unlikely(!dma_direct_possible(dev, dma_addr, size))) {
> +       if (unlikely(swiotlb_force == SWIOTLB_FORCE || !dev->dma_mask ||

You don't want the swiotlb_force check in there - we can't bounce-buffer 
device-to-device accesses, regardless of the SWIOTLB policy.

Robin.

> +                    end > min_not_zero(*dev->dma_mask,
> dev->bus_dma_mask))) {
>                   report_addr(dev, dma_addr, size);
>                   return DMA_MAPPING_ERROR;
>           }
> 
> Christoph: Let me know if this is a proper fix for you, then I will send
> it as a full patch.
> 
> Best regards
>