Re: [PATCH] staging: android: ion: Set the length of the DMA sg entries in buffer
From: Laura Abbott
Date: Thu Jan 21 2016 - 19:59:08 EST
On 01/21/2016 12:19 PM, Jon Medhurst (Tixy) wrote:
On Thu, 2016-01-21 at 09:39 -0800, Laura Abbott wrote:
On 01/21/2016 03:57 AM, Jon Medhurst (Tixy) wrote:
From: Liviu Dudau <Liviu.Dudau@xxxxxxx>
ion_buffer_create() will allocate a buffer and then create a DMA
mapping for it, but it forgot to set the length of the page entries.
Signed-off-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
Signed-off-by: Jon Medhurst <tixy@xxxxxxxxxx>
---
drivers/staging/android/ion/ion.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index e237e9f..df56021 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -251,8 +251,10 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
* memory coming from the heaps is ready for dma, ie if it has a
* cached mapping that mapping has been invalidated
*/
- for_each_sg(buffer->sg_table->sgl, sg, buffer->sg_table->nents, i)
+ for_each_sg(buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) {
sg_dma_address(sg) = sg_phys(sg);
+ sg_dma_len(sg) = sg->length;
+ }
mutex_lock(&dev->buffer_lock);
ion_buffer_add(dev, buffer);
mutex_unlock(&dev->buffer_lock);
So Ion is really doing it wrong by setting the sg_dma_address manually as
the comment above notes. Ion has moved away from sg_dma_len though
(see 06e0dcaeb4fd72a010a1f5ad0c03abd8e0a58ef9). This isn't technically
a mapping as well. What's broken by not having sg_dma_len set?
I fear this could end up being embarrassing...
What's broken is that the out-of-tree kernel driver for ARM's Mali GPU
is getting passed a dma_buf corresponding to the ION buffer. It is then
calling dma_buf_map_attachment [1] on that and then parsing the
resultant scatter-gather list to get the physical pages so it can pass
them to the GPU hardware. In the process, it is using sg_dma_len() to
get the length, which is garbage for ION buffers if ion_buffer_create()
doesn't set it.
[1] http://git.linaro.org/landing-teams/working/arm/kernel-release.git/blob/9660bff61ab296be02aad111d0bc2b9919493de5:/drivers/gpu/arm/midgard/mali_kbase_jd.c#l333
Now, I just tried making the Mali driver use sg->length rather than
sg_dma_len() and, unsurprisingly, that also fixes the problem. So, my
questions would be...
Is it acceptable for a driver getting a dma_buf to parse the
scatter-gather list for that by had?
If so, should it use ->length or sg_dma_len() to get the length of each
element?
If sg_dma_len() is correct or acceptable then it seems to me that the
ION code should set that length. Especially as the comment in the code
implies it's faking a call to map_sg and grepping the kernel tree for
real implementations of that functionality seems to show the dma_address
getting set.
As you can probably tell, I feel I may be on shaky ground. This is
because I don't fully understanding the code and suspecting both the ION
and GPU code is rather dodgy (and possibly the bits in between :-)
I blame the Ion code completely. I remember hitting a similar problem
with other out of tree drivers. The solution then was to have drivers
switch to using sg->length instead of sg_dma_len given the state of that
tree. For the Mali driver, if it is ever going to be backed by an IOMMU
you will need to use sg_dma_len so I think at least that part of your
code is correct.
Thinking about it some, I'm okay with the patch going in. I thought
there was some reason why the out of tree code from before didn't just
do this hack but I can't remember it. It may have been an out of tree
use case. This does go well with Ion's behavior of pretending to do
DMA mapping. More out of tree users can plead their case if it breaks.
Acked-by: Laura Abbott <labbott@xxxxxxxxxx>