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 <hguju...@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 <hguju...@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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to