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

[Tomasz Stanislawski <t.stanislaws@samsung.com>]

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;