Re: [PATCHv10 21/26] v4l: vb2-dma-contig: add reference counting for a device from allocator context

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomasz,

On Friday 12 October 2012 08:28:23 Tomasz Stanislawski wrote:
> On 10/11/2012 11:49 PM, Laurent Pinchart wrote:
> > On Wednesday 10 October 2012 16:46:40 Tomasz Stanislawski wrote:
> >> This patch adds taking reference to the device for MMAP buffers.
> >> 
> >> Such buffers, may be exported using DMABUF mechanism. If the driver that
> >> created a queue is unloaded then the queue is released, the device might
> >> be
> >> released too.  However, buffers cannot be released if they are referenced
> >> by DMABUF descriptor(s). The device pointer kept in a buffer must be
> >> valid for the whole buffer's lifetime. Therefore MMAP buffers should
> >> take a reference to the device to avoid risk of dangling pointers.
> >> 
> >> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > But two small comments below.
> > 
> >> ---
> >> 
> >>  drivers/media/v4l2-core/videobuf2-dma-contig.c |    4 ++++
> >>  1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index b138b5c..2d661fd
> >> 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> @@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv)
> >>  		kfree(buf->sgt_base);
> >>  	}
> >>  	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
> >> +	put_device(buf->dev);
> >>  	kfree(buf);
> >>  }
> >> 
> >> @@ -168,6 +169,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned
> >> long size)
> >>  		return ERR_PTR(-ENOMEM);
> >>  	}
> >> 
> >> +	/* prevent the device from release while the buffer is exported */
> > 
> > s/prevent/Prevent/ ?
> 
> s/release/being released/ ?

Oops. Of course :-)

> >> +	get_device(dev);
> >> +
> >>  	buf->dev = dev;
> > 
> > What about just
> > 
> > 	buf->dev = get_device(dev);
> 
> Right, sorry I missed that from your previous review :).

-- 
Regards,

Laurent Pinchart