Add debug messages to v4l2-ctrls

In preparation to cleanup the debug logic, simplify the dev_debug usage. In particular, make sure that a single flag is used to control each debug print.

Before this commit V4L2_DEV_DEBUG_STREAMING and V4L2_DEV_DEBUG_FOP were needed to enable read and write debugging. After this commit only the former is needed.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/media/v4l2-core/v4l2-dev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/media/v4l2-core/v4l2-dev.c +2 -4

index d7528f82a66a..34e4958663bf 100644
@@ -315,8 +315,7 @@ static ssize_t v4l2_read(struct file *filp, char __user *buf,
 		return -EINVAL;
 	if (video_is_registered(vdev))
 		ret = vdev->fops->read(filp, buf, sz, off);
-	if ((vdev->dev_debug & V4L2_DEV_DEBUG_FOP) &&
-	    (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING))
+	if (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING)
 		dprintk("%s: read: %zd (%d)\n",
 			video_device_node_name(vdev), sz, ret);
 	return ret;

@@ -332,8 +331,7 @@ static ssize_t v4l2_write(struct file *filp, const char __user *buf,
 		return -EINVAL;
 	if (video_is_registered(vdev))
 		ret = vdev->fops->write(filp, buf, sz, off);
-	if ((vdev->dev_debug & V4L2_DEV_DEBUG_FOP) &&
-	    (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING))
+	if (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING)
 		dprintk("%s: write: %zd (%d)\n",
 			video_device_node_name(vdev), sz, ret);
 	return ret;

Quoted 7 lines · Ezequiel Garcia

In preparation to cleanup the debug logic, simplify the dev_debug usage. In particular, make sure that a single flag is used to control each debug print.

Before this commit V4L2_DEV_DEBUG_STREAMING and V4L2_DEV_DEBUG_FOP were needed to enable read and write debugging. After this commit only the former is needed.

The original idea was that ioctls are logged with V4L2_DEV_DEBUG_IOCTL and file ops with V4L2_DEV_DEBUG_FOP. And to see the streaming ioctls or fops you would have to add V4L2_DEV_DEBUG_STREAMING in addition to DEBUG_IOCTL/FOP.

This patch changes the behavior in that the streaming fops are now solely controlled by V4L2_DEV_DEBUG_STREAMING.

I do agree with this change, but this requires that the same change is done for the streaming ioctls (DQBUF/QBUF) and that the documentation in Documentation/media/kapi/v4l2-dev.rst is updated (section "video device debugging").

Of course, the documentation should also mention the new dev_debug module parameter and the new debug flag for debugging controls.

Regards,

Hans

Quoted 30 lines

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/media/v4l2-core/v4l2-dev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/media/v4l2-core/v4l2-dev.c +2 -4

index d7528f82a66a..34e4958663bf 100644
@@ -315,8 +315,7 @@ static ssize_t v4l2_read(struct file *filp, char __user *buf,
 		return -EINVAL;
 	if (video_is_registered(vdev))
 		ret = vdev->fops->read(filp, buf, sz, off);
-	if ((vdev->dev_debug & V4L2_DEV_DEBUG_FOP) &&
-	    (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING))
+	if (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING)
 		dprintk("%s: read: %zd (%d)\n",
 			video_device_node_name(vdev), sz, ret);
 	return ret;

@@ -332,8 +331,7 @@ static ssize_t v4l2_write(struct file *filp, const char __user *buf,
 		return -EINVAL;
 	if (video_is_registered(vdev))
 		ret = vdev->fops->write(filp, buf, sz, off);
-	if ((vdev->dev_debug & V4L2_DEV_DEBUG_FOP) &&
-	    (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING))
+	if (vdev->dev_debug & V4L2_DEV_DEBUG_STREAMING)
 		dprintk("%s: write: %zd (%d)\n",
 			video_device_node_name(vdev), sz, ret);
 	return ret;

Currently, the v4l2 control code is a bit silent on errors. Now that we have a debug parameter, it's possible to enable debugging messages here.

Add debug messages on (hopefully) most of the error paths. Since it's really hard to associate all these errors to video device instance, we are forced to use the global debug parameter only.

Add a warning in case the user enables control debugging at the per-device dev_debug level.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/media/v4l2-core/v4l2-ctrls.c  | 93 +++++++++++++++++++++------
 drivers/media/v4l2-core/v4l2-dev.c    |  2 +
 drivers/media/v4l2-core/v4l2-ioctl.c  |  8 +--
 drivers/media/v4l2-core/v4l2-subdev.c |  4 +-
 include/media/v4l2-ctrls.h            |  9 ++-
 include/media/v4l2-ioctl.h            |  2 +
 6 files changed, 91 insertions(+), 27 deletions(-)

drivers/media/v4l2-core/v4l2-ctrls.c +59 -13

index b79d3bbd8350..af8ad83d1e08 100644
@@ -18,6 +18,8 @@
     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#define pr_fmt(fmt) "v4l2-ctrls: " fmt
+
 #include <linux/ctype.h>
 #include <linux/mm.h>
 #include <linux/slab.h>

@@ -28,6 +30,14 @@
 #include <media/v4l2-event.h>
 #include <media/v4l2-dev.h>
 
+extern unsigned int videodev_debug;
+
+#define dprintk(fmt, arg...) do {					\
+	if (videodev_debug & V4L2_DEV_DEBUG_CTRL)			\
+		printk(KERN_DEBUG pr_fmt("%s: " fmt),			\
+		       __func__, ##arg);				\
+} while (0)
+
 #define has_op(master, op) \
 	(master->ops && master->ops->op)
 #define call_op(master, op) \

@@ -1952,8 +1962,11 @@ static int validate_new(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr p_new)
 	unsigned idx;
 	int err = 0;
 
-	for (idx = 0; !err && idx < ctrl->elems; idx++)
+	for (idx = 0; !err && idx < ctrl->elems; idx++) {
 		err = ctrl->type_ops->validate(ctrl, idx, p_new);
+		if (err)
+			dprintk("failed to validate control id 0x%x (%d)\n", ctrl->id, err);
+	}
 	return err;
 }
 

@@ -3136,20 +3149,28 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		if (cs->which &&
 		    cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
 		    cs->which != V4L2_CTRL_WHICH_REQUEST_VAL &&
-		    V4L2_CTRL_ID2WHICH(id) != cs->which)
+		    V4L2_CTRL_ID2WHICH(id) != cs->which) {
+			dprintk("invalid which 0x%x or control id 0x%x\n", cs->which, id);
 			return -EINVAL;
+		}
 
 		/* Old-style private controls are not allowed for
 		   extended controls */
-		if (id >= V4L2_CID_PRIVATE_BASE)
+		if (id >= V4L2_CID_PRIVATE_BASE) {
+			dprintk("old-style private controls not allowed for extended controls\n");
 			return -EINVAL;
+		}
 		ref = find_ref_lock(hdl, id);
-		if (ref == NULL)
+		if (ref == NULL) {
+			dprintk("cannot find control id 0x%x\n", id);
 			return -EINVAL;
+		}
 		h->ref = ref;
 		ctrl = ref->ctrl;
-		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
+		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
+			dprintk("control id 0x%x is disabled\n", id);
 			return -EINVAL;
+		}
 
 		if (ctrl->cluster[0]->ncontrols > 1)
 			have_clusters = true;

@@ -3159,10 +3180,16 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 			unsigned tot_size = ctrl->elems * ctrl->elem_size;
 
 			if (c->size < tot_size) {
+				/*
+				 * In the get case the application first queries
+				 * to obtain the size of the control.
+				 */
 				if (get) {
 					c->size = tot_size;
 					return -ENOSPC;
 				}
+				dprintk("pointer control id 0x%x size too small, %d bytes but %d bytes needed\n",
+					id, c->size, tot_size);
 				return -EFAULT;
 			}
 			c->size = tot_size;

@@ -3534,16 +3561,20 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
 
 		cs->error_idx = i;
 
-		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
+		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY) {
+			dprintk("control id 0x%x is read-only\n", ctrl->id);
 			return -EACCES;
+		}
 		/* This test is also done in try_set_control_cluster() which
 		   is called in atomic context, so that has the final say,
 		   but it makes sense to do an up-front check as well. Once
 		   an error occurs in try_set_control_cluster() some other
 		   controls may have been set already and we want to do a
 		   best-effort to avoid that. */
-		if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
+		if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED)) {
+			dprintk("control id 0x%x is grabbed, cannot set\n", ctrl->id);
 			return -EBUSY;
+		}
 		/*
 		 * Skip validation for now if the payload needs to be copied
 		 * from userspace into kernelspace. We'll validate those later.

@@ -3576,7 +3607,8 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master)
 }
 
 /* Try or try-and-set controls */
-static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
+static int try_set_ext_ctrls_common(struct video_device *vdev,
+				    struct v4l2_fh *fh,
 				    struct v4l2_ctrl_handler *hdl,
 				    struct v4l2_ext_controls *cs, bool set)
 {

@@ -3588,13 +3620,17 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
 	cs->error_idx = cs->count;
 
 	/* Default value cannot be changed */
-	if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
+	if (cs->which == V4L2_CTRL_WHICH_DEF_VAL) {
+		dprintk("%s: cannot change default value\n", video_device_node_name(vdev));
 		return -EINVAL;
+	}
 
 	cs->which = V4L2_CTRL_ID2WHICH(cs->which);
 
-	if (hdl == NULL)
+	if (hdl == NULL) {
+		dprintk("%s: invalid null control handler\n", video_device_node_name(vdev));
 		return -EINVAL;
+	}
 
 	if (cs->count == 0)
 		return class_check(hdl, cs->which);

@@ -3691,7 +3727,8 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
 	return ret;
 }
 
-static int try_set_ext_ctrls(struct v4l2_fh *fh,
+static int try_set_ext_ctrls(struct video_device *vdev,
+			     struct v4l2_fh *fh,
 			     struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
 			     struct v4l2_ext_controls *cs, bool set)
 {

@@ -3700,21 +3737,32 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 	int ret;
 
 	if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) {
-		if (!mdev || cs->request_fd < 0)
+		if (!mdev) {
+			dprintk("%s: missing media device\n", video_device_node_name(vdev));
+			return -EINVAL;
+		}
+
+		if (cs->request_fd < 0) {
+			dprintk("%s: invalid request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			return -EINVAL;
+		}
 
 		req = media_request_get_by_fd(mdev, cs->request_fd);
-		if (IS_ERR(req))
+		if (IS_ERR(req)) {
+			dprintk("%s: cannot find request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			return PTR_ERR(req);
+		}
 

drivers/media/v4l2-core/v4l2-ctrls.c +13 -6

 			media_request_unlock_for_update(req);
 			media_request_put(req);
 			return PTR_ERR(obj);

@@ -3723,7 +3771,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 				   req_obj);
 	}
 
-	ret = try_set_ext_ctrls_common(fh, hdl, cs, set);
+	ret = try_set_ext_ctrls_common(vdev, fh, hdl, cs, set);
+	if (ret)
+		dprintk("%s: try_set_ext_ctrls_common failed (%d)\n", video_device_node_name(vdev), ret);
 
 	if (obj) {
 		media_request_unlock_for_update(req);

@@ -3734,17 +3784,22 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 	return ret;
 }
 
-int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
+int v4l2_try_ext_ctrls(struct video_device *vdev,
+		       struct v4l2_ctrl_handler *hdl,
+		       struct media_device *mdev,
 		       struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(NULL, hdl, mdev, cs, false);
+	return try_set_ext_ctrls(vdev, NULL, hdl, mdev, cs, false);
 }
 EXPORT_SYMBOL(v4l2_try_ext_ctrls);
 
-int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
-		     struct media_device *mdev, struct v4l2_ext_controls *cs)
+int v4l2_s_ext_ctrls(struct video_device *vdev,
+		     struct v4l2_fh *fh,
+		     struct v4l2_ctrl_handler *hdl,
+		     struct media_device *mdev,
+		     struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(fh, hdl, mdev, cs, true);
+	return try_set_ext_ctrls(vdev, fh, hdl, mdev, cs, true);
 }
 EXPORT_SYMBOL(v4l2_s_ext_ctrls);
 

drivers/media/v4l2-core/v4l2-dev.c +0 -0

index 39d22bfbe420..c6bcc9ea1122 100644

drivers/media/v4l2-core/v4l2-dev.c +0 -0

 	vdev->dev_debug = value;
 	return len;
 }

drivers/media/v4l2-core/v4l2-ioctl.c +0 -0

index 6f707466b5d2..078f75eb0a19 100644

drivers/media/v4l2-core/v4l2-ioctl.c +2 -2

 	if (ops->vidioc_s_ext_ctrls == NULL)
 		return -ENOTTY;
 	return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) :

@@ -2199,9 +2199,9 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 
 	p->error_idx = p->count;
 	if (vfh && vfh->ctrl_handler)
-		return v4l2_try_ext_ctrls(vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
+		return v4l2_try_ext_ctrls(vfd, vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	if (vfd->ctrl_handler)
-		return v4l2_try_ext_ctrls(vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
+		return v4l2_try_ext_ctrls(vfd, vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	if (ops->vidioc_try_ext_ctrls == NULL)
 		return -ENOTTY;
 	return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) :

drivers/media/v4l2-core/v4l2-subdev.c +2 -2

index f5f0d71ec745..3a09d4441ca3 100644
@@ -228,13 +228,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_S_EXT_CTRLS:
 		if (!vfh->ctrl_handler)
 			return -ENOTTY;
-		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler,
+		return v4l2_s_ext_ctrls(vdev, vfh, vfh->ctrl_handler,
 					sd->v4l2_dev->mdev, arg);
 
 	case VIDIOC_TRY_EXT_CTRLS:
 		if (!vfh->ctrl_handler)
 			return -ENOTTY;
-		return v4l2_try_ext_ctrls(vfh->ctrl_handler,
+		return v4l2_try_ext_ctrls(vdev, vfh->ctrl_handler,
 					  sd->v4l2_dev->mdev, arg);
 
 	case VIDIOC_DQEVENT:

include/media/v4l2-ctrls.h +7 -2

index d63cf227b0ab..0e38a59c80dd 100644
@@ -1272,13 +1272,15 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
  * v4l2_try_ext_ctrls - Helper function to implement
  *	:ref:`VIDIOC_TRY_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl
  *
+ * @vdev: pointer to &struct video_device
  * @hdl: pointer to &struct v4l2_ctrl_handler
  * @mdev: pointer to &struct media_device
  * @c: pointer to &struct v4l2_ext_controls
  *
  * If hdl == NULL then they will all return -EINVAL.
  */
-int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+int v4l2_try_ext_ctrls(struct video_device *vdev,
+		       struct v4l2_ctrl_handler *hdl,
 		       struct media_device *mdev,
 		       struct v4l2_ext_controls *c);
 

@@ -1286,6 +1288,7 @@ int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,
  * v4l2_s_ext_ctrls - Helper function to implement
  *	:ref:`VIDIOC_S_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl
  *
+ * @vdev: pointer to &struct video_device
  * @fh: pointer to &struct v4l2_fh
  * @hdl: pointer to &struct v4l2_ctrl_handler
  * @mdev: pointer to &struct media_device

@@ -1293,7 +1296,9 @@ int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,
  *
  * If hdl == NULL then they will all return -EINVAL.
  */
-int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+int v4l2_s_ext_ctrls(struct video_device *vdev,
+		     struct v4l2_fh *fh,
+		     struct v4l2_ctrl_handler *hdl,
 		     struct media_device *mdev,
 		     struct v4l2_ext_controls *c);
 

include/media/v4l2-ioctl.h +0 -0

index 8533ece5026e..0ecd4e3e76a4 100644

Quoted 198 lines · Ezequiel Garcia

Currently, the v4l2 control code is a bit silent on errors. Now that we have a debug parameter, it's possible to enable debugging messages here.

Add debug messages on (hopefully) most of the error paths. Since it's really hard to associate all these errors to video device instance, we are forced to use the global debug parameter only.

Add a warning in case the user enables control debugging at the per-device dev_debug level.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/media/v4l2-core/v4l2-ctrls.c  | 93 +++++++++++++++++++++------
 drivers/media/v4l2-core/v4l2-dev.c    |  2 +
 drivers/media/v4l2-core/v4l2-ioctl.c  |  8 +--
 drivers/media/v4l2-core/v4l2-subdev.c |  4 +-
 include/media/v4l2-ctrls.h            |  9 ++-
 include/media/v4l2-ioctl.h            |  2 +
 6 files changed, 91 insertions(+), 27 deletions(-)

drivers/media/v4l2-core/v4l2-ctrls.c +59 -13

index b79d3bbd8350..af8ad83d1e08 100644
@@ -18,6 +18,8 @@
     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#define pr_fmt(fmt) "v4l2-ctrls: " fmt
+
 #include <linux/ctype.h>
 #include <linux/mm.h>
 #include <linux/slab.h>

@@ -28,6 +30,14 @@
 #include <media/v4l2-event.h>
 #include <media/v4l2-dev.h>
 
+extern unsigned int videodev_debug;
+
+#define dprintk(fmt, arg...) do {					\
+	if (videodev_debug & V4L2_DEV_DEBUG_CTRL)			\
+		printk(KERN_DEBUG pr_fmt("%s: " fmt),			\
+		       __func__, ##arg);				\
+} while (0)
+
 #define has_op(master, op) \
 	(master->ops && master->ops->op)
 #define call_op(master, op) \

@@ -1952,8 +1962,11 @@ static int validate_new(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr p_new)
 	unsigned idx;
 	int err = 0;
 
-	for (idx = 0; !err && idx < ctrl->elems; idx++)
+	for (idx = 0; !err && idx < ctrl->elems; idx++) {
 		err = ctrl->type_ops->validate(ctrl, idx, p_new);
+		if (err)
+			dprintk("failed to validate control id 0x%x (%d)\n", ctrl->id, err);
+	}
 	return err;
 }
 

@@ -3136,20 +3149,28 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		if (cs->which &&
 		    cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
 		    cs->which != V4L2_CTRL_WHICH_REQUEST_VAL &&
-		    V4L2_CTRL_ID2WHICH(id) != cs->which)
+		    V4L2_CTRL_ID2WHICH(id) != cs->which) {
+			dprintk("invalid which 0x%x or control id 0x%x\n", cs->which, id);
 			return -EINVAL;
+		}
 
 		/* Old-style private controls are not allowed for
 		   extended controls */
-		if (id >= V4L2_CID_PRIVATE_BASE)
+		if (id >= V4L2_CID_PRIVATE_BASE) {
+			dprintk("old-style private controls not allowed for extended controls\n");
 			return -EINVAL;
+		}
 		ref = find_ref_lock(hdl, id);
-		if (ref == NULL)
+		if (ref == NULL) {
+			dprintk("cannot find control id 0x%x\n", id);
 			return -EINVAL;
+		}
 		h->ref = ref;
 		ctrl = ref->ctrl;
-		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
+		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
+			dprintk("control id 0x%x is disabled\n", id);
 			return -EINVAL;
+		}
 
 		if (ctrl->cluster[0]->ncontrols > 1)
 			have_clusters = true;

@@ -3159,10 +3180,16 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 			unsigned tot_size = ctrl->elems * ctrl->elem_size;
 
 			if (c->size < tot_size) {
+				/*
+				 * In the get case the application first queries
+				 * to obtain the size of the control.
+				 */
 				if (get) {
 					c->size = tot_size;
 					return -ENOSPC;
 				}
+				dprintk("pointer control id 0x%x size too small, %d bytes but %d bytes needed\n",
+					id, c->size, tot_size);
 				return -EFAULT;
 			}
 			c->size = tot_size;

@@ -3534,16 +3561,20 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
 
 		cs->error_idx = i;
 
-		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
+		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY) {
+			dprintk("control id 0x%x is read-only\n", ctrl->id);
 			return -EACCES;
+		}
 		/* This test is also done in try_set_control_cluster() which
 		   is called in atomic context, so that has the final say,
 		   but it makes sense to do an up-front check as well. Once
 		   an error occurs in try_set_control_cluster() some other
 		   controls may have been set already and we want to do a
 		   best-effort to avoid that. */
-		if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
+		if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED)) {
+			dprintk("control id 0x%x is grabbed, cannot set\n", ctrl->id);
 			return -EBUSY;
+		}
 		/*
 		 * Skip validation for now if the payload needs to be copied
 		 * from userspace into kernelspace. We'll validate those later.

@@ -3576,7 +3607,8 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master)
 }
 
 /* Try or try-and-set controls */
-static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
+static int try_set_ext_ctrls_common(struct video_device *vdev,
+				    struct v4l2_fh *fh,
 				    struct v4l2_ctrl_handler *hdl,
 				    struct v4l2_ext_controls *cs, bool set)
 {

@@ -3588,13 +3620,17 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
 	cs->error_idx = cs->count;
 
 	/* Default value cannot be changed */
-	if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
+	if (cs->which == V4L2_CTRL_WHICH_DEF_VAL) {
+		dprintk("%s: cannot change default value\n", video_device_node_name(vdev));
 		return -EINVAL;
+	}
 
 	cs->which = V4L2_CTRL_ID2WHICH(cs->which);
 
-	if (hdl == NULL)
+	if (hdl == NULL) {
+		dprintk("%s: invalid null control handler\n", video_device_node_name(vdev));
 		return -EINVAL;
+	}
 
 	if (cs->count == 0)
 		return class_check(hdl, cs->which);

@@ -3691,7 +3727,8 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
 	return ret;
 }
 
-static int try_set_ext_ctrls(struct v4l2_fh *fh,
+static int try_set_ext_ctrls(struct video_device *vdev,
+			     struct v4l2_fh *fh,
 			     struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
 			     struct v4l2_ext_controls *cs, bool set)
 {

@@ -3700,21 +3737,32 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 	int ret;
 
 	if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) {
-		if (!mdev || cs->request_fd < 0)
+		if (!mdev) {
+			dprintk("%s: missing media device\n", video_device_node_name(vdev));
+			return -EINVAL;
+		}
+
+		if (cs->request_fd < 0) {
+			dprintk("%s: invalid request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			return -EINVAL;
+		}
 
 		req = media_request_get_by_fd(mdev, cs->request_fd);
-		if (IS_ERR(req))
+		if (IS_ERR(req)) {
+			dprintk("%s: cannot find request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			return PTR_ERR(req);
+		}
 

drivers/media/v4l2-core/v4l2-ctrls.c static int try_set_ext_ctrls(struct v4l2_fh *fh, · line 3765

Quoted 10 lines

drivers/media/v4l2-core/v4l2-ctrls.c +2 -0

 		ret = media_request_lock_for_update(req);
 		if (ret) {
+			dprintk("%s: cannot lock request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			media_request_put(req);
 			return ret;
 		}
 
 		obj = v4l2_ctrls_find_req_obj(hdl, req, set);
 		if (IS_ERR(obj)) {
+			dprintk("%s: cannot find request object for request fd %d\n", video_device_node_name(vdev), cs->request_fd);

These lines are way too long. Just add a newline after the first comma.

Same elsewhere.

Quoted 50 lines

drivers/media/v4l2-core/v4l2-ctrls.c +13 -6

 			media_request_unlock_for_update(req);
 			media_request_put(req);
 			return PTR_ERR(obj);

@@ -3723,7 +3771,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 				   req_obj);
 	}
 
-	ret = try_set_ext_ctrls_common(fh, hdl, cs, set);
+	ret = try_set_ext_ctrls_common(vdev, fh, hdl, cs, set);
+	if (ret)
+		dprintk("%s: try_set_ext_ctrls_common failed (%d)\n", video_device_node_name(vdev), ret);
 
 	if (obj) {
 		media_request_unlock_for_update(req);

@@ -3734,17 +3784,22 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 	return ret;
 }
 
-int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
+int v4l2_try_ext_ctrls(struct video_device *vdev,
+		       struct v4l2_ctrl_handler *hdl,
+		       struct media_device *mdev,
 		       struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(NULL, hdl, mdev, cs, false);
+	return try_set_ext_ctrls(vdev, NULL, hdl, mdev, cs, false);
 }
 EXPORT_SYMBOL(v4l2_try_ext_ctrls);
 
-int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
-		     struct media_device *mdev, struct v4l2_ext_controls *cs)
+int v4l2_s_ext_ctrls(struct video_device *vdev,
+		     struct v4l2_fh *fh,
+		     struct v4l2_ctrl_handler *hdl,
+		     struct media_device *mdev,
+		     struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(fh, hdl, mdev, cs, true);
+	return try_set_ext_ctrls(vdev, fh, hdl, mdev, cs, true);
 }
 EXPORT_SYMBOL(v4l2_s_ext_ctrls);
 

drivers/media/v4l2-core/v4l2-dev.c +0 -0

index 39d22bfbe420..c6bcc9ea1122 100644
@@ -83,6 +83,8 @@ static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr,
 	if (res)
 		return res;
 

drivers/media/v4l2-core/v4l2-dev.c static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr, · line 87

Quoted 2 lines

drivers/media/v4l2-core/v4l2-dev.c +2 -0

+	if (value & V4L2_DEV_DEBUG_CTRL)
+		pr_warn_once("Warning: V4L2_DEV_DEBUG_CTRL cannot be enabled via the dev_debug attribute.\n");

Actually, you can for those functions that have the vdev pointer. And I think you can pass vdev on to more functions. Certainly validate_ctrls() and possibly all of them.

Quoted 264 lines · Hans Verkuil

Quoted 198 lines · Ezequiel Garcia

Currently, the v4l2 control code is a bit silent on errors. Now that we have a debug parameter, it's possible to enable debugging messages here.

Add debug messages on (hopefully) most of the error paths. Since it's really hard to associate all these errors to video device instance, we are forced to use the global debug parameter only.

Add a warning in case the user enables control debugging at the per-device dev_debug level.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/media/v4l2-core/v4l2-ctrls.c  | 93 +++++++++++++++++++++------
 drivers/media/v4l2-core/v4l2-dev.c    |  2 +
 drivers/media/v4l2-core/v4l2-ioctl.c  |  8 +--
 drivers/media/v4l2-core/v4l2-subdev.c |  4 +-
 include/media/v4l2-ctrls.h            |  9 ++-
 include/media/v4l2-ioctl.h            |  2 +
 6 files changed, 91 insertions(+), 27 deletions(-)

drivers/media/v4l2-core/v4l2-ctrls.c +59 -13

index b79d3bbd8350..af8ad83d1e08 100644
@@ -18,6 +18,8 @@
     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#define pr_fmt(fmt) "v4l2-ctrls: " fmt
+
 #include <linux/ctype.h>
 #include <linux/mm.h>
 #include <linux/slab.h>

@@ -28,6 +30,14 @@
 #include <media/v4l2-event.h>
 #include <media/v4l2-dev.h>
 
+extern unsigned int videodev_debug;
+
+#define dprintk(fmt, arg...) do {					\
+	if (videodev_debug & V4L2_DEV_DEBUG_CTRL)			\
+		printk(KERN_DEBUG pr_fmt("%s: " fmt),			\
+		       __func__, ##arg);				\
+} while (0)
+
 #define has_op(master, op) \
 	(master->ops && master->ops->op)
 #define call_op(master, op) \

@@ -1952,8 +1962,11 @@ static int validate_new(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr p_new)
 	unsigned idx;
 	int err = 0;
 
-	for (idx = 0; !err && idx < ctrl->elems; idx++)
+	for (idx = 0; !err && idx < ctrl->elems; idx++) {
 		err = ctrl->type_ops->validate(ctrl, idx, p_new);
+		if (err)
+			dprintk("failed to validate control id 0x%x (%d)\n", ctrl->id, err);
+	}
 	return err;
 }
 

@@ -3136,20 +3149,28 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		if (cs->which &&
 		    cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
 		    cs->which != V4L2_CTRL_WHICH_REQUEST_VAL &&
-		    V4L2_CTRL_ID2WHICH(id) != cs->which)
+		    V4L2_CTRL_ID2WHICH(id) != cs->which) {
+			dprintk("invalid which 0x%x or control id 0x%x\n", cs->which, id);
 			return -EINVAL;
+		}
 
 		/* Old-style private controls are not allowed for
 		   extended controls */
-		if (id >= V4L2_CID_PRIVATE_BASE)
+		if (id >= V4L2_CID_PRIVATE_BASE) {
+			dprintk("old-style private controls not allowed for extended controls\n");
 			return -EINVAL;
+		}
 		ref = find_ref_lock(hdl, id);
-		if (ref == NULL)
+		if (ref == NULL) {
+			dprintk("cannot find control id 0x%x\n", id);
 			return -EINVAL;
+		}
 		h->ref = ref;
 		ctrl = ref->ctrl;
-		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
+		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
+			dprintk("control id 0x%x is disabled\n", id);
 			return -EINVAL;
+		}
 
 		if (ctrl->cluster[0]->ncontrols > 1)
 			have_clusters = true;

@@ -3159,10 +3180,16 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 			unsigned tot_size = ctrl->elems * ctrl->elem_size;
 
 			if (c->size < tot_size) {
+				/*
+				 * In the get case the application first queries
+				 * to obtain the size of the control.
+				 */
 				if (get) {
 					c->size = tot_size;
 					return -ENOSPC;
 				}
+				dprintk("pointer control id 0x%x size too small, %d bytes but %d bytes needed\n",
+					id, c->size, tot_size);
 				return -EFAULT;
 			}
 			c->size = tot_size;

@@ -3534,16 +3561,20 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
 
 		cs->error_idx = i;
 
-		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
+		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY) {
+			dprintk("control id 0x%x is read-only\n", ctrl->id);
 			return -EACCES;
+		}
 		/* This test is also done in try_set_control_cluster() which
 		   is called in atomic context, so that has the final say,
 		   but it makes sense to do an up-front check as well. Once
 		   an error occurs in try_set_control_cluster() some other
 		   controls may have been set already and we want to do a
 		   best-effort to avoid that. */
-		if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
+		if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED)) {
+			dprintk("control id 0x%x is grabbed, cannot set\n", ctrl->id);
 			return -EBUSY;
+		}
 		/*
 		 * Skip validation for now if the payload needs to be copied
 		 * from userspace into kernelspace. We'll validate those later.

@@ -3576,7 +3607,8 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master)
 }
 
 /* Try or try-and-set controls */
-static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
+static int try_set_ext_ctrls_common(struct video_device *vdev,
+				    struct v4l2_fh *fh,
 				    struct v4l2_ctrl_handler *hdl,
 				    struct v4l2_ext_controls *cs, bool set)
 {

@@ -3588,13 +3620,17 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
 	cs->error_idx = cs->count;
 
 	/* Default value cannot be changed */
-	if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
+	if (cs->which == V4L2_CTRL_WHICH_DEF_VAL) {
+		dprintk("%s: cannot change default value\n", video_device_node_name(vdev));
 		return -EINVAL;
+	}
 
 	cs->which = V4L2_CTRL_ID2WHICH(cs->which);
 
-	if (hdl == NULL)
+	if (hdl == NULL) {
+		dprintk("%s: invalid null control handler\n", video_device_node_name(vdev));
 		return -EINVAL;
+	}
 
 	if (cs->count == 0)
 		return class_check(hdl, cs->which);

@@ -3691,7 +3727,8 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
 	return ret;
 }
 
-static int try_set_ext_ctrls(struct v4l2_fh *fh,
+static int try_set_ext_ctrls(struct video_device *vdev,
+			     struct v4l2_fh *fh,
 			     struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
 			     struct v4l2_ext_controls *cs, bool set)
 {

@@ -3700,21 +3737,32 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 	int ret;
 
 	if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) {
-		if (!mdev || cs->request_fd < 0)
+		if (!mdev) {
+			dprintk("%s: missing media device\n", video_device_node_name(vdev));
+			return -EINVAL;
+		}
+
+		if (cs->request_fd < 0) {
+			dprintk("%s: invalid request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			return -EINVAL;
+		}
 
 		req = media_request_get_by_fd(mdev, cs->request_fd);
-		if (IS_ERR(req))
+		if (IS_ERR(req)) {
+			dprintk("%s: cannot find request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			return PTR_ERR(req);
+		}
 

drivers/media/v4l2-core/v4l2-ctrls.c static int try_set_ext_ctrls(struct v4l2_fh *fh, · line 3765

Quoted 10 lines

drivers/media/v4l2-core/v4l2-ctrls.c +2 -0

 		ret = media_request_lock_for_update(req);
 		if (ret) {
+			dprintk("%s: cannot lock request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			media_request_put(req);
 			return ret;
 		}
 
 		obj = v4l2_ctrls_find_req_obj(hdl, req, set);
 		if (IS_ERR(obj)) {
+			dprintk("%s: cannot find request object for request fd %d\n", video_device_node_name(vdev), cs->request_fd);

These lines are way too long. Just add a newline after the first comma.

Same elsewhere.

Quoted 50 lines

drivers/media/v4l2-core/v4l2-ctrls.c +13 -6

 			media_request_unlock_for_update(req);
 			media_request_put(req);
 			return PTR_ERR(obj);

@@ -3723,7 +3771,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 				   req_obj);
 	}
 
-	ret = try_set_ext_ctrls_common(fh, hdl, cs, set);
+	ret = try_set_ext_ctrls_common(vdev, fh, hdl, cs, set);
+	if (ret)
+		dprintk("%s: try_set_ext_ctrls_common failed (%d)\n", video_device_node_name(vdev), ret);
 
 	if (obj) {
 		media_request_unlock_for_update(req);

@@ -3734,17 +3784,22 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 	return ret;
 }
 
-int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
+int v4l2_try_ext_ctrls(struct video_device *vdev,
+		       struct v4l2_ctrl_handler *hdl,
+		       struct media_device *mdev,
 		       struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(NULL, hdl, mdev, cs, false);
+	return try_set_ext_ctrls(vdev, NULL, hdl, mdev, cs, false);
 }
 EXPORT_SYMBOL(v4l2_try_ext_ctrls);
 
-int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
-		     struct media_device *mdev, struct v4l2_ext_controls *cs)
+int v4l2_s_ext_ctrls(struct video_device *vdev,
+		     struct v4l2_fh *fh,
+		     struct v4l2_ctrl_handler *hdl,
+		     struct media_device *mdev,
+		     struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(fh, hdl, mdev, cs, true);
+	return try_set_ext_ctrls(vdev, fh, hdl, mdev, cs, true);
 }
 EXPORT_SYMBOL(v4l2_s_ext_ctrls);
 

drivers/media/v4l2-core/v4l2-dev.c +0 -0

index 39d22bfbe420..c6bcc9ea1122 100644
@@ -83,6 +83,8 @@ static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr,
 	if (res)
 		return res;
 

Quoted 2 lines

drivers/media/v4l2-core/v4l2-dev.c static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr, · line 87

Quoted 2 lines

drivers/media/v4l2-core/v4l2-dev.c +2 -0

+	if (value & V4L2_DEV_DEBUG_CTRL)
+		pr_warn_once("Warning: V4L2_DEV_DEBUG_CTRL cannot be enabled via the dev_debug attribute.\n");

Quoted 5 lines

Actually, you can for those functions that have the vdev pointer. And I think you can pass vdev on to more functions. Certainly validate_ctrls() and possibly all of them.

Before sending this patch, I tried different options, but failed to find a proper way of associating all error paths with a struct video_device.

For instance, __v4l2_ctrl_s_ctrl eventually calls validate_new, and it seems really nasty to change its prototype, as it's called by so many drivers.

I think it's a too invasive change, and not worth it just to add one debug print.

So one option would be to drop the validate_new print.

Another option would be have a slightly inconsistent behavior between setting the module debug parameter and the per-device debug attribute.

I think for debugging, consistency is very important, and that's why I prefered keeping the debug parameter and produce this warning.

Thanks,
Ezequiel

Quoted 265 lines · Ezequiel Garcia

Quoted 264 lines · Hans Verkuil

Quoted 198 lines · Ezequiel Garcia

Currently, the v4l2 control code is a bit silent on errors. Now that we have a debug parameter, it's possible to enable debugging messages here.

Add debug messages on (hopefully) most of the error paths. Since it's really hard to associate all these errors to video device instance, we are forced to use the global debug parameter only.

Add a warning in case the user enables control debugging at the per-device dev_debug level.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/media/v4l2-core/v4l2-ctrls.c  | 93 +++++++++++++++++++++------
 drivers/media/v4l2-core/v4l2-dev.c    |  2 +
 drivers/media/v4l2-core/v4l2-ioctl.c  |  8 +--
 drivers/media/v4l2-core/v4l2-subdev.c |  4 +-
 include/media/v4l2-ctrls.h            |  9 ++-
 include/media/v4l2-ioctl.h            |  2 +
 6 files changed, 91 insertions(+), 27 deletions(-)

drivers/media/v4l2-core/v4l2-ctrls.c +59 -13

index b79d3bbd8350..af8ad83d1e08 100644
@@ -18,6 +18,8 @@
     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#define pr_fmt(fmt) "v4l2-ctrls: " fmt
+
 #include <linux/ctype.h>
 #include <linux/mm.h>
 #include <linux/slab.h>

@@ -28,6 +30,14 @@
 #include <media/v4l2-event.h>
 #include <media/v4l2-dev.h>
 
+extern unsigned int videodev_debug;
+
+#define dprintk(fmt, arg...) do {					\
+	if (videodev_debug & V4L2_DEV_DEBUG_CTRL)			\
+		printk(KERN_DEBUG pr_fmt("%s: " fmt),			\
+		       __func__, ##arg);				\
+} while (0)
+
 #define has_op(master, op) \
 	(master->ops && master->ops->op)
 #define call_op(master, op) \

@@ -1952,8 +1962,11 @@ static int validate_new(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr p_new)
 	unsigned idx;
 	int err = 0;
 
-	for (idx = 0; !err && idx < ctrl->elems; idx++)
+	for (idx = 0; !err && idx < ctrl->elems; idx++) {
 		err = ctrl->type_ops->validate(ctrl, idx, p_new);
+		if (err)
+			dprintk("failed to validate control id 0x%x (%d)\n", ctrl->id, err);
+	}
 	return err;
 }
 

@@ -3136,20 +3149,28 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		if (cs->which &&
 		    cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
 		    cs->which != V4L2_CTRL_WHICH_REQUEST_VAL &&
-		    V4L2_CTRL_ID2WHICH(id) != cs->which)
+		    V4L2_CTRL_ID2WHICH(id) != cs->which) {
+			dprintk("invalid which 0x%x or control id 0x%x\n", cs->which, id);
 			return -EINVAL;
+		}
 
 		/* Old-style private controls are not allowed for
 		   extended controls */
-		if (id >= V4L2_CID_PRIVATE_BASE)
+		if (id >= V4L2_CID_PRIVATE_BASE) {
+			dprintk("old-style private controls not allowed for extended controls\n");
 			return -EINVAL;
+		}
 		ref = find_ref_lock(hdl, id);
-		if (ref == NULL)
+		if (ref == NULL) {
+			dprintk("cannot find control id 0x%x\n", id);
 			return -EINVAL;
+		}
 		h->ref = ref;
 		ctrl = ref->ctrl;
-		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
+		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
+			dprintk("control id 0x%x is disabled\n", id);
 			return -EINVAL;
+		}
 
 		if (ctrl->cluster[0]->ncontrols > 1)
 			have_clusters = true;

@@ -3159,10 +3180,16 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 			unsigned tot_size = ctrl->elems * ctrl->elem_size;
 
 			if (c->size < tot_size) {
+				/*
+				 * In the get case the application first queries
+				 * to obtain the size of the control.
+				 */
 				if (get) {
 					c->size = tot_size;
 					return -ENOSPC;
 				}
+				dprintk("pointer control id 0x%x size too small, %d bytes but %d bytes needed\n",
+					id, c->size, tot_size);
 				return -EFAULT;
 			}
 			c->size = tot_size;

@@ -3534,16 +3561,20 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
 
 		cs->error_idx = i;
 
-		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
+		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY) {
+			dprintk("control id 0x%x is read-only\n", ctrl->id);
 			return -EACCES;
+		}
 		/* This test is also done in try_set_control_cluster() which
 		   is called in atomic context, so that has the final say,
 		   but it makes sense to do an up-front check as well. Once
 		   an error occurs in try_set_control_cluster() some other
 		   controls may have been set already and we want to do a
 		   best-effort to avoid that. */
-		if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
+		if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED)) {
+			dprintk("control id 0x%x is grabbed, cannot set\n", ctrl->id);
 			return -EBUSY;
+		}
 		/*
 		 * Skip validation for now if the payload needs to be copied
 		 * from userspace into kernelspace. We'll validate those later.

@@ -3576,7 +3607,8 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master)
 }
 
 /* Try or try-and-set controls */
-static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
+static int try_set_ext_ctrls_common(struct video_device *vdev,
+				    struct v4l2_fh *fh,
 				    struct v4l2_ctrl_handler *hdl,
 				    struct v4l2_ext_controls *cs, bool set)
 {

@@ -3588,13 +3620,17 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
 	cs->error_idx = cs->count;
 
 	/* Default value cannot be changed */
-	if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
+	if (cs->which == V4L2_CTRL_WHICH_DEF_VAL) {
+		dprintk("%s: cannot change default value\n", video_device_node_name(vdev));
 		return -EINVAL;
+	}
 
 	cs->which = V4L2_CTRL_ID2WHICH(cs->which);
 
-	if (hdl == NULL)
+	if (hdl == NULL) {
+		dprintk("%s: invalid null control handler\n", video_device_node_name(vdev));
 		return -EINVAL;
+	}
 
 	if (cs->count == 0)
 		return class_check(hdl, cs->which);

@@ -3691,7 +3727,8 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,
 	return ret;
 }
 
-static int try_set_ext_ctrls(struct v4l2_fh *fh,
+static int try_set_ext_ctrls(struct video_device *vdev,
+			     struct v4l2_fh *fh,
 			     struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
 			     struct v4l2_ext_controls *cs, bool set)
 {

@@ -3700,21 +3737,32 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 	int ret;
 
 	if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) {
-		if (!mdev || cs->request_fd < 0)
+		if (!mdev) {
+			dprintk("%s: missing media device\n", video_device_node_name(vdev));
+			return -EINVAL;
+		}
+
+		if (cs->request_fd < 0) {
+			dprintk("%s: invalid request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			return -EINVAL;
+		}
 
 		req = media_request_get_by_fd(mdev, cs->request_fd);
-		if (IS_ERR(req))
+		if (IS_ERR(req)) {
+			dprintk("%s: cannot find request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			return PTR_ERR(req);
+		}
 

drivers/media/v4l2-core/v4l2-ctrls.c static int try_set_ext_ctrls(struct v4l2_fh *fh, · line 3765

Quoted 10 lines

drivers/media/v4l2-core/v4l2-ctrls.c +2 -0

 		ret = media_request_lock_for_update(req);
 		if (ret) {
+			dprintk("%s: cannot lock request fd %d\n", video_device_node_name(vdev), cs->request_fd);
 			media_request_put(req);
 			return ret;
 		}
 
 		obj = v4l2_ctrls_find_req_obj(hdl, req, set);
 		if (IS_ERR(obj)) {
+			dprintk("%s: cannot find request object for request fd %d\n", video_device_node_name(vdev), cs->request_fd);

These lines are way too long. Just add a newline after the first comma.

Same elsewhere.

Quoted 50 lines

drivers/media/v4l2-core/v4l2-ctrls.c +13 -6

 			media_request_unlock_for_update(req);
 			media_request_put(req);
 			return PTR_ERR(obj);

@@ -3723,7 +3771,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 				   req_obj);
 	}
 
-	ret = try_set_ext_ctrls_common(fh, hdl, cs, set);
+	ret = try_set_ext_ctrls_common(vdev, fh, hdl, cs, set);
+	if (ret)
+		dprintk("%s: try_set_ext_ctrls_common failed (%d)\n", video_device_node_name(vdev), ret);
 
 	if (obj) {
 		media_request_unlock_for_update(req);

@@ -3734,17 +3784,22 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 	return ret;
 }
 
-int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
+int v4l2_try_ext_ctrls(struct video_device *vdev,
+		       struct v4l2_ctrl_handler *hdl,
+		       struct media_device *mdev,
 		       struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(NULL, hdl, mdev, cs, false);
+	return try_set_ext_ctrls(vdev, NULL, hdl, mdev, cs, false);
 }
 EXPORT_SYMBOL(v4l2_try_ext_ctrls);
 
-int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
-		     struct media_device *mdev, struct v4l2_ext_controls *cs)
+int v4l2_s_ext_ctrls(struct video_device *vdev,
+		     struct v4l2_fh *fh,
+		     struct v4l2_ctrl_handler *hdl,
+		     struct media_device *mdev,
+		     struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(fh, hdl, mdev, cs, true);
+	return try_set_ext_ctrls(vdev, fh, hdl, mdev, cs, true);
 }
 EXPORT_SYMBOL(v4l2_s_ext_ctrls);
 

drivers/media/v4l2-core/v4l2-dev.c +0 -0

index 39d22bfbe420..c6bcc9ea1122 100644
@@ -83,6 +83,8 @@ static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr,
 	if (res)
 		return res;
 

Quoted 2 lines

Quoted 2 lines

drivers/media/v4l2-core/v4l2-dev.c static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr, · line 87

Quoted 2 lines

drivers/media/v4l2-core/v4l2-dev.c +2 -0

+	if (value & V4L2_DEV_DEBUG_CTRL)
+		pr_warn_once("Warning: V4L2_DEV_DEBUG_CTRL cannot be enabled via the dev_debug attribute.\n");

BTW, you should clear the V4L2_DEV_DEBUG_CTRL bit before setting vdev->dev_debug.

Quoted 13 lines

Quoted 4 lines

Actually, you can for those functions that have the vdev pointer. And I think you can pass vdev on to more functions. Certainly validate_ctrls() and possibly all of them.

Before sending this patch, I tried different options, but failed to find a proper way of associating all error paths with a struct video_device.

For instance, __v4l2_ctrl_s_ctrl eventually calls validate_new, and it seems really nasty to change its prototype, as it's called by so many drivers.

Quoted 4 lines

I think it's a too invasive change, and not worth it just to add one debug print.

So one option would be to drop the validate_new print.

Yeah, that's probably best.

Quoted 5 lines

Another option would be have a slightly inconsistent behavior between setting the module debug parameter and the per-device debug attribute.

I think for debugging, consistency is very important, and that's why I prefered keeping the debug parameter and produce this warning.

OK, post a v3 and I'll take it.