[0/2] Fix incorrect warning from dma-debug
diff mbox

Message ID 57305FD8.7070006@arm.com
State Under Review
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Robin Murphy May 9, 2016, 10 a.m. UTC
On 09/05/16 10:37, Robin Murphy wrote:
> Hi Niklas,
>
> On 08/05/16 11:59, Niklas Söderlund wrote:
>> Hi,
>>
>> While using CONFIG_DMA_API_DEBUG i came across this warning which I
>> think is a false positive. As shown dma_sync_single_for_device() are
>> called from the dma_map_single() call path. This triggers the warning
>> since the dma-debug code have not yet been made aware of the mapping.
>
> Almost right ;) The thing being mapped (the SPI device's buffer) and the
> thing being synced (the IOMMU's PTE) are entirely unrelated. Due to the
> current of_iommu_init() setup, the IOMMU is probed long before
> dma_debug_init() gets called, therefore DMA debug is missing entries for
> some of the initial page table mappings and gets confused when we update
> them later.
>
>> I try to solve this by introducing __dma_sync_single_for_device() which
>> do not call into the dma-debug code. I'm no expert and this might be a
>> bad way of solving the problem but it allowed me to keep working.
>
> The simple fix should be to just call dma_debug_init() from a
> sufficiently earlier initcall level. The best would be to sort out a
> proper device dependency order to avoid the whole early-IOMMU-creation
> thing entirely.

A tangential idea, as Will just suggested to me, would be to make this 
false-positive situation more explicit, something like (untested):
---8<---
--->8---

but it seems like this a sufficiently rare case that it's probably not 
worth cluttering up the code for.

Robin.

>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 1 at lib/dma-debug.c:1209 check_sync+0x154/0x5e4
>> ipmmu-vmsa e6740000.mmu: DMA-API: device driver tries to sync DMA
>> memory it has not allocated [device address=0x000000006e89b008]
>> [size=8 bytes]
>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
>> 4.6.0-rc5-00012-g52e78c1-dirty #1
>> Hardware name: Generic R8A7791 (Flattened Device Tree)
>> Backtrace:
>> [<c020a0a4>] (dump_backtrace) from [<c020a250>] (show_stack+0x18/0x1c)
>>   r7:c03ebad0 r6:00000009 r5:60000093 r4:00000000
>> [<c020a238>] (show_stack) from [<c03cb7e4>] (dump_stack+0x84/0xa4)
>> [<c03cb760>] (dump_stack) from [<c021e3ac>] (__warn+0xd0/0x100)
>>   r5:00000000 r4:ef05dac8
>> [<c021e2dc>] (__warn) from [<c021e41c>] (warn_slowpath_fmt+0x40/0x48)
>>   r9:00010000 r8:00000000 r7:c0c69c40 r6:c0c02754 r5:ef05db78 r4:ef222810
>> [<c021e3e0>] (warn_slowpath_fmt) from [<c03ebad0>]
>> (check_sync+0x154/0x5e4)
>>   r3:c0945b6f r2:c0936e85
>> [<c03eb97c>] (check_sync) from [<c03ed594>]
>> (debug_dma_sync_single_for_device+0x64/0x70)
>>   r10:00000009 r9:0000000c r8:ee89b008 r7:00000000 r6:6e89b008
>> r5:00000000
>>   r4:6e89b008
>> [<c03ed530>] (debug_dma_sync_single_for_device) from [<c0465298>]
>> (__arm_lpae_set_pte.part.0+0x80/0x8c)
>>   r5:00000001 r4:ef222810
>> [<c0465218>] (__arm_lpae_set_pte.part.0) from [<c046553c>]
>> (__arm_lpae_map+0x298/0x2f0)
>>   r7:00000008 r6:ef25e000 r4:ee898500
>> [<c04652a4>] (__arm_lpae_map) from [<c0465b18>] (arm_lpae_map+0xcc/0xe4)
>>   r10:c0465f40 r9:00001000 r8:40000000 r7:00000000 r6:6f25c000
>> r5:00000000
>>   r4:000008c0
>> [<c0465a4c>] (arm_lpae_map) from [<c0465f78>] (ipmmu_map+0x38/0x40)
>>   r7:40000000 r6:00000000 r5:00001000 r4:c0465a4c
>> [<c0465f40>] (ipmmu_map) from [<c0463d90>] (iommu_map+0xf8/0x15c)
>>   r4:ee895608
>> [<c0463c98>] (iommu_map) from [<c0213524>]
>> (arm_coherent_iommu_map_page+0x1d4/0x2d4)
>>   r10:ef386940 r9:00001000 r8:00000000 r7:00000000 r6:40000000
>> r5:00000000
>>   r4:00000000
>> [<c0213350>] (arm_coherent_iommu_map_page) from [<c0213690>]
>> (arm_iommu_map_page+0x6c/0x74)
>>   r10:c0c61f44 r9:ef19d210 r8:00000001 r7:00001000 r6:00000000
>> r5:ec5ddb80
>>   r4:00000000
>> [<c0213624>] (arm_iommu_map_page) from [<c04f7d64>]
>> (sh_msiof_spi_probe+0x430/0x7b0)
>>   r9:00000000 r8:c0213624 r7:ef19d210 r6:ef242800 r5:ef1b4010 r4:ef242af0
>> [<c04f7934>] (sh_msiof_spi_probe) from [<c049fd3c>]
>> (platform_drv_probe+0x58/0xa8)
>>   r10:00000000 r9:00000000 r8:c0c1f8d4 r7:c0c7e910 r6:c0c1f8d4
>> r5:ef1b4010
>>   r4:c04f7934
>> [<c049fce4>] (platform_drv_probe) from [<c049e788>]
>> (driver_probe_device+0x13c/0x2a4)
>>   r7:c0c7e910 r6:00000000 r5:c0c7e900 r4:ef1b4010
>> [<c049e64c>] (driver_probe_device) from [<c049e978>]
>> (__driver_attach+0x88/0xac)
>>   r9:c0c34000 r8:c0a3c010 r7:c0c1cf08 r6:c0c1f8d4 r5:ef1b4044 r4:ef1b4010
>> [<c049e8f0>] (__driver_attach) from [<c049ce44>]
>> (bus_for_each_dev+0x74/0x98)
>>   r7:c0c1cf08 r6:c049e8f0 r5:c0c1f8d4 r4:00000000
>> [<c049cdd0>] (bus_for_each_dev) from [<c049ebd0>]
>> (driver_attach+0x20/0x28)
>>   r6:ef1cc200 r5:00000000 r4:c0c1f8d4
>> [<c049ebb0>] (driver_attach) from [<c049d5d8>]
>> (bus_add_driver+0xd4/0x1e4)
>> [<c049d504>] (bus_add_driver) from [<c049f2dc>]
>> (driver_register+0xa4/0xe8)
>>   r7:c0a3c010 r6:00000000 r5:c0a1e400 r4:c0c1f8d4
>> [<c049f238>] (driver_register) from [<c04a0778>]
>> (__platform_driver_register+0x38/0x4c)
>>   r5:c0a1e400 r4:ee958fc0
>> [<c04a0740>] (__platform_driver_register) from [<c0a1e418>]
>> (sh_msiof_spi_drv_init+0x18/0x20)
>> [<c0a1e400>] (sh_msiof_spi_drv_init) from [<c0a00e1c>]
>> (do_one_initcall+0x10c/0x1c0)
>> [<c0a00d10>] (do_one_initcall) from [<c0a00ff8>]
>> (kernel_init_freeable+0x128/0x1f4)
>>   r9:c0c34000 r8:c0c34000 r7:c0a47e60 r6:c0a3c83c r5:000000bd r4:00000006
>> [<c0a00ed0>] (kernel_init_freeable) from [<c071067c>]
>> (kernel_init+0x10/0x118)
>>   r9:00000000[    1.879529] ata1: link resume succeeded after 1 retries
>>   r8:00000000 r7:00000000 r6:00000000 r5:c071066c r4:00000000
>> [<c071066c>] (kernel_init) from [<c0206e08>] (ret_from_fork+0x14/0x2c)
>>   r5:c071066c r4:00000000
>> ---[ end trace 6eb9a3df3009d491 ]---
>>
>> Niklas Söderlund (2):
>>    dma-mapping: add __dma_sync_single_for_device()
>>    iommu/io-pgtable-arm: use __dma_sync_single_for_device()
>>
>>   drivers/iommu/io-pgtable-arm.c | 2 +-
>>   include/linux/dma-mapping.h    | 9 ++++++++-
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> --
>> 2.8.2
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Comments

Jon Masters May 7, 2017, 12:06 a.m. UTC | #1
On 05/09/2016 06:00 AM, Robin Murphy wrote:
> On 09/05/16 10:37, Robin Murphy wrote:
>> Hi Niklas,
>>
>> On 08/05/16 11:59, Niklas Söderlund wrote:
>>> Hi,
>>>
>>> While using CONFIG_DMA_API_DEBUG i came across this warning which I
>>> think is a false positive. As shown dma_sync_single_for_device() are
>>> called from the dma_map_single() call path. This triggers the warning
>>> since the dma-debug code have not yet been made aware of the mapping.
>>
>> Almost right ;) The thing being mapped (the SPI device's buffer) and the
>> thing being synced (the IOMMU's PTE) are entirely unrelated. Due to the
>> current of_iommu_init() setup, the IOMMU is probed long before
>> dma_debug_init() gets called, therefore DMA debug is missing entries for
>> some of the initial page table mappings and gets confused when we update
>> them later.

What was the eventual followup/fix for this? We're seeing similar traces
to this during internal testing of 4.11 kernels.

Jon.
Geert Uytterhoeven May 8, 2017, 9:19 a.m. UTC | #2
Hi Jon,

On Sun, May 7, 2017 at 2:06 AM, Jon Masters <jcm@jonmasters.org> wrote:
> On 05/09/2016 06:00 AM, Robin Murphy wrote:
>> On 09/05/16 10:37, Robin Murphy wrote:
>>> On 08/05/16 11:59, Niklas Söderlund wrote:
>>>> While using CONFIG_DMA_API_DEBUG i came across this warning which I
>>>> think is a false positive. As shown dma_sync_single_for_device() are
>>>> called from the dma_map_single() call path. This triggers the warning
>>>> since the dma-debug code have not yet been made aware of the mapping.
>>>
>>> Almost right ;) The thing being mapped (the SPI device's buffer) and the
>>> thing being synced (the IOMMU's PTE) are entirely unrelated. Due to the
>>> current of_iommu_init() setup, the IOMMU is probed long before
>>> dma_debug_init() gets called, therefore DMA debug is missing entries for
>>> some of the initial page table mappings and gets confused when we update
>>> them later.
>
> What was the eventual followup/fix for this? We're seeing similar traces
> to this during internal testing of 4.11 kernels.

A quick hack is to change arch/arm64/mm/dma-mapping.c:dma_debug_do_init()
from fs_initcall() to arch_initcall().

However, then you loose the debugfs features. A proper fix would involve
splitting dma_debug_init() in two phases: an early one doing the real work,
and a late one registering the debugfs interface.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Robin Murphy May 8, 2017, 9:39 a.m. UTC | #3
On 07/05/17 01:06, Jon Masters wrote:
> On 05/09/2016 06:00 AM, Robin Murphy wrote:
>> On 09/05/16 10:37, Robin Murphy wrote:
>>> Hi Niklas,
>>>
>>> On 08/05/16 11:59, Niklas Söderlund wrote:
>>>> Hi,
>>>>
>>>> While using CONFIG_DMA_API_DEBUG i came across this warning which I
>>>> think is a false positive. As shown dma_sync_single_for_device() are
>>>> called from the dma_map_single() call path. This triggers the warning
>>>> since the dma-debug code have not yet been made aware of the mapping.
>>>
>>> Almost right ;) The thing being mapped (the SPI device's buffer) and the
>>> thing being synced (the IOMMU's PTE) are entirely unrelated. Due to the
>>> current of_iommu_init() setup, the IOMMU is probed long before
>>> dma_debug_init() gets called, therefore DMA debug is missing entries for
>>> some of the initial page table mappings and gets confused when we update
>>> them later.
> 
> What was the eventual followup/fix for this? We're seeing similar traces
> to this during internal testing of 4.11 kernels.

The IOMMU probe-deferral patches queued for the current merge window get
rid of the need for early device creation bodges that cause the problem
(of IOMMU pagetables being mapped for DMA before dma-debug is up and
running). In the meantime, I'd suggest either adjusting the initcall
level per 256ff1cf6b44, or turning off the IOMMU driver and/or using
dma-debug's driver-filter option if you're doing more targeted debugging.

Robin.

> 
> Jon.
>

Patch
diff mbox

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 4a1515f4b452..e16684a4ce86 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -107,7 +107,8 @@  static bool dma_debug_initialized __read_mostly;

  static inline bool dma_debug_disabled(void)
  {
-	return global_disable || !dma_debug_initialized;
+	return global_disable || WARN_ONCE(!dma_debug_initialized,
+					   "early DMA-API call not tracked");
  }

  /* Global error count */