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