| Message ID | 20190227170706.6258-5-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add debug messages to v4l2-ctrls | expand |
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
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
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 >
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.
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 */
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(-)