[PATCH] staging: android: ion: Set the length of the DMA sg entries in buffer

Laura Abbott labbott at redhat.com
Fri Jan 22 00:58:59 UTC 2016


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 at arm.com>
>>>
>>> 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 at arm.com>
>>> Signed-off-by: Jon Medhurst <tixy at linaro.org>
>>> ---
>>>    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 at redhat.com>


More information about the devel mailing list