Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1186709 > unrolled thread

[PATCH] staging: ion: ion_cma_heap: Don't directly use dma_common_get_sgtable

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging: ion: ion_cma_heap: Don't directly use  dma_common_get_sgtable "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2015-07-17 13:10 +0200
    Re: [PATCH] staging: ion: ion_cma_heap: Don't directly use dma_common_get_sgtable Robin Murphy <robin.murphy@arm.com> - 2015-07-20 20:40 +0200

#1186709 — [PATCH] staging: ion: ion_cma_heap: Don't directly use dma_common_get_sgtable

Use dma_get_sgtable rather than dma_common_get_sgtable so a device's
dma_ops aren't bypassed. This is essential in situations where a device
uses an IOMMU and the physical memory is not contiguous (as the common
function assumes).

Signed-off-by: Jon Medhurst <tixy@linaro.org>
---

This also begs the question as to what happens if the memory region _is_
contiguous but is in highmem or an ioremapped region. Should a device
always provide dma_ops for that case? Because I believe the current
implementation of dma_common_get_sgtable won't work for those as it uses
virt_to_page.

I see that this point has been raised before [1] by Zeng Tao, and I
myself have been given a different fix to apply to a Linaro kernel tree.
However, both solutions looked wrong to me as they treat a dma_addr_t as
a physical address, so should at least be using dma_to_phys.

So, should we fix dma_common_get_sgtable or mandate that the device
has dma_ops? The latter seems to be implied by the commit message which
introduced the function:

        This patch provides a generic implementation based on
        virt_to_page() call. Architectures which require more
        sophisticated translation might provide their own get_sgtable()
        methods.

Note, I don't have a system where any of this code is used to test
things, and have never looked at this area before yesterday, so I may
have misunderstood what’s going on in the code.

[1] https://lkml.org/lkml/2014/12/1/584

 drivers/staging/android/ion/ion_cma_heap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
index f4211f1..86b91fd 100644
--- a/drivers/staging/android/ion/ion_cma_heap.c
+++ b/drivers/staging/android/ion/ion_cma_heap.c
@@ -73,8 +73,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
 	if (!info->table)
 		goto free_mem;
 
-	if (dma_common_get_sgtable
-	    (dev, info->table, info->cpu_addr, info->handle, len))
+	if (dma_get_sgtable(dev, info->table, info->cpu_addr, info->handle, len))
 		goto free_table;
 	/* keep this for memory release */
 	buffer->priv_virt = info;
-- 
2.1.4


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1188430 — Re: [PATCH] staging: ion: ion_cma_heap: Don't directly use dma_common_get_sgtable

Hi Laura,

On 17/07/15 17:50, Laura Abbott wrote:
> On 07/17/2015 08:21 AM, Robin Murphy wrote:
>> Hi Tixy,
>>
>> On 17/07/15 12:01, Jon Medhurst (Tixy) wrote:
>>> Use dma_get_sgtable rather than dma_common_get_sgtable so a device's
>>> dma_ops aren't bypassed. This is essential in situations where a device
>>> uses an IOMMU and the physical memory is not contiguous (as the common
>>> function assumes).
>>>
>>> Signed-off-by: Jon Medhurst <tixy@linaro.org>
>>
>> The lack of obvious users of this code makes it hard to tell if "dev"
>>   hereis always the right, real, device pointer and never null or some
>>   dummy device with the wrong dma_ops, but the rest of the calls in this
>>   file are to the proper DMA API interface so at least this patch definitely
>>   makes things less wrong in that respect.
>>
>
>
> Ion currently lacks any standard way to set up heaps and associate a device
> with a heap. This means it's basically a free for all for what devices get
> associated (getting something mainlined might help...). I agree that using
> the proper DMA APIs is a step in the right direction.

I suspected as much, thanks for the confirmation.

>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>>
>>> ---
>>>
>>> This also begs the question as to what happens if the memory region _is_
>>> contiguous but is in highmem or an ioremapped region. Should a device
>>> always provide dma_ops for that case? Because I believe the current
>>> implementation of dma_common_get_sgtable won't work for those as it uses
>>> virt_to_page.
>>>
>>> I see that this point has been raised before [1] by Zeng Tao, and I
>>> myself have been given a different fix to apply to a Linaro kernel tree.
>>> However, both solutions looked wrong to me as they treat a dma_addr_t as
>>> a physical address, so should at least be using dma_to_phys.
>>> So, should we fix dma_common_get_sgtable or mandate that the device
>>> has dma_ops? The latter seems to be implied by the commit message which
>>> introduced the function:
>>>
>>>           This patch provides a generic implementation based on
>>>           virt_to_page() call. Architectures which require more
>>>           sophisticated translation might provide their own get_sgtable()
>>>           methods.
>>
>> Given that we're largely here due to having poked this on arm64 systems,
>>   I'm inclined to think that implementing our own get_sgtable as per arch/arm
>>   is the right course of action. Since a lot of architectures using
>>   dma_common_get_sgtable don't even implement dma_to_phys, I don't think it
>>   would be right to try complicating the common code for a case that seems to
>>   be all but common. I can spin an arm64 patch if you like.
>>
>
> This would be hit on any system that has non-coherent DMA or highmem. I'm
> not sure I agree this isn't a common case. How many of the other
> architectures are actually using the dma_get_sgtable and would have the
> potential to find a problem?

This appears to be pretty much exclusively a graphics/video thing. 
Surveying in-tree callers (other than Ion) gives DRM, V4L, and a couple 
of specific ARM SoC drivers - my hunch is that none of those see much 
action on the likes of Blackfin and 68k.

That said, going through the git logs, the primary purpose of 
dma_common_get_sgtable would appear to be not breaking allmodconfig 
builds on architectures other than ARM. Thus I'm not really sure which 
is the least worst option - having "common" code which doesn't actually 
represent the common use case, or adding bogus dma_to_phys definitions 
to loads of architectures that don't even have proper DMA mapping 
implementations for the sake of some code they don't even use...

Robin.

>
> Thanks,
> Laura
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web