[PATCH v2 0/4] Add debug messages to v4l2-ctrls

[PATCH v2 0/4] Add debug messages to v4l2-ctrls

[Ezequiel Garcia]

v2:
  * Print the video device node name where possible, in v4l2-ctrls.c,
    by passing the struct video_device as a parameter.
  * Added a warning to dev_debug attribute store, to warn
    the user about V4L2_DEV_DEBUG_CTRL being a no-op for dev_debug.

Even though the goal was fairly simple (adding debug to v4l2-ctrls)
it ended up spanning this little patchset.

The motivation for this cleanup is being able to turn on/off
debugging messages in v4l2-ctrls.c, using a knob common to the
videodev driver's core.

So, in addition to the dev_debug attribute, this series introduces
a debug module parameter, and a new debug level, which enables
v4l2-ctrl debugging.

Having this is quite useful when bringing-up stateless codecs,
using the Request API.

Ezequiel Garcia (4):
  media: v4l: Simplify dev_debug flags
  media: v4l: Improve debug dprintk macro
  media: v4l: Add a module parameter to control global debugging
  media: v4l: ctrls: Add debug messages

 drivers/media/v4l2-core/v4l2-ctrls.c  | 93 +++++++++++++++++++++------
 drivers/media/v4l2-core/v4l2-dev.c    | 47 ++++++--------
 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, 110 insertions(+), 53 deletions(-)

-- 
2.20.1

[PATCH v2 1/4] media: v4l: Simplify dev_debug flags

[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.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index d7528f82a66a..34e4958663bf 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -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;
-- 
2.20.1

Re: [PATCH v2 1/4] media: v4l: Simplify dev_debug flags

[Hans Verkuil]

On 2/27/19 6:07 PM, Ezequiel Garcia wrote:
> 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

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index d7528f82a66a..34e4958663bf 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -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;
> 

Re: [PATCH v2 1/4] media: v4l: Simplify dev_debug flags

[Ezequiel Garcia]

Hi Hans,

I'm sorry for the delay.

This series still sounds useful, so let's try to
revive it.

On Mon, 2019-03-11 at 12:16 +0100, Hans Verkuil wrote:
> On 2/27/19 6:07 PM, Ezequiel Garcia wrote:
> > 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").
> 

Oops, I managed to somehow miss (D)QBUF, even though it's perfectly
well documented in the header!

Will fix, and will fix the documentation as well.
  
> Of course, the documentation should also mention the new dev_debug
> module parameter and the new debug flag for debugging controls.
> 
> Regards,
> 
> 	Hans
> 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index d7528f82a66a..34e4958663bf 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -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;
> > 

[PATCH v2 2/4] media: v4l: Improve debug dprintk macro

[Ezequiel Garcia]

Instead of checking the dev_debug flags before each dprintk call,
make the macro smarter by passing the parameters.

This makes the code simpler and will allow to add more debug logic.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 35 ++++++++++--------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 34e4958663bf..7cfb05204065 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -36,9 +36,10 @@
 #define VIDEO_NUM_DEVICES	256
 #define VIDEO_NAME              "video4linux"
 
-#define dprintk(fmt, arg...) do {					\
-		printk(KERN_DEBUG pr_fmt("%s: " fmt),			\
-		       __func__, ##arg);				\
+#define dprintk(vdev, flags, fmt, arg...) do {				\
+	if (vdev->dev_debug & flags)					\
+		printk(KERN_DEBUG pr_fmt("%s: %s: " fmt),		\
+		       __func__, video_device_node_name(vdev), ##arg);	\
 } while (0)
 
 
@@ -315,9 +316,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_STREAMING)
-		dprintk("%s: read: %zd (%d)\n",
-			video_device_node_name(vdev), sz, ret);
+	dprintk(vdev, V4L2_DEV_DEBUG_STREAMING, "read: %zd (%d)\n", sz, ret);
 	return ret;
 }
 
@@ -331,9 +330,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_STREAMING)
-		dprintk("%s: write: %zd (%d)\n",
-			video_device_node_name(vdev), sz, ret);
+	dprintk(vdev, V4L2_DEV_DEBUG_STREAMING, "write: %zd (%d)\n", sz, ret);
 	return ret;
 }
 
@@ -346,9 +343,7 @@ static __poll_t v4l2_poll(struct file *filp, struct poll_table_struct *poll)
 		return DEFAULT_POLLMASK;
 	if (video_is_registered(vdev))
 		res = vdev->fops->poll(filp, poll);
-	if (vdev->dev_debug & V4L2_DEV_DEBUG_POLL)
-		dprintk("%s: poll: %08x\n",
-			video_device_node_name(vdev), res);
+	dprintk(vdev, V4L2_DEV_DEBUG_POLL, "poll: %08x\n", res);
 	return res;
 }
 
@@ -381,9 +376,7 @@ static unsigned long v4l2_get_unmapped_area(struct file *filp,
 	if (!video_is_registered(vdev))
 		return -ENODEV;
 	ret = vdev->fops->get_unmapped_area(filp, addr, len, pgoff, flags);
-	if (vdev->dev_debug & V4L2_DEV_DEBUG_FOP)
-		dprintk("%s: get_unmapped_area (%d)\n",
-			video_device_node_name(vdev), ret);
+	dprintk(vdev, V4L2_DEV_DEBUG_FOP, "get_unmapped_area (%d)\n", ret);
 	return ret;
 }
 #endif
@@ -397,9 +390,7 @@ static int v4l2_mmap(struct file *filp, struct vm_area_struct *vm)
 		return -ENODEV;
 	if (video_is_registered(vdev))
 		ret = vdev->fops->mmap(filp, vm);
-	if (vdev->dev_debug & V4L2_DEV_DEBUG_FOP)
-		dprintk("%s: mmap (%d)\n",
-			video_device_node_name(vdev), ret);
+	dprintk(vdev, V4L2_DEV_DEBUG_FOP, "mmap (%d)\n", ret);
 	return ret;
 }
 
@@ -427,9 +418,7 @@ static int v4l2_open(struct inode *inode, struct file *filp)
 			ret = -ENODEV;
 	}
 
-	if (vdev->dev_debug & V4L2_DEV_DEBUG_FOP)
-		dprintk("%s: open (%d)\n",
-			video_device_node_name(vdev), ret);
+	dprintk(vdev, V4L2_DEV_DEBUG_FOP, "open (%d)\n", ret);
 	/* decrease the refcount in case of an error */
 	if (ret)
 		video_put(vdev);
@@ -458,9 +447,7 @@ static int v4l2_release(struct inode *inode, struct file *filp)
 		}
 	}
 
-	if (vdev->dev_debug & V4L2_DEV_DEBUG_FOP)
-		dprintk("%s: release\n",
-			video_device_node_name(vdev));
+	dprintk(vdev, V4L2_DEV_DEBUG_FOP, "release\n");
 
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
-- 
2.20.1

[PATCH v2 3/4] media: v4l: Add a module parameter to control global debugging

[Ezequiel Garcia]

In addition to the dev_debug device attribute, which controls
per-device debugging, we now add a module parameter to control
debugging globally.

This will allow to add debugging of v4l2 control logic,
using the newly introduced debug parameter.

In addition, this module parameter adds consistency to the
subsystem, since other v4l2 modules expose the same parameter.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 7cfb05204065..39d22bfbe420 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -36,8 +36,16 @@
 #define VIDEO_NUM_DEVICES	256
 #define VIDEO_NAME              "video4linux"
 
+unsigned int videodev_debug;
+module_param_named(debug, videodev_debug, uint, 0644);
+
+/*
+ * The videodev_debug module parameter controls the global debug level,
+ * while the dev_debug device attribute controls the local
+ * per-device debug level.
+ */
 #define dprintk(vdev, flags, fmt, arg...) do {				\
-	if (vdev->dev_debug & flags)					\
+	if ((videodev_debug & flags) || (vdev->dev_debug & flags))	\
 		printk(KERN_DEBUG pr_fmt("%s: %s: " fmt),		\
 		       __func__, video_device_node_name(vdev), ##arg);	\
 } while (0)
-- 
2.20.1

[PATCH v2 4/4] media: v4l: ctrls: Add debug messages

[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 <ezequiel@collabora.com>
---
 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(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index b79d3bbd8350..af8ad83d1e08 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -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);
+		}
 
 		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);
 			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);
 
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 39d22bfbe420..c6bcc9ea1122 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -83,6 +83,8 @@ static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr,
 	if (res)
 		return res;
 
+	if (value & V4L2_DEV_DEBUG_CTRL)
+		pr_warn_once("Warning: V4L2_DEV_DEBUG_CTRL cannot be enabled via the dev_debug attribute.\n");
 	vdev->dev_debug = value;
 	return len;
 }
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 6f707466b5d2..078f75eb0a19 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2180,9 +2180,9 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 
 	p->error_idx = p->count;
 	if (vfh && vfh->ctrl_handler)
-		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
+		return v4l2_s_ext_ctrls(vfd, vfh, vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	if (vfd->ctrl_handler)
-		return v4l2_s_ext_ctrls(NULL, vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
+		return v4l2_s_ext_ctrls(vfd, vfh, vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	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) :
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index f5f0d71ec745..3a09d4441ca3 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -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:
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index d63cf227b0ab..0e38a59c80dd 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -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);
 
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 8533ece5026e..0ecd4e3e76a4 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -612,6 +612,8 @@ struct v4l2_ioctl_ops {
 #define V4L2_DEV_DEBUG_STREAMING	0x08
 /* Log poll() */
 #define V4L2_DEV_DEBUG_POLL		0x10
+/* Log controls */
+#define V4L2_DEV_DEBUG_CTRL		0x20
 
 /*  Video standard functions  */
 
-- 
2.20.1

Re: [PATCH v2 4/4] media: v4l: ctrls: Add debug messages

[Hans Verkuil]

On 2/27/19 6:07 PM, Ezequiel Garcia wrote:
> 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 <ezequiel@collabora.com>
> ---
>  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(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index b79d3bbd8350..af8ad83d1e08 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -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);
> +		}
>  
>  		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.

>  			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);
>  
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 39d22bfbe420..c6bcc9ea1122 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -83,6 +83,8 @@ static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr,
>  	if (res)
>  		return res;
>  
> +	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.

>  	vdev->dev_debug = value;
>  	return len;
>  }
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 6f707466b5d2..078f75eb0a19 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2180,9 +2180,9 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  
>  	p->error_idx = p->count;
>  	if (vfh && vfh->ctrl_handler)
> -		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
> +		return v4l2_s_ext_ctrls(vfd, vfh, vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
>  	if (vfd->ctrl_handler)
> -		return v4l2_s_ext_ctrls(NULL, vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
> +		return v4l2_s_ext_ctrls(vfd, vfh, vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);

Copy-and-paste error: vfh should be NULL.

>  	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) :
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index f5f0d71ec745..3a09d4441ca3 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -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:
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index d63cf227b0ab..0e38a59c80dd 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -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);
>  
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index 8533ece5026e..0ecd4e3e76a4 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -612,6 +612,8 @@ struct v4l2_ioctl_ops {
>  #define V4L2_DEV_DEBUG_STREAMING	0x08
>  /* Log poll() */
>  #define V4L2_DEV_DEBUG_POLL		0x10
> +/* Log controls */
> +#define V4L2_DEV_DEBUG_CTRL		0x20
>  
>  /*  Video standard functions  */
>  
> 

Regards,

	Hans

Re: [PATCH v2 4/4] media: v4l: ctrls: Add debug messages

[Ezequiel Garcia]

On Mon, 2019-03-11 at 12:36 +0100, Hans Verkuil wrote:
> On 2/27/19 6:07 PM, Ezequiel Garcia wrote:
> > 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 <ezequiel@collabora.com>
> > ---
> >  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(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index b79d3bbd8350..af8ad83d1e08 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -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);
> > +		}
> >  
> >  		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.
> 
> >  			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);
> >  
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 39d22bfbe420..c6bcc9ea1122 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -83,6 +83,8 @@ static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr,
> >  	if (res)
> >  		return res;
> >  
> > +	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.
> 

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

Re: [PATCH v2 4/4] media: v4l: ctrls: Add debug messages

[Hans Verkuil]

On 6/1/19 7:57 PM, Ezequiel Garcia wrote:
> On Mon, 2019-03-11 at 12:36 +0100, Hans Verkuil wrote:
>> On 2/27/19 6:07 PM, Ezequiel Garcia wrote:
>>> 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 <ezequiel@collabora.com>
>>> ---
>>>  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(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index b79d3bbd8350..af8ad83d1e08 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -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);
>>> +		}
>>>  
>>>  		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.
>>
>>>  			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);
>>>  
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>> index 39d22bfbe420..c6bcc9ea1122 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -83,6 +83,8 @@ static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr,
>>>  	if (res)
>>>  		return res;
>>>  
>>> +	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.

>>
>> 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.

Yeah, that's probably best.

> 
> 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.

Regards,

	Hans

> 
> Thanks,
> Ezequiel
> 

Re: [PATCH v2 4/4] media: v4l: ctrls: Add debug messages

[Ezequiel Garcia]

On Mon, 2019-06-03 at 09:16 +0200, Hans Verkuil wrote:
> On 6/1/19 7:57 PM, Ezequiel Garcia wrote:
> > On Mon, 2019-03-11 at 12:36 +0100, Hans Verkuil wrote:
> > > On 2/27/19 6:07 PM, Ezequiel Garcia wrote:
> > > > 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 <ezequiel@collabora.com>
> > > > ---
> > > >  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(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > index b79d3bbd8350..af8ad83d1e08 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > @@ -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);
> > > > +		}
> > > >  
> > > >  		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.
> > > 
> > > >  			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);
> > > >  
> > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > > index 39d22bfbe420..c6bcc9ea1122 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > > @@ -83,6 +83,8 @@ static ssize_t dev_debug_store(struct device *cd, struct device_attribute *attr,
> > > >  	if (res)
> > > >  		return res;
> > > >  
> > > > +	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.
> 
> > > 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.
> 
> Yeah, that's probably best.
> 

OK, in that case, it's possible to avoid the warning and
allow per-device V4L2_DEV_DEBUG_CTRL.

> > 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.
> 

OK, I'm on it.