[v6,3/5] media: uvcvideo: Increase/decrease the PM counter per IOCTL

Now we call uvc_pm_get/put from the device open/close. This low
level of granularity might leave the camera powered on in situations
where it is not needed.

Increase the granularity by increasing and decreasing the Power
Management counter per ioctl. There are two special cases where the
power management outlives the ioctl: async controls and streamon. Handle
those cases as well.

In a future patch, we will remove the uvc_pm_get/put from open/close.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 37 +++++++++++++++++++++++++++----------
 drivers/media/usb/uvc/uvc_v4l2.c | 39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 64 insertions(+), 12 deletions(-)

Hi Ricardo,

Thank you for the patch.

On Thu, Mar 27, 2025 at 09:05:29PM +0000, Ricardo Ribalda wrote:
> Now we call uvc_pm_get/put from the device open/close. This low
> level of granularity might leave the camera powered on in situations
> where it is not needed.
> 
> Increase the granularity by increasing and decreasing the Power
> Management counter per ioctl. There are two special cases where the
> power management outlives the ioctl: async controls and streamon. Handle
> those cases as well.
> 
> In a future patch, we will remove the uvc_pm_get/put from open/close.
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 37 +++++++++++++++++++++++++++----------
>  drivers/media/usb/uvc/uvc_v4l2.c | 39 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 64 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index cbf19aa1d82374a08cf79b6a6787fa348b83523a..3fad289e41fd5a757f8dcf30a6238c694fc4250c 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1812,38 +1812,49 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>  	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
>  }
>  
> -static void 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,
> +			       struct uvc_fh *new_handle)
>  {
>  	lockdep_assert_held(&handle->chain->ctrl_mutex);
>  
>  	if (new_handle) {
> +		int ret;
> +
>  		if (ctrl->handle)
>  			dev_warn_ratelimited(&handle->stream->dev->udev->dev,
>  					     "UVC non compliance: Setting an async control with a pending operation.");
>  
>  		if (new_handle == ctrl->handle)
> -			return;
> +			return 0;
>  
>  		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++;

As commented previously, your usage of the handle variable is confusing.

			ctrl->handle->pending_async_ctrls++;

> +			return 0;
>  		}
>  
> +		ret = uvc_pm_get(handle->chain->dev);
> +		if (ret)
> +			return ret;
> +
>  		ctrl->handle = new_handle;
>  		handle->pending_async_ctrls++;

  		ctrl->handle->pending_async_ctrls++;

> -		return;
> +		return 0;
>  	}
>  
>  	/* Cannot clear the handle for a control not owned by us.*/
>  	if (WARN_ON(ctrl->handle != handle))
> -		return;
> +		return -EINVAL;
>  
>  	ctrl->handle = NULL;
>  	if (WARN_ON(!handle->pending_async_ctrls))
> -		return;
> +		return -EINVAL;
>  	handle->pending_async_ctrls--;
> +	uvc_pm_put(handle->chain->dev);
> +	return 0;
>  }
>  
>  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> @@ -2137,15 +2148,16 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  
>  		ctrl->dirty = 0;
>  
> +		if (!rollback && handle && !ret &&
> +		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> +			ret = uvc_ctrl_set_handle(handle, ctrl, handle);
> +
>  		if (ret < 0) {
>  			if (err_ctrl)
>  				*err_ctrl = ctrl;
>  			return ret;
>  		}
>  
> -		if (!rollback && handle &&
> -		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> -			uvc_ctrl_set_handle(handle, ctrl, handle);
>  	}
>  
>  	return 0;
> @@ -3222,6 +3234,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>  {
>  	struct uvc_entity *entity;
> +	int i;
>  
>  	guard(mutex)(&handle->chain->ctrl_mutex);
>  
> @@ -3236,7 +3249,11 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>  		}
>  	}
>  
> -	WARN_ON(handle->pending_async_ctrls);
> +	if (!WARN_ON(handle->pending_async_ctrls))
> +		return;
> +
> +	for (i = 0; i < handle->pending_async_ctrls; i++)
> +		uvc_pm_put(handle->stream->dev);
>  }
>  
>  /*
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 1d5be045d04ecbf17e65e14b390e494a294b735f..8bccf7e17528b62f2594c0dad99405034532973d 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -697,6 +697,9 @@ static int uvc_v4l2_release(struct file *file)
>  	if (uvc_has_privileges(handle))
>  		uvc_queue_release(&stream->queue);
>  
> +	if (handle->is_streaming)
> +		uvc_pm_put(stream->dev);
> +
>  	/* Release the file handle. */
>  	uvc_dismiss_privileges(handle);
>  	v4l2_fh_del(&handle->vfh);
> @@ -862,6 +865,11 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  	if (ret)
>  		return ret;
>  
> +	ret = uvc_pm_get(stream->dev);

Shouldn't this be done before calling uvc_queue_streamon() ? There's
another PM reference being held by the ioctl handler, but if the code is
refactored later, it would be good to make sure we resume the device
before starting streaming.

> +	if (ret) {
> +		uvc_queue_streamoff(&stream->queue, type);
> +		return ret;
> +	}
>  	handle->is_streaming = true;
>  
>  	return 0;
> @@ -879,7 +887,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
>  	guard(mutex)(&stream->mutex);
>  
>  	uvc_queue_streamoff(&stream->queue, type);
> -	handle->is_streaming = false;
> +	if (handle->is_streaming) {
> +		handle->is_streaming = false;
> +		uvc_pm_put(stream->dev);
> +	}
>  
>  	return 0;
>  }
> @@ -1378,9 +1389,11 @@ static int uvc_v4l2_put_xu_query(const struct uvc_xu_control_query *kp,
>  #define UVCIOC_CTRL_MAP32	_IOWR('u', 0x20, struct uvc_xu_control_mapping32)
>  #define UVCIOC_CTRL_QUERY32	_IOWR('u', 0x21, struct uvc_xu_control_query32)
>  
> +DEFINE_FREE(uvc_pm_put, struct uvc_device *, if (_T) uvc_pm_put(_T))
>  static long uvc_v4l2_compat_ioctl32(struct file *file,
>  		     unsigned int cmd, unsigned long arg)
>  {
> +	struct uvc_device *uvc_device __free(uvc_pm_put) = NULL;
>  	struct uvc_fh *handle = file->private_data;
>  	union {
>  		struct uvc_xu_control_mapping xmap;
> @@ -1389,6 +1402,12 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  	void __user *up = compat_ptr(arg);
>  	long ret;
>  
> +	ret = uvc_pm_get(handle->stream->dev);
> +	if (ret)
> +		return ret;
> +
> +	uvc_device = handle->stream->dev;

Ouch... That's not nice very :-/

If you want to use the cleanup API, I think we could use guards with an
init function such as

struct uvc_device *__uvc_pm_get_init(struct uvc_device *dev, int *ret)
{
	*ret = uvc_pm_get(dev);
	return *ret ? NULL : dev;
}

You can use DEFINE_CLASS() instead of DEFINE_GUARD() to control the
arguments to the init function. Users of the guard could do

	int ret;

	guard(uvc_pm)(dev, &ret);
	if (ret)
		return ret;

	...

Another, simpler option would be to replace returns with breaks in
uvc_v4l2_compat_ioctl32(). I'm tempted to do that in this patch, and
switching to the cleanup API as a patch on top if desired.

> +
>  	switch (cmd) {
>  	case UVCIOC_CTRL_MAP32:
>  		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> @@ -1423,6 +1442,22 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  }
>  #endif
>  
> +static long uvc_v4l2_unlocked_ioctl(struct file *file,
> +				    unsigned int cmd, unsigned long arg)
> +{
> +	struct uvc_fh *handle = file->private_data;
> +	int ret;
> +
> +	ret = uvc_pm_get(handle->stream->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = video_ioctl2(file, cmd, arg);
> +
> +	uvc_pm_put(handle->stream->dev);
> +	return ret;
> +}
> +
>  static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
>  		    size_t count, loff_t *ppos)
>  {
> @@ -1507,7 +1542,7 @@ const struct v4l2_file_operations uvc_fops = {
>  	.owner		= THIS_MODULE,
>  	.open		= uvc_v4l2_open,
>  	.release	= uvc_v4l2_release,
> -	.unlocked_ioctl	= video_ioctl2,
> +	.unlocked_ioctl	= uvc_v4l2_unlocked_ioctl,
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
>  #endif