From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:34713 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753675AbaHEWPW (ORCPT ); Tue, 5 Aug 2014 18:15:22 -0400 From: Laurent Pinchart To: Hans Verkuil Cc: linux-media , Pawel Osciak , Marek Szyprowski , sasha.levin@oracle.com Subject: Re: [PATCHv3] videobuf2: fix lockdep warning Date: Wed, 06 Aug 2014 00:15:53 +0200 Message-ID: <9597115.RDDXHorZIT@avalon> In-Reply-To: <53E0B84E.3050803@xs4all.nl> References: <53E0B84E.3050803@xs4all.nl> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: 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 Acked-by: Laurent Pinchart 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