Re: [PATCH v3 2/6] media: uvcvideo: Move usb_autopm_(get|put)_interface to status_get

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

Hi Ricardo,

Thank you for the patch.

On Thu, Feb 06, 2025 at 07:47:01PM +0000, Ricardo Ribalda wrote:
> Right now PM operations are always called at the same locations as
> uvc_status_(get|put).
> 
> Combine them into uvc_status_(get|put). This simplifies the current
> code and future PM changes in the driver.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_status.c | 38 +++++++++++++++++++++++++++++++++-----
>  drivers/media/usb/uvc/uvc_v4l2.c   | 11 +----------
>  2 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index ee01dce4b783..caa673b0279d 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -382,7 +382,7 @@ void uvc_status_suspend(struct uvc_device *dev)
>  		uvc_status_stop(dev);
>  }
>  
> -int uvc_status_get(struct uvc_device *dev)
> +static int _uvc_status_get(struct uvc_device *dev)

s/_uvc_status_get/__uvc_status_get/

>  {
>  	int ret;
>  
> @@ -399,13 +399,41 @@ int uvc_status_get(struct uvc_device *dev)
>  	return 0;
>  }
>  
> -void uvc_status_put(struct uvc_device *dev)
> +int uvc_status_get(struct uvc_device *dev)
> +{
> +	int ret;
> +
> +	ret = usb_autopm_get_interface(dev->intf);
> +	if (ret)
> +		return ret;
> +
> +	ret = _uvc_status_get(dev);
> +
> +	if (ret)
> +		usb_autopm_put_interface(dev->intf);
> +
> +	return ret;
> +}
> +
> +static int _uvc_status_put(struct uvc_device *dev)

s/_uvc_status_put/__uvc_status_put/

But unless you need to call this function in subsequent patches in the
series, I would merge it with uvc_status_put(). I think the same could
be done for get() too.

>  {
>  	guard(mutex)(&dev->status_lock);
>  
>  	if (dev->status_users == 1)
>  		uvc_status_stop(dev);
> -	WARN_ON(!dev->status_users);
> -	if (dev->status_users)
> -		dev->status_users--;
> +
> +	if (WARN_ON(!dev->status_users))
> +		return -EIO;

That's a change in behaviour that should be at least explained in the
commit message.

> +
> +	dev->status_users--;
> +	return 0;
> +}
> +
> +void uvc_status_put(struct uvc_device *dev)
> +{
> +	int ret;
> +
> +	ret = _uvc_status_put(dev);
> +	if (!ret)
> +		usb_autopm_put_interface(dev->intf);
>  }
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 856eaa23e703..5d4e967938af 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -636,20 +636,13 @@ static int uvc_v4l2_open(struct file *file)
>  	stream = video_drvdata(file);
>  	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
>  
> -	ret = usb_autopm_get_interface(stream->dev->intf);
> -	if (ret < 0)
> -		return ret;
> -
>  	/* Create the device handle. */
>  	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> -	if (handle == NULL) {
> -		usb_autopm_put_interface(stream->dev->intf);
> +	if (!handle)
>  		return -ENOMEM;
> -	}
>  
>  	ret = uvc_status_get(stream->dev);
>  	if (ret) {
> -		usb_autopm_put_interface(stream->dev->intf);
>  		kfree(handle);
>  		return ret;
>  	}
> @@ -685,8 +678,6 @@ static int uvc_v4l2_release(struct file *file)
>  	file->private_data = NULL;
>  
>  	uvc_status_put(stream->dev);
> -
> -	usb_autopm_put_interface(stream->dev->intf);

This isn't right. The usb_autopm_get_interface() and
usb_autopm_put_interface() calls here are not mean to support UVC status
operation only. Sure, the patch doesn't introduce an issue as such, but
it bundles two things that are not related in a way that is confusing.

I expect that the code will improve in subsequent patches and the reason
will become clear, but at least the commit message here really needs to
explain why there's a temporary step backwards. Ideally the series
should be reorganized to avoid this.

>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart