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@xxxxxxxxxxxx>
> ---
> 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
