[RFC PATCH 0/6] v4l2 core: push ioctl lock down to ioctl handler

[RFC PATCH 0/6] v4l2 core: push ioctl lock down to ioctl handler

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

While working on the DMA fence API and the Request API it became
clear that the core ioctl scheme was done at a too-high level.

Being able to actually look at the struct passed as the ioctl
argument would help a lot in decide what lock(s) to take.

This patch series pushes the lock down into v4l2-ioctl.c, after
video_usercopy() was called.

The first patch is for the only driver that does not set
unlocked_ioctl to video_ioctl2: pvrusb2. It actually does
call it in its own unlocked_ioctl function.

Mike, can you test this patch? I tried to test it but my pvrusb2
fails in a USB transfer (unrelated to this patch). I'll mail you
separately with the details, since I've no idea what is going on.

The second patch pushes the lock down.

The third patch adds support for mem2mem devices by selecting
the correct queue lock (capture vs output): this was never
possible before.

Note: in practice it appears that all m2m devices use the same
lock for both capture and output queues. Perhaps this should
be standardized?

The final three patches require that queue->lock is always
set. There are some drivers that do not set this (and Ezequiel
will look at that and provide patches that will have to go
in between patch 3 and 4 of this RFC series), but without that
you will have performance issues with a blocking DQBUF (it
will never release the core ioctl serialization lock while
waiting for a new frame).

I have added a test for this to v4l2-compliance. We never tested
this before.

Another note: the gspca vb2 conversion series I posted yesterday
also remove the v4l2_disable_ioctl_locking() function, so that
cleans up another little locking-related dark corner in the core.

Regards,

	Hans

Hans Verkuil (6):
  pvrusb2: replace pvr2_v4l2_ioctl by video_ioctl2
  v4l2-core: push taking ioctl mutex down to ioctl handler.
  v4l2-ioctl.c: use correct vb2_queue lock for m2m devices
  videobuf2-core: require q->lock
  videobuf2: assume q->lock is always set
  v4l2-ioctl.c: assume queue->lock is always set

 .../media/common/videobuf2/videobuf2-core.c   | 22 ++---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 27 ++----
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c      | 83 +++++++------------
 drivers/media/v4l2-core/v4l2-dev.c            |  6 --
 drivers/media/v4l2-core/v4l2-ioctl.c          | 75 +++++++++++++++--
 drivers/media/v4l2-core/v4l2-subdev.c         | 17 +++-
 include/media/v4l2-dev.h                      |  9 --
 include/media/v4l2-ioctl.h                    | 12 ---
 include/media/videobuf2-core.h                |  2 -
 9 files changed, 133 insertions(+), 120 deletions(-)

-- 
2.17.0

[RFC PATCH 1/6] pvrusb2: replace pvr2_v4l2_ioctl by video_ioctl2

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

This driver is the only V4L driver that does not set unlocked_ioctl
to video_ioctl2.

The only thing that pvr2_v4l2_ioctl does besides calling video_ioctl2
is calling pvr2_hdw_commit_ctl(). Add pvr2_hdw_commit_ctl() calls to
the various ioctls that need this, and we can replace pvr2_v4l2_ioctl
by video_ioctl2.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 83 +++++++++---------------
 1 file changed, 31 insertions(+), 52 deletions(-)

diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index 9fdc57c1658f..e53a80b589a1 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -159,9 +159,12 @@ static int pvr2_s_std(struct file *file, void *priv, v4l2_std_id std)
 {
 	struct pvr2_v4l2_fh *fh = file->private_data;
 	struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
+	int ret;
 
-	return pvr2_ctrl_set_value(
+	ret = pvr2_ctrl_set_value(
 		pvr2_hdw_get_ctrl_by_id(hdw, PVR2_CID_STDCUR), std);
+	pvr2_hdw_commit_ctl(hdw);
+	return ret;
 }
 
 static int pvr2_querystd(struct file *file, void *priv, v4l2_std_id *std)
@@ -251,12 +254,15 @@ static int pvr2_s_input(struct file *file, void *priv, unsigned int inp)
 {
 	struct pvr2_v4l2_fh *fh = file->private_data;
 	struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
+	int ret;
 
 	if (inp >= fh->input_cnt)
 		return -EINVAL;
-	return pvr2_ctrl_set_value(
+	ret = pvr2_ctrl_set_value(
 			pvr2_hdw_get_ctrl_by_id(hdw, PVR2_CID_INPUT),
 			fh->input_map[inp]);
+	pvr2_hdw_commit_ctl(hdw);
+	return ret;
 }
 
 static int pvr2_enumaudio(struct file *file, void *priv, struct v4l2_audio *vin)
@@ -315,13 +321,16 @@ static int pvr2_s_tuner(struct file *file, void *priv, const struct v4l2_tuner *
 {
 	struct pvr2_v4l2_fh *fh = file->private_data;
 	struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
+	int ret;
 
 	if (vt->index != 0)
 		return -EINVAL;
 
-	return pvr2_ctrl_set_value(
+	ret = pvr2_ctrl_set_value(
 			pvr2_hdw_get_ctrl_by_id(hdw, PVR2_CID_AUDIOMODE),
 			vt->audmode);
+	pvr2_hdw_commit_ctl(hdw);
+	return ret;
 }
 
 static int pvr2_s_frequency(struct file *file, void *priv, const struct v4l2_frequency *vf)
@@ -353,8 +362,10 @@ static int pvr2_s_frequency(struct file *file, void *priv, const struct v4l2_fre
 		fv = (fv * 125) / 2;
 	else
 		fv = fv * 62500;
-	return pvr2_ctrl_set_value(
+	ret = pvr2_ctrl_set_value(
 			pvr2_hdw_get_ctrl_by_id(hdw,PVR2_CID_FREQUENCY),fv);
+	pvr2_hdw_commit_ctl(hdw);
+	return ret;
 }
 
 static int pvr2_g_frequency(struct file *file, void *priv, struct v4l2_frequency *vf)
@@ -470,6 +481,7 @@ static int pvr2_s_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format
 	vcp = pvr2_hdw_get_ctrl_by_id(hdw, PVR2_CID_VRES);
 	pvr2_ctrl_set_value(hcp, vf->fmt.pix.width);
 	pvr2_ctrl_set_value(vcp, vf->fmt.pix.height);
+	pvr2_hdw_commit_ctl(hdw);
 	return 0;
 }
 
@@ -597,9 +609,12 @@ static int pvr2_s_ctrl(struct file *file, void *priv, struct v4l2_control *vc)
 {
 	struct pvr2_v4l2_fh *fh = file->private_data;
 	struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
+	int ret;
 
-	return pvr2_ctrl_set_value(pvr2_hdw_get_ctrl_v4l(hdw, vc->id),
+	ret = pvr2_ctrl_set_value(pvr2_hdw_get_ctrl_v4l(hdw, vc->id),
 			vc->value);
+	pvr2_hdw_commit_ctl(hdw);
+	return ret;
 }
 
 static int pvr2_g_ext_ctrls(struct file *file, void *priv,
@@ -658,10 +673,12 @@ static int pvr2_s_ext_ctrls(struct file *file, void *priv,
 				ctrl->value);
 		if (ret) {
 			ctls->error_idx = idx;
-			return ret;
+			goto commit;
 		}
 	}
-	return 0;
+commit:
+	pvr2_hdw_commit_ctl(hdw);
+	return ret;
 }
 
 static int pvr2_try_ext_ctrls(struct file *file, void *priv,
@@ -764,23 +781,23 @@ static int pvr2_s_selection(struct file *file, void *priv,
 			pvr2_hdw_get_ctrl_by_id(hdw, PVR2_CID_CROPL),
 			sel->r.left);
 	if (ret != 0)
-		return -EINVAL;
+		goto commit;
 	ret = pvr2_ctrl_set_value(
 			pvr2_hdw_get_ctrl_by_id(hdw, PVR2_CID_CROPT),
 			sel->r.top);
 	if (ret != 0)
-		return -EINVAL;
+		goto commit;
 	ret = pvr2_ctrl_set_value(
 			pvr2_hdw_get_ctrl_by_id(hdw, PVR2_CID_CROPW),
 			sel->r.width);
 	if (ret != 0)
-		return -EINVAL;
+		goto commit;
 	ret = pvr2_ctrl_set_value(
 			pvr2_hdw_get_ctrl_by_id(hdw, PVR2_CID_CROPH),
 			sel->r.height);
-	if (ret != 0)
-		return -EINVAL;
-	return 0;
+commit:
+	pvr2_hdw_commit_ctl(hdw);
+	return ret;
 }
 
 static int pvr2_log_status(struct file *file, void *priv)
@@ -905,44 +922,6 @@ static void pvr2_v4l2_internal_check(struct pvr2_channel *chp)
 }
 
 
-static long pvr2_v4l2_ioctl(struct file *file,
-			   unsigned int cmd, unsigned long arg)
-{
-
-	struct pvr2_v4l2_fh *fh = file->private_data;
-	struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
-	long ret = -EINVAL;
-
-	if (pvrusb2_debug & PVR2_TRACE_V4LIOCTL)
-		v4l_printk_ioctl(pvr2_hdw_get_driver_name(hdw), cmd);
-
-	if (!pvr2_hdw_dev_ok(hdw)) {
-		pvr2_trace(PVR2_TRACE_ERROR_LEGS,
-			   "ioctl failed - bad or no context");
-		return -EFAULT;
-	}
-
-	ret = video_ioctl2(file, cmd, arg);
-
-	pvr2_hdw_commit_ctl(hdw);
-
-	if (ret < 0) {
-		if (pvrusb2_debug & PVR2_TRACE_V4LIOCTL) {
-			pvr2_trace(PVR2_TRACE_V4LIOCTL,
-				   "pvr2_v4l2_do_ioctl failure, ret=%ld command was:",
-ret);
-			v4l_printk_ioctl(pvr2_hdw_get_driver_name(hdw), cmd);
-		}
-	} else {
-		pvr2_trace(PVR2_TRACE_V4LIOCTL,
-			   "pvr2_v4l2_do_ioctl complete, ret=%ld (0x%lx)",
-			   ret, ret);
-	}
-	return ret;
-
-}
-
-
 static int pvr2_v4l2_release(struct file *file)
 {
 	struct pvr2_v4l2_fh *fhp = file->private_data;
@@ -1205,7 +1184,7 @@ static const struct v4l2_file_operations vdev_fops = {
 	.open       = pvr2_v4l2_open,
 	.release    = pvr2_v4l2_release,
 	.read       = pvr2_v4l2_read,
-	.unlocked_ioctl = pvr2_v4l2_ioctl,
+	.unlocked_ioctl = video_ioctl2,
 	.poll       = pvr2_v4l2_poll,
 };
 
-- 
2.17.0

[RFC PATCH 2/6] v4l2-core: push taking ioctl mutex down to ioctl handler.

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

The ioctl serialization mutex (vdev->lock or q->lock for vb2 queues)
was taken at the highest level in v4l2-dev.c. This prevents more
fine-grained locking since at that level we cannot examine the ioctl
arguments, we can only do that after video_usercopy is called.

So push the locking down to __video_do_ioctl() and subdev_do_ioctl_lock().

This also allows us to make a few functions in v4l2-ioctl.c static and
video_usercopy() is no longer exported.

The locking scheme is not changed by this patch, just pushed down.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/v4l2-core/v4l2-dev.c    |  6 ------
 drivers/media/v4l2-core/v4l2-ioctl.c  | 17 ++++++++++++++---
 drivers/media/v4l2-core/v4l2-subdev.c | 17 ++++++++++++++++-
 include/media/v4l2-dev.h              |  9 ---------
 include/media/v4l2-ioctl.h            | 12 ------------
 5 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c4f4357e9ca4..4ffd7d60a901 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -360,14 +360,8 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	int ret = -ENODEV;
 
 	if (vdev->fops->unlocked_ioctl) {
-		struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
-
-		if (lock && mutex_lock_interruptible(lock))
-			return -ERESTARTSYS;
 		if (video_is_registered(vdev))
 			ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
-		if (lock)
-			mutex_unlock(lock);
 	} else
 		ret = -ENOTTY;
 
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index de5d96dbe69e..a12c66ead30d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2619,14 +2619,14 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
 };
 #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
 
-bool v4l2_is_known_ioctl(unsigned int cmd)
+static bool v4l2_is_known_ioctl(unsigned int cmd)
 {
 	if (_IOC_NR(cmd) >= V4L2_IOCTLS)
 		return false;
 	return v4l2_ioctls[_IOC_NR(cmd)].ioctl == cmd;
 }
 
-struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
+static struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
 {
 	if (_IOC_NR(cmd) >= V4L2_IOCTLS)
 		return vdev->lock;
@@ -2679,6 +2679,7 @@ static long __video_do_ioctl(struct file *file,
 		unsigned int cmd, void *arg)
 {
 	struct video_device *vfd = video_devdata(file);
+	struct mutex *lock = v4l2_ioctl_get_lock(vfd, cmd);
 	const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops;
 	bool write_only = false;
 	struct v4l2_ioctl_info default_info;
@@ -2697,6 +2698,14 @@ static long __video_do_ioctl(struct file *file,
 	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
 		vfh = file->private_data;
 
+	if (lock && mutex_lock_interruptible(lock))
+		return -ERESTARTSYS;
+
+	if (!video_is_registered(vfd)) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
 	if (v4l2_is_known_ioctl(cmd)) {
 		info = &v4l2_ioctls[_IOC_NR(cmd)];
 
@@ -2752,6 +2761,9 @@ static long __video_do_ioctl(struct file *file,
 		}
 	}
 
+unlock:
+	if (lock)
+		mutex_unlock(lock);
 	return ret;
 }
 
@@ -2941,7 +2953,6 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	kvfree(mbuf);
 	return err;
 }
-EXPORT_SYMBOL(video_usercopy);
 
 long video_ioctl2(struct file *file,
 	       unsigned int cmd, unsigned long arg)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index f9eed938d348..6a7f7f75dfd7 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -502,10 +502,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	return 0;
 }
 
+static long subdev_do_ioctl_lock(struct file *file, unsigned int cmd, void *arg)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct mutex *lock = vdev->lock;
+	long ret = -ENODEV;
+
+	if (lock && mutex_lock_interruptible(lock))
+		return -ERESTARTSYS;
+	if (video_is_registered(vdev))
+		ret = subdev_do_ioctl(file, cmd, arg);
+	if (lock)
+		mutex_unlock(lock);
+	return ret;
+}
+
 static long subdev_ioctl(struct file *file, unsigned int cmd,
 	unsigned long arg)
 {
-	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
+	return video_usercopy(file, cmd, arg, subdev_do_ioctl_lock);
 }
 
 #ifdef CONFIG_COMPAT
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 73073f6ee48c..eb23f025aa6b 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -437,15 +437,6 @@ void video_device_release(struct video_device *vdev);
  */
 void video_device_release_empty(struct video_device *vdev);
 
-/**
- * v4l2_is_known_ioctl - Checks if a given cmd is a known V4L ioctl
- *
- * @cmd: ioctl command
- *
- * returns true if cmd is a known V4L2 ioctl
- */
-bool v4l2_is_known_ioctl(unsigned int cmd);
-
 /** v4l2_disable_ioctl_locking - mark that a given command
  *	shouldn't use core locking
  *
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index a7b3f7c75d62..a8dbf5b54b5c 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -658,18 +658,6 @@ void v4l_printk_ioctl(const char *prefix, unsigned int cmd);
 
 struct video_device;
 
-
-/**
- * v4l2_ioctl_get_lock - get the mutex (if any) that it is need to lock for
- *	a given command.
- *
- * @vdev: Pointer to struct &video_device.
- * @cmd: Ioctl name.
- *
- * .. note:: Internal use only. Should not be used outside V4L2 core.
- */
-struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned int cmd);
-
 /* names for fancy debug output */
 extern const char *v4l2_field_names[];
 extern const char *v4l2_type_names[];
-- 
2.17.0

[RFC PATCH 3/6] v4l2-ioctl.c: use correct vb2_queue lock for m2m devices

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

For m2m devices the vdev->queue lock was always taken instead of the
lock for the specific capture or output queue. Now that we pushed
the locking down into __video_do_ioctl() we can pick the correct
lock and improve the performance of m2m devices.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 59 +++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a12c66ead30d..b871b8fe5105 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -29,6 +29,7 @@
 #include <media/v4l2-device.h>
 #include <media/videobuf2-v4l2.h>
 #include <media/v4l2-mc.h>
+#include <media/v4l2-mem2mem.h>
 
 #include <trace/events/v4l2.h>
 
@@ -2626,12 +2627,64 @@ static bool v4l2_is_known_ioctl(unsigned int cmd)
 	return v4l2_ioctls[_IOC_NR(cmd)].ioctl == cmd;
 }
 
-static struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev, unsigned cmd)
+#if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
+static bool v4l2_ioctl_m2m_queue_is_output(unsigned int cmd, void *arg)
+{
+	switch (cmd) {
+	case VIDIOC_CREATE_BUFS: {
+		struct v4l2_create_buffers *cbufs = arg;
+
+		return V4L2_TYPE_IS_OUTPUT(cbufs->format.type);
+	}
+	case VIDIOC_REQBUFS: {
+		struct v4l2_requestbuffers *rbufs = arg;
+
+		return V4L2_TYPE_IS_OUTPUT(rbufs->type);
+	}
+	case VIDIOC_QBUF:
+	case VIDIOC_DQBUF:
+	case VIDIOC_QUERYBUF:
+	case VIDIOC_PREPARE_BUF: {
+		struct v4l2_buffer *buf = arg;
+
+		return V4L2_TYPE_IS_OUTPUT(buf->type);
+	}
+	case VIDIOC_EXPBUF: {
+		struct v4l2_exportbuffer *expbuf = arg;
+
+		return V4L2_TYPE_IS_OUTPUT(expbuf->type);
+	}
+	case VIDIOC_STREAMON:
+	case VIDIOC_STREAMOFF: {
+		int *type = arg;
+
+		return V4L2_TYPE_IS_OUTPUT(*type);
+	}
+	default:
+		return false;
+	}
+}
+#endif
+
+static struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev,
+					 struct v4l2_fh *vfh, unsigned cmd,
+					 void *arg)
 {
 	if (_IOC_NR(cmd) >= V4L2_IOCTLS)
 		return vdev->lock;
 	if (test_bit(_IOC_NR(cmd), vdev->disable_locking))
 		return NULL;
+#if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
+	if (vfh && vfh->m2m_ctx &&
+	    (v4l2_ioctls[_IOC_NR(cmd)].flags & INFO_FL_QUEUE)) {
+		bool is_output = v4l2_ioctl_m2m_queue_is_output(cmd, arg);
+		struct v4l2_m2m_queue_ctx *ctx = is_output ?
+			&vfh->m2m_ctx->out_q_ctx : &vfh->m2m_ctx->cap_q_ctx;
+
+		if (ctx->q.lock)
+			return ctx->q.lock;
+	}
+#endif
 	if (vdev->queue && vdev->queue->lock &&
 			(v4l2_ioctls[_IOC_NR(cmd)].flags & INFO_FL_QUEUE))
 		return vdev->queue->lock;
@@ -2679,7 +2732,7 @@ static long __video_do_ioctl(struct file *file,
 		unsigned int cmd, void *arg)
 {
 	struct video_device *vfd = video_devdata(file);
-	struct mutex *lock = v4l2_ioctl_get_lock(vfd, cmd);
+	struct mutex *lock;
 	const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops;
 	bool write_only = false;
 	struct v4l2_ioctl_info default_info;
@@ -2698,6 +2751,8 @@ static long __video_do_ioctl(struct file *file,
 	if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
 		vfh = file->private_data;
 
+	lock = v4l2_ioctl_get_lock(vfd, vfh, cmd, arg);
+
 	if (lock && mutex_lock_interruptible(lock))
 		return -ERESTARTSYS;
 
-- 
2.17.0

[RFC PATCH 4/6] videobuf2-core: require q->lock

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

Refuse to initialize a vb2 queue if there is no lock specified.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index d3f7bb33a54d..3b89ec5e0b2f 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2002,6 +2002,7 @@ int vb2_core_queue_init(struct vb2_queue *q)
 	if (WARN_ON(!q)			  ||
 	    WARN_ON(!q->ops)		  ||
 	    WARN_ON(!q->mem_ops)	  ||
+	    WARN_ON(!q->lock)		  ||
 	    WARN_ON(!q->type)		  ||
 	    WARN_ON(!q->io_modes)	  ||
 	    WARN_ON(!q->ops->queue_setup) ||
-- 
2.17.0

[RFC PATCH 5/6] videobuf2: assume q->lock is always set

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

Drop checks for q->lock. Drop calls to wait_finish/prepare, just lock/unlock
q->lock.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++---------
 .../media/common/videobuf2/videobuf2-v4l2.c   | 27 +++++--------------
 include/media/videobuf2-core.h                |  2 --
 3 files changed, 15 insertions(+), 35 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 3b89ec5e0b2f..8ca279a43549 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -462,8 +462,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 	 * counters to the kernel log.
 	 */
 	if (q->num_buffers) {
-		bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming ||
-				  q->cnt_wait_prepare != q->cnt_wait_finish;
+		bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming;
 
 		if (unbalanced || debug) {
 			pr_info("counters for queue %p:%s\n", q,
@@ -471,12 +470,8 @@ 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("     wait_prepare: %u wait_finish: %u\n",
-				q->cnt_wait_prepare, q->cnt_wait_finish);
 		}
 		q->cnt_queue_setup = 0;
-		q->cnt_wait_prepare = 0;
-		q->cnt_wait_finish = 0;
 		q->cnt_start_streaming = 0;
 		q->cnt_stop_streaming = 0;
 	}
@@ -1484,10 +1479,10 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 
 		/*
 		 * We are streaming and blocking, wait for another buffer to
-		 * become ready or for streamoff. Driver's lock is released to
+		 * become ready or for streamoff. The queue's lock is released to
 		 * allow streamoff or qbuf to be called while waiting.
 		 */
-		call_void_qop(q, wait_prepare, q);
+		mutex_unlock(q->lock);
 
 		/*
 		 * All locks have been released, it is safe to sleep now.
@@ -1501,7 +1496,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
 		 * We need to reevaluate both conditions again after reacquiring
 		 * the locks or return an error if one occurred.
 		 */
-		call_void_qop(q, wait_finish, q);
+		mutex_lock(q->lock);
 		if (ret) {
 			dprintk(1, "sleep was interrupted\n");
 			return ret;
@@ -2528,10 +2523,10 @@ static int vb2_thread(void *data)
 			vb = q->bufs[index++];
 			prequeue--;
 		} else {
-			call_void_qop(q, wait_finish, q);
+			mutex_lock(q->lock);
 			if (!threadio->stop)
 				ret = vb2_core_dqbuf(q, &index, NULL, 0);
-			call_void_qop(q, wait_prepare, q);
+			mutex_unlock(q->lock);
 			dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
 			if (!ret)
 				vb = q->bufs[index];
@@ -2543,12 +2538,12 @@ static int vb2_thread(void *data)
 		if (vb->state != VB2_BUF_STATE_ERROR)
 			if (threadio->fnc(vb, threadio->priv))
 				break;
-		call_void_qop(q, wait_finish, q);
+		mutex_lock(q->lock);
 		if (copy_timestamp)
 			vb->timestamp = ktime_get_ns();
 		if (!threadio->stop)
 			ret = vb2_core_qbuf(q, vb->index, NULL);
-		call_void_qop(q, wait_prepare, q);
+		mutex_unlock(q->lock);
 		if (ret || threadio->stop)
 			break;
 	}
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 886a2d8d5c6c..7d2172468f72 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -852,9 +852,8 @@ EXPORT_SYMBOL_GPL(_vb2_fop_release);
 int vb2_fop_release(struct file *file)
 {
 	struct video_device *vdev = video_devdata(file);
-	struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
 
-	return _vb2_fop_release(file, lock);
+	return _vb2_fop_release(file, vdev->queue->lock);
 }
 EXPORT_SYMBOL_GPL(vb2_fop_release);
 
@@ -862,12 +861,11 @@ ssize_t vb2_fop_write(struct file *file, const char __user *buf,
 		size_t count, loff_t *ppos)
 {
 	struct video_device *vdev = video_devdata(file);
-	struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
 	int err = -EBUSY;
 
 	if (!(vdev->queue->io_modes & VB2_WRITE))
 		return -EINVAL;
-	if (lock && mutex_lock_interruptible(lock))
+	if (mutex_lock_interruptible(vdev->queue->lock))
 		return -ERESTARTSYS;
 	if (vb2_queue_is_busy(vdev, file))
 		goto exit;
@@ -876,8 +874,7 @@ ssize_t vb2_fop_write(struct file *file, const char __user *buf,
 	if (vdev->queue->fileio)
 		vdev->queue->owner = file->private_data;
 exit:
-	if (lock)
-		mutex_unlock(lock);
+	mutex_unlock(vdev->queue->lock);
 	return err;
 }
 EXPORT_SYMBOL_GPL(vb2_fop_write);
@@ -886,12 +883,11 @@ ssize_t vb2_fop_read(struct file *file, char __user *buf,
 		size_t count, loff_t *ppos)
 {
 	struct video_device *vdev = video_devdata(file);
-	struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
 	int err = -EBUSY;
 
 	if (!(vdev->queue->io_modes & VB2_READ))
 		return -EINVAL;
-	if (lock && mutex_lock_interruptible(lock))
+	if (mutex_lock_interruptible(vdev->queue->lock))
 		return -ERESTARTSYS;
 	if (vb2_queue_is_busy(vdev, file))
 		goto exit;
@@ -900,8 +896,7 @@ ssize_t vb2_fop_read(struct file *file, char __user *buf,
 	if (vdev->queue->fileio)
 		vdev->queue->owner = file->private_data;
 exit:
-	if (lock)
-		mutex_unlock(lock);
+	mutex_unlock(vdev->queue->lock);
 	return err;
 }
 EXPORT_SYMBOL_GPL(vb2_fop_read);
@@ -910,17 +905,10 @@ __poll_t vb2_fop_poll(struct file *file, poll_table *wait)
 {
 	struct video_device *vdev = video_devdata(file);
 	struct vb2_queue *q = vdev->queue;
-	struct mutex *lock = q->lock ? q->lock : vdev->lock;
 	__poll_t res;
 	void *fileio;
 
-	/*
-	 * If this helper doesn't know how to lock, then you shouldn't be using
-	 * it but you should write your own.
-	 */
-	WARN_ON(!lock);
-
-	if (lock && mutex_lock_interruptible(lock))
+	if (mutex_lock_interruptible(q->lock))
 		return EPOLLERR;
 
 	fileio = q->fileio;
@@ -930,8 +918,7 @@ __poll_t vb2_fop_poll(struct file *file, poll_table *wait)
 	/* If fileio was started, then we have a new queue owner. */
 	if (!fileio && q->fileio)
 		q->owner = file->private_data;
-	if (lock)
-		mutex_unlock(lock);
+	mutex_unlock(q->lock);
 	return res;
 }
 EXPORT_SYMBOL_GPL(vb2_fop_poll);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f6818f732f34..d4e557b4f820 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -565,8 +565,6 @@ struct vb2_queue {
 	 * called. Used to check for unbalanced ops.
 	 */
 	u32				cnt_queue_setup;
-	u32				cnt_wait_prepare;
-	u32				cnt_wait_finish;
 	u32				cnt_start_streaming;
 	u32				cnt_stop_streaming;
 #endif
-- 
2.17.0

[RFC PATCH 6/6] v4l2-ioctl.c: assume queue->lock is always set

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

vb2_queue now expects a valid lock pointer, so drop the checks for
that in v4l2-ioctl.c.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index b871b8fe5105..15bfbc6ce6c0 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2681,12 +2681,11 @@ static struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev,
 		struct v4l2_m2m_queue_ctx *ctx = is_output ?
 			&vfh->m2m_ctx->out_q_ctx : &vfh->m2m_ctx->cap_q_ctx;
 
-		if (ctx->q.lock)
-			return ctx->q.lock;
+		return ctx->q.lock;
 	}
 #endif
-	if (vdev->queue && vdev->queue->lock &&
-			(v4l2_ioctls[_IOC_NR(cmd)].flags & INFO_FL_QUEUE))
+	if (vdev->queue &&
+	    (v4l2_ioctls[_IOC_NR(cmd)].flags & INFO_FL_QUEUE))
 		return vdev->queue->lock;
 	return vdev->lock;
 }
-- 
2.17.0

Re: [RFC PATCH 0/6] v4l2 core: push ioctl lock down to ioctl handler

[Hans Verkuil]

Hi Mike,

On 05/14/2018 01:55 PM, Hans Verkuil wrote:
> Mike, can you test this patch? I tried to test it but my pvrusb2
> fails in a USB transfer (unrelated to this patch). I'll mail you
> separately with the details, since I've no idea what is going on.

Never mind. After unplugging the power and plugging it back in it is
now working.

Not sure what happened, but at least I can test this now, and it looks
fine.

BTW, v4l2-compliance complains about a lot of things, and I get a lot
of sysfs kernel warnings when I unplug the device.

Regards,

	Has

Re: [RFC PATCH 1/6] pvrusb2: replace pvr2_v4l2_ioctl by video_ioctl2

[Hans Verkuil]

On 05/14/2018 01:55 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> This driver is the only V4L driver that does not set unlocked_ioctl
> to video_ioctl2.
> 
> The only thing that pvr2_v4l2_ioctl does besides calling video_ioctl2
> is calling pvr2_hdw_commit_ctl(). Add pvr2_hdw_commit_ctl() calls to
> the various ioctls that need this, and we can replace pvr2_v4l2_ioctl
> by video_ioctl2.
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>

Tested-by: Hans Verkuil <hansverk@cisco.com>

Re: [RFC PATCH 2/6] v4l2-core: push taking ioctl mutex down to ioctl handler.

[Ezequiel Garcia]

On Mon, 2018-05-14 at 13:55 +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> The ioctl serialization mutex (vdev->lock or q->lock for vb2 queues)
> was taken at the highest level in v4l2-dev.c. This prevents more
> fine-grained locking since at that level we cannot examine the ioctl
> arguments, we can only do that after video_usercopy is called.
> 
> So push the locking down to __video_do_ioctl() and subdev_do_ioctl_lock().
> 
> This also allows us to make a few functions in v4l2-ioctl.c static and
> video_usercopy() is no longer exported.
> 
> The locking scheme is not changed by this patch, just pushed down.
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c    |  6 ------
>  drivers/media/v4l2-core/v4l2-ioctl.c  | 17 ++++++++++++++---
>  drivers/media/v4l2-core/v4l2-subdev.c | 17 ++++++++++++++++-
>  include/media/v4l2-dev.h              |  9 ---------
>  include/media/v4l2-ioctl.h            | 12 ------------
>  5 files changed, 30 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index c4f4357e9ca4..4ffd7d60a901 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -360,14 +360,8 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	int ret = -ENODEV;
>  
>  	if (vdev->fops->unlocked_ioctl) {
> -		struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
> -
> -		if (lock && mutex_lock_interruptible(lock))
> -			return -ERESTARTSYS;
>  		if (video_is_registered(vdev))

This is_registered check looks spurious.

Other than that, it looks good.