diff mbox

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

Message ID 1349880405-26049-22-git-send-email-t.stanislaws@samsung.com
State New
Headers show

Commit Message

Tomasz Stanislawski Oct. 10, 2012, 2:46 p.m. UTC 
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>
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c |    4 ++++
 1 file changed, 4 insertions(+)

Comments

Laurent Pinchart Oct. 11, 2012, 9:49 p.m. UTC | #1 
Hi Tomasz,

Thanks for the patch.

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/ ?

> +	get_device(dev);
> +
>  	buf->dev = dev;

What about just

	buf->dev = get_device(dev);

>  	buf->size = size;
Tomasz Stanislawski Oct. 12, 2012, 6:28 a.m. UTC | #2 
Hi Laurent,
Thank your your review.

On 10/11/2012 11:49 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> Thanks for the patch.
> 
> 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/ ?

>> +	get_device(dev);
>> +
>>  	buf->dev = dev;
> 
> What about just
> 
> 	buf->dev = get_device(dev);
> 

Right, sorry I missed that from your previous review :).

Regards,
Tomasz Stanislawski

>>  	buf->size = size;
Laurent Pinchart Oct. 12, 2012, 9:32 a.m. UTC | #3 
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 :).
diff mbox

Patch

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 */
+	get_device(dev);
+
 	buf->dev = dev;
 	buf->size = size;