| Message ID | 20210316180004.1605727-5-ribalda@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | uvcvideo: Fix v4l2-compliance errors | expand |
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@chromium.org> > --- > 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); > } > >
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); + if (ret < 0) + break; + if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) { ctrl->value = qc.default_value; + 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; +} + +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); }
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@chromium.org> --- drivers/media/usb/uvc/uvc_v4l2.c | 69 +++++++++++++++++++++++--------- 1 file changed, 51 insertions(+), 18 deletions(-)