| Message ID | 1437130889.3221.53.camel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 56fa2ece881348d314800b858581f2ae950f8e20 |
| Headers | show |
On Fri, 2015-07-17 at 16:21 +0100, Robin Murphy wrote: > 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 had another check and that seems to be true. > 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'm inclined to agree, however I'm rather new to this area. > I can spin an arm64 patch if you like. That would be good. Especially as from what I see on the arm kernel lists you are already working in that area... And my inbox has just pinged with that patch from you, so I'll add a reference here [2] so people coming across this thread can find it easily. For 32-bit arm my $subject patch should fix ION as that already has the DMA ops.
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;
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(-)