Re: [PATCH v14 03/11] media: uvcvideo: introduce __uvc_ctrl_get_std()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Yunke,

Thank you for the patch.

On Fri, Dec 01, 2023 at 04:18:54PM +0900, Yunke Cao wrote:
> Refactor uvc_ctrl to make adding compound control easier.
> 
> Currently __uvc_ctrl_get() only work for non-compound controls.
> Move the logic into __uvc_ctrl_std(), return error for compound
> controls.
> 
> Make __uvc_ctrl_get() call __uvc_ctrl_std() inside. Also modify some of the
> callers of __uvc_ctrl_get() to call __uvc_ctrl_get_std() instead.
> 
> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> Changelog since v12:
> - No change.
> Changelog since v11:
> - Minor change to avoid negative if statement.
> Changelog since v10:
> - Better commit message.
> Changelog since v9:
> - No change.
> Changelog since v8:
> - No change.
> Changelog since v7:
> - Newly added patch. Split the refactoring of uvc_ctrl_get from v7 3/7.
> 
>  drivers/media/usb/uvc/uvc_ctrl.c | 42 +++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 8d8333786333..4a685532f7eb 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1081,15 +1081,15 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
>  	return ret;
>  }
>  
> -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> -			  struct uvc_control *ctrl,
> -			  struct uvc_control_mapping *mapping,
> -			  s32 *value)
> +static int __uvc_ctrl_get_std(struct uvc_video_chain *chain,
> +			      struct uvc_control *ctrl,
> +			      struct uvc_control_mapping *mapping,
> +			      s32 *value)
>  {
>  	int ret;
>  
> -	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> -		return -EACCES;
> +	if (uvc_ctrl_mapping_is_compound(mapping))
> +		return -EINVAL;

This duplicates the check that you have added in multiple callers. With
the whole series applied, this function is called without a compound
check from uvc_ctrl_is_accessible() only. I think it would be better to
explicitly handle compound controls there. Even better possible, you
could check at driver initialization time that V4L2 master controls are
never compound controls.

>  
>  	ret = __uvc_ctrl_load_cur(chain, ctrl);
>  	if (ret < 0)
> @@ -1199,7 +1199,7 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
>  	if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
>  		return 0;
>  
> -	ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> +	ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val);
>  	if (ret >= 0 && val != mapping->master_manual)
>  		return -EACCES;
>  
> @@ -1264,8 +1264,13 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  		__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)) {
> -		s32 val;
> -		int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> +		s32 val = 0;
> +		int ret;
> +
> +		if (uvc_ctrl_mapping_is_compound(master_map))
> +			return -EINVAL;
> +
> +		ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -1532,7 +1537,8 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>  	if (ctrl == NULL)
>  		return;
>  
> -	if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)
> +	if (uvc_ctrl_mapping_is_compound(mapping) ||
> +	    __uvc_ctrl_get_std(chain, ctrl, mapping, &val) == 0)
>  		changes |= V4L2_EVENT_CTRL_CH_VALUE;
>  
>  	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> @@ -1703,7 +1709,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
>  		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
>  		s32 val = 0;
>  
> -		if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
> +		if (uvc_ctrl_mapping_is_compound(mapping) ||
> +		    __uvc_ctrl_get_std(handle->chain, ctrl, mapping, &val) == 0)
>  			changes |= V4L2_EVENT_CTRL_CH_VALUE;
>  
>  		uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val,
> @@ -1883,7 +1890,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
>  	if (ctrl == NULL)
>  		return -EINVAL;
>  
> -	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
> +	if (uvc_ctrl_mapping_is_compound(mapping))
> +		return -EINVAL;
> +	else
> +		return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value);
>  }
>  
>  static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
> @@ -2042,8 +2052,12 @@ int uvc_ctrl_set(struct uvc_fh *handle,
>  		       ctrl->info.size);
>  	}
>  
> -	mapping->set(mapping, value,
> -		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> +	if (uvc_ctrl_mapping_is_compound(mapping))
> +		return -EINVAL;
> +	else
> +		mapping->set(mapping, value,
> +			     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));

This seems unrelated to the commit message.

> +
>  
>  	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
>  		ctrl->handle = handle;

-- 
Regards,

Laurent Pinchart