[BUG] dma-ranges, reserved memory regions, dma_alloc_coherent: possible bug?
Alexandre Torgue
alexandre.torgue at st.com
Thu Oct 17 10:33:56 UTC 2019
On 10/17/19 12:16 PM, Vladimir Murzin wrote:
> On 10/17/19 11:03 AM, Alexandre Torgue wrote:
>> Hi Vlad
>>
>> On 10/17/19 11:46 AM, Vladimir Murzin wrote:
>>> On 10/14/19 4:01 PM, Vladimir Murzin wrote:
>>>> On 10/14/19 2:54 PM, Robin Murphy wrote:
>>>>> On 13/10/2019 15:28, Daniele Alessandrelli wrote:
>>>>>> Hi,
>>>>>>
>>>>>> It looks like dma_alloc_coherent() is setting the dma_handle output
>>>>>> parameter to the memory physical address and not the device bus
>>>>>> address when the device is using reserved memory regions for DMA
>>>>>> allocation. This is despite using 'dma_ranges' in the device tree to
>>>>>> describe the DMA memory mapping. Is this expected behavior or a bug?
>>>>>
>>>>> That does sound like a bug :(
>>>>>
>>>>>> Here is a reduced version of the device tree I'm using:
>>>>>> \ {
>>>>>> reserved-memory {
>>>>>> #address-cells = <2>;
>>>>>> #size-cells = <2>;
>>>>>> ranges;
>>>>>> mydev_rsvd: rsvd_mem at 494800000 {
>>>>>> compatible = "shared-dma-pool";
>>>>>> reg = <0x4 0x94800000 0x0 0x200000>;
>>>>>> no-map;
>>>>>> };
>>>>>> };
>>>>>> soc {
>>>>>> compatible = "simple-bus";
>>>>>> #address-cells = <2>;
>>>>>> #size-cells = <2>;
>>>>>> ranges;
>>>>>> dma_ranges;
>>>>>>
>>>>>> mybus {
>>>>>> ranges = <>;
>>>>>> dma-ranges = <>;
>>>>>> compatible = "simple-bus";
>>>>>> #address-cells = <2>;
>>>>>> #size-cells = <2>;
>>>>>> ranges = <0x0 0x0 0x0 0x0 0x0 0x80000000>;
>>>>>> dma-ranges = <0x0 0x80000000 0x4 0x80000000
>>>>>> 0x0 0x80000000>;
>>>>>>
>>>>>> mydevice {
>>>>>> compatible = "my-compatible-string";
>>>>>> memory-region = <&mydev_rsvd>;
>>>>>> }
>>>>>> }
>>>>>> }
>>>>>> };
>>>>>>
>>>>>> It looks like this issue was previously fixed by commit c41f9ea998f3
>>>>>> ("drivers: dma-coherent: Account dma_pfn_offset when used with device
>>>>>> tree") which introduced a new function ('dma_get_device_base()') to
>>>>>> return the reserved memory address as seen by the device. However,
>>>>>> such a function, even if still there, is not used anymore in latest
>>>>>> code (as of v5.4-rc2). Was that done for a specific reason? Or is it
>>>>>> just a mistake?
>>>>>
>>>>> Hmm, it looks like 43fc509c3efb ("dma-coherent: introduce interface for default DMA pool") removed the caller of dma_get_device_base() in the alloc path shortly after it was introduced, which certainly appears as if it may have been unintentional - Vladimir?
>>>>
>>>> I do not remember it was intentional. Looking at history, default DMA pool was a response
>>>> to another report. However, now I'm wondering why it was not caught by STM32 - most of that
>>>> work was required to support "dma-ranges" with NOMMU+caches (Cortex-M7).
>>>>
>>>> Alex (or anybody else from ST), maybe you have some input?
>>>
>>> Seem they do not care :)
>>>
>>
>>
>> Concerning, the default dma pool:
>>
>> - It was introduced because dma on cortexM7 couldn't use cache-able memory region. So we configured MPU in bootloader (I think it can be done now by Kernel) a small "no cache" memory region and then declare this same region in kernel device tree for the dma pool. Without that we can't execute any dma trasferts.
>>
>> Sorry for the late answer :(
>
> Still better then no answer ;)
>
> Yes, I remember why we have default DMA pool. IIRC, STM used dma-ranges, it is how
> c41f9ea998f3 ("drivers: dma-coherent: Account dma_pfn_offset when used with device
> tree") was discovered. It also means that STM is broken after 878ec36 (ARM: NOMMU:
> Wire-up default DMA interface) with combination of dma-ranges and default DMA pool.
> Hoverer, I've never seen report from STM, so either I'm missing something or such
> combination is not in use nowdays...
IIRC, I didn't added this support by default in device tree because I
got some troubles with XIP setup (it would be nice that I check the
status with a latest kernel). SO I don't verify this configuration when
I execute my no-regression tests for pull STM32 pull request.
Nevertheless, I think I tested it since kernel 4.13 without issue but I
didn't use "dma-ranges" property.
>
> Cheers
> Vladimir
>
>>
>> alex> I'm wondering if I've missed something with diff bellow (it was a long time ago when I touched DMA)?
>>>
>>> diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
>>> index db92478..287ef89 100644
>>> --- a/arch/arm/mm/dma-mapping-nommu.c
>>> +++ b/arch/arm/mm/dma-mapping-nommu.c
>>> @@ -35,7 +35,7 @@ static void *arm_nommu_dma_alloc(struct device *dev, size_t size,
>>> unsigned long attrs)
>>> {
>>> - void *ret = dma_alloc_from_global_coherent(size, dma_handle);
>>> + void *ret = dma_alloc_from_global_coherent(dev, size, dma_handle);
>>> /*
>>> * dma_alloc_from_global_coherent() may fail because:
>>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>>> index 4a1c4fc..10918c5 100644
>>> --- a/include/linux/dma-mapping.h
>>> +++ b/include/linux/dma-mapping.h
>>> @@ -162,7 +162,7 @@ int dma_release_from_dev_coherent(struct device *dev, int order, void *vaddr);
>>> int dma_mmap_from_dev_coherent(struct device *dev, struct vm_area_struct *vma,
>>> void *cpu_addr, size_t size, int *ret);
>>> -void *dma_alloc_from_global_coherent(ssize_t size, dma_addr_t *dma_handle);
>>> +void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size, dma_addr_t *dma_handle);
>>> int dma_release_from_global_coherent(int order, void *vaddr);
>>> int dma_mmap_from_global_coherent(struct vm_area_struct *vma, void *cpu_addr,
>>> size_t size, int *ret);
>>> @@ -172,7 +172,7 @@ int dma_mmap_from_global_coherent(struct vm_area_struct *vma, void *cpu_addr,
>>> #define dma_release_from_dev_coherent(dev, order, vaddr) (0)
>>> #define dma_mmap_from_dev_coherent(dev, vma, vaddr, order, ret) (0)
>>> -static inline void *dma_alloc_from_global_coherent(ssize_t size,
>>> +static inline void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size,
>>> dma_addr_t *dma_handle)
>>> {
>>> return NULL;
>>> diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
>>> index 545e386..551b0eb 100644
>>> --- a/kernel/dma/coherent.c
>>> +++ b/kernel/dma/coherent.c
>>> @@ -123,8 +123,9 @@ int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
>>> return ret;
>>> }
>>> -static void *__dma_alloc_from_coherent(struct dma_coherent_mem *mem,
>>> - ssize_t size, dma_addr_t *dma_handle)
>>> +static void *__dma_alloc_from_coherent(struct device *dev,
>>> + struct dma_coherent_mem *mem,
>>> + ssize_t size, dma_addr_t *dma_handle)
>>> {
>>> int order = get_order(size);
>>> unsigned long flags;
>>> @@ -143,7 +144,7 @@ static void *__dma_alloc_from_coherent(struct dma_coherent_mem *mem,
>>> /*
>>> * Memory was found in the coherent area.
>>> */
>>> - *dma_handle = mem->device_base + (pageno << PAGE_SHIFT);
>>> + *dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT);
>>> ret = mem->virt_base + (pageno << PAGE_SHIFT);
>>> spin_unlock_irqrestore(&mem->spinlock, flags);
>>> memset(ret, 0, size);
>>> @@ -175,17 +176,18 @@ int dma_alloc_from_dev_coherent(struct device *dev, ssize_t size,
>>> if (!mem)
>>> return 0;
>>> - *ret = __dma_alloc_from_coherent(mem, size, dma_handle);
>>> + *ret = __dma_alloc_from_coherent(dev, mem, size, dma_handle);
>>> return 1;
>>> }
>>> -void *dma_alloc_from_global_coherent(ssize_t size, dma_addr_t *dma_handle)
>>> +void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size,
>>> + dma_addr_t *dma_handle)
>>> {
>>> if (!dma_coherent_default_memory)
>>> return NULL;
>>> - return __dma_alloc_from_coherent(dma_coherent_default_memory, size,
>>> - dma_handle);
>>> + return __dma_alloc_from_coherent(dev, dma_coherent_default_memory, size,
>>> + dma_handle);
>>> }
>>> static int __dma_release_from_coherent(struct dma_coherent_mem *mem,
>>>
>>>>
>>>> Cheers
>>>> Vladimir
>>>>
>>>>>
>>>>> Robin.
>>>>
>>>
>
More information about the iommu
mailing list