Re: [PATCH 1/4] media: uvcvideo: Refactor uvc_ctrl_set_handle()

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

Hi Ricardo,

On Fri, May 09, 2025 at 06:24:13PM +0000, Ricardo Ribalda wrote:
> Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and
> clearing the handle. The only common code between the two cases is the
> lockdep_assert_held.
> 
> The code looks cleaner if we split these two usecases in two functions.

It does indeed. Thanks for pushing for this :-)

> We also take this opportunity to use pending_async_ctrls from ctrl where
> possible.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 65 +++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 44b6513c526421943bb9841fb53dc5f8e9f93f02..26be1d0a1891cf3a9a7489f60f9a2931c78d3239 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1812,48 +1812,53 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>  	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
>  }
>  
> -static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> -			       struct uvc_fh *new_handle)
> +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
>  {
> -	lockdep_assert_held(&handle->chain->ctrl_mutex);
> -
> -	if (new_handle) {
> -		int ret;
> +	int ret;
>  
> -		if (ctrl->handle)
> -			dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> -					     "UVC non compliance: Setting an async control with a pending operation.");
> +	lockdep_assert_held(&handle->chain->ctrl_mutex);
>  
> -		if (new_handle == ctrl->handle)
> -			return 0;
> +	if (ctrl->handle) {
> +		dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> +				     "UVC non compliance: Setting an async control with a pending operation.");
>  
> -		if (ctrl->handle) {
> -			WARN_ON(!ctrl->handle->pending_async_ctrls);
> -			if (ctrl->handle->pending_async_ctrls)
> -				ctrl->handle->pending_async_ctrls--;
> -			ctrl->handle = new_handle;
> -			handle->pending_async_ctrls++;
> +		if (ctrl->handle == handle)
>  			return 0;
> -		}
> -
> -		ret = uvc_pm_get(handle->chain->dev);
> -		if (ret)
> -			return ret;
>  
> -		ctrl->handle = new_handle;
> -		handle->pending_async_ctrls++;
> +		WARN_ON(!ctrl->handle->pending_async_ctrls);
> +		if (ctrl->handle->pending_async_ctrls)
> +			ctrl->handle->pending_async_ctrls--;
> +		ctrl->handle = handle;
> +		ctrl->handle->pending_async_ctrls++;
>  		return 0;
>  	}
>  
> +	ret = uvc_pm_get(handle->chain->dev);
> +	if (ret)
> +		return ret;
> +
> +	ctrl->handle = handle;
> +	ctrl->handle->pending_async_ctrls++;
> +	return 0;
> +}
> +
> +static int uvc_ctrl_clear_handle(struct uvc_fh *handle,
> +				 struct uvc_control *ctrl)
> +{
> +	lockdep_assert_held(&handle->chain->ctrl_mutex);
> +
>  	/* Cannot clear the handle for a control not owned by us.*/

While at it, I'll add a space before */ when applying.

>  	if (WARN_ON(ctrl->handle != handle))
>  		return -EINVAL;

But actually, as the caller guarantees that handle == ctrl->handle in
both call sites (renaming the function makes this clear), can we drop
the handle argument to this function ?

If that's fine with you, I'd like to also change the
uvc_ctrl_set_handle() function to pass the ctrl argument first.

>  
> -	ctrl->handle = NULL;
> -	if (WARN_ON(!handle->pending_async_ctrls))
> +	if (WARN_ON(!ctrl->handle->pending_async_ctrls)) {
> +		ctrl->handle = NULL;
>  		return -EINVAL;
> -	handle->pending_async_ctrls--;
> +	}
> +
> +	ctrl->handle->pending_async_ctrls--;
>  	uvc_pm_put(handle->chain->dev);

This will need to become

 	uvc_pm_put(ctrl->handle->chain->dev);

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

> +	ctrl->handle = NULL;
>  	return 0;
>  }
>  
> @@ -1871,7 +1876,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>  
>  	handle = ctrl->handle;
>  	if (handle)
> -		uvc_ctrl_set_handle(handle, ctrl, NULL);
> +		uvc_ctrl_clear_handle(handle, ctrl);
>  
>  	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
>  		s32 value;
> @@ -2161,7 +2166,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  
>  		if (!rollback && handle && !ret &&
>  		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> -			ret = uvc_ctrl_set_handle(handle, ctrl, handle);
> +			ret = uvc_ctrl_set_handle(handle, ctrl);
>  
>  		if (ret < 0 && !rollback) {
>  			if (err_ctrl)
> @@ -3271,7 +3276,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>  		for (unsigned int i = 0; i < entity->ncontrols; ++i) {
>  			if (entity->controls[i].handle != handle)
>  				continue;
> -			uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
> +			uvc_ctrl_clear_handle(handle, &entity->controls[i]);
>  		}
>  	}
>  

-- 
Regards,

Laurent Pinchart