The VIDIOC_G_EXT_CTRLS with which V4L2_CTRL_WHICH_(MIN|MAX)_VAL can only work for controls that have previously announced support for it.
This patch fixes the following v4l2-compliance error:
info: checking extended control 'User Controls' (0x00980001) fail: v4l2-test-controls.cpp(980): ret != EINVAL (got 13) test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
Fixes: 39d2c891c96e ("media: uvcvideo: support V4L2_CTRL_WHICH_MIN/MAX_VAL") Cc: stable@vger.kernel.org Signed-off-by: Ricardo Ribalda ribalda@chromium.org --- drivers/media/usb/uvc/uvc_ctrl.c | 11 +++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 9 ++++++--- drivers/media/usb/uvc/uvcvideo.h | 2 +- 3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 2905505c240c060e5034ea12d33b59d5702f2e1f..2f7d5cdd18e072a47fb5906da0f847dd449911b4 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1432,7 +1432,7 @@ static bool uvc_ctrl_is_readable(u32 which, struct uvc_control *ctrl, * auto_exposure=1, exposure_time_absolute=251. */ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, - const struct v4l2_ext_controls *ctrls, + const struct v4l2_ext_controls *ctrls, u32 which, unsigned long ioctl) { struct uvc_control_mapping *master_map = NULL; @@ -1442,14 +1442,21 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, s32 val; int ret; int i; + bool is_which_min_max = (ioctl == VIDIOC_G_EXT_CTRLS && + (which == V4L2_CTRL_WHICH_MIN_VAL || + which == V4L2_CTRL_WHICH_MAX_VAL));
if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0) - return -EACCES; + return is_which_min_max ? -EINVAL : -EACCES;
ctrl = uvc_find_control(chain, v4l2_id, &mapping); if (!ctrl) return -EINVAL;
+ if ((!(ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) || + !(ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)) && is_which_min_max) + return -EINVAL; + if (ioctl == VIDIOC_G_EXT_CTRLS) return uvc_ctrl_is_readable(ctrls->which, ctrl, mapping);
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 9e4a251eca88085a1b4e0e854370015855be92ee..d5274dc94da3c60f1f4566b307dd445f30c4f45f 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -765,6 +765,7 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *priv,
static int uvc_ctrl_check_access(struct uvc_video_chain *chain, struct v4l2_ext_controls *ctrls, + u32 which, unsigned long ioctl) { struct v4l2_ext_control *ctrl = ctrls->controls; @@ -772,7 +773,8 @@ static int uvc_ctrl_check_access(struct uvc_video_chain *chain, int ret = 0;
for (i = 0; i < ctrls->count; ++ctrl, ++i) { - ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls, ioctl); + ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls, which, + ioctl); if (ret) break; } @@ -806,7 +808,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *priv, which = V4L2_CTRL_WHICH_CUR_VAL; }
- ret = uvc_ctrl_check_access(chain, ctrls, VIDIOC_G_EXT_CTRLS); + ret = uvc_ctrl_check_access(chain, ctrls, which, VIDIOC_G_EXT_CTRLS); if (ret < 0) return ret;
@@ -840,7 +842,8 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle, if (!ctrls->count) return 0;
- ret = uvc_ctrl_check_access(chain, ctrls, ioctl); + ret = uvc_ctrl_check_access(chain, ctrls, V4L2_CTRL_WHICH_CUR_VAL, + ioctl); if (ret < 0) return ret;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index ed7bad31f75ca474c1037d666d5310c78dd764df..d583425893a5f716185153a07aae9bfe20182964 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -786,7 +786,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, u32 which, struct v4l2_ext_control *xctrl); int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl); int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, - const struct v4l2_ext_controls *ctrls, + const struct v4l2_ext_controls *ctrls, u32 which, unsigned long ioctl);
int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
--- base-commit: c218ce4f98eccf5a40de64c559c52d61e9cc78ee change-id: 20251028-uvc-fix-which-c3ba1fb68ed5
Best regards,
Hi Ricardo,
Thank you for the patch.
On Tue, Oct 28, 2025 at 05:55:10PM +0000, Ricardo Ribalda wrote:
The VIDIOC_G_EXT_CTRLS with which V4L2_CTRL_WHICH_(MIN|MAX)_VAL can only work for controls that have previously announced support for it.
This patch fixes the following v4l2-compliance error:
info: checking extended control 'User Controls' (0x00980001) fail: v4l2-test-controls.cpp(980): ret != EINVAL (got 13) test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
Fixes: 39d2c891c96e ("media: uvcvideo: support V4L2_CTRL_WHICH_MIN/MAX_VAL") Cc: stable@vger.kernel.org Signed-off-by: Ricardo Ribalda ribalda@chromium.org
drivers/media/usb/uvc/uvc_ctrl.c | 11 +++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 9 ++++++--- drivers/media/usb/uvc/uvcvideo.h | 2 +- 3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 2905505c240c060e5034ea12d33b59d5702f2e1f..2f7d5cdd18e072a47fb5906da0f847dd449911b4 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1432,7 +1432,7 @@ static bool uvc_ctrl_is_readable(u32 which, struct uvc_control *ctrl,
- auto_exposure=1, exposure_time_absolute=251.
*/ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
const struct v4l2_ext_controls *ctrls,
const struct v4l2_ext_controls *ctrls, u32 which, unsigned long ioctl){ struct uvc_control_mapping *master_map = NULL; @@ -1442,14 +1442,21 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, s32 val; int ret; int i;
- bool is_which_min_max = (ioctl == VIDIOC_G_EXT_CTRLS &&
Is the ioctl check needed, given that this function will be called with which set to V4L2_CTRL_WHICH_CUR_VAL if ioctl is not VIDIOC_G_EXT_CTRLS ?
(which == V4L2_CTRL_WHICH_MIN_VAL ||which == V4L2_CTRL_WHICH_MAX_VAL));
Let's move this up to have longer lines first.
if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
return -EACCES;
return is_which_min_max ? -EINVAL : -EACCES;ctrl = uvc_find_control(chain, v4l2_id, &mapping); if (!ctrl) return -EINVAL;
- if ((!(ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) ||
!(ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)) && is_which_min_max)
Please put MIN before MAX.
Do we have to bundle min and max, or could we handle them separately ? Something like
if ((which == V4L2_CTRL_WHICH_MIN_VAL && !(ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)) || (which == V4L2_CTRL_WHICH_M1X_VAL && !(ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX))) return -EINVAL;
return -EINVAL;- if (ioctl == VIDIOC_G_EXT_CTRLS) return uvc_ctrl_is_readable(ctrls->which, ctrl, mapping);
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 9e4a251eca88085a1b4e0e854370015855be92ee..d5274dc94da3c60f1f4566b307dd445f30c4f45f 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -765,6 +765,7 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *priv, static int uvc_ctrl_check_access(struct uvc_video_chain *chain, struct v4l2_ext_controls *ctrls,
u32 which,
This fits on the previous line.
unsigned long ioctl){ struct v4l2_ext_control *ctrl = ctrls->controls; @@ -772,7 +773,8 @@ static int uvc_ctrl_check_access(struct uvc_video_chain *chain, int ret = 0; for (i = 0; i < ctrls->count; ++ctrl, ++i) {
ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls, ioctl);
ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls, which,ioctl);@@ -806,7 +808,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *priv, which = V4L2_CTRL_WHICH_CUR_VAL; }
- ret = uvc_ctrl_check_access(chain, ctrls, VIDIOC_G_EXT_CTRLS);
- ret = uvc_ctrl_check_access(chain, ctrls, which, VIDIOC_G_EXT_CTRLS); if (ret < 0) return ret;
@@ -840,7 +842,8 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle, if (!ctrls->count) return 0;
- ret = uvc_ctrl_check_access(chain, ctrls, ioctl);
- ret = uvc_ctrl_check_access(chain, ctrls, V4L2_CTRL_WHICH_CUR_VAL,
ioctl);diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index ed7bad31f75ca474c1037d666d5310c78dd764df..d583425893a5f716185153a07aae9bfe20182964 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -786,7 +786,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, u32 which, struct v4l2_ext_control *xctrl); int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl); int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
const struct v4l2_ext_controls *ctrls,
const struct v4l2_ext_controls *ctrls, u32 which, unsigned long ioctl);int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
base-commit: c218ce4f98eccf5a40de64c559c52d61e9cc78ee change-id: 20251028-uvc-fix-which-c3ba1fb68ed5
Hi Laurent
On Thu, 13 Nov 2025 at 22:59, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ricardo,
Thank you for the patch.
On Tue, Oct 28, 2025 at 05:55:10PM +0000, Ricardo Ribalda wrote:
The VIDIOC_G_EXT_CTRLS with which V4L2_CTRL_WHICH_(MIN|MAX)_VAL can only work for controls that have previously announced support for it.
This patch fixes the following v4l2-compliance error:
info: checking extended control 'User Controls' (0x00980001) fail: v4l2-test-controls.cpp(980): ret != EINVAL (got 13) test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
Fixes: 39d2c891c96e ("media: uvcvideo: support V4L2_CTRL_WHICH_MIN/MAX_VAL") Cc: stable@vger.kernel.org Signed-off-by: Ricardo Ribalda ribalda@chromium.org
drivers/media/usb/uvc/uvc_ctrl.c | 11 +++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 9 ++++++--- drivers/media/usb/uvc/uvcvideo.h | 2 +- 3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 2905505c240c060e5034ea12d33b59d5702f2e1f..2f7d5cdd18e072a47fb5906da0f847dd449911b4 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1432,7 +1432,7 @@ static bool uvc_ctrl_is_readable(u32 which, struct uvc_control *ctrl,
- auto_exposure=1, exposure_time_absolute=251.
*/ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
const struct v4l2_ext_controls *ctrls,
const struct v4l2_ext_controls *ctrls, u32 which, unsigned long ioctl){ struct uvc_control_mapping *master_map = NULL; @@ -1442,14 +1442,21 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, s32 val; int ret; int i;
bool is_which_min_max = (ioctl == VIDIOC_G_EXT_CTRLS &&Is the ioctl check needed, given that this function will be called with which set to V4L2_CTRL_WHICH_CUR_VAL if ioctl is not VIDIOC_G_EXT_CTRLS
Right now it is not needed. But I wanted to be on the protective side here. Let me add a comment and remove the ioctl check.
?
(which == V4L2_CTRL_WHICH_MIN_VAL ||which == V4L2_CTRL_WHICH_MAX_VAL));Let's move this up to have longer lines first.
if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
return -EACCES;
return is_which_min_max ? -EINVAL : -EACCES; ctrl = uvc_find_control(chain, v4l2_id, &mapping); if (!ctrl) return -EINVAL;if ((!(ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) ||!(ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)) && is_which_min_max)Please put MIN before MAX.
Do we have to bundle min and max, or could we handle them separately ? Something like
I believe we have to bundle min and max. Controls that support VIDIOC_G_EXT_CTRLS with V4L2_CTRL_WHICH_(MIN|MAX)_VAL have this flag: V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX
``` Whether a control supports querying the minimum and maximum values using V4L2_CTRL_WHICH_MIN_VAL and V4L2_CTRL_WHICH_MAX_VAL is indicated by the V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX flag. ```
if ((which == V4L2_CTRL_WHICH_MIN_VAL && !(ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN)) || (which == V4L2_CTRL_WHICH_M1X_VAL && !(ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX))) return -EINVAL;
return -EINVAL;if (ioctl == VIDIOC_G_EXT_CTRLS) return uvc_ctrl_is_readable(ctrls->which, ctrl, mapping);diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 9e4a251eca88085a1b4e0e854370015855be92ee..d5274dc94da3c60f1f4566b307dd445f30c4f45f 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -765,6 +765,7 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *priv,
static int uvc_ctrl_check_access(struct uvc_video_chain *chain, struct v4l2_ext_controls *ctrls,
u32 which,This fits on the previous line.
unsigned long ioctl){ struct v4l2_ext_control *ctrl = ctrls->controls; @@ -772,7 +773,8 @@ static int uvc_ctrl_check_access(struct uvc_video_chain *chain, int ret = 0;
for (i = 0; i < ctrls->count; ++ctrl, ++i) {
ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls, ioctl);
ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls, which,ioctl); if (ret) break; }@@ -806,7 +808,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *priv, which = V4L2_CTRL_WHICH_CUR_VAL; }
ret = uvc_ctrl_check_access(chain, ctrls, VIDIOC_G_EXT_CTRLS);
ret = uvc_ctrl_check_access(chain, ctrls, which, VIDIOC_G_EXT_CTRLS); if (ret < 0) return ret;@@ -840,7 +842,8 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle, if (!ctrls->count) return 0;
ret = uvc_ctrl_check_access(chain, ctrls, ioctl);
ret = uvc_ctrl_check_access(chain, ctrls, V4L2_CTRL_WHICH_CUR_VAL,ioctl); if (ret < 0) return ret;diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index ed7bad31f75ca474c1037d666d5310c78dd764df..d583425893a5f716185153a07aae9bfe20182964 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -786,7 +786,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, u32 which, struct v4l2_ext_control *xctrl); int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl); int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
const struct v4l2_ext_controls *ctrls,
const struct v4l2_ext_controls *ctrls, u32 which, unsigned long ioctl);int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
base-commit: c218ce4f98eccf5a40de64c559c52d61e9cc78ee change-id: 20251028-uvc-fix-which-c3ba1fb68ed5
-- Regards,
Laurent Pinchart
linux-stable-mirror@lists.linaro.org
-
Laurent Pinchart -
Ricardo Ribalda