[BUG] dma-ranges, reserved memory regions, dma_alloc_coherent: possible bug?

Alexandre Torgue alexandre.torgue at st.com
Thu Oct 17 10:03:29 UTC 2019


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 :(

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