| Message ID | 20210315173609.1547857-5-ribalda@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | uvcvideo: Fix v4l2-compliance errors | expand |
On 15/03/2021 18:36, Ricardo Ribalda wrote: > 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. > > It would have been much nicer of course if error_idx could point to the > control index that failed the validation, but sadly that's not how the > API was designed. > > Fixes v4l2-compliance: > Control ioctls (Input 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> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 157310c0ca87..36eb48622d48 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -1073,7 +1073,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, > ret = uvc_ctrl_get(chain, ctrl); > if (ret < 0) { > uvc_ctrl_rollback(handle); > - ctrls->error_idx = i; > + ctrls->error_idx = (ret == -EACCES) ? > + ctrls->count : i; This isn't right. For G_EXT_CTRLS error_idx should be set to either ctrls->count or the index of the failing control depending on whether the hardware has been touched or not. In the case of the 'if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {' the hardware has not been touched, so there is should set error_idx to ctrls->count. In the case where we obtain the actual values with uvc_ctrl_get() it must return the index of the failing control. According to the spec if VIDIOC_G_EXT_CTRLS returns an error and error_idx is equal to the total count, then no hardware was touched, so it was during the validation phase that some error was detected. If it returns an error and error_idx < count, then the hardware was accessed and the contents of the controls at indices >= error_idx is undefined. So setting error_idx to i is the right thing to do, regardless of the EACCES test. This does create a problem in v4l2-compliance, since it assumes that the control framework is used, and that framework validates the control first in a separate step before accessing the hardware. That's missing in uvc. I think v4l2-compliance should be adjusted for uvcvideo since uvc isn't doing anything illegal by returning i here since it really accessed hardware. An alternative would be to introduce an initial validation phase in uvc_ioctl_g_ext_ctrls as well, but I'm not sure that it worth the effort. It's quite difficult to get it really right. Relaxing the tests in v4l2-compliance for uvc is a better approach IMHO. Or rewrite uvc to use the control framework :-) :-) Regards, Hans > return ret; > } > } >
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 157310c0ca87..36eb48622d48 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -1073,7 +1073,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, ret = uvc_ctrl_get(chain, ctrl); if (ret < 0) { uvc_ctrl_rollback(handle); - ctrls->error_idx = i; + ctrls->error_idx = (ret == -EACCES) ? + ctrls->count : i; return ret; } }
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. It would have been much nicer of course if error_idx could point to the control index that failed the validation, but sadly that's not how the API was designed. Fixes v4l2-compliance: Control ioctls (Input 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> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)