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

[Laura Abbott <labbott@redhat.com>]

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.


> 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?

Thanks,
Laura