* [PATCH v6 01/17] media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL [not found] <20210317164511.39967-1-ribalda@chromium.org> @ 2021-03-17 16:44 ` Ricardo Ribalda 2021-03-18 7:14 ` Hans Verkuil 2021-03-17 16:44 ` [PATCH v6 02/17] media: v4l2-ioctl: check_ext_ctrls: Return -EINVAL on V4L2_CTRL_WHICH_REQUEST_VAL Ricardo Ribalda ` (2 subsequent siblings) 3 siblings, 1 reply; 8+ messages in thread From: Ricardo Ribalda @ 2021-03-17 16:44 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Hans Verkuil, Sergey Senozhatsky, linux-media, linux-kernel, tfiga Cc: Ricardo Ribalda, stable Drivers that do not use the ctrl-framework use this function instead. - Do not check for multiple classes when getting the DEF_VAL. Fixes v4l2-compliance: Control ioctls (Input 0): fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL Cc: stable@vger.kernel.org Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++++++++++++++++------------ 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 31d1342e61e8..403f957a1012 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only) pr_cont("driver-specific ioctl\n"); } -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) { __u32 i; @@ -917,23 +917,30 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) for (i = 0; i < c->count; i++) c->controls[i].reserved2[0] = 0; - /* V4L2_CID_PRIVATE_BASE cannot be used as control class - when using extended controls. - Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL - is it allowed for backwards compatibility. - */ - if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) - return 0; - if (!c->which) - return 1; + switch (c->which) { + case V4L2_CID_PRIVATE_BASE: + /* + * V4L2_CID_PRIVATE_BASE cannot be used as control class + * when using extended controls. + * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL + * is it allowed for backwards compatibility. + */ + if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP) + return false; + break; + case V4L2_CTRL_WHICH_DEF_VAL: + case V4L2_CTRL_WHICH_CUR_VAL: + return true; + } + /* Check that all controls are from the same control class. */ for (i = 0; i < c->count; i++) { if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { c->error_idx = i; - return 0; + return false; } } - return 1; + return true; } static int check_fmt(struct file *file, enum v4l2_buf_type type) @@ -2229,7 +2236,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, ctrls.controls = &ctrl; ctrl.id = p->id; ctrl.value = p->value; - if (check_ext_ctrls(&ctrls, 1)) { + if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) { int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls); if (ret == 0) @@ -2263,7 +2270,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, ctrls.controls = &ctrl; ctrl.id = p->id; ctrl.value = p->value; - if (check_ext_ctrls(&ctrls, 1)) + if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL)) return ops->vidioc_s_ext_ctrls(file, fh, &ctrls); return -EINVAL; } @@ -2285,8 +2292,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, 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) : - -EINVAL; + return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ? + ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL; } static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, @@ -2306,8 +2313,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, 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) : - -EINVAL; + return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ? + ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL; } static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, @@ -2327,8 +2334,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, 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) : - -EINVAL; + return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ? + ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL; } /* -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v6 01/17] media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL 2021-03-17 16:44 ` [PATCH v6 01/17] media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL Ricardo Ribalda @ 2021-03-18 7:14 ` Hans Verkuil 2021-03-18 7:17 ` Ricardo Ribalda 0 siblings, 1 reply; 8+ messages in thread From: Hans Verkuil @ 2021-03-18 7:14 UTC (permalink / raw) To: Ricardo Ribalda, Laurent Pinchart, Mauro Carvalho Chehab, Sergey Senozhatsky, linux-media, linux-kernel, tfiga Cc: stable Hi Ricardo, On 17/03/2021 17:44, Ricardo Ribalda wrote: > Drivers that do not use the ctrl-framework use this function instead. > > - Do not check for multiple classes when getting the DEF_VAL. > > Fixes v4l2-compliance: > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL Can you merge patches 1-4 into a single patch? It's really one big fix since this code was never updated when new 'which' values were added. So keeping it together is, for once, actually preferred. You can add my: Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> after these 4 patches are merged. It looks much nicer now. Regards, Hans > > Cc: stable@vger.kernel.org > Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++++++++++++++++------------ > 1 file changed, 27 insertions(+), 20 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 31d1342e61e8..403f957a1012 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only) > pr_cont("driver-specific ioctl\n"); > } > > -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) > { > __u32 i; > > @@ -917,23 +917,30 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > for (i = 0; i < c->count; i++) > c->controls[i].reserved2[0] = 0; > > - /* V4L2_CID_PRIVATE_BASE cannot be used as control class > - when using extended controls. > - Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > - is it allowed for backwards compatibility. > - */ > - if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) > - return 0; > - if (!c->which) > - return 1; > + switch (c->which) { > + case V4L2_CID_PRIVATE_BASE: > + /* > + * V4L2_CID_PRIVATE_BASE cannot be used as control class > + * when using extended controls. > + * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > + * is it allowed for backwards compatibility. > + */ > + if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP) > + return false; > + break; > + case V4L2_CTRL_WHICH_DEF_VAL: > + case V4L2_CTRL_WHICH_CUR_VAL: > + return true; > + } > + > /* Check that all controls are from the same control class. */ > for (i = 0; i < c->count; i++) { > if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { > c->error_idx = i; > - return 0; > + return false; > } > } > - return 1; > + return true; > } > > static int check_fmt(struct file *file, enum v4l2_buf_type type) > @@ -2229,7 +2236,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, > ctrls.controls = &ctrl; > ctrl.id = p->id; > ctrl.value = p->value; > - if (check_ext_ctrls(&ctrls, 1)) { > + if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) { > int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls); > > if (ret == 0) > @@ -2263,7 +2270,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, > ctrls.controls = &ctrl; > ctrl.id = p->id; > ctrl.value = p->value; > - if (check_ext_ctrls(&ctrls, 1)) > + if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL)) > return ops->vidioc_s_ext_ctrls(file, fh, &ctrls); > return -EINVAL; > } > @@ -2285,8 +2292,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, > 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) : > - -EINVAL; > + return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ? > + ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL; > } > > static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > @@ -2306,8 +2313,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > 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) : > - -EINVAL; > + return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ? > + ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL; > } > > static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > @@ -2327,8 +2334,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > 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) : > - -EINVAL; > + return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ? > + ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL; > } > > /* > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 01/17] media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL 2021-03-18 7:14 ` Hans Verkuil @ 2021-03-18 7:17 ` Ricardo Ribalda 2021-03-18 7:48 ` Hans Verkuil 0 siblings, 1 reply; 8+ messages in thread From: Ricardo Ribalda @ 2021-03-18 7:17 UTC (permalink / raw) To: Hans Verkuil Cc: Laurent Pinchart, Mauro Carvalho Chehab, Sergey Senozhatsky, Linux Media Mailing List, Linux Kernel Mailing List, Tomasz Figa, stable Hi Hans Can I merge 1-3, but leave 4 as a separate one? It helps to tell a story for 5 and 6. Thanks On Thu, Mar 18, 2021 at 8:14 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > Hi Ricardo, > > On 17/03/2021 17:44, Ricardo Ribalda wrote: > > Drivers that do not use the ctrl-framework use this function instead. > > > > - Do not check for multiple classes when getting the DEF_VAL. > > > > Fixes v4l2-compliance: > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Can you merge patches 1-4 into a single patch? It's really one big fix since > this code was never updated when new 'which' values were added. So keeping it > together is, for once, actually preferred. > > You can add my: > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > after these 4 patches are merged. It looks much nicer now. > > Regards, > > Hans > > > > > Cc: stable@vger.kernel.org > > Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") > > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++++++++++++++++------------ > > 1 file changed, 27 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > index 31d1342e61e8..403f957a1012 100644 > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only) > > pr_cont("driver-specific ioctl\n"); > > } > > > > -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > > +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) > > { > > __u32 i; > > > > @@ -917,23 +917,30 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > > for (i = 0; i < c->count; i++) > > c->controls[i].reserved2[0] = 0; > > > > - /* V4L2_CID_PRIVATE_BASE cannot be used as control class > > - when using extended controls. > > - Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > > - is it allowed for backwards compatibility. > > - */ > > - if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) > > - return 0; > > - if (!c->which) > > - return 1; > > + switch (c->which) { > > + case V4L2_CID_PRIVATE_BASE: > > + /* > > + * V4L2_CID_PRIVATE_BASE cannot be used as control class > > + * when using extended controls. > > + * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > > + * is it allowed for backwards compatibility. > > + */ > > + if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP) > > + return false; > > + break; > > + case V4L2_CTRL_WHICH_DEF_VAL: > > + case V4L2_CTRL_WHICH_CUR_VAL: > > + return true; > > + } > > + > > /* Check that all controls are from the same control class. */ > > for (i = 0; i < c->count; i++) { > > if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { > > c->error_idx = i; > > - return 0; > > + return false; > > } > > } > > - return 1; > > + return true; > > } > > > > static int check_fmt(struct file *file, enum v4l2_buf_type type) > > @@ -2229,7 +2236,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, > > ctrls.controls = &ctrl; > > ctrl.id = p->id; > > ctrl.value = p->value; > > - if (check_ext_ctrls(&ctrls, 1)) { > > + if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) { > > int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls); > > > > if (ret == 0) > > @@ -2263,7 +2270,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, > > ctrls.controls = &ctrl; > > ctrl.id = p->id; > > ctrl.value = p->value; > > - if (check_ext_ctrls(&ctrls, 1)) > > + if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL)) > > return ops->vidioc_s_ext_ctrls(file, fh, &ctrls); > > return -EINVAL; > > } > > @@ -2285,8 +2292,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > 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) : > > - -EINVAL; > > + return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ? > > + ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL; > > } > > > > static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > @@ -2306,8 +2313,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > 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) : > > - -EINVAL; > > + return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ? > > + ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL; > > } > > > > static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > @@ -2327,8 +2334,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > > 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) : > > - -EINVAL; > > + return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ? > > + ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL; > > } > > > > /* > > > -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 01/17] media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL 2021-03-18 7:17 ` Ricardo Ribalda @ 2021-03-18 7:48 ` Hans Verkuil 2021-03-18 7:49 ` Ricardo Ribalda 0 siblings, 1 reply; 8+ messages in thread From: Hans Verkuil @ 2021-03-18 7:48 UTC (permalink / raw) To: Ricardo Ribalda Cc: Laurent Pinchart, Mauro Carvalho Chehab, Sergey Senozhatsky, Linux Media Mailing List, Linux Kernel Mailing List, Tomasz Figa, stable On 18/03/2021 08:17, Ricardo Ribalda wrote: > Hi Hans > > Can I merge 1-3, but leave 4 as a separate one? It helps to tell a > story for 5 and 6. I really prefer it as a single patch. All four patches are basically a single big fix for v4l2-ioctl.c where the code for drivers that do not use the control framework had become very outdated. Fixing it in a single patch helps backporting to stable, and it is easier to review and see everything that had to be done to fix this. In this case I wondered when I was reviewing patch 1 why V4L2_CTRL_WHICH_DEF_VAL was just accepted without checking for S/TRY_EXT_CTRLS. Basically patch 1 is a broken fix w.r.t. DEF_VAL until patch 4, which really fixes it. Just do it all in a single patch, splitting it up doesn't work in this particular case. Regards, Hans > > Thanks > > On Thu, Mar 18, 2021 at 8:14 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> Hi Ricardo, >> >> On 17/03/2021 17:44, Ricardo Ribalda wrote: >>> Drivers that do not use the ctrl-framework use this function instead. >>> >>> - Do not check for multiple classes when getting the DEF_VAL. >>> >>> Fixes v4l2-compliance: >>> Control ioctls (Input 0): >>> fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) >>> test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL >> >> Can you merge patches 1-4 into a single patch? It's really one big fix since >> this code was never updated when new 'which' values were added. So keeping it >> together is, for once, actually preferred. >> >> You can add my: >> >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> after these 4 patches are merged. It looks much nicer now. >> >> Regards, >> >> Hans >> >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++++++++++++++++------------ >>> 1 file changed, 27 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>> index 31d1342e61e8..403f957a1012 100644 >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>> @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only) >>> pr_cont("driver-specific ioctl\n"); >>> } >>> >>> -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) >>> +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) >>> { >>> __u32 i; >>> >>> @@ -917,23 +917,30 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) >>> for (i = 0; i < c->count; i++) >>> c->controls[i].reserved2[0] = 0; >>> >>> - /* V4L2_CID_PRIVATE_BASE cannot be used as control class >>> - when using extended controls. >>> - Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL >>> - is it allowed for backwards compatibility. >>> - */ >>> - if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) >>> - return 0; >>> - if (!c->which) >>> - return 1; >>> + switch (c->which) { >>> + case V4L2_CID_PRIVATE_BASE: >>> + /* >>> + * V4L2_CID_PRIVATE_BASE cannot be used as control class >>> + * when using extended controls. >>> + * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL >>> + * is it allowed for backwards compatibility. >>> + */ >>> + if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP) >>> + return false; >>> + break; >>> + case V4L2_CTRL_WHICH_DEF_VAL: >>> + case V4L2_CTRL_WHICH_CUR_VAL: >>> + return true; >>> + } >>> + >>> /* Check that all controls are from the same control class. */ >>> for (i = 0; i < c->count; i++) { >>> if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { >>> c->error_idx = i; >>> - return 0; >>> + return false; >>> } >>> } >>> - return 1; >>> + return true; >>> } >>> >>> static int check_fmt(struct file *file, enum v4l2_buf_type type) >>> @@ -2229,7 +2236,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, >>> ctrls.controls = &ctrl; >>> ctrl.id = p->id; >>> ctrl.value = p->value; >>> - if (check_ext_ctrls(&ctrls, 1)) { >>> + if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) { >>> int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls); >>> >>> if (ret == 0) >>> @@ -2263,7 +2270,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, >>> ctrls.controls = &ctrl; >>> ctrl.id = p->id; >>> ctrl.value = p->value; >>> - if (check_ext_ctrls(&ctrls, 1)) >>> + if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL)) >>> return ops->vidioc_s_ext_ctrls(file, fh, &ctrls); >>> return -EINVAL; >>> } >>> @@ -2285,8 +2292,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> 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) : >>> - -EINVAL; >>> + return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ? >>> + ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL; >>> } >>> >>> static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> @@ -2306,8 +2313,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> 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) : >>> - -EINVAL; >>> + return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ? >>> + ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL; >>> } >>> >>> static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> @@ -2327,8 +2334,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, >>> 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) : >>> - -EINVAL; >>> + return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ? >>> + ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL; >>> } >>> >>> /* >>> >> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6 01/17] media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL 2021-03-18 7:48 ` Hans Verkuil @ 2021-03-18 7:49 ` Ricardo Ribalda 0 siblings, 0 replies; 8+ messages in thread From: Ricardo Ribalda @ 2021-03-18 7:49 UTC (permalink / raw) To: Hans Verkuil Cc: Laurent Pinchart, Mauro Carvalho Chehab, Sergey Senozhatsky, Linux Media Mailing List, Linux Kernel Mailing List, Tomasz Figa, stable Hi Hans On Thu, Mar 18, 2021 at 8:48 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 18/03/2021 08:17, Ricardo Ribalda wrote: > > Hi Hans > > > > Can I merge 1-3, but leave 4 as a separate one? It helps to tell a > > story for 5 and 6. > > I really prefer it as a single patch. All four patches are basically a single big fix > for v4l2-ioctl.c where the code for drivers that do not use the control framework had > become very outdated. Fixing it in a single patch helps backporting to stable, and > it is easier to review and see everything that had to be done to fix this. > > In this case I wondered when I was reviewing patch 1 why V4L2_CTRL_WHICH_DEF_VAL was > just accepted without checking for S/TRY_EXT_CTRLS. Basically patch 1 is a broken fix > w.r.t. DEF_VAL until patch 4, which really fixes it. > > Just do it all in a single patch, splitting it up doesn't work in this particular case. Ok, thanks for the clarification :) > > Regards, > > Hans > > > > > Thanks > > > > On Thu, Mar 18, 2021 at 8:14 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> Hi Ricardo, > >> > >> On 17/03/2021 17:44, Ricardo Ribalda wrote: > >>> Drivers that do not use the ctrl-framework use this function instead. > >>> > >>> - Do not check for multiple classes when getting the DEF_VAL. > >>> > >>> Fixes v4l2-compliance: > >>> Control ioctls (Input 0): > >>> fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) > >>> test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > >> > >> Can you merge patches 1-4 into a single patch? It's really one big fix since > >> this code was never updated when new 'which' values were added. So keeping it > >> together is, for once, actually preferred. > >> > >> You can add my: > >> > >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> > >> after these 4 patches are merged. It looks much nicer now. > >> > >> Regards, > >> > >> Hans > >> > >>> > >>> Cc: stable@vger.kernel.org > >>> Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") > >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++++++++++++++++------------ > >>> 1 file changed, 27 insertions(+), 20 deletions(-) > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > >>> index 31d1342e61e8..403f957a1012 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c > >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > >>> @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only) > >>> pr_cont("driver-specific ioctl\n"); > >>> } > >>> > >>> -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > >>> +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) > >>> { > >>> __u32 i; > >>> > >>> @@ -917,23 +917,30 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > >>> for (i = 0; i < c->count; i++) > >>> c->controls[i].reserved2[0] = 0; > >>> > >>> - /* V4L2_CID_PRIVATE_BASE cannot be used as control class > >>> - when using extended controls. > >>> - Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > >>> - is it allowed for backwards compatibility. > >>> - */ > >>> - if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) > >>> - return 0; > >>> - if (!c->which) > >>> - return 1; > >>> + switch (c->which) { > >>> + case V4L2_CID_PRIVATE_BASE: > >>> + /* > >>> + * V4L2_CID_PRIVATE_BASE cannot be used as control class > >>> + * when using extended controls. > >>> + * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL > >>> + * is it allowed for backwards compatibility. > >>> + */ > >>> + if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CROP) > >>> + return false; > >>> + break; > >>> + case V4L2_CTRL_WHICH_DEF_VAL: > >>> + case V4L2_CTRL_WHICH_CUR_VAL: > >>> + return true; > >>> + } > >>> + > >>> /* Check that all controls are from the same control class. */ > >>> for (i = 0; i < c->count; i++) { > >>> if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { > >>> c->error_idx = i; > >>> - return 0; > >>> + return false; > >>> } > >>> } > >>> - return 1; > >>> + return true; > >>> } > >>> > >>> static int check_fmt(struct file *file, enum v4l2_buf_type type) > >>> @@ -2229,7 +2236,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops, > >>> ctrls.controls = &ctrl; > >>> ctrl.id = p->id; > >>> ctrl.value = p->value; > >>> - if (check_ext_ctrls(&ctrls, 1)) { > >>> + if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) { > >>> int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls); > >>> > >>> if (ret == 0) > >>> @@ -2263,7 +2270,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops, > >>> ctrls.controls = &ctrl; > >>> ctrl.id = p->id; > >>> ctrl.value = p->value; > >>> - if (check_ext_ctrls(&ctrls, 1)) > >>> + if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL)) > >>> return ops->vidioc_s_ext_ctrls(file, fh, &ctrls); > >>> return -EINVAL; > >>> } > >>> @@ -2285,8 +2292,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> 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) : > >>> - -EINVAL; > >>> + return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ? > >>> + ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL; > >>> } > >>> > >>> static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> @@ -2306,8 +2313,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> 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) : > >>> - -EINVAL; > >>> + return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ? > >>> + ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL; > >>> } > >>> > >>> static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> @@ -2327,8 +2334,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, > >>> 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) : > >>> - -EINVAL; > >>> + return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ? > >>> + ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL; > >>> } > >>> > >>> /* > >>> > >> > > > > > -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v6 02/17] media: v4l2-ioctl: check_ext_ctrls: Return -EINVAL on V4L2_CTRL_WHICH_REQUEST_VAL [not found] <20210317164511.39967-1-ribalda@chromium.org> 2021-03-17 16:44 ` [PATCH v6 01/17] media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL Ricardo Ribalda @ 2021-03-17 16:44 ` Ricardo Ribalda 2021-03-17 16:44 ` [PATCH v6 03/17] media: v4l2-ioctl: check_ext_ctrls: Return the right error_idx Ricardo Ribalda 2021-03-17 16:44 ` [PATCH v6 04/17] media: v4l2-ioctl: check_ext_ctrls: Fix V4L2_CTRL_WHICH_DEF_VAL Ricardo Ribalda 3 siblings, 0 replies; 8+ messages in thread From: Ricardo Ribalda @ 2021-03-17 16:44 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Hans Verkuil, Sergey Senozhatsky, linux-media, linux-kernel, tfiga Cc: Ricardo Ribalda, stable Drivers that do not use the ctrl-framework use this function instead. - Return -EINVAL for request_api calls Fixes v4l2-compliance: Buffer ioctls (Input 0): fail: v4l2-test-buffers.cpp(1994): ret != EINVAL && ret != EBADR && ret != ENOTTY test Requests: FAIL Cc: stable@vger.kernel.org Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 403f957a1012..76e2e595d88d 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -931,6 +931,8 @@ static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) case V4L2_CTRL_WHICH_DEF_VAL: case V4L2_CTRL_WHICH_CUR_VAL: return true; + case V4L2_CTRL_WHICH_REQUEST_VAL: + return false; } /* Check that all controls are from the same control class. */ -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v6 03/17] media: v4l2-ioctl: check_ext_ctrls: Return the right error_idx [not found] <20210317164511.39967-1-ribalda@chromium.org> 2021-03-17 16:44 ` [PATCH v6 01/17] media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL Ricardo Ribalda 2021-03-17 16:44 ` [PATCH v6 02/17] media: v4l2-ioctl: check_ext_ctrls: Return -EINVAL on V4L2_CTRL_WHICH_REQUEST_VAL Ricardo Ribalda @ 2021-03-17 16:44 ` Ricardo Ribalda 2021-03-17 16:44 ` [PATCH v6 04/17] media: v4l2-ioctl: check_ext_ctrls: Fix V4L2_CTRL_WHICH_DEF_VAL Ricardo Ribalda 3 siblings, 0 replies; 8+ messages in thread From: Ricardo Ribalda @ 2021-03-17 16:44 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Hans Verkuil, Sergey Senozhatsky, linux-media, linux-kernel, tfiga Cc: Ricardo Ribalda, stable Drivers that do not use the ctrl-framework use this function instead. - Return the right error_idx If an error is found when validating the list of controls passed with VIDIOC_G_EXT_CTRLS, then error_idx shall be set to ctrls->count to indicate to userspace that no actual hardware was touched. It would have been much nicer of course if error_idx could point to the control index that failed the validation, but sadly that's not how the API was designed. Fixes v4l2-compliance: Control ioctls (Input 0): warn: v4l2-test-controls.cpp(834): error_idx should be equal to count warn: v4l2-test-controls.cpp(855): error_idx should be equal to count Cc: stable@vger.kernel.org Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 76e2e595d88d..528eb5f420f6 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -932,13 +932,14 @@ static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) case V4L2_CTRL_WHICH_CUR_VAL: return true; case V4L2_CTRL_WHICH_REQUEST_VAL: + c->error_idx = c->count; return false; } /* Check that all controls are from the same control class. */ for (i = 0; i < c->count; i++) { if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { - c->error_idx = i; + c->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i : c->count; return false; } } -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v6 04/17] media: v4l2-ioctl: check_ext_ctrls: Fix V4L2_CTRL_WHICH_DEF_VAL [not found] <20210317164511.39967-1-ribalda@chromium.org> ` (2 preceding siblings ...) 2021-03-17 16:44 ` [PATCH v6 03/17] media: v4l2-ioctl: check_ext_ctrls: Return the right error_idx Ricardo Ribalda @ 2021-03-17 16:44 ` Ricardo Ribalda 3 siblings, 0 replies; 8+ messages in thread From: Ricardo Ribalda @ 2021-03-17 16:44 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Hans Verkuil, Sergey Senozhatsky, linux-media, linux-kernel, tfiga Cc: Ricardo Ribalda, stable Drivers that do not use the ctrl-framework use this function instead. Default value cannot be changed, return EINVAL as soon as possible. Cc: stable@vger.kernel.org Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/v4l2-core/v4l2-ioctl.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 528eb5f420f6..ccd21b4d9c72 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -929,6 +929,13 @@ static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) return false; break; case V4L2_CTRL_WHICH_DEF_VAL: + /* Default value cannot be changed */ + if (ioctl == VIDIOC_S_EXT_CTRLS || + ioctl == VIDIOC_TRY_EXT_CTRLS) { + c->error_idx = c->count; + return false; + } + return true; case V4L2_CTRL_WHICH_CUR_VAL: return true; case V4L2_CTRL_WHICH_REQUEST_VAL: -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-03-18 7:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210317164511.39967-1-ribalda@chromium.org>
2021-03-17 16:44 ` [PATCH v6 01/17] media: v4l2-ioctl: check_ext_ctrls: Fix multiclass V4L2_CTRL_WHICH_DEF_VAL Ricardo Ribalda
2021-03-18 7:14 ` Hans Verkuil
2021-03-18 7:17 ` Ricardo Ribalda
2021-03-18 7:48 ` Hans Verkuil
2021-03-18 7:49 ` Ricardo Ribalda
2021-03-17 16:44 ` [PATCH v6 02/17] media: v4l2-ioctl: check_ext_ctrls: Return -EINVAL on V4L2_CTRL_WHICH_REQUEST_VAL Ricardo Ribalda
2021-03-17 16:44 ` [PATCH v6 03/17] media: v4l2-ioctl: check_ext_ctrls: Return the right error_idx Ricardo Ribalda
2021-03-17 16:44 ` [PATCH v6 04/17] media: v4l2-ioctl: check_ext_ctrls: Fix V4L2_CTRL_WHICH_DEF_VAL Ricardo Ribalda
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).