Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Dec 2022 22:54:52 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Ricardo Ribalda <ribalda@chromium.org>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Hans Verkuil <hans.verkuil@cisco.com>
Subject: Re: [PATCH RESEND v2 1/7] media: uvcvideo: uvc_ctrl_is_accessible:
 check for INACTIVE
Message-ID: <Y63+nF2PqKad8S18@pendragon.ideasonboard.com>
References: <20220920-resend-v4l2-compliance-v2-0-b0ceb15353ac@chromium.org>
 <20220920-resend-v4l2-compliance-v2-1-b0ceb15353ac@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220920-resend-v4l2-compliance-v2-1-b0ceb15353ac@chromium.org>
Precedence: bulk

Hi Ricardo and Hans,

Thank you for the patch.

On Fri, Dec 02, 2022 at 06:21:35PM +0100, Ricardo Ribalda wrote:
> From: Hans Verkuil <hverkuil@xs4all.nl>
> 
> Check for inactive controls in uvc_ctrl_is_accessible().
> Use the new value for the master_id controls if present,
> otherwise use the existing value to determine if it is OK
> to set the control. Doing this here avoids attempting to
> set an inactive control, which will return an error from the
> USB device, which returns an invalid errorcode.

I'd write the subject line as

media: uvcvideo: Check for INACTIVE in uvc_ctrl_is_accessible()

and reflow the commit message to 72 columns here.

> This fixes:
>   warn: v4l2-test-controls.cpp(483): s_ctrl returned EIO
>   warn: v4l2-test-controls.cpp(483): s_ctrl returned EIO
> test VIDIOC_G/S_CTRL: OK
>   warn: v4l2-test-controls.cpp(739): s_ext_ctrls returned EIO
>   warn: v4l2-test-controls.cpp(739): s_ext_ctrls returned EIO
>   warn: v4l2-test-controls.cpp(816): s_ext_ctrls returned EIO
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> 
> Tested with:
> v4l2-ctl -c auto_exposure=1
> OK
> v4l2-ctl -c exposure_time_absolute=251
> OK
> v4l2-ctl -c auto_exposure=3
> OK
> v4l2-ctl -c exposure_time_absolute=251
> VIDIOC_S_EXT_CTRLS: failed: Input/output error
> exposure_time_absolute: Input/output error
> ERROR
> v4l2-ctl -c auto_exposure=3,exposure_time_absolute=251,auto_exposure=1
> v4l2-ctl -C auto_exposure,exposure_time_absolute  
> auto_exposure: 1
> exposure_time_absolute: 251
> 
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 47 +++++++++++++++++++++++++++++++++++++++-
>  drivers/media/usb/uvc/uvc_v4l2.c |  4 ++--
>  drivers/media/usb/uvc/uvcvideo.h |  3 ++-
>  3 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 8c208db9600b..7153ee5aabb1 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1064,11 +1064,33 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>  	return 0;
>  }
>  
> +/**
> + * uvc_ctrl_is_accessible() - Check if a control can be read/writen/tried.
> + * @chain: uvc_video_chain that the controls belong to.
> + * @v4l2_id: video4linux id of the control.
> + * @ctrl: Other controls that will be accessed in the ioctl.
> + * @ioctl: ioctl used to access the control.

The driver doesn't use kerneldoc anywhere, so to match the current style
I'd use /* instead of /**, and drop the documentation of the function
arguments as it's trivial.

> + *
> + * Check if a control can be accessed by a specicific ioctl operation,

s/specicific/specific/

> + * assuming that other controls are also going to be accessed by that ioctl.
> + * We need to check the value of the other controls, to support operations
> + * where a master value is changed with a slave value. Eg.
> + * auto_exposure=1,exposure_time_absolute=251
> + *

Extra blank line.

I'd rephrase that a bit to make it more precise:

 * Check if control @v4l2_id can be accessed by the given control @ioctl
 * (VIDIOC_G_EXT_CTRLS, VIDIOC_TRY_EXT_CTRLS or VIDIOC_S_EXT_CTRLS).
 *
 * For set operations on slave controls, check if the master's value is set to
 * manual, either in the others controls set in the same ioctl call, or from the
 * master's current value. This catches VIDIOC_S_EXT_CTRLS calls that
 * set both the master and slave control, such as for instance setting
 * auto_exposure=1,exposure_time_absolute=251.

> + */
>  int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> -			   bool read)
> +			   const struct v4l2_ext_controls *ctrls,
> +			   unsigned long ioctl)
>  {
> +	struct uvc_control_mapping *master_map = NULL;
> +	struct uvc_control *master_ctrl = NULL;
>  	struct uvc_control_mapping *mapping;
>  	struct uvc_control *ctrl;
> +	bool read = ioctl == VIDIOC_G_EXT_CTRLS;
> +	bool try = ioctl == VIDIOC_TRY_EXT_CTRLS;
> +	s32 val;
> +	int ret;
> +	int i;
>  
>  	if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
>  		return -EACCES;
> @@ -1083,6 +1105,29 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
>  	if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
>  		return -EACCES;
>  
> +	if (read || try || !mapping->master_id)

I'd write this

	if (ioctl != VIDIOC_S_EXT_CTRLS || !mapping->master_id)

and drop the try variable.

> +		return 0;
> +
> +	/*
> +	 * Iterate backwards in cases where the master control is accessed
> +	 * multiple times in the same ioctl. We want the last value.
> +	 */
> +	for (i = ctrls->count - 1; i >= 0; i--) {
> +		if (ctrls->controls[i].id == mapping->master_id)
> +			return ctrls->controls[i].value ==
> +					mapping->master_manual ? 0 : -EACCES;
> +	}
> +
> +	__uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
> +			   &master_ctrl, 0);
> +
> +	if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
> +		return 0;
> +
> +	ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> +	if (ret >= 0 && val != mapping->master_manual)
> +		return -EACCES;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 4cc3fa6b8c98..d95168cdc2d1 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1020,8 +1020,8 @@ static int uvc_ctrl_check_access(struct uvc_video_chain *chain,
>  	int ret = 0;
>  
>  	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> -		ret = uvc_ctrl_is_accessible(chain, ctrl->id,
> -					    ioctl == VIDIOC_G_EXT_CTRLS);
> +		ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls,
> +					    ioctl);

This holds on a single line.

Conditionally-Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I can address all the review comments when applying.

>  		if (ret)
>  			break;
>  	}
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 24c911aeebce..644d5fcf2eef 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -905,7 +905,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
>  int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
>  int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
>  int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> -			   bool read);
> +			   const struct v4l2_ext_controls *ctrls,
> +			   unsigned long ioctl);
>  
>  int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>  		      struct uvc_xu_control_query *xqry);
> 

-- 
Regards,

Laurent Pinchart