* [PATCHv2] staging: android: ion: use the manged version of DMA memory allocation
@ 2016-02-03 6:49 Gujulan Elango, Hari Prasath (H.)
2016-02-03 12:03 ` Robin Murphy
0 siblings, 1 reply; 4+ messages in thread
From: Gujulan Elango, Hari Prasath (H.) @ 2016-02-03 6:49 UTC (permalink / raw)
To: gregkh, arve, riandrews, robin.murphy, tixy, sriram; +Cc: devel, linux-kernel
From: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
Use the managed version of the dma_alloc_coherent() i.e. the
dmam_alloc_coherent() & accordingly cleanup the error handling
part.Also,remove the references to dma_free_coherent
Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
---
v2:kbuild test robot reported warnings on ununsed
variables.Those warnings are fixed.
---
drivers/staging/android/ion/ion_cma_heap.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
index a3446da..8cd720b 100644
--- a/drivers/staging/android/ion/ion_cma_heap.c
+++ b/drivers/staging/android/ion/ion_cma_heap.c
@@ -61,7 +61,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
if (!info)
return ION_CMA_ALLOCATE_FAILED;
- info->cpu_addr = dma_alloc_coherent(dev, len, &(info->handle),
+ info->cpu_addr = dmam_alloc_coherent(dev, len, &(info->handle),
GFP_HIGHUSER | __GFP_ZERO);
if (!info->cpu_addr) {
@@ -71,7 +71,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
info->table = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
if (!info->table)
- goto free_mem;
+ goto err;
if (dma_get_sgtable(dev, info->table, info->cpu_addr, info->handle,
len))
@@ -83,8 +83,6 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
free_table:
kfree(info->table);
-free_mem:
- dma_free_coherent(dev, len, info->cpu_addr, info->handle);
err:
kfree(info);
return ION_CMA_ALLOCATE_FAILED;
@@ -92,13 +90,8 @@ err:
static void ion_cma_free(struct ion_buffer *buffer)
{
- struct ion_cma_heap *cma_heap = to_cma_heap(buffer->heap);
- struct device *dev = cma_heap->dev;
struct ion_cma_buffer_info *info = buffer->priv_virt;
- dev_dbg(dev, "Release buffer %p\n", buffer);
- /* release memory */
- dma_free_coherent(dev, buffer->size, info->cpu_addr, info->handle);
/* release sg table */
sg_free_table(info->table);
kfree(info->table);
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCHv2] staging: android: ion: use the manged version of DMA memory allocation
2016-02-03 6:49 [PATCHv2] staging: android: ion: use the manged version of DMA memory allocation Gujulan Elango, Hari Prasath (H.)
@ 2016-02-03 12:03 ` Robin Murphy
2016-02-03 18:11 ` Laura Abbott
0 siblings, 1 reply; 4+ messages in thread
From: Robin Murphy @ 2016-02-03 12:03 UTC (permalink / raw)
To: Gujulan Elango, Hari Prasath (H.)
Cc: gregkh, arve, riandrews, tixy, sriram, devel, linux-kernel, Laura Abbott
On 03/02/16 06:49, Gujulan Elango, Hari Prasath (H.) wrote:
> From: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
>
> Use the managed version of the dma_alloc_coherent() i.e. the
> dmam_alloc_coherent() & accordingly cleanup the error handling
> part.Also,remove the references to dma_free_coherent
That last aspect looks a bit off to me - the heaps don't seem to be
something that exist for the lifetime of the ION "device", given that
these are specific runtime alloc and free calls, rather than the probe
and remove routines. I don't know if CMA heaps are among those which ION
creates and destroys frequently (enough that it apparently kicks off a
whole background thread to manage the freeing), but this looks like a
recipe for leaks. If the free call doesn't actually free the buffer,
it's going to remain hanging around consuming precious CMA area until
the ION device itself is torn down, which is likely never.
I wouldn't say it's necessarily inappropriate to use managed DMA
resources here to cover unexpected failure cases for the ION device
itself (I see the comment in ion_device_remove()), but that means still
using dmam_free_coherent() when naturally releasing allocations for
other reasons (i.e. both cases here). Think Java finalisers, rather than
C++ destructors.
Robin.
> Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
> ---
> v2:kbuild test robot reported warnings on ununsed
> variables.Those warnings are fixed.
> ---
> drivers/staging/android/ion/ion_cma_heap.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> index a3446da..8cd720b 100644
> --- a/drivers/staging/android/ion/ion_cma_heap.c
> +++ b/drivers/staging/android/ion/ion_cma_heap.c
> @@ -61,7 +61,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
> if (!info)
> return ION_CMA_ALLOCATE_FAILED;
>
> - info->cpu_addr = dma_alloc_coherent(dev, len, &(info->handle),
> + info->cpu_addr = dmam_alloc_coherent(dev, len, &(info->handle),
> GFP_HIGHUSER | __GFP_ZERO);
>
> if (!info->cpu_addr) {
> @@ -71,7 +71,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
>
> info->table = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
> if (!info->table)
> - goto free_mem;
> + goto err;
>
> if (dma_get_sgtable(dev, info->table, info->cpu_addr, info->handle,
> len))
> @@ -83,8 +83,6 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
>
> free_table:
> kfree(info->table);
> -free_mem:
> - dma_free_coherent(dev, len, info->cpu_addr, info->handle);
> err:
> kfree(info);
> return ION_CMA_ALLOCATE_FAILED;
> @@ -92,13 +90,8 @@ err:
>
> static void ion_cma_free(struct ion_buffer *buffer)
> {
> - struct ion_cma_heap *cma_heap = to_cma_heap(buffer->heap);
> - struct device *dev = cma_heap->dev;
> struct ion_cma_buffer_info *info = buffer->priv_virt;
>
> - dev_dbg(dev, "Release buffer %p\n", buffer);
> - /* release memory */
> - dma_free_coherent(dev, buffer->size, info->cpu_addr, info->handle);
> /* release sg table */
> sg_free_table(info->table);
> kfree(info->table);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCHv2] staging: android: ion: use the manged version of DMA memory allocation
2016-02-03 12:03 ` Robin Murphy
@ 2016-02-03 18:11 ` Laura Abbott
2016-02-04 4:59 ` Gujulan Elango, Hari Prasath (H.)
0 siblings, 1 reply; 4+ messages in thread
From: Laura Abbott @ 2016-02-03 18:11 UTC (permalink / raw)
To: Robin Murphy, Gujulan Elango, Hari Prasath (H.)
Cc: gregkh, arve, riandrews, tixy, sriram, devel, linux-kernel
On 02/03/2016 04:03 AM, Robin Murphy wrote:
> On 03/02/16 06:49, Gujulan Elango, Hari Prasath (H.) wrote:
>> From: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
>>
>> Use the managed version of the dma_alloc_coherent() i.e. the
>> dmam_alloc_coherent() & accordingly cleanup the error handling
>> part.Also,remove the references to dma_free_coherent
>
> That last aspect looks a bit off to me - the heaps don't seem to be something that exist for the lifetime of the ION "device", given that these are specific runtime alloc and free calls, rather than the probe and remove routines. I don't know if CMA heaps are among those which ION creates and destroys frequently (enough that it apparently kicks off a whole background thread to manage the freeing), but this looks like a recipe for leaks. If the free call doesn't actually free the buffer, it's going to remain hanging around consuming precious CMA area until the ION device itself is torn down, which is likely never.
>
> I wouldn't say it's necessarily inappropriate to use managed DMA resources here to cover unexpected failure cases for the ION device itself (I see the comment in ion_device_remove()), but that means still using dmam_free_coherent() when naturally releasing allocations for other reasons (i.e. both cases here). Think Java finalisers, rather than C++ destructors.
>
> Robin.
>
Yes, Robin is correct. These allocations are not tied to the lifetime of the
device so it is incorrect to move to the manged APIs. The dma_alloc_coherent
allocations are done on request.
Ion isn't a good candidate to look at to switch APIs over to the devm
interface since it does many things in a non-standard way. You should
probably focus devm efforts outside of Ion (although if you find a
potential patch I'll certainly review it)
Thanks,
Laura
>> Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
>> ---
>> v2:kbuild test robot reported warnings on ununsed
>> variables.Those warnings are fixed.
>> ---
>> drivers/staging/android/ion/ion_cma_heap.c | 11 ++---------
>> 1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
>> index a3446da..8cd720b 100644
>> --- a/drivers/staging/android/ion/ion_cma_heap.c
>> +++ b/drivers/staging/android/ion/ion_cma_heap.c
>> @@ -61,7 +61,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
>> if (!info)
>> return ION_CMA_ALLOCATE_FAILED;
>>
>> - info->cpu_addr = dma_alloc_coherent(dev, len, &(info->handle),
>> + info->cpu_addr = dmam_alloc_coherent(dev, len, &(info->handle),
>> GFP_HIGHUSER | __GFP_ZERO);
>>
>> if (!info->cpu_addr) {
>> @@ -71,7 +71,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
>>
>> info->table = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
>> if (!info->table)
>> - goto free_mem;
>> + goto err;
>>
>> if (dma_get_sgtable(dev, info->table, info->cpu_addr, info->handle,
>> len))
>> @@ -83,8 +83,6 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
>>
>> free_table:
>> kfree(info->table);
>> -free_mem:
>> - dma_free_coherent(dev, len, info->cpu_addr, info->handle);
>> err:
>> kfree(info);
>> return ION_CMA_ALLOCATE_FAILED;
>> @@ -92,13 +90,8 @@ err:
>>
>> static void ion_cma_free(struct ion_buffer *buffer)
>> {
>> - struct ion_cma_heap *cma_heap = to_cma_heap(buffer->heap);
>> - struct device *dev = cma_heap->dev;
>> struct ion_cma_buffer_info *info = buffer->priv_virt;
>>
>> - dev_dbg(dev, "Release buffer %p\n", buffer);
>> - /* release memory */
>> - dma_free_coherent(dev, buffer->size, info->cpu_addr, info->handle);
>> /* release sg table */
>> sg_free_table(info->table);
>> kfree(info->table);
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCHv2] staging: android: ion: use the manged version of DMA memory allocation
2016-02-03 18:11 ` Laura Abbott
@ 2016-02-04 4:59 ` Gujulan Elango, Hari Prasath (H.)
0 siblings, 0 replies; 4+ messages in thread
From: Gujulan Elango, Hari Prasath (H.) @ 2016-02-04 4:59 UTC (permalink / raw)
To: Laura Abbott
Cc: Robin Murphy, gregkh, arve, riandrews, tixy, sriram, devel, linux-kernel
On Wed, Feb 03, 2016 at 10:11:04AM -0800, Laura Abbott wrote:
> On 02/03/2016 04:03 AM, Robin Murphy wrote:
> >On 03/02/16 06:49, Gujulan Elango, Hari Prasath (H.) wrote:
> >>From: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
> >>
> >>Use the managed version of the dma_alloc_coherent() i.e. the
> >>dmam_alloc_coherent() & accordingly cleanup the error handling
> >>part.Also,remove the references to dma_free_coherent
> >
> >That last aspect looks a bit off to me - the heaps don't seem to be something that exist for the lifetime of the ION "device", given that these are specific runtime alloc and free calls, rather than the probe and remove routines. I don't know if CMA heaps are among those which ION creates and destroys frequently (enough that it apparently kicks off a whole background thread to manage the freeing), but this looks like a recipe for leaks. If the free call doesn't actually free the buffer, it's going to remain hanging around consuming precious CMA area until the ION device itself is torn down, which is likely never.
> >
> >I wouldn't say it's necessarily inappropriate to use managed DMA resources here to cover unexpected failure cases for the ION device itself (I see the comment in ion_device_remove()), but that means still using dmam_free_coherent() when naturally releasing allocations for other reasons (i.e. both cases here). Think Java finalisers, rather than C++ destructors.
> >
> >Robin.
> >
>
> Yes, Robin is correct. These allocations are not tied to the lifetime of the
> device so it is incorrect to move to the manged APIs. The dma_alloc_coherent
> allocations are done on request.
>
> Ion isn't a good candidate to look at to switch APIs over to the devm
> interface since it does many things in a non-standard way. You should
> probably focus devm efforts outside of Ion (although if you find a
> potential patch I'll certainly review it)
>
> Thanks,
> Laura
>
> >>Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@visteon.com>
> >>---
> >> v2:kbuild test robot reported warnings on ununsed
> >> variables.Those warnings are fixed.
> >>---
> >> drivers/staging/android/ion/ion_cma_heap.c | 11 ++---------
> >> 1 file changed, 2 insertions(+), 9 deletions(-)
> >>
> >>diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> >>index a3446da..8cd720b 100644
> >>--- a/drivers/staging/android/ion/ion_cma_heap.c
> >>+++ b/drivers/staging/android/ion/ion_cma_heap.c
> >>@@ -61,7 +61,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
> >> if (!info)
> >> return ION_CMA_ALLOCATE_FAILED;
> >>
> >>- info->cpu_addr = dma_alloc_coherent(dev, len, &(info->handle),
> >>+ info->cpu_addr = dmam_alloc_coherent(dev, len, &(info->handle),
> >> GFP_HIGHUSER | __GFP_ZERO);
> >>
> >> if (!info->cpu_addr) {
> >>@@ -71,7 +71,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
> >>
> >> info->table = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
> >> if (!info->table)
> >>- goto free_mem;
> >>+ goto err;
> >>
> >> if (dma_get_sgtable(dev, info->table, info->cpu_addr, info->handle,
> >> len))
> >>@@ -83,8 +83,6 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
> >>
> >> free_table:
> >> kfree(info->table);
> >>-free_mem:
> >>- dma_free_coherent(dev, len, info->cpu_addr, info->handle);
> >> err:
> >> kfree(info);
> >> return ION_CMA_ALLOCATE_FAILED;
> >>@@ -92,13 +90,8 @@ err:
> >>
> >> static void ion_cma_free(struct ion_buffer *buffer)
> >> {
> >>- struct ion_cma_heap *cma_heap = to_cma_heap(buffer->heap);
> >>- struct device *dev = cma_heap->dev;
> >> struct ion_cma_buffer_info *info = buffer->priv_virt;
> >>
> >>- dev_dbg(dev, "Release buffer %p\n", buffer);
> >>- /* release memory */
> >>- dma_free_coherent(dev, buffer->size, info->cpu_addr, info->handle);
> >> /* release sg table */
> >> sg_free_table(info->table);
> >> kfree(info->table);
> >>
> >
>
Robin & Laura,
Thanks for your review comments & I agreee both of you on this. Let's
drop this patch.
Regards
Hari Prasath
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-04 5:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-03 6:49 [PATCHv2] staging: android: ion: use the manged version of DMA memory allocation Gujulan Elango, Hari Prasath (H.)
2016-02-03 12:03 ` Robin Murphy
2016-02-03 18:11 ` Laura Abbott
2016-02-04 4:59 ` Gujulan Elango, Hari Prasath (H.)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.