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

Reply via email to