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
