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@xxxxxxxxx>
>
> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> 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;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
