On 16/03/2021 18:59, Ricardo Ribalda wrote:
> We can figure out if reading/writing a set of controls can fail without
> accessing them by checking their flags.
>
> This way we can honor the API closer:
>
> 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.
>
> Fixes v4l2-compliance:
> Control ioctls (Input 0):
> warn: v4l2-test-controls.cpp(765): g_ext_ctrls(0) invalid error_idx 0
> fail: v4l2-test-controls.cpp(645): invalid error index write only control
> test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> drivers/media/usb/uvc/uvc_v4l2.c | 69 +++++++++++++++++++++++---------
> 1 file changed, 51 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 157310c0ca87..e956d833ed84 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1040,31 +1040,54 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
> return 0;
> }
>
> -static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
> - struct v4l2_ext_controls *ctrls)
> +static int uvc_ctrl_check_access(struct uvc_video_chain *chain,
> + struct v4l2_ext_controls *ctrls, bool read)
> {
> - struct uvc_fh *handle = fh;
> - struct uvc_video_chain *chain = handle->chain;
> struct v4l2_ext_control *ctrl = ctrls->controls;
> unsigned int i;
> - int ret;
> + int ret = 0;
>
> - if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
> - for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> - struct v4l2_queryctrl qc = { .id = ctrl->id };
> + for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> + struct v4l2_queryctrl qc = { .id = ctrl->id };
>
> - ret = uvc_query_v4l2_ctrl(chain, &qc);
> - if (ret < 0) {
> - ctrls->error_idx = i;
> - return ret;
> - }
> + ret = uvc_query_v4l2_ctrl(chain, &qc);
You can't call this. If I am not mistaken, this call can actually call
the hardware.
Instead you need to call the lower level function uvc_find_control() and
use ctrl->info to check for read/write only.
> + if (ret < 0)
> + break;
>
> + if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
> ctrl->value = qc.default_value;
This needs to use the old code in uvc_ioctl_g_ext_ctrls() since it depends
on uvc_query_v4l2_ctrl() which accesses the hardware.
> + continue;
> }
>
> - return 0;
> + if (qc.flags & V4L2_CTRL_FLAG_WRITE_ONLY && read) {
> + ret = -EACCES;
> + break;
> + }
> +
> + if (qc.flags & V4L2_CTRL_FLAG_READ_ONLY && !read) {
> + ret = -EACCES;
> + break;
> + }
> }
>
> + ctrls->error_idx = ctrls->count;
> +
> + return ret;
> +}
So uvc_ctrl_check_access() is a good idea, but it does a bit too much.
It should just check if all controls in the list are known and check for
write/read only.
Regards,
Hans
> +
> +static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
> + struct v4l2_ext_controls *ctrls)
> +{
> + struct uvc_fh *handle = fh;
> + struct uvc_video_chain *chain = handle->chain;
> + struct v4l2_ext_control *ctrl = ctrls->controls;
> + unsigned int i;
> + int ret;
> +
> + ret = uvc_ctrl_check_access(chain, ctrls, true);
> + if (ret < 0 || ctrls->which == V4L2_CTRL_WHICH_DEF_VAL)
> + return ret;
> +
> ret = uvc_ctrl_begin(chain);
> if (ret < 0)
> return ret;
> @@ -1092,10 +1115,6 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
> unsigned int i;
> int ret;
>
> - /* Default value cannot be changed */
> - if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL)
> - return -EINVAL;
> -
> ret = uvc_ctrl_begin(chain);
> if (ret < 0)
> return ret;
> @@ -1121,6 +1140,16 @@ static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh,
> struct v4l2_ext_controls *ctrls)
> {
> struct uvc_fh *handle = fh;
> + struct uvc_video_chain *chain = handle->chain;
> + int ret;
> +
> + /* Default value cannot be changed */
> + if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL)
> + return -EINVAL;
> +
> + ret = uvc_ctrl_check_access(chain, ctrls, false);
> + if (ret < 0)
> + return ret;
>
> return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, true);
> }
> @@ -1130,6 +1159,10 @@ static int uvc_ioctl_try_ext_ctrls(struct file *file, void *fh,
> {
> struct uvc_fh *handle = fh;
>
> + /* Default value cannot be changed */
> + if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL)
> + return -EINVAL;
> +
> return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, false);
> }
>
>
