Hi Hans,

On Tue, Jan 03, 2023 at 03:36:19PM +0100, Ricardo Ribalda wrote:
> From: Hans Verkuil <[email protected]>

There's a mismatch between the From and Signed-off-by tag. Which one
should I modify to match the other ?

> Check for inactive controls in uvc_ctrl_is_accessible().
>
> Use the new value for the master_id controls if present, otherwise
> use the existing value to determine if it is OK to set the control.
> Doing this here avoids attempting to set an inactive control, which
> will return an error from the USB device, which returns an invalid
> errorcode.
>
> This fixes:
> warn: v4l2-test-controls.cpp(483): s_ctrl returned EIO
> warn: v4l2-test-controls.cpp(483): s_ctrl returned EIO
> test VIDIOC_G/S_CTRL: OK
> warn: v4l2-test-controls.cpp(739): s_ext_ctrls returned EIO
> warn: v4l2-test-controls.cpp(739): s_ext_ctrls returned EIO
> warn: v4l2-test-controls.cpp(816): s_ext_ctrls returned EIO
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK
>
> Tested with:
> v4l2-ctl -c auto_exposure=1
> OK
> v4l2-ctl -c exposure_time_absolute=251
> OK
> v4l2-ctl -c auto_exposure=3
> OK
> v4l2-ctl -c exposure_time_absolute=251
> VIDIOC_S_EXT_CTRLS: failed: Input/output error
> exposure_time_absolute: Input/output error
> ERROR
> v4l2-ctl -c auto_exposure=3,exposure_time_absolute=251,auto_exposure=1
> v4l2-ctl -C auto_exposure,exposure_time_absolute
> auto_exposure: 1
> exposure_time_absolute: 251
>
> Reviewed-by: Laurent Pinchart <[email protected]>
> Reviewed-by: Ricardo Ribalda <[email protected]>
> Signed-off-by: Hans Verkuil <[email protected]>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 42 +++++++++++++++++++++++++++++++++++++++-
> drivers/media/usb/uvc/uvc_v4l2.c | 3 +--
> drivers/media/usb/uvc/uvcvideo.h | 3 ++-
> 3 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index c95a2229f4fa..6165d6b8e855 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1085,11 +1085,28 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
> return 0;
> }
>
> +/*
> + * Check if control @v4l2_id can be accessed by the given control @ioctl
> + * (VIDIOC_G_EXT_CTRLS, VIDIOC_TRY_EXT_CTRLS or VIDIOC_S_EXT_CTRLS).
> + *
> + * For set operations on slave controls, check if the master's value is set to
> + * manual, either in the others controls set in the same ioctl call, or from
> + * the master's current value. This catches VIDIOC_S_EXT_CTRLS calls that
> + * set both the master and slave control, such as for instance setting
> + * auto_exposure=1, exposure_time_absolute=251.
> + */
> int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> - bool read)
> + const struct v4l2_ext_controls *ctrls,
> + unsigned long ioctl)
> {
> + struct uvc_control_mapping *master_map = NULL;
> + struct uvc_control *master_ctrl = NULL;
> struct uvc_control_mapping *mapping;
> struct uvc_control *ctrl;
> + bool read = ioctl == VIDIOC_G_EXT_CTRLS;
> + s32 val;
> + int ret;
> + int i;
>
> if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
> return -EACCES;
> @@ -1104,6 +1121,29 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
> return -EACCES;
>
> + if (ioctl != VIDIOC_S_EXT_CTRLS || !mapping->master_id)
> + return 0;
> +
> + /*
> + * Iterate backwards in cases where the master control is accessed
> + * multiple times in the same ioctl. We want the last value.
> + */
> + for (i = ctrls->count - 1; i >= 0; i--) {
> + if (ctrls->controls[i].id == mapping->master_id)
> + return ctrls->controls[i].value ==
> + mapping->master_manual ? 0 : -EACCES;
> + }
> +
> + __uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
> + &master_ctrl, 0);
> +
> + if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
> + return 0;
> +
> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> + if (ret >= 0 && val != mapping->master_manual)
> + return -EACCES;
> +
> return 0;
> }
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index f4d4c33b6dfb..3edb54c086b2 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1020,8 +1020,7 @@ 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,
> - ioctl == VIDIOC_G_EXT_CTRLS);
> + ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls, ioctl);
> if (ret)
> break;
> }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..a151f583cd15 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -761,7 +761,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
> int uvc_ctrl_get(struct uvc_video_chain *chain, 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,
> - bool read);
> + const struct v4l2_ext_controls *ctrls,
> + unsigned long ioctl);
>
> int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> struct uvc_xu_control_query *xqry);
>

--
Regards,

Laurent Pinchart