Re: [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS
Hi Hans,
On Fri, Mar 12, 2021 at 08:14:13AM +0100, Hans Verkuil wrote:
> On 11/03/2021 23:19, Ricardo Ribalda wrote:
> > According to the doc:
> > The, in hindsight quite poor, solution for that is to set error_idx to
> > count if the validation failed.
>
> I think this needs a bit more explanation. How about this:
>
> "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."
Here's what I commented on v1:
The [V4L2 spec] states:
"This check is done to avoid leaving the hardware in an inconsistent
state due to easy-to-avoid problems. But it leads to another problem:
the application needs to know whether an error came from the validation
step (meaning that the hardware was not touched) or from an error during
the actual reading from/writing to hardware."
I'm not sure this is correct though. -EACCES is returned by
__uvc_ctrl_get() when the control is found and is a write-only control.
The uvc_ctrl_get() calls for the previous controls will have potentially
touched the device to read the current control value if it wasn't cached
already, to this contradicts the rationale from the specification.
I understand the need for this when setting controls, but when reading
them, it's more puzzling, as the interactions with the hardware to read
the controls are not supposed to affect the hardware state in a way that
applications should care about. It may be an issue in the V4L2
specification.
> >
> > 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
>
> After improving the commit log you can add my:
>
> Reviewed-by: Hans Verkuil
>
> > ---
> > 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;
> > return ret;
> > }
> > }
--
Regards,
Laurent Pinchart
Re: [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS
On 11/03/2021 23:19, Ricardo Ribalda wrote:
> According to the doc:
> The, in hindsight quite poor, solution for that is to set error_idx to
> count if the validation failed.
I think this needs a bit more explanation. How about this:
"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
After improving the commit log you can add my:
Reviewed-by: Hans Verkuil
Thanks!
Hans
> ---
> 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;
> return ret;
> }
> }
>
Re: [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS
Hi Ricardo,
Thank you for the patch.
On Thu, Mar 11, 2021 at 11:19:44PM +0100, Ricardo Ribalda wrote:
> According to the doc:
> The, in hindsight quite poor, solution for that is to set error_idx to
> count if the validation failed.
>
> 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
I'll hold off on this one until we conclude the discussion with Hans on
v1.
> ---
> 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;
> return ret;
> }
> }
--
Regards,
Laurent Pinchart
[PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS
According to the doc:
The, in hindsight quite poor, solution for that is to set error_idx to
count if the validation failed.
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
---
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;
return ret;
}
}
--
2.31.0.rc2.261.g7f71774620-goog
