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.
> * @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;
--
Regards,
Laurent Pinchart
--
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
