[PATCHv2 0/4] vb2: fix syzkaller race conditions

[PATCHv2 0/4] vb2: fix syzkaller race conditions

[Hans Verkuil]

These four patches fix syzkaller race conditions. The first patch
fixes the case where VIDIOC_DQBUF (and indirectly via read()/write())
can release the core serialization mutex, thus allowing another thread
to access the same vb2 queue through a dup()ped filehandle.

If no new buffer is available the DQBUF ioctl will block and wait for
a new buffer to arrive. Before it waits it releases the serialization
lock, and afterwards it reacquires it. This is intentional, since you
do not want to block other ioctls while waiting for a buffer.

But this means that you need to flag that you are waiting for a buffer
and check the flag in the appropriate places.

Specifically, that has to happen for VIDIOC_REQBUFS and VIDIOC_CREATE_BUFS
since those can free/reallocate all buffers. Obviously you should not do
that while waiting for a new frame to arrive. The other place where the
flag should be checked is in VIDIOC_DQBUF and read/write since it makes
not sense to call those while another fd is already waiting for a new
frame.

The remaining three patches fix a problem with vivid: due to the way
vivid was designed it had to release the dev->mutex lock when stop_streaming
was called. However, that was the same lock that was assigned to
queue->lock, so that caused a race condition as well. It really is a
vivid bug, which I fixed by giving each queue its own lock instead of
relying on dev->mutex.

It is a good idea to have vivid do this, since, while vb2 has allowed this
for a long time, no drivers were actually using that feature.

But while analyzing the behavior of vivid and vb2 in this scenario I
realized that doing this (i.e. separate mutexes per queue) caused another
race between calling queue_setup and VIDIOC_S_FMT: if queue->lock and
the ioctl serialization lock are the same, then those operations are
nicely serialized. But if the locks are different, then it is possible
that S_FMT changes the buffer size right after queue_setup returns.

So queue_setup might report that each buffer is 1 MB, while the S_FMT
changes it to 2 MB. So there is now a mismatch between what vb2 thinks
the size should be and what the driver thinks.

So to do this correctly the ioctl serialization lock (or whatever the
driver uses for that) should be taken before calling queue_setup and
released once q->num_buffers has been updated (so vb2_is_busy()
will return true).

The final two patches add support for that.

Regards,

	Hans

Hans Verkuil (4):
  vb2: add waiting_in_dqbuf flag
  vivid: use per-queue mutexes instead of one global mutex.
  vb2 core: add new queue_setup_lock/unlock ops
  vivid: add queue_setup_(un)lock ops

 .../media/common/videobuf2/videobuf2-core.c   | 71 +++++++++++++++----
 drivers/media/platform/vivid/vivid-core.c     | 29 ++++++--
 drivers/media/platform/vivid/vivid-core.h     |  8 +++
 .../media/platform/vivid/vivid-kthread-cap.c  |  2 -
 .../media/platform/vivid/vivid-kthread-out.c  |  2 -
 drivers/media/platform/vivid/vivid-sdr-cap.c  |  4 +-
 drivers/media/platform/vivid/vivid-vbi-cap.c  |  2 +
 drivers/media/platform/vivid/vivid-vbi-out.c  |  2 +
 drivers/media/platform/vivid/vivid-vid-cap.c  |  2 +
 drivers/media/platform/vivid/vivid-vid-out.c  |  2 +
 include/media/videobuf2-core.h                | 20 ++++++
 11 files changed, 119 insertions(+), 25 deletions(-)

-- 
2.19.1

[PATCHv2 1/4] vb2: add waiting_in_dqbuf flag

[Hans Verkuil]

Calling VIDIOC_DQBUF can release the core serialization lock pointed to
by vb2_queue->lock if it has to wait for a new buffer to arrive.

However, if userspace dup()ped the video device filehandle, then it is
possible to read or call DQBUF from two filehandles at the same time.

It is also possible to call REQBUFS from one filehandle while the other
is waiting for a buffer. This will remove all the buffers and reallocate
new ones. Removing all the buffers isn't the problem here (that's already
handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
aware that the buffers have changed.

This is fixed by setting a flag whenever the lock is released while waiting
for a buffer to arrive. And checking the flag where needed so we can return
-EBUSY.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
Reported-by: syzbot+4180ff9ca6810b06c1e9@syzkaller.appspotmail.com
---
 .../media/common/videobuf2/videobuf2-core.c   | 22 +++++++++++++++++++
 include/media/videobuf2-core.h                |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 975ff5669f72..f7e7e633bcd7 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 		return -EBUSY;
 	}
 
+	if (q->waiting_in_dqbuf && *count) {
+		dprintk(1, "another dup()ped fd is waiting for a buffer\n");
+		return -EBUSY;
+	}
+
 	if (*count == 0 || q->num_buffers != 0 ||
 	    (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
 		/*
@@ -809,6 +814,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 	}
 
 	if (!q->num_buffers) {
+		if (q->waiting_in_dqbuf && *count) {
+			dprintk(1, "another dup()ped fd is waiting for a buffer\n");
+			return -EBUSY;
+		}
 		memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
 		q->memory = memory;
 		q->waiting_for_buffers = !q->is_output;
@@ -1621,6 +1630,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 	for (;;) {
 		int ret;
 
+		if (q->waiting_in_dqbuf) {
+			dprintk(1, "another dup()ped fd is waiting for a buffer\n");
+			return -EBUSY;
+		}
+
 		if (!q->streaming) {
 			dprintk(1, "streaming off, will not wait for buffers\n");
 			return -EINVAL;
@@ -1648,6 +1662,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 			return -EAGAIN;
 		}
 
+		q->waiting_in_dqbuf = 1;
 		/*
 		 * We are streaming and blocking, wait for another buffer to
 		 * become ready or for streamoff. Driver's lock is released to
@@ -1668,6 +1683,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 		 * the locks or return an error if one occurred.
 		 */
 		call_void_qop(q, wait_finish, q);
+		q->waiting_in_dqbuf = 0;
 		if (ret) {
 			dprintk(1, "sleep was interrupted\n");
 			return ret;
@@ -2539,6 +2555,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 	if (!data)
 		return -EINVAL;
 
+	if (q->waiting_in_dqbuf) {
+		dprintk(3, "another dup()ped fd is %s\n",
+			read ? "reading" : "writing");
+		return -EBUSY;
+	}
+
 	/*
 	 * Initialize emulator on first call.
 	 */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e86981d615ae..613f22910174 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -584,6 +584,7 @@ struct vb2_queue {
 	unsigned int			start_streaming_called:1;
 	unsigned int			error:1;
 	unsigned int			waiting_for_buffers:1;
+	unsigned int			waiting_in_dqbuf:1;
 	unsigned int			is_multiplanar:1;
 	unsigned int			is_output:1;
 	unsigned int			copy_timestamp:1;
-- 
2.19.1

[PATCHv2 2/4] vivid: use per-queue mutexes instead of one global mutex.

[Hans Verkuil]

This avoids having to unlock the queue lock in stop_streaming.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
---
 drivers/media/platform/vivid/vivid-core.c        | 15 ++++++++++-----
 drivers/media/platform/vivid/vivid-core.h        |  5 +++++
 drivers/media/platform/vivid/vivid-kthread-cap.c |  2 --
 drivers/media/platform/vivid/vivid-kthread-out.c |  2 --
 drivers/media/platform/vivid/vivid-sdr-cap.c     |  2 --
 5 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 626e2b24a403..38389af97b16 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -1075,7 +1075,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_vid_cap_q_lock);
+		q->lock = &dev->vb_vid_cap_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;
 
@@ -1096,7 +1097,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_vid_out_q_lock);
+		q->lock = &dev->vb_vid_out_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;
 
@@ -1117,7 +1119,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_vbi_cap_q_lock);
+		q->lock = &dev->vb_vbi_cap_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;
 
@@ -1138,7 +1141,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_vbi_out_q_lock);
+		q->lock = &dev->vb_vbi_out_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;
 
@@ -1158,7 +1162,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 8;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_sdr_cap_q_lock);
+		q->lock = &dev->vb_sdr_cap_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;
 
diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
index 1891254c8f0b..337ccb563f9b 100644
--- a/drivers/media/platform/vivid/vivid-core.h
+++ b/drivers/media/platform/vivid/vivid-core.h
@@ -385,8 +385,10 @@ struct vivid_dev {
 	struct v4l2_rect		compose_cap;
 	struct v4l2_rect		crop_bounds_cap;
 	struct vb2_queue		vb_vid_cap_q;
+	struct mutex			vb_vid_cap_q_lock;
 	struct list_head		vid_cap_active;
 	struct vb2_queue		vb_vbi_cap_q;
+	struct mutex			vb_vbi_cap_q_lock;
 	struct list_head		vbi_cap_active;
 
 	/* thread for generating video capture stream */
@@ -413,8 +415,10 @@ struct vivid_dev {
 	struct v4l2_rect		compose_out;
 	struct v4l2_rect		compose_bounds_out;
 	struct vb2_queue		vb_vid_out_q;
+	struct mutex			vb_vid_out_q_lock;
 	struct list_head		vid_out_active;
 	struct vb2_queue		vb_vbi_out_q;
+	struct mutex			vb_vbi_out_q_lock;
 	struct list_head		vbi_out_active;
 
 	/* video loop precalculated rectangles */
@@ -459,6 +463,7 @@ struct vivid_dev {
 
 	/* SDR capture */
 	struct vb2_queue		vb_sdr_cap_q;
+	struct mutex			vb_sdr_cap_q_lock;
 	struct list_head		sdr_cap_active;
 	u32				sdr_pixelformat; /* v4l2 format id */
 	unsigned			sdr_buffersize;
diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c b/drivers/media/platform/vivid/vivid-kthread-cap.c
index eebfff2126be..d8bb59e9bcc7 100644
--- a/drivers/media/platform/vivid/vivid-kthread-cap.c
+++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
@@ -927,8 +927,6 @@ void vivid_stop_generating_vid_cap(struct vivid_dev *dev, bool *pstreaming)
 
 	/* shutdown control thread */
 	vivid_grab_controls(dev, false);
-	mutex_unlock(&dev->mutex);
 	kthread_stop(dev->kthread_vid_cap);
 	dev->kthread_vid_cap = NULL;
-	mutex_lock(&dev->mutex);
 }
diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c b/drivers/media/platform/vivid/vivid-kthread-out.c
index 5a14810eeb69..8b864cb0ed52 100644
--- a/drivers/media/platform/vivid/vivid-kthread-out.c
+++ b/drivers/media/platform/vivid/vivid-kthread-out.c
@@ -298,8 +298,6 @@ void vivid_stop_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
 
 	/* shutdown control thread */
 	vivid_grab_controls(dev, false);
-	mutex_unlock(&dev->mutex);
 	kthread_stop(dev->kthread_vid_out);
 	dev->kthread_vid_out = NULL;
-	mutex_lock(&dev->mutex);
 }
diff --git a/drivers/media/platform/vivid/vivid-sdr-cap.c b/drivers/media/platform/vivid/vivid-sdr-cap.c
index dcdc80e272c2..5dfb598af742 100644
--- a/drivers/media/platform/vivid/vivid-sdr-cap.c
+++ b/drivers/media/platform/vivid/vivid-sdr-cap.c
@@ -305,10 +305,8 @@ static void sdr_cap_stop_streaming(struct vb2_queue *vq)
 	}
 
 	/* shutdown control thread */
-	mutex_unlock(&dev->mutex);
 	kthread_stop(dev->kthread_sdr_cap);
 	dev->kthread_sdr_cap = NULL;
-	mutex_lock(&dev->mutex);
 }
 
 static void sdr_cap_buf_request_complete(struct vb2_buffer *vb)
-- 
2.19.1

[PATCHv2 3/4] vb2 core: add new queue_setup_lock/unlock ops

[Hans Verkuil]

If queue->lock is different from the video_device lock, then
you need to serialize queue_setup with VIDIOC_S_FMT, and this
should be done by the driver.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 .../media/common/videobuf2/videobuf2-core.c   | 51 +++++++++++++------
 include/media/videobuf2-core.h                | 19 +++++++
 2 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index f7e7e633bcd7..269485920beb 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -465,7 +465,8 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 	 */
 	if (q->num_buffers) {
 		bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming ||
-				  q->cnt_wait_prepare != q->cnt_wait_finish;
+				  q->cnt_wait_prepare != q->cnt_wait_finish ||
+				  q->cnt_queue_setup_lock != q->cnt_queue_setup_unlock;
 
 		if (unbalanced || debug) {
 			pr_info("counters for queue %p:%s\n", q,
@@ -473,10 +474,14 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 			pr_info("     setup: %u start_streaming: %u stop_streaming: %u\n",
 				q->cnt_queue_setup, q->cnt_start_streaming,
 				q->cnt_stop_streaming);
+			pr_info("     queue_setup_lock: %u queue_setup_unlock: %u\n",
+				q->cnt_queue_setup_lock, q->cnt_queue_setup_unlock);
 			pr_info("     wait_prepare: %u wait_finish: %u\n",
 				q->cnt_wait_prepare, q->cnt_wait_finish);
 		}
 		q->cnt_queue_setup = 0;
+		q->cnt_queue_setup_lock = 0;
+		q->cnt_queue_setup_unlock = 0;
 		q->cnt_wait_prepare = 0;
 		q->cnt_wait_finish = 0;
 		q->cnt_start_streaming = 0;
@@ -717,6 +722,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 	num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
 	memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
 	q->memory = memory;
+	call_void_qop(q, queue_setup_lock, q);
 
 	/*
 	 * Ask the driver how many buffers and planes per buffer it requires.
@@ -725,22 +731,27 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 	ret = call_qop(q, queue_setup, q, &num_buffers, &num_planes,
 		       plane_sizes, q->alloc_devs);
 	if (ret)
-		return ret;
+		goto unlock;
 
 	/* Check that driver has set sane values */
-	if (WARN_ON(!num_planes))
-		return -EINVAL;
+	if (WARN_ON(!num_planes)) {
+		ret = -EINVAL;
+		goto unlock;
+	}
 
 	for (i = 0; i < num_planes; i++)
-		if (WARN_ON(!plane_sizes[i]))
-			return -EINVAL;
+		if (WARN_ON(!plane_sizes[i])) {
+			ret = -EINVAL;
+			goto unlock;
+		}
 
 	/* Finally, allocate buffers and video memory */
 	allocated_buffers =
 		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
 	if (allocated_buffers == 0) {
 		dprintk(1, "memory allocation failed\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto unlock;
 	}
 
 	/*
@@ -775,19 +786,19 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 		 */
 	}
 
-	mutex_lock(&q->mmap_lock);
 	q->num_buffers = allocated_buffers;
+	call_void_qop(q, queue_setup_unlock, q);
 
 	if (ret < 0) {
 		/*
 		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
 		 * from q->num_buffers.
 		 */
+		mutex_lock(&q->mmap_lock);
 		__vb2_queue_free(q, allocated_buffers);
 		mutex_unlock(&q->mmap_lock);
 		return ret;
 	}
-	mutex_unlock(&q->mmap_lock);
 
 	/*
 	 * Return the number of successfully allocated buffers
@@ -795,8 +806,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 	 */
 	*count = allocated_buffers;
 	q->waiting_for_buffers = !q->is_output;
-
 	return 0;
+
+unlock:
+	call_void_qop(q, queue_setup_unlock, q);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
 
@@ -813,10 +827,12 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 		return -ENOBUFS;
 	}
 
+	call_void_qop(q, queue_setup_lock, q);
 	if (!q->num_buffers) {
 		if (q->waiting_in_dqbuf && *count) {
 			dprintk(1, "another dup()ped fd is waiting for a buffer\n");
-			return -EBUSY;
+			ret = -EBUSY;
+			goto unlock;
 		}
 		memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
 		q->memory = memory;
@@ -837,14 +853,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 	ret = call_qop(q, queue_setup, q, &num_buffers,
 		       &num_planes, plane_sizes, q->alloc_devs);
 	if (ret)
-		return ret;
+		goto unlock;
 
 	/* Finally, allocate buffers and video memory */
 	allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
 				num_planes, plane_sizes);
 	if (allocated_buffers == 0) {
 		dprintk(1, "memory allocation failed\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto unlock;
 	}
 
 	/*
@@ -869,19 +886,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 		 */
 	}
 
-	mutex_lock(&q->mmap_lock);
 	q->num_buffers += allocated_buffers;
+	call_void_qop(q, queue_setup_unlock, q);
 
 	if (ret < 0) {
 		/*
 		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
 		 * from q->num_buffers.
 		 */
+		mutex_lock(&q->mmap_lock);
 		__vb2_queue_free(q, allocated_buffers);
 		mutex_unlock(&q->mmap_lock);
 		return -ENOMEM;
 	}
-	mutex_unlock(&q->mmap_lock);
 
 	/*
 	 * Return the number of successfully allocated buffers
@@ -890,6 +907,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 	*count = allocated_buffers;
 
 	return 0;
+
+unlock:
+	call_void_qop(q, queue_setup_unlock, q);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vb2_core_create_bufs);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 613f22910174..92861b6fe7f8 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -333,6 +333,20 @@ struct vb2_buffer {
  *			\*num_buffers are being allocated additionally to
  *			q->num_buffers. If either \*num_planes or the requested
  *			sizes are invalid callback must return %-EINVAL.
+ * @queue_setup_lock:	called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS()
+ *			to serialize @queue_setup with ioctls like
+ *			VIDIOC_S_FMT() that change the buffer size. Only
+ *			required if queue->lock differs from the mutex that is
+ *			used to serialize the ioctls that change the buffer
+ *			size. This callback should lock the ioctl serialization
+ *			mutex.
+ * @queue_setup_unlock:	called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS()
+ *			to serialize @queue_setup with ioctls like
+ *			VIDIOC_S_FMT() that change the buffer size. Only
+ *			required if queue->lock differs from the mutex that is
+ *			used to serialize the ioctls that change the buffer
+ *			size. This callback should unlock the ioctl
+ *			serialization mutex.
  * @wait_prepare:	release any locks taken while calling vb2 functions;
  *			it is called before an ioctl needs to wait for a new
  *			buffer to arrive; required to avoid a deadlock in
@@ -403,10 +417,13 @@ struct vb2_ops {
 	int (*queue_setup)(struct vb2_queue *q,
 			   unsigned int *num_buffers, unsigned int *num_planes,
 			   unsigned int sizes[], struct device *alloc_devs[]);
+	void (*queue_setup_lock)(struct vb2_queue *q);
+	void (*queue_setup_unlock)(struct vb2_queue *q);
 
 	void (*wait_prepare)(struct vb2_queue *q);
 	void (*wait_finish)(struct vb2_queue *q);
 
+
 	int (*buf_init)(struct vb2_buffer *vb);
 	int (*buf_prepare)(struct vb2_buffer *vb);
 	void (*buf_finish)(struct vb2_buffer *vb);
@@ -599,6 +616,8 @@ struct vb2_queue {
 	 * called. Used to check for unbalanced ops.
 	 */
 	u32				cnt_queue_setup;
+	u32				cnt_queue_setup_lock;
+	u32				cnt_queue_setup_unlock;
 	u32				cnt_wait_prepare;
 	u32				cnt_wait_finish;
 	u32				cnt_start_streaming;
-- 
2.19.1

[PATCHv2 4/4] vivid: add queue_setup_(un)lock ops

[Hans Verkuil]

Add these ops to serialize queue_setup with VIDIOC_S_FMT.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/platform/vivid/vivid-core.c    | 14 ++++++++++++++
 drivers/media/platform/vivid/vivid-core.h    |  3 +++
 drivers/media/platform/vivid/vivid-sdr-cap.c |  2 ++
 drivers/media/platform/vivid/vivid-vbi-cap.c |  2 ++
 drivers/media/platform/vivid/vivid-vbi-out.c |  2 ++
 drivers/media/platform/vivid/vivid-vid-cap.c |  2 ++
 drivers/media/platform/vivid/vivid-vid-out.c |  2 ++
 7 files changed, 27 insertions(+)

diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 38389af97b16..51b0a5c99365 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -475,6 +475,20 @@ static int vivid_fop_release(struct file *file)
 	return v4l2_fh_release(file);
 }
 
+void vivid_queue_setup_lock(struct vb2_queue *q)
+{
+	struct vivid_dev *dev = vb2_get_drv_priv(q);
+
+	mutex_lock(&dev->mutex);
+}
+
+void vivid_queue_setup_unlock(struct vb2_queue *q)
+{
+	struct vivid_dev *dev = vb2_get_drv_priv(q);
+
+	mutex_unlock(&dev->mutex);
+}
+
 static const struct v4l2_file_operations vivid_fops = {
 	.owner		= THIS_MODULE,
 	.open           = v4l2_fh_open,
diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
index 337ccb563f9b..78c97c1dcd25 100644
--- a/drivers/media/platform/vivid/vivid-core.h
+++ b/drivers/media/platform/vivid/vivid-core.h
@@ -564,4 +564,7 @@ static inline bool vivid_is_hdmi_out(const struct vivid_dev *dev)
 	return dev->output_type[dev->output] == HDMI;
 }
 
+void vivid_queue_setup_lock(struct vb2_queue *q);
+void vivid_queue_setup_unlock(struct vb2_queue *q);
+
 #endif
diff --git a/drivers/media/platform/vivid/vivid-sdr-cap.c b/drivers/media/platform/vivid/vivid-sdr-cap.c
index 5dfb598af742..bac0dc47e24e 100644
--- a/drivers/media/platform/vivid/vivid-sdr-cap.c
+++ b/drivers/media/platform/vivid/vivid-sdr-cap.c
@@ -318,6 +318,8 @@ static void sdr_cap_buf_request_complete(struct vb2_buffer *vb)
 
 const struct vb2_ops vivid_sdr_cap_qops = {
 	.queue_setup		= sdr_cap_queue_setup,
+	.queue_setup_lock	= vivid_queue_setup_lock,
+	.queue_setup_unlock	= vivid_queue_setup_unlock,
 	.buf_prepare		= sdr_cap_buf_prepare,
 	.buf_queue		= sdr_cap_buf_queue,
 	.start_streaming	= sdr_cap_start_streaming,
diff --git a/drivers/media/platform/vivid/vivid-vbi-cap.c b/drivers/media/platform/vivid/vivid-vbi-cap.c
index 903cebeb5ce5..b5c0ea8b848c 100644
--- a/drivers/media/platform/vivid/vivid-vbi-cap.c
+++ b/drivers/media/platform/vivid/vivid-vbi-cap.c
@@ -231,6 +231,8 @@ static void vbi_cap_buf_request_complete(struct vb2_buffer *vb)
 
 const struct vb2_ops vivid_vbi_cap_qops = {
 	.queue_setup		= vbi_cap_queue_setup,
+	.queue_setup_lock	= vivid_queue_setup_lock,
+	.queue_setup_unlock	= vivid_queue_setup_unlock,
 	.buf_prepare		= vbi_cap_buf_prepare,
 	.buf_queue		= vbi_cap_buf_queue,
 	.start_streaming	= vbi_cap_start_streaming,
diff --git a/drivers/media/platform/vivid/vivid-vbi-out.c b/drivers/media/platform/vivid/vivid-vbi-out.c
index 9357c07e30d6..8f8ce00edaa2 100644
--- a/drivers/media/platform/vivid/vivid-vbi-out.c
+++ b/drivers/media/platform/vivid/vivid-vbi-out.c
@@ -126,6 +126,8 @@ static void vbi_out_buf_request_complete(struct vb2_buffer *vb)
 
 const struct vb2_ops vivid_vbi_out_qops = {
 	.queue_setup		= vbi_out_queue_setup,
+	.queue_setup_lock	= vivid_queue_setup_lock,
+	.queue_setup_unlock	= vivid_queue_setup_unlock,
 	.buf_prepare		= vbi_out_buf_prepare,
 	.buf_queue		= vbi_out_buf_queue,
 	.start_streaming	= vbi_out_start_streaming,
diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
index 9c8e8be81ce3..f315f5f72616 100644
--- a/drivers/media/platform/vivid/vivid-vid-cap.c
+++ b/drivers/media/platform/vivid/vivid-vid-cap.c
@@ -271,6 +271,8 @@ static void vid_cap_buf_request_complete(struct vb2_buffer *vb)
 
 const struct vb2_ops vivid_vid_cap_qops = {
 	.queue_setup		= vid_cap_queue_setup,
+	.queue_setup_lock	= vivid_queue_setup_lock,
+	.queue_setup_unlock	= vivid_queue_setup_unlock,
 	.buf_prepare		= vid_cap_buf_prepare,
 	.buf_finish		= vid_cap_buf_finish,
 	.buf_queue		= vid_cap_buf_queue,
diff --git a/drivers/media/platform/vivid/vivid-vid-out.c b/drivers/media/platform/vivid/vivid-vid-out.c
index aaf13f03d5d4..0fe7f449e416 100644
--- a/drivers/media/platform/vivid/vivid-vid-out.c
+++ b/drivers/media/platform/vivid/vivid-vid-out.c
@@ -190,6 +190,8 @@ static void vid_out_buf_request_complete(struct vb2_buffer *vb)
 
 const struct vb2_ops vivid_vid_out_qops = {
 	.queue_setup		= vid_out_queue_setup,
+	.queue_setup_lock	= vivid_queue_setup_lock,
+	.queue_setup_unlock	= vivid_queue_setup_unlock,
 	.buf_prepare		= vid_out_buf_prepare,
 	.buf_queue		= vid_out_buf_queue,
 	.start_streaming	= vid_out_start_streaming,
-- 
2.19.1

Re: [PATCHv2 2/4] vivid: use per-queue mutexes instead of one global mutex.

[Sakari Ailus]

Hi Hans,

On Mon, Nov 19, 2018 at 12:09:01PM +0100, Hans Verkuil wrote:
> This avoids having to unlock the queue lock in stop_streaming.
> 
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
> ---
>  drivers/media/platform/vivid/vivid-core.c        | 15 ++++++++++-----
>  drivers/media/platform/vivid/vivid-core.h        |  5 +++++
>  drivers/media/platform/vivid/vivid-kthread-cap.c |  2 --
>  drivers/media/platform/vivid/vivid-kthread-out.c |  2 --
>  drivers/media/platform/vivid/vivid-sdr-cap.c     |  2 --
>  5 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
> index 626e2b24a403..38389af97b16 100644
> --- a/drivers/media/platform/vivid/vivid-core.c
> +++ b/drivers/media/platform/vivid/vivid-core.c
> @@ -1075,7 +1075,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 2;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_vid_cap_q_lock);

Could you add the corresponding mutex_destroy()'s for these mutex_init()
calls?

> +		q->lock = &dev->vb_vid_cap_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
>  
> @@ -1096,7 +1097,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 2;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_vid_out_q_lock);
> +		q->lock = &dev->vb_vid_out_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
>  
> @@ -1117,7 +1119,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 2;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_vbi_cap_q_lock);
> +		q->lock = &dev->vb_vbi_cap_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
>  
> @@ -1138,7 +1141,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 2;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_vbi_out_q_lock);
> +		q->lock = &dev->vb_vbi_out_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
>  
> @@ -1158,7 +1162,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 8;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_sdr_cap_q_lock);
> +		q->lock = &dev->vb_sdr_cap_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
>  
> diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
> index 1891254c8f0b..337ccb563f9b 100644
> --- a/drivers/media/platform/vivid/vivid-core.h
> +++ b/drivers/media/platform/vivid/vivid-core.h
> @@ -385,8 +385,10 @@ struct vivid_dev {
>  	struct v4l2_rect		compose_cap;
>  	struct v4l2_rect		crop_bounds_cap;
>  	struct vb2_queue		vb_vid_cap_q;
> +	struct mutex			vb_vid_cap_q_lock;
>  	struct list_head		vid_cap_active;
>  	struct vb2_queue		vb_vbi_cap_q;
> +	struct mutex			vb_vbi_cap_q_lock;
>  	struct list_head		vbi_cap_active;
>  
>  	/* thread for generating video capture stream */
> @@ -413,8 +415,10 @@ struct vivid_dev {
>  	struct v4l2_rect		compose_out;
>  	struct v4l2_rect		compose_bounds_out;
>  	struct vb2_queue		vb_vid_out_q;
> +	struct mutex			vb_vid_out_q_lock;
>  	struct list_head		vid_out_active;
>  	struct vb2_queue		vb_vbi_out_q;
> +	struct mutex			vb_vbi_out_q_lock;
>  	struct list_head		vbi_out_active;
>  
>  	/* video loop precalculated rectangles */
> @@ -459,6 +463,7 @@ struct vivid_dev {
>  
>  	/* SDR capture */
>  	struct vb2_queue		vb_sdr_cap_q;
> +	struct mutex			vb_sdr_cap_q_lock;
>  	struct list_head		sdr_cap_active;
>  	u32				sdr_pixelformat; /* v4l2 format id */
>  	unsigned			sdr_buffersize;
> diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c b/drivers/media/platform/vivid/vivid-kthread-cap.c
> index eebfff2126be..d8bb59e9bcc7 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
> @@ -927,8 +927,6 @@ void vivid_stop_generating_vid_cap(struct vivid_dev *dev, bool *pstreaming)
>  
>  	/* shutdown control thread */
>  	vivid_grab_controls(dev, false);
> -	mutex_unlock(&dev->mutex);
>  	kthread_stop(dev->kthread_vid_cap);
>  	dev->kthread_vid_cap = NULL;
> -	mutex_lock(&dev->mutex);
>  }
> diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c b/drivers/media/platform/vivid/vivid-kthread-out.c
> index 5a14810eeb69..8b864cb0ed52 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-out.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-out.c
> @@ -298,8 +298,6 @@ void vivid_stop_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
>  
>  	/* shutdown control thread */
>  	vivid_grab_controls(dev, false);
> -	mutex_unlock(&dev->mutex);
>  	kthread_stop(dev->kthread_vid_out);
>  	dev->kthread_vid_out = NULL;
> -	mutex_lock(&dev->mutex);
>  }
> diff --git a/drivers/media/platform/vivid/vivid-sdr-cap.c b/drivers/media/platform/vivid/vivid-sdr-cap.c
> index dcdc80e272c2..5dfb598af742 100644
> --- a/drivers/media/platform/vivid/vivid-sdr-cap.c
> +++ b/drivers/media/platform/vivid/vivid-sdr-cap.c
> @@ -305,10 +305,8 @@ static void sdr_cap_stop_streaming(struct vb2_queue *vq)
>  	}
>  
>  	/* shutdown control thread */
> -	mutex_unlock(&dev->mutex);
>  	kthread_stop(dev->kthread_sdr_cap);
>  	dev->kthread_sdr_cap = NULL;
> -	mutex_lock(&dev->mutex);
>  }
>  
>  static void sdr_cap_buf_request_complete(struct vb2_buffer *vb)
> -- 
> 2.19.1
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCHv2.1 2/4] vivid: use per-queue mutexes instead of one global mutex.

[Hans Verkuil]

>From d15ccd98e557a8cef1362cb591eb3011a6d8e1fd Mon Sep 17 00:00:00 2001
From: Hans Verkuil <hverkuil@xs4all.nl>
Date: Fri, 16 Nov 2018 12:14:31 +0100
Subject: [PATCH 2/4] vivid: use per-queue mutexes instead of one global mutex.

This avoids having to unlock the queue lock in stop_streaming.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
---
Changes since v2:
- add mutex_destroy()
---
 drivers/media/platform/vivid/vivid-core.c     | 26 +++++++++++++++----
 drivers/media/platform/vivid/vivid-core.h     |  5 ++++
 .../media/platform/vivid/vivid-kthread-cap.c  |  2 --
 .../media/platform/vivid/vivid-kthread-out.c  |  2 --
 drivers/media/platform/vivid/vivid-sdr-cap.c  |  2 --
 5 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 626e2b24a403..9a548eea75cd 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -624,6 +624,17 @@ static void vivid_dev_release(struct v4l2_device *v4l2_dev)
 	vfree(dev->bitmap_out);
 	tpg_free(&dev->tpg);
 	kfree(dev->query_dv_timings_qmenu);
+	if (dev->has_vid_cap)
+		mutex_destroy(&dev->vb_vid_cap_q_lock);
+	if (dev->has_vid_out)
+		mutex_destroy(&dev->vb_vid_out_q_lock);
+	if (dev->has_vbi_cap)
+		mutex_destroy(&dev->vb_vbi_cap_q_lock);
+	if (dev->has_vbi_out)
+		mutex_destroy(&dev->vb_vbi_out_q_lock);
+	if (dev->has_sdr_cap)
+		mutex_destroy(&dev->vb_sdr_cap_q_lock);
+	mutex_destroy(&dev->mutex);
 	kfree(dev);
 }

@@ -1075,7 +1086,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_vid_cap_q_lock);
+		q->lock = &dev->vb_vid_cap_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;

@@ -1096,7 +1108,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_vid_out_q_lock);
+		q->lock = &dev->vb_vid_out_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;

@@ -1117,7 +1130,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_vbi_cap_q_lock);
+		q->lock = &dev->vb_vbi_cap_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;

@@ -1138,7 +1152,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_vbi_out_q_lock);
+		q->lock = &dev->vb_vbi_out_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;

@@ -1158,7 +1173,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->mem_ops = vivid_mem_ops[allocator];
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 8;
-		q->lock = &dev->mutex;
+		mutex_init(&dev->vb_sdr_cap_q_lock);
+		q->lock = &dev->vb_sdr_cap_q_lock;
 		q->dev = dev->v4l2_dev.dev;
 		q->supports_requests = true;

diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
index 1891254c8f0b..337ccb563f9b 100644
--- a/drivers/media/platform/vivid/vivid-core.h
+++ b/drivers/media/platform/vivid/vivid-core.h
@@ -385,8 +385,10 @@ struct vivid_dev {
 	struct v4l2_rect		compose_cap;
 	struct v4l2_rect		crop_bounds_cap;
 	struct vb2_queue		vb_vid_cap_q;
+	struct mutex			vb_vid_cap_q_lock;
 	struct list_head		vid_cap_active;
 	struct vb2_queue		vb_vbi_cap_q;
+	struct mutex			vb_vbi_cap_q_lock;
 	struct list_head		vbi_cap_active;

 	/* thread for generating video capture stream */
@@ -413,8 +415,10 @@ struct vivid_dev {
 	struct v4l2_rect		compose_out;
 	struct v4l2_rect		compose_bounds_out;
 	struct vb2_queue		vb_vid_out_q;
+	struct mutex			vb_vid_out_q_lock;
 	struct list_head		vid_out_active;
 	struct vb2_queue		vb_vbi_out_q;
+	struct mutex			vb_vbi_out_q_lock;
 	struct list_head		vbi_out_active;

 	/* video loop precalculated rectangles */
@@ -459,6 +463,7 @@ struct vivid_dev {

 	/* SDR capture */
 	struct vb2_queue		vb_sdr_cap_q;
+	struct mutex			vb_sdr_cap_q_lock;
 	struct list_head		sdr_cap_active;
 	u32				sdr_pixelformat; /* v4l2 format id */
 	unsigned			sdr_buffersize;
diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c b/drivers/media/platform/vivid/vivid-kthread-cap.c
index eebfff2126be..d8bb59e9bcc7 100644
--- a/drivers/media/platform/vivid/vivid-kthread-cap.c
+++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
@@ -927,8 +927,6 @@ void vivid_stop_generating_vid_cap(struct vivid_dev *dev, bool *pstreaming)

 	/* shutdown control thread */
 	vivid_grab_controls(dev, false);
-	mutex_unlock(&dev->mutex);
 	kthread_stop(dev->kthread_vid_cap);
 	dev->kthread_vid_cap = NULL;
-	mutex_lock(&dev->mutex);
 }
diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c b/drivers/media/platform/vivid/vivid-kthread-out.c
index 5a14810eeb69..8b864cb0ed52 100644
--- a/drivers/media/platform/vivid/vivid-kthread-out.c
+++ b/drivers/media/platform/vivid/vivid-kthread-out.c
@@ -298,8 +298,6 @@ void vivid_stop_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)

 	/* shutdown control thread */
 	vivid_grab_controls(dev, false);
-	mutex_unlock(&dev->mutex);
 	kthread_stop(dev->kthread_vid_out);
 	dev->kthread_vid_out = NULL;
-	mutex_lock(&dev->mutex);
 }
diff --git a/drivers/media/platform/vivid/vivid-sdr-cap.c b/drivers/media/platform/vivid/vivid-sdr-cap.c
index dcdc80e272c2..5dfb598af742 100644
--- a/drivers/media/platform/vivid/vivid-sdr-cap.c
+++ b/drivers/media/platform/vivid/vivid-sdr-cap.c
@@ -305,10 +305,8 @@ static void sdr_cap_stop_streaming(struct vb2_queue *vq)
 	}

 	/* shutdown control thread */
-	mutex_unlock(&dev->mutex);
 	kthread_stop(dev->kthread_sdr_cap);
 	dev->kthread_sdr_cap = NULL;
-	mutex_lock(&dev->mutex);
 }

 static void sdr_cap_buf_request_complete(struct vb2_buffer *vb)
-- 
2.19.1

Re: [PATCHv2.1 2/4] vivid: use per-queue mutexes instead of one global mutex.

[Hans Verkuil]

On 11/19/2018 01:22 PM, Hans Verkuil wrote:
> From d15ccd98e557a8cef1362cb591eb3011a6d8e1fd Mon Sep 17 00:00:00 2001
> From: Hans Verkuil <hverkuil@xs4all.nl>
> Date: Fri, 16 Nov 2018 12:14:31 +0100
> Subject: [PATCH 2/4] vivid: use per-queue mutexes instead of one global mutex.

Oops, forgot to delete these four lines above. Just ignore them.

Regards,

	Hans

> 
> This avoids having to unlock the queue lock in stop_streaming.
> 
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
> ---
> Changes since v2:
> - add mutex_destroy()
> ---
>  drivers/media/platform/vivid/vivid-core.c     | 26 +++++++++++++++----
>  drivers/media/platform/vivid/vivid-core.h     |  5 ++++
>  .../media/platform/vivid/vivid-kthread-cap.c  |  2 --
>  .../media/platform/vivid/vivid-kthread-out.c  |  2 --
>  drivers/media/platform/vivid/vivid-sdr-cap.c  |  2 --
>  5 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
> index 626e2b24a403..9a548eea75cd 100644
> --- a/drivers/media/platform/vivid/vivid-core.c
> +++ b/drivers/media/platform/vivid/vivid-core.c
> @@ -624,6 +624,17 @@ static void vivid_dev_release(struct v4l2_device *v4l2_dev)
>  	vfree(dev->bitmap_out);
>  	tpg_free(&dev->tpg);
>  	kfree(dev->query_dv_timings_qmenu);
> +	if (dev->has_vid_cap)
> +		mutex_destroy(&dev->vb_vid_cap_q_lock);
> +	if (dev->has_vid_out)
> +		mutex_destroy(&dev->vb_vid_out_q_lock);
> +	if (dev->has_vbi_cap)
> +		mutex_destroy(&dev->vb_vbi_cap_q_lock);
> +	if (dev->has_vbi_out)
> +		mutex_destroy(&dev->vb_vbi_out_q_lock);
> +	if (dev->has_sdr_cap)
> +		mutex_destroy(&dev->vb_sdr_cap_q_lock);
> +	mutex_destroy(&dev->mutex);
>  	kfree(dev);
>  }
> 
> @@ -1075,7 +1086,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 2;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_vid_cap_q_lock);
> +		q->lock = &dev->vb_vid_cap_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
> 
> @@ -1096,7 +1108,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 2;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_vid_out_q_lock);
> +		q->lock = &dev->vb_vid_out_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
> 
> @@ -1117,7 +1130,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 2;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_vbi_cap_q_lock);
> +		q->lock = &dev->vb_vbi_cap_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
> 
> @@ -1138,7 +1152,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 2;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_vbi_out_q_lock);
> +		q->lock = &dev->vb_vbi_out_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
> 
> @@ -1158,7 +1173,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		q->mem_ops = vivid_mem_ops[allocator];
>  		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		q->min_buffers_needed = 8;
> -		q->lock = &dev->mutex;
> +		mutex_init(&dev->vb_sdr_cap_q_lock);
> +		q->lock = &dev->vb_sdr_cap_q_lock;
>  		q->dev = dev->v4l2_dev.dev;
>  		q->supports_requests = true;
> 
> diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
> index 1891254c8f0b..337ccb563f9b 100644
> --- a/drivers/media/platform/vivid/vivid-core.h
> +++ b/drivers/media/platform/vivid/vivid-core.h
> @@ -385,8 +385,10 @@ struct vivid_dev {
>  	struct v4l2_rect		compose_cap;
>  	struct v4l2_rect		crop_bounds_cap;
>  	struct vb2_queue		vb_vid_cap_q;
> +	struct mutex			vb_vid_cap_q_lock;
>  	struct list_head		vid_cap_active;
>  	struct vb2_queue		vb_vbi_cap_q;
> +	struct mutex			vb_vbi_cap_q_lock;
>  	struct list_head		vbi_cap_active;
> 
>  	/* thread for generating video capture stream */
> @@ -413,8 +415,10 @@ struct vivid_dev {
>  	struct v4l2_rect		compose_out;
>  	struct v4l2_rect		compose_bounds_out;
>  	struct vb2_queue		vb_vid_out_q;
> +	struct mutex			vb_vid_out_q_lock;
>  	struct list_head		vid_out_active;
>  	struct vb2_queue		vb_vbi_out_q;
> +	struct mutex			vb_vbi_out_q_lock;
>  	struct list_head		vbi_out_active;
> 
>  	/* video loop precalculated rectangles */
> @@ -459,6 +463,7 @@ struct vivid_dev {
> 
>  	/* SDR capture */
>  	struct vb2_queue		vb_sdr_cap_q;
> +	struct mutex			vb_sdr_cap_q_lock;
>  	struct list_head		sdr_cap_active;
>  	u32				sdr_pixelformat; /* v4l2 format id */
>  	unsigned			sdr_buffersize;
> diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c b/drivers/media/platform/vivid/vivid-kthread-cap.c
> index eebfff2126be..d8bb59e9bcc7 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
> @@ -927,8 +927,6 @@ void vivid_stop_generating_vid_cap(struct vivid_dev *dev, bool *pstreaming)
> 
>  	/* shutdown control thread */
>  	vivid_grab_controls(dev, false);
> -	mutex_unlock(&dev->mutex);
>  	kthread_stop(dev->kthread_vid_cap);
>  	dev->kthread_vid_cap = NULL;
> -	mutex_lock(&dev->mutex);
>  }
> diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c b/drivers/media/platform/vivid/vivid-kthread-out.c
> index 5a14810eeb69..8b864cb0ed52 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-out.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-out.c
> @@ -298,8 +298,6 @@ void vivid_stop_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
> 
>  	/* shutdown control thread */
>  	vivid_grab_controls(dev, false);
> -	mutex_unlock(&dev->mutex);
>  	kthread_stop(dev->kthread_vid_out);
>  	dev->kthread_vid_out = NULL;
> -	mutex_lock(&dev->mutex);
>  }
> diff --git a/drivers/media/platform/vivid/vivid-sdr-cap.c b/drivers/media/platform/vivid/vivid-sdr-cap.c
> index dcdc80e272c2..5dfb598af742 100644
> --- a/drivers/media/platform/vivid/vivid-sdr-cap.c
> +++ b/drivers/media/platform/vivid/vivid-sdr-cap.c
> @@ -305,10 +305,8 @@ static void sdr_cap_stop_streaming(struct vb2_queue *vq)
>  	}
> 
>  	/* shutdown control thread */
> -	mutex_unlock(&dev->mutex);
>  	kthread_stop(dev->kthread_sdr_cap);
>  	dev->kthread_sdr_cap = NULL;
> -	mutex_lock(&dev->mutex);
>  }
> 
>  static void sdr_cap_buf_request_complete(struct vb2_buffer *vb)
> 

Re: [PATCHv2 1/4] vb2: add waiting_in_dqbuf flag

[Tomasz Figa]

Hi Hans,

On Mon, Nov 19, 2018 at 8:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>
> However, if userspace dup()ped the video device filehandle, then it is
> possible to read or call DQBUF from two filehandles at the same time.
>
> It is also possible to call REQBUFS from one filehandle while the other
> is waiting for a buffer. This will remove all the buffers and reallocate
> new ones. Removing all the buffers isn't the problem here (that's already
> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
> aware that the buffers have changed.
>
> This is fixed by setting a flag whenever the lock is released while waiting
> for a buffer to arrive. And checking the flag where needed so we can return
> -EBUSY.
>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> Reported-by: syzbot+4180ff9ca6810b06c1e9@syzkaller.appspotmail.com
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 22 +++++++++++++++++++
>  include/media/videobuf2-core.h                |  1 +
>  2 files changed, 23 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 975ff5669f72..f7e7e633bcd7 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>                 return -EBUSY;
>         }
>
> +       if (q->waiting_in_dqbuf && *count) {
> +               dprintk(1, "another dup()ped fd is waiting for a buffer\n");

Actually, couldn't it also happen with the same FD just another thread?

That said, it's just a debugging message, so feel free to just add

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

Re: [PATCHv2.1 2/4] vivid: use per-queue mutexes instead of one global mutex.

[Tomasz Figa]

On Mon, Nov 19, 2018 at 9:22 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> From d15ccd98e557a8cef1362cb591eb3011a6d8e1fd Mon Sep 17 00:00:00 2001
> From: Hans Verkuil <hverkuil@xs4all.nl>
> Date: Fri, 16 Nov 2018 12:14:31 +0100
> Subject: [PATCH 2/4] vivid: use per-queue mutexes instead of one global mutex.
>
> This avoids having to unlock the queue lock in stop_streaming.
>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> Reported-by: syzbot+736c3aae4af7b50d9683@syzkaller.appspotmail.com
> ---
> Changes since v2:
> - add mutex_destroy()
> ---
>  drivers/media/platform/vivid/vivid-core.c     | 26 +++++++++++++++----
>  drivers/media/platform/vivid/vivid-core.h     |  5 ++++
>  .../media/platform/vivid/vivid-kthread-cap.c  |  2 --
>  .../media/platform/vivid/vivid-kthread-out.c  |  2 --
>  drivers/media/platform/vivid/vivid-sdr-cap.c  |  2 --
>  5 files changed, 26 insertions(+), 11 deletions(-)
>

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

Re: [PATCHv2 3/4] vb2 core: add new queue_setup_lock/unlock ops

[Tomasz Figa]

Hi Hans,

On Mon, Nov 19, 2018 at 8:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> If queue->lock is different from the video_device lock, then
> you need to serialize queue_setup with VIDIOC_S_FMT, and this
> should be done by the driver.
>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 51 +++++++++++++------
>  include/media/videobuf2-core.h                | 19 +++++++
>  2 files changed, 55 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index f7e7e633bcd7..269485920beb 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -465,7 +465,8 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>          */
>         if (q->num_buffers) {
>                 bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming ||
> -                                 q->cnt_wait_prepare != q->cnt_wait_finish;
> +                                 q->cnt_wait_prepare != q->cnt_wait_finish ||
> +                                 q->cnt_queue_setup_lock != q->cnt_queue_setup_unlock;
>
>                 if (unbalanced || debug) {
>                         pr_info("counters for queue %p:%s\n", q,
> @@ -473,10 +474,14 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>                         pr_info("     setup: %u start_streaming: %u stop_streaming: %u\n",
>                                 q->cnt_queue_setup, q->cnt_start_streaming,
>                                 q->cnt_stop_streaming);
> +                       pr_info("     queue_setup_lock: %u queue_setup_unlock: %u\n",
> +                               q->cnt_queue_setup_lock, q->cnt_queue_setup_unlock);
>                         pr_info("     wait_prepare: %u wait_finish: %u\n",
>                                 q->cnt_wait_prepare, q->cnt_wait_finish);
>                 }
>                 q->cnt_queue_setup = 0;
> +               q->cnt_queue_setup_lock = 0;
> +               q->cnt_queue_setup_unlock = 0;
>                 q->cnt_wait_prepare = 0;
>                 q->cnt_wait_finish = 0;
>                 q->cnt_start_streaming = 0;
> @@ -717,6 +722,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>         num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
>         memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
>         q->memory = memory;
> +       call_void_qop(q, queue_setup_lock, q);
>
>         /*
>          * Ask the driver how many buffers and planes per buffer it requires.
> @@ -725,22 +731,27 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>         ret = call_qop(q, queue_setup, q, &num_buffers, &num_planes,
>                        plane_sizes, q->alloc_devs);
>         if (ret)
> -               return ret;
> +               goto unlock;
>
>         /* Check that driver has set sane values */
> -       if (WARN_ON(!num_planes))
> -               return -EINVAL;
> +       if (WARN_ON(!num_planes)) {
> +               ret = -EINVAL;
> +               goto unlock;
> +       }
>
>         for (i = 0; i < num_planes; i++)
> -               if (WARN_ON(!plane_sizes[i]))
> -                       return -EINVAL;
> +               if (WARN_ON(!plane_sizes[i])) {
> +                       ret = -EINVAL;
> +                       goto unlock;
> +               }
>
>         /* Finally, allocate buffers and video memory */
>         allocated_buffers =
>                 __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
>         if (allocated_buffers == 0) {
>                 dprintk(1, "memory allocation failed\n");
> -               return -ENOMEM;
> +               ret = -ENOMEM;
> +               goto unlock;
>         }
>
>         /*
> @@ -775,19 +786,19 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>                  */
>         }
>
> -       mutex_lock(&q->mmap_lock);
>         q->num_buffers = allocated_buffers;

Why is it safe to stop acquiring mmap_lock here? Looking at other
code, I feel like it's assumed that q->num_buffers is actually
protected by it.

> +       call_void_qop(q, queue_setup_unlock, q);
>
>         if (ret < 0) {
>                 /*
>                  * Note: __vb2_queue_free() will subtract 'allocated_buffers'
>                  * from q->num_buffers.
>                  */
> +               mutex_lock(&q->mmap_lock);
>                 __vb2_queue_free(q, allocated_buffers);
>                 mutex_unlock(&q->mmap_lock);
>                 return ret;
>         }
> -       mutex_unlock(&q->mmap_lock);
>
>         /*
>          * Return the number of successfully allocated buffers
> @@ -795,8 +806,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>          */
>         *count = allocated_buffers;
>         q->waiting_for_buffers = !q->is_output;
> -
>         return 0;
> +
> +unlock:
> +       call_void_qop(q, queue_setup_unlock, q);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>
> @@ -813,10 +827,12 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>                 return -ENOBUFS;
>         }
>
> +       call_void_qop(q, queue_setup_lock, q);
>         if (!q->num_buffers) {
>                 if (q->waiting_in_dqbuf && *count) {
>                         dprintk(1, "another dup()ped fd is waiting for a buffer\n");
> -                       return -EBUSY;
> +                       ret = -EBUSY;
> +                       goto unlock;
>                 }
>                 memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
>                 q->memory = memory;
> @@ -837,14 +853,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>         ret = call_qop(q, queue_setup, q, &num_buffers,
>                        &num_planes, plane_sizes, q->alloc_devs);
>         if (ret)
> -               return ret;
> +               goto unlock;
>
>         /* Finally, allocate buffers and video memory */
>         allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
>                                 num_planes, plane_sizes);
>         if (allocated_buffers == 0) {
>                 dprintk(1, "memory allocation failed\n");
> -               return -ENOMEM;
> +               ret = -ENOMEM;
> +               goto unlock;
>         }
>
>         /*
> @@ -869,19 +886,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>                  */
>         }
>
> -       mutex_lock(&q->mmap_lock);
>         q->num_buffers += allocated_buffers;

Ditto.

> +       call_void_qop(q, queue_setup_unlock, q);
>
>         if (ret < 0) {
>                 /*
>                  * Note: __vb2_queue_free() will subtract 'allocated_buffers'
>                  * from q->num_buffers.
>                  */
> +               mutex_lock(&q->mmap_lock);
>                 __vb2_queue_free(q, allocated_buffers);
>                 mutex_unlock(&q->mmap_lock);
>                 return -ENOMEM;
>         }
> -       mutex_unlock(&q->mmap_lock);
>
>         /*
>          * Return the number of successfully allocated buffers
> @@ -890,6 +907,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>         *count = allocated_buffers;
>
>         return 0;
> +
> +unlock:
> +       call_void_qop(q, queue_setup_unlock, q);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_create_bufs);
>
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 613f22910174..92861b6fe7f8 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -333,6 +333,20 @@ struct vb2_buffer {
>   *                     \*num_buffers are being allocated additionally to
>   *                     q->num_buffers. If either \*num_planes or the requested
>   *                     sizes are invalid callback must return %-EINVAL.
> + * @queue_setup_lock:  called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS()
> + *                     to serialize @queue_setup with ioctls like
> + *                     VIDIOC_S_FMT() that change the buffer size. Only
> + *                     required if queue->lock differs from the mutex that is
> + *                     used to serialize the ioctls that change the buffer
> + *                     size. This callback should lock the ioctl serialization
> + *                     mutex.
> + * @queue_setup_unlock:        called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS()
> + *                     to serialize @queue_setup with ioctls like
> + *                     VIDIOC_S_FMT() that change the buffer size. Only
> + *                     required if queue->lock differs from the mutex that is
> + *                     used to serialize the ioctls that change the buffer
> + *                     size. This callback should unlock the ioctl
> + *                     serialization mutex.
>   * @wait_prepare:      release any locks taken while calling vb2 functions;
>   *                     it is called before an ioctl needs to wait for a new
>   *                     buffer to arrive; required to avoid a deadlock in
> @@ -403,10 +417,13 @@ struct vb2_ops {
>         int (*queue_setup)(struct vb2_queue *q,
>                            unsigned int *num_buffers, unsigned int *num_planes,
>                            unsigned int sizes[], struct device *alloc_devs[]);
> +       void (*queue_setup_lock)(struct vb2_queue *q);
> +       void (*queue_setup_unlock)(struct vb2_queue *q);
>
>         void (*wait_prepare)(struct vb2_queue *q);
>         void (*wait_finish)(struct vb2_queue *q);
>
> +

Stray blank line?

Best regards,
Tomasz

Re: [PATCHv2 4/4] vivid: add queue_setup_(un)lock ops

[Tomasz Figa]

On Mon, Nov 19, 2018 at 8:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Add these ops to serialize queue_setup with VIDIOC_S_FMT.
>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  drivers/media/platform/vivid/vivid-core.c    | 14 ++++++++++++++
>  drivers/media/platform/vivid/vivid-core.h    |  3 +++
>  drivers/media/platform/vivid/vivid-sdr-cap.c |  2 ++
>  drivers/media/platform/vivid/vivid-vbi-cap.c |  2 ++
>  drivers/media/platform/vivid/vivid-vbi-out.c |  2 ++
>  drivers/media/platform/vivid/vivid-vid-cap.c |  2 ++
>  drivers/media/platform/vivid/vivid-vid-out.c |  2 ++
>  7 files changed, 27 insertions(+)
>

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz