| Message ID | 20190621225006.31773-1-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: v4l: ctrls: Add debug messages | expand |
On 6/22/19 12:50 AM, Ezequiel Garcia wrote: > Currently, the v4l2 control code is a bit silent on errors. > Add debug messages on (hopefully) most of the error paths. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > Changes from v1: > * Drop changes in the debug parameter semantics. > * Drop new module debug parameter. > * Add documentation. > * Add a debug error in all places where control can fail. > * Reorder the vdev parameter, to make the patch less invasive. > --- > Documentation/media/kapi/v4l2-dev.rst | 1 + > drivers/media/platform/omap3isp/ispvideo.c | 2 +- > drivers/media/v4l2-core/v4l2-ctrls.c | 106 ++++++++++++++++----- > drivers/media/v4l2-core/v4l2-ioctl.c | 12 +-- > drivers/media/v4l2-core/v4l2-subdev.c | 6 +- > include/media/v4l2-ctrls.h | 9 +- > include/media/v4l2-ioctl.h | 2 + > 7 files changed, 100 insertions(+), 38 deletions(-) > > diff --git a/Documentation/media/kapi/v4l2-dev.rst b/Documentation/media/kapi/v4l2-dev.rst > index b359f1804bbe..4c5a15c53dbf 100644 > --- a/Documentation/media/kapi/v4l2-dev.rst > +++ b/Documentation/media/kapi/v4l2-dev.rst > @@ -288,6 +288,7 @@ Mask Description > 0x08 Log the read and write file operations and the VIDIOC_QBUF and > VIDIOC_DQBUF ioctls. > 0x10 Log the poll file operation. > +0x20 Log error and messages in the control operations. > ===== ================================================================ > > Video device cleanup > diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c > index 175bbed9a235..abc945cc05c9 100644 > --- a/drivers/media/platform/omap3isp/ispvideo.c > +++ b/drivers/media/platform/omap3isp/ispvideo.c > @@ -1028,7 +1028,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video, > ctrls.count = 1; > ctrls.controls = &ctrl; > > - ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, NULL, &ctrls); > + ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &video->video, NULL, &ctrls); > if (ret < 0) { > dev_warn(isp->dev, "no pixel rate control in subdev %s\n", > pipe->external->name); > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 2d7525e2d9eb..4ee703e04ef2 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -6,6 +6,8 @@ > > */ > > +#define pr_fmt(fmt) "v4l2-ctrls: " fmt > + > #include <linux/ctype.h> > #include <linux/mm.h> > #include <linux/slab.h> > @@ -16,6 +18,12 @@ > #include <media/v4l2-event.h> > #include <media/v4l2-dev.h> > > +#define dprintk(vdev, fmt, arg...) do { \ > + if (vdev->dev_debug & V4L2_DEV_DEBUG_CTRL) \ This isn't very robust if vdev is NULL. I know it shouldn't, but it's better to check for this. I'd use this instead: if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \ That way nothing crashes if vdev is NULL, and you get a warning as well in that case. Regards, Hans > + printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \ > + __func__, video_device_node_name(vdev), ##arg); \ > +} while (0) > + > #define has_op(master, op) \ > (master->ops && master->ops->op) > #define call_op(master, op) \ > @@ -3211,6 +3219,7 @@ static int v4l2_ctrl_request_bind(struct media_request *req, > static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl, > struct v4l2_ext_controls *cs, > struct v4l2_ctrl_helper *helpers, > + struct video_device *vdev, > bool get) > { > struct v4l2_ctrl_helper *h; > @@ -3228,20 +3237,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(vdev, "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(vdev, "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(vdev, "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(vdev, "control id 0x%x is disabled\n", id); > return -EINVAL; > + } > > if (ctrl->cluster[0]->ncontrols > 1) > have_clusters = true; > @@ -3251,10 +3268,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(vdev, "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; > @@ -3315,7 +3338,8 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 which) > > /* Get extended controls. Allocates the helpers array if needed. */ > static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, > - struct v4l2_ext_controls *cs) > + struct v4l2_ext_controls *cs, > + struct video_device *vdev) > { > struct v4l2_ctrl_helper helper[4]; > struct v4l2_ctrl_helper *helpers = helper; > @@ -3341,7 +3365,7 @@ static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, > return -ENOMEM; > } > > - ret = prepare_ext_ctrls(hdl, cs, helpers, true); > + ret = prepare_ext_ctrls(hdl, cs, helpers, vdev, true); > cs->error_idx = cs->count; > > for (i = 0; !ret && i < cs->count; i++) > @@ -3434,8 +3458,8 @@ v4l2_ctrls_find_req_obj(struct v4l2_ctrl_handler *hdl, > return obj; > } > > -int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, > - struct v4l2_ext_controls *cs) > +int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct video_device *vdev, > + struct media_device *mdev, struct v4l2_ext_controls *cs) > { > struct media_request_object *obj = NULL; > struct media_request *req = NULL; > @@ -3471,7 +3495,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, > req_obj); > } > > - ret = v4l2_g_ext_ctrls_common(hdl, cs); > + ret = v4l2_g_ext_ctrls_common(hdl, cs, vdev); > > if (obj) { > media_request_unlock_for_access(req); > @@ -3614,7 +3638,9 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master, > > /* Validate controls. */ > static int validate_ctrls(struct v4l2_ext_controls *cs, > - struct v4l2_ctrl_helper *helpers, bool set) > + struct v4l2_ctrl_helper *helpers, > + struct video_device *vdev, > + bool set) > { > unsigned i; > int ret = 0; > @@ -3626,16 +3652,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(vdev, "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(vdev, "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. > @@ -3670,7 +3700,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, > struct v4l2_ctrl_handler *hdl, > - struct v4l2_ext_controls *cs, bool set) > + struct v4l2_ext_controls *cs, > + struct video_device *vdev, bool set) > { > struct v4l2_ctrl_helper helper[4]; > struct v4l2_ctrl_helper *helpers = helper; > @@ -3680,13 +3711,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(vdev, "%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(vdev, "%s: invalid null control handler\n", video_device_node_name(vdev)); > return -EINVAL; > + } > > if (cs->count == 0) > return class_check(hdl, cs->which); > @@ -3697,9 +3732,9 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh, > if (!helpers) > return -ENOMEM; > } > - ret = prepare_ext_ctrls(hdl, cs, helpers, false); > + ret = prepare_ext_ctrls(hdl, cs, helpers, vdev, false); > if (!ret) > - ret = validate_ctrls(cs, helpers, set); > + ret = validate_ctrls(cs, helpers, vdev, set); > if (ret && set) > cs->error_idx = cs->count; > for (i = 0; !ret && i < cs->count; i++) { > @@ -3784,7 +3819,8 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh, > } > > static int try_set_ext_ctrls(struct v4l2_fh *fh, > - struct v4l2_ctrl_handler *hdl, struct media_device *mdev, > + struct v4l2_ctrl_handler *hdl, > + struct video_device *vdev, struct media_device *mdev, > struct v4l2_ext_controls *cs, bool set) > { > struct media_request_object *obj = NULL; > @@ -3792,21 +3828,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(vdev, "%s: missing media device\n", video_device_node_name(vdev)); > + return -EINVAL; > + } > + > + if (cs->request_fd < 0) { > + dprintk(vdev, "%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(vdev, "%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(vdev, "%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(vdev, "%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); > @@ -3815,7 +3862,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(fh, hdl, cs, vdev, set); > + if (ret) > + dprintk(vdev, "%s: try_set_ext_ctrls_common failed (%d)\n", video_device_node_name(vdev), ret); > > if (obj) { > media_request_unlock_for_update(req); > @@ -3826,17 +3875,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 v4l2_ctrl_handler *hdl, > + struct video_device *vdev, > + struct media_device *mdev, > struct v4l2_ext_controls *cs) > { > - return try_set_ext_ctrls(NULL, hdl, mdev, cs, false); > + return try_set_ext_ctrls(NULL, hdl, vdev, 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 v4l2_fh *fh, > + struct v4l2_ctrl_handler *hdl, > + struct video_device *vdev, > + struct media_device *mdev, > + struct v4l2_ext_controls *cs) > { > - return try_set_ext_ctrls(fh, hdl, mdev, cs, true); > + return try_set_ext_ctrls(fh, hdl, vdev, mdev, cs, true); > } > EXPORT_SYMBOL(v4l2_s_ext_ctrls); > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index b1f4b991dba6..e95efea1a9ca 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -2165,9 +2165,9 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > p->error_idx = p->count; > if (vfh && vfh->ctrl_handler) > - return v4l2_g_ext_ctrls(vfh->ctrl_handler, vfd->v4l2_dev->mdev, p); > + return v4l2_g_ext_ctrls(vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p); > if (vfd->ctrl_handler) > - return v4l2_g_ext_ctrls(vfd->ctrl_handler, vfd->v4l2_dev->mdev, p); > + return v4l2_g_ext_ctrls(vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p); > if (ops->vidioc_g_ext_ctrls == NULL) > return -ENOTTY; > return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) : > @@ -2184,9 +2184,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(vfh, vfh->ctrl_handler, vfd, 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(NULL, vfd->ctrl_handler, vfd, 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) : > @@ -2203,9 +2203,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(vfh->ctrl_handler, vfd, 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->ctrl_handler, vfd, 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 f24978b80440..1b5edd3b1e6c 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -211,19 +211,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > if (!vfh->ctrl_handler) > return -ENOTTY; > return v4l2_g_ext_ctrls(vfh->ctrl_handler, > - sd->v4l2_dev->mdev, arg); > + vdev, sd->v4l2_dev->mdev, arg); > > case VIDIOC_S_EXT_CTRLS: > if (!vfh->ctrl_handler) > return -ENOTTY; > return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, > - sd->v4l2_dev->mdev, arg); > + vdev, sd->v4l2_dev->mdev, arg); > > case VIDIOC_TRY_EXT_CTRLS: > if (!vfh->ctrl_handler) > return -ENOTTY; > return v4l2_try_ext_ctrls(vfh->ctrl_handler, > - sd->v4l2_dev->mdev, arg); > + vdev, sd->v4l2_dev->mdev, arg); > > case VIDIOC_DQEVENT: > if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)) > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index b4433483af23..c08d6cc56743 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -1265,25 +1265,28 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl, > * :ref:`VIDIOC_G_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl > * > * @hdl: pointer to &struct v4l2_ctrl_handler > + * @vdev: pointer to &struct video_device > * @mdev: pointer to &struct media_device > * @c: pointer to &struct v4l2_ext_controls > * > * If hdl == NULL then they will all return -EINVAL. > */ > -int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, > - struct v4l2_ext_controls *c); > +int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct video_device *vdev, > + struct media_device *mdev, struct v4l2_ext_controls *c); > > /** > * v4l2_try_ext_ctrls - Helper function to implement > * :ref:`VIDIOC_TRY_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl > * > * @hdl: pointer to &struct v4l2_ctrl_handler > + * @vdev: pointer to &struct video_device > * @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, > + struct video_device *vdev, > struct media_device *mdev, > struct v4l2_ext_controls *c); > > @@ -1293,12 +1296,14 @@ int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, > * > * @fh: pointer to &struct v4l2_fh > * @hdl: pointer to &struct v4l2_ctrl_handler > + * @vdev: pointer to &struct video_device > * @mdev: pointer to &struct media_device > * @c: pointer to &struct v4l2_ext_controls > * > * If hdl == NULL then they will all return -EINVAL. > */ > int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl, > + struct video_device *vdev, > struct media_device *mdev, > struct v4l2_ext_controls *c); > > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h > index 400f2e46c108..4bba65a59d46 100644 > --- a/include/media/v4l2-ioctl.h > +++ b/include/media/v4l2-ioctl.h > @@ -602,6 +602,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 */ > >
On Sat, 2019-06-22 at 08:53 +0200, Hans Verkuil wrote: > On 6/22/19 12:50 AM, Ezequiel Garcia wrote: > > Currently, the v4l2 control code is a bit silent on errors. > > Add debug messages on (hopefully) most of the error paths. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > Changes from v1: > > * Drop changes in the debug parameter semantics. > > * Drop new module debug parameter. > > * Add documentation. > > * Add a debug error in all places where control can fail. > > * Reorder the vdev parameter, to make the patch less invasive. > > --- > > Documentation/media/kapi/v4l2-dev.rst | 1 + > > drivers/media/platform/omap3isp/ispvideo.c | 2 +- > > drivers/media/v4l2-core/v4l2-ctrls.c | 106 ++++++++++++++++----- > > drivers/media/v4l2-core/v4l2-ioctl.c | 12 +-- > > drivers/media/v4l2-core/v4l2-subdev.c | 6 +- > > include/media/v4l2-ctrls.h | 9 +- > > include/media/v4l2-ioctl.h | 2 + > > 7 files changed, 100 insertions(+), 38 deletions(-) > > > > diff --git a/Documentation/media/kapi/v4l2-dev.rst b/Documentation/media/kapi/v4l2-dev.rst > > index b359f1804bbe..4c5a15c53dbf 100644 > > --- a/Documentation/media/kapi/v4l2-dev.rst > > +++ b/Documentation/media/kapi/v4l2-dev.rst > > @@ -288,6 +288,7 @@ Mask Description > > 0x08 Log the read and write file operations and the VIDIOC_QBUF and > > VIDIOC_DQBUF ioctls. > > 0x10 Log the poll file operation. > > +0x20 Log error and messages in the control operations. > > ===== ================================================================ > > > > Video device cleanup > > diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c > > index 175bbed9a235..abc945cc05c9 100644 > > --- a/drivers/media/platform/omap3isp/ispvideo.c > > +++ b/drivers/media/platform/omap3isp/ispvideo.c > > @@ -1028,7 +1028,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video, > > ctrls.count = 1; > > ctrls.controls = &ctrl; > > > > - ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, NULL, &ctrls); > > + ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &video->video, NULL, &ctrls); > > if (ret < 0) { > > dev_warn(isp->dev, "no pixel rate control in subdev %s\n", > > pipe->external->name); > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > > index 2d7525e2d9eb..4ee703e04ef2 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > > @@ -6,6 +6,8 @@ > > > > */ > > > > +#define pr_fmt(fmt) "v4l2-ctrls: " fmt > > + > > #include <linux/ctype.h> > > #include <linux/mm.h> > > #include <linux/slab.h> > > @@ -16,6 +18,12 @@ > > #include <media/v4l2-event.h> > > #include <media/v4l2-dev.h> > > > > +#define dprintk(vdev, fmt, arg...) do { \ > > + if (vdev->dev_debug & V4L2_DEV_DEBUG_CTRL) \ > > This isn't very robust if vdev is NULL. I know it shouldn't, but it's > better to check for this. > > I'd use this instead: > > if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \ > > That way nothing crashes if vdev is NULL, and you get a warning as well in that case. > OK, no problem. Thanks, Eze
diff --git a/Documentation/media/kapi/v4l2-dev.rst b/Documentation/media/kapi/v4l2-dev.rst index b359f1804bbe..4c5a15c53dbf 100644 --- a/Documentation/media/kapi/v4l2-dev.rst +++ b/Documentation/media/kapi/v4l2-dev.rst @@ -288,6 +288,7 @@ Mask Description 0x08 Log the read and write file operations and the VIDIOC_QBUF and VIDIOC_DQBUF ioctls. 0x10 Log the poll file operation. +0x20 Log error and messages in the control operations. ===== ================================================================ Video device cleanup diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c index 175bbed9a235..abc945cc05c9 100644 --- a/drivers/media/platform/omap3isp/ispvideo.c +++ b/drivers/media/platform/omap3isp/ispvideo.c @@ -1028,7 +1028,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video, ctrls.count = 1; ctrls.controls = &ctrl; - ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, NULL, &ctrls); + ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &video->video, NULL, &ctrls); if (ret < 0) { dev_warn(isp->dev, "no pixel rate control in subdev %s\n", pipe->external->name); diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 2d7525e2d9eb..4ee703e04ef2 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -6,6 +6,8 @@ */ +#define pr_fmt(fmt) "v4l2-ctrls: " fmt + #include <linux/ctype.h> #include <linux/mm.h> #include <linux/slab.h> @@ -16,6 +18,12 @@ #include <media/v4l2-event.h> #include <media/v4l2-dev.h> +#define dprintk(vdev, fmt, arg...) do { \ + if (vdev->dev_debug & V4L2_DEV_DEBUG_CTRL) \ + printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \ + __func__, video_device_node_name(vdev), ##arg); \ +} while (0) + #define has_op(master, op) \ (master->ops && master->ops->op) #define call_op(master, op) \ @@ -3211,6 +3219,7 @@ static int v4l2_ctrl_request_bind(struct media_request *req, static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs, struct v4l2_ctrl_helper *helpers, + struct video_device *vdev, bool get) { struct v4l2_ctrl_helper *h; @@ -3228,20 +3237,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(vdev, "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(vdev, "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(vdev, "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(vdev, "control id 0x%x is disabled\n", id); return -EINVAL; + } if (ctrl->cluster[0]->ncontrols > 1) have_clusters = true; @@ -3251,10 +3268,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(vdev, "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; @@ -3315,7 +3338,8 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 which) /* Get extended controls. Allocates the helpers array if needed. */ static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, - struct v4l2_ext_controls *cs) + struct v4l2_ext_controls *cs, + struct video_device *vdev) { struct v4l2_ctrl_helper helper[4]; struct v4l2_ctrl_helper *helpers = helper; @@ -3341,7 +3365,7 @@ static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, return -ENOMEM; } - ret = prepare_ext_ctrls(hdl, cs, helpers, true); + ret = prepare_ext_ctrls(hdl, cs, helpers, vdev, true); cs->error_idx = cs->count; for (i = 0; !ret && i < cs->count; i++) @@ -3434,8 +3458,8 @@ v4l2_ctrls_find_req_obj(struct v4l2_ctrl_handler *hdl, return obj; } -int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, - struct v4l2_ext_controls *cs) +int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct video_device *vdev, + struct media_device *mdev, struct v4l2_ext_controls *cs) { struct media_request_object *obj = NULL; struct media_request *req = NULL; @@ -3471,7 +3495,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, req_obj); } - ret = v4l2_g_ext_ctrls_common(hdl, cs); + ret = v4l2_g_ext_ctrls_common(hdl, cs, vdev); if (obj) { media_request_unlock_for_access(req); @@ -3614,7 +3638,9 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master, /* Validate controls. */ static int validate_ctrls(struct v4l2_ext_controls *cs, - struct v4l2_ctrl_helper *helpers, bool set) + struct v4l2_ctrl_helper *helpers, + struct video_device *vdev, + bool set) { unsigned i; int ret = 0; @@ -3626,16 +3652,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(vdev, "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(vdev, "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. @@ -3670,7 +3700,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, struct v4l2_ctrl_handler *hdl, - struct v4l2_ext_controls *cs, bool set) + struct v4l2_ext_controls *cs, + struct video_device *vdev, bool set) { struct v4l2_ctrl_helper helper[4]; struct v4l2_ctrl_helper *helpers = helper; @@ -3680,13 +3711,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(vdev, "%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(vdev, "%s: invalid null control handler\n", video_device_node_name(vdev)); return -EINVAL; + } if (cs->count == 0) return class_check(hdl, cs->which); @@ -3697,9 +3732,9 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh, if (!helpers) return -ENOMEM; } - ret = prepare_ext_ctrls(hdl, cs, helpers, false); + ret = prepare_ext_ctrls(hdl, cs, helpers, vdev, false); if (!ret) - ret = validate_ctrls(cs, helpers, set); + ret = validate_ctrls(cs, helpers, vdev, set); if (ret && set) cs->error_idx = cs->count; for (i = 0; !ret && i < cs->count; i++) { @@ -3784,7 +3819,8 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh, } static int try_set_ext_ctrls(struct v4l2_fh *fh, - struct v4l2_ctrl_handler *hdl, struct media_device *mdev, + struct v4l2_ctrl_handler *hdl, + struct video_device *vdev, struct media_device *mdev, struct v4l2_ext_controls *cs, bool set) { struct media_request_object *obj = NULL; @@ -3792,21 +3828,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(vdev, "%s: missing media device\n", video_device_node_name(vdev)); + return -EINVAL; + } + + if (cs->request_fd < 0) { + dprintk(vdev, "%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(vdev, "%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(vdev, "%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(vdev, "%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); @@ -3815,7 +3862,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(fh, hdl, cs, vdev, set); + if (ret) + dprintk(vdev, "%s: try_set_ext_ctrls_common failed (%d)\n", video_device_node_name(vdev), ret); if (obj) { media_request_unlock_for_update(req); @@ -3826,17 +3875,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 v4l2_ctrl_handler *hdl, + struct video_device *vdev, + struct media_device *mdev, struct v4l2_ext_controls *cs) { - return try_set_ext_ctrls(NULL, hdl, mdev, cs, false); + return try_set_ext_ctrls(NULL, hdl, vdev, 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 v4l2_fh *fh, + struct v4l2_ctrl_handler *hdl, + struct video_device *vdev, + struct media_device *mdev, + struct v4l2_ext_controls *cs) { - return try_set_ext_ctrls(fh, hdl, mdev, cs, true); + return try_set_ext_ctrls(fh, hdl, vdev, mdev, cs, true); } EXPORT_SYMBOL(v4l2_s_ext_ctrls); diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index b1f4b991dba6..e95efea1a9ca 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -2165,9 +2165,9 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, p->error_idx = p->count; if (vfh && vfh->ctrl_handler) - return v4l2_g_ext_ctrls(vfh->ctrl_handler, vfd->v4l2_dev->mdev, p); + return v4l2_g_ext_ctrls(vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p); if (vfd->ctrl_handler) - return v4l2_g_ext_ctrls(vfd->ctrl_handler, vfd->v4l2_dev->mdev, p); + return v4l2_g_ext_ctrls(vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p); if (ops->vidioc_g_ext_ctrls == NULL) return -ENOTTY; return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) : @@ -2184,9 +2184,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(vfh, vfh->ctrl_handler, vfd, 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(NULL, vfd->ctrl_handler, vfd, 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) : @@ -2203,9 +2203,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(vfh->ctrl_handler, vfd, 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->ctrl_handler, vfd, 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 f24978b80440..1b5edd3b1e6c 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -211,19 +211,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) if (!vfh->ctrl_handler) return -ENOTTY; return v4l2_g_ext_ctrls(vfh->ctrl_handler, - sd->v4l2_dev->mdev, arg); + vdev, sd->v4l2_dev->mdev, arg); case VIDIOC_S_EXT_CTRLS: if (!vfh->ctrl_handler) return -ENOTTY; return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, - sd->v4l2_dev->mdev, arg); + vdev, sd->v4l2_dev->mdev, arg); case VIDIOC_TRY_EXT_CTRLS: if (!vfh->ctrl_handler) return -ENOTTY; return v4l2_try_ext_ctrls(vfh->ctrl_handler, - sd->v4l2_dev->mdev, arg); + vdev, sd->v4l2_dev->mdev, arg); case VIDIOC_DQEVENT: if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)) diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h index b4433483af23..c08d6cc56743 100644 --- a/include/media/v4l2-ctrls.h +++ b/include/media/v4l2-ctrls.h @@ -1265,25 +1265,28 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl, * :ref:`VIDIOC_G_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl * * @hdl: pointer to &struct v4l2_ctrl_handler + * @vdev: pointer to &struct video_device * @mdev: pointer to &struct media_device * @c: pointer to &struct v4l2_ext_controls * * If hdl == NULL then they will all return -EINVAL. */ -int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, - struct v4l2_ext_controls *c); +int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct video_device *vdev, + struct media_device *mdev, struct v4l2_ext_controls *c); /** * v4l2_try_ext_ctrls - Helper function to implement * :ref:`VIDIOC_TRY_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl * * @hdl: pointer to &struct v4l2_ctrl_handler + * @vdev: pointer to &struct video_device * @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, + struct video_device *vdev, struct media_device *mdev, struct v4l2_ext_controls *c); @@ -1293,12 +1296,14 @@ int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, * * @fh: pointer to &struct v4l2_fh * @hdl: pointer to &struct v4l2_ctrl_handler + * @vdev: pointer to &struct video_device * @mdev: pointer to &struct media_device * @c: pointer to &struct v4l2_ext_controls * * If hdl == NULL then they will all return -EINVAL. */ int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl, + struct video_device *vdev, struct media_device *mdev, struct v4l2_ext_controls *c); diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h index 400f2e46c108..4bba65a59d46 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -602,6 +602,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. Add debug messages on (hopefully) most of the error paths. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- Changes from v1: * Drop changes in the debug parameter semantics. * Drop new module debug parameter. * Add documentation. * Add a debug error in all places where control can fail. * Reorder the vdev parameter, to make the patch less invasive. --- Documentation/media/kapi/v4l2-dev.rst | 1 + drivers/media/platform/omap3isp/ispvideo.c | 2 +- drivers/media/v4l2-core/v4l2-ctrls.c | 106 ++++++++++++++++----- drivers/media/v4l2-core/v4l2-ioctl.c | 12 +-- drivers/media/v4l2-core/v4l2-subdev.c | 6 +- include/media/v4l2-ctrls.h | 9 +- include/media/v4l2-ioctl.h | 2 + 7 files changed, 100 insertions(+), 38 deletions(-)