Re: [PATCH v9 19/22] uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE

From: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
To: Ricardo Ribalda <ribalda@chromium.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	linux-media <linux-media@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Tomasz Figa <tfiga@chromium.org>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Hans Verkuil <hans.verkuil@cisco.com>
Subject: Re: [PATCH v9 19/22] uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
Date: Sat, 27 Mar 2021 13:26:23 +0100	[thread overview]
Message-ID: <CAPybu_1S0mLJQPhuz8dZfvVeybqsST0RBsXJKGi+FoFWivCQpw@mail.gmail.com> (raw)
In-Reply-To: <20210326095840.364424-20-ribalda@chromium.org>

Hello Hans

On Fri, Mar 26, 2021 at 11:01 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> From: Hans Verkuil <hverkuil@xs4all.nl>
>
> 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.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 28 +++++++++++++++++++++++++++-
>  drivers/media/usb/uvc/uvc_v4l2.c |  4 ++--
>  drivers/media/usb/uvc/uvcvideo.h |  3 ++-
>  3 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index d9d4add1e813..6e7b904bc33d 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1047,10 +1047,18 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>  }
>
>  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;
> +       bool try = ioctl == VIDIOC_TRY_EXT_CTRLS;
> +       s32 val;
> +       int ret;
> +       int i;
>
>         if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
>                 return -EACCES;
> @@ -1065,6 +1073,24 @@ 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 (read || try || !mapping->master_id)
> +               return 0;
> +
> +       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;

Maybe we could use  uvc_ctrl_is_inactive().

Please checkout:
https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/commit/?h=uvc-compliance-v10&id=f49f7c84ece18df72094a18011f2fbeb20dc1b15

and if you like it better that will be what I resend for v10 ;)

Thanks!

> +
>         return 0;
>  }
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 8d8b12a4db34..0f4d893eff46 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1000,8 +1000,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,
> -                                           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 0313b30f0cea..20bc681315fb 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -901,7 +901,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);
> --
> 2.31.0.291.g576ba9dcdaf-goog
>

--
Ricardo Ribalda