Re: [PATCHv3] videobuf2: fix lockdep warning

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/06/2014 12:15 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Tuesday 05 August 2014 12:56:14 Hans Verkuil wrote:
>> Changes since v2: use a mutex instead of a spinlock due to possible sleeps
>> inside the mmap memop. Use the mutex when buffers are allocated/freed, so
>> it's a much coarser lock.
>>
>> The following lockdep warning has been there ever since commit
>> a517cca6b24fc54ac209e44118ec8962051662e3 one year ago:
> 
> [snip]
> 
>> The reason is that vb2_fop_mmap and vb2_fop_get_unmapped_area take the core
>> lock while they are called with the mmap_sem semaphore held. But elsewhere
>> in the code the core lock is taken first but calls to copy_to/from_user()
>> can take the mmap_sem semaphore as well, potentially causing a classical
>> A-B/B-A deadlock.
>>
>> However, the mmap/get_unmapped_area calls really shouldn't take the core
>> lock at all. So what would happen if they don't take the core lock anymore?
>>
>> There are two situations that need to be taken into account: calling mmap
>> while new buffers are being added and calling mmap while buffers are being
>> deleted.
>>
>> The first case works almost fine without a lock: in all cases mmap relies on
>> correctly filled-in q->num_buffers/q->num_planes values and those are only
>> updated by reqbufs and create_buffers *after* any new buffers have been
>> initialized completely. Except in one case: if an error occurred while
>> allocating the buffers it will increase num_buffers and rely on
>> __vb2_queue_free to decrease it again. So there is a short period where the
>> buffer information may be wrong.
>>
>> The second case definitely does pose a problem: buffers may be in the
>> process of being deleted, without the internal structure being updated.
>>
>> In order to fix this a new mutex is added to vb2_queue that is taken when
>> buffers are allocated or deleted, and in vb2_mmap. That way vb2_mmap won't
>> get stale buffer data. Note that this is a problem only for MEMORY_MMAP, so
>> even though __qbuf_userptr and __qbuf_dmabuf also mess around with buffers
>> (mem_priv in particular), this doesn't clash with vb2_mmap or
>> vb2_get_unmapped_area since those are MMAP specific.
>>
>> As an additional bonus the hack in __buf_prepare, the USERPTR case, can be
>> removed as well since mmap() no longer takes the core lock.
>>
>> All in all a much cleaner solution.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> with one small comment, please see below.
> 
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 56 ++++++++++-------------------
>>  include/media/videobuf2-core.h           |  2 ++
>>  2 files changed, 21 insertions(+), 37 deletions(-)
> 
> [snip]
> 
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index fc910a6..ae1289b 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -366,6 +366,7 @@ struct v4l2_fh;
>>   *		cannot be started unless at least this number of buffers
>>   *		have been queued into the driver.
>>   *
>> + * @queue_lock:	private mutex used when buffers are allocated/freed/mmapped
> 
> queue_lock sounds a bit too generic to me, it might confuse readers into 
> thinking the lock protects the whole queue.

How about mmap_lock?

Regards,

	Hans

> 
>> * @memory:	current memory type used
>>   * @bufs:	videobuf buffer structures
>>   * @num_buffers: number of allocated/used buffers
>> @@ -399,6 +400,7 @@ struct vb2_queue {
>>  	u32				min_buffers_needed;
>>
>>  /* private: internal use only */
>> +	struct mutex			queue_lock;
>>  	enum v4l2_memory		memory;
>>  	struct vb2_buffer		*bufs[VIDEO_MAX_FRAME];
>>  	unsigned int			num_buffers;
>