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@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 :)
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.
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
