From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: Ricardo Ribalda <ribalda@chromium.org>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 04/11] media: uvcvideo: set error_idx to count on EACCESS
Date: Tue, 16 Mar 2021 12:20:59 +0100 [thread overview]
Message-ID: <a9e7df54-3b2e-0f95-1975-25c424eb6735@xs4all.nl> (raw)
In-Reply-To: <20210315173609.1547857-5-ribalda@chromium.org>
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;
> }
> }
>
next prev parent reply other threads:[~2021-03-16 11:21 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-15 17:35 [PATCH v4 00/11] uvcvideo: Fix v4l2-compliance errors Ricardo Ribalda
2021-03-15 17:35 ` [PATCH v4 01/11] media: v4l2-ioctl: Fix check_ext_ctrls Ricardo Ribalda
2021-03-15 17:36 ` [PATCH v4 02/11] media: uvcvideo: Set capability in s_param Ricardo Ribalda
2021-03-15 17:36 ` [PATCH v4 03/11] media: uvcvideo: Return -EIO for control errors Ricardo Ribalda
2021-03-15 17:36 ` [PATCH v4 04/11] media: uvcvideo: set error_idx to count on EACCESS Ricardo Ribalda
2021-03-16 11:20 ` Hans Verkuil [this message]
2021-03-15 17:36 ` [PATCH v4 05/11] media: uvcvideo: refactor __uvc_ctrl_add_mapping Ricardo Ribalda
2021-03-15 17:36 ` [PATCH v4 06/11] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS Ricardo Ribalda
2021-03-16 8:37 ` Hans Verkuil
2021-03-16 10:08 ` Laurent Pinchart
2021-03-16 10:12 ` Ricardo Ribalda
2021-03-16 11:04 ` Laurent Pinchart
2021-03-15 17:36 ` [PATCH v4 07/11] media: uvcvideo: Use dev->name for querycap() Ricardo Ribalda
2021-03-15 17:36 ` [PATCH v4 08/11] media: uvcvideo: Set unique vdev name based in type Ricardo Ribalda
2021-03-16 10:10 ` Hans Verkuil
2021-03-15 17:36 ` [PATCH v4 09/11] media: uvcvideo: Increase the size of UVC_METADATA_BUF_SIZE Ricardo Ribalda
2021-03-16 9:45 ` Hans Verkuil
2021-03-15 17:36 ` [PATCH v4 10/11] media: uvcvideo: Return -EACCES to inactive controls Ricardo Ribalda
2021-03-15 17:36 ` [PATCH v4 11/11] uvc: use vb2 ioctl and fop helpers Ricardo Ribalda
2021-03-16 11:29 ` Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a9e7df54-3b2e-0f95-1975-25c424eb6735@xs4all.nl \
--to=hverkuil-cisco@xs4all.nl \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=ribalda@chromium.org \
--cc=sergey.senozhatsky@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.