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

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

[Jon Medhurst (Tixy)]

From: Liviu Dudau <Liviu.Dudau@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@arm.com>
Signed-off-by: Jon Medhurst <tixy@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);
-- 
2.1.4

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

[Laura Abbott]

On 01/21/2016 03:57 AM, Jon Medhurst (Tixy) wrote:
> From: Liviu Dudau <Liviu.Dudau@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@arm.com>
> Signed-off-by: Jon Medhurst <tixy@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?

Thanks,
Laura

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

[Jon Medhurst (Tixy)]

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@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@arm.com>
> > Signed-off-by: Jon Medhurst <tixy@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 :-)

-- 
Tixy

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

[Laura Abbott]

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

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

[Jon Medhurst (Tixy)]

On Thu, 2016-01-21 at 16:58 -0800, Laura Abbott wrote:
> On 01/21/2016 12:19 PM, Jon Medhurst (Tixy) wrote:
[...]
> > 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.

Well, the $subject patch is copying the value of 'length' into
'dma_length', and if dma_length was previously uninitialised then I
don't see that it can really cause additional problems for ION users. (I
hate the way I've been using 'if' a lot so I'm going to spend some time
educating myself.)

> Acked-by: Laura Abbott <labbott@redhat.com>

Thanks

-- 
Tixy