Re: [PATCH 3/6 v5] uvcvideo: convert from using an atomic variable to a reference count

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

Hi Guennadi,

Thank you for the patch.

As commented on patch 1/6, please capitalize the first word after the prefix 
in the subject line (you're using the correct prefix this time ;-)). The 
comment holds for the other patches in this series.

On Friday 28 Jul 2017 14:33:22 Guennadi Liakhovetski wrote:
> When adding support for metadata nodes, we'll have to keep video
> devices registered until all metadata nodes are closed too. Since
> this has nothing to do with stream counting, replace the nstreams
> atomic variable with a reference counter.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 31 +++++++++++++++++--------------
>  drivers/media/usb/uvc/uvcvideo.h   |  2 +-
>  2 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 70842c5..cfe33bf 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1849,16 +1849,20 @@ static void uvc_delete(struct uvc_device *dev)
>  	kfree(dev);
>  }
> 
> +static void uvc_kref_release(struct kref *kref)
> +{
> +	struct uvc_device *dev = container_of(kref, struct uvc_device, ref);
> +
> +	uvc_delete(dev);

uvc_delete() is now called from kref_put() only, so I'd merge the two 
functions. I think I'd call the resulting function uvc_delete() to 
differentiate it from uvc_release(), but if you prefer uvc_kref_release() I'm 
OK with that too.

> +}
> +
>  static void uvc_release(struct video_device *vdev)
>  {
>  	struct uvc_streaming *stream = video_get_drvdata(vdev);
>  	struct uvc_device *dev = stream->dev;
> 
> -	/* Decrement the registered streams count and delete the device when
> it
> -	 * reaches zero.
> -	 */
> -	if (atomic_dec_and_test(&dev->nstreams))
> -		uvc_delete(dev);
> +	/* Decrement the refcount and delete the device when it reaches zero
> */

s/zero/zero./

Or you could remove the comment altogether, I don't think it adds much 
anymore.

> +	kref_put(&dev->ref, uvc_kref_release);
>  }
> 
>  /*
> @@ -1870,10 +1874,10 @@ static void uvc_unregister_video(struct uvc_device
> *dev)
> 
>  	/* Unregistering all video devices might result in uvc_delete() being
>  	 * called from inside the loop if there's no open file handle. To
> avoid
> -	 * that, increment the stream count before iterating over the streams
> -	 * and decrement it when done.
> +	 * that, increment the refcount before iterating over the streams and
> +	 * decrement it when done.
>  	 */
> -	atomic_inc(&dev->nstreams);
> +	kref_get(&dev->ref);
> 
>  	list_for_each_entry(stream, &dev->streams, list) {
>  		if (!video_is_registered(&stream->vdev))
> @@ -1884,11 +1888,10 @@ static void uvc_unregister_video(struct uvc_device
> *dev) uvc_debugfs_cleanup_stream(stream);
>  	}
> 
> -	/* Decrement the stream count and call uvc_delete explicitly if there
> -	 * are no stream left.
> +	/* Decrement the refcount and call uvc_delete explicitly if there are 
> no
> +	 * stream left.
>  	 */

Similarly we could drop this comment. Otherwise you should update it to not 
mention streams anymore.

The rest of the patch looks good. With those small issues fixed,

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

> -	if (atomic_dec_and_test(&dev->nstreams))
> -		uvc_delete(dev);
> +	kref_put(&dev->ref, uvc_kref_release);
>  }
> 
>  static int uvc_register_video(struct uvc_device *dev,
> @@ -1946,7 +1949,7 @@ static int uvc_register_video(struct uvc_device *dev,
>  	else
>  		stream->chain->caps |= V4L2_CAP_VIDEO_OUTPUT;
> 
> -	atomic_inc(&dev->nstreams);
> +	kref_get(&dev->ref);
>  	return 0;
>  }
> 
> @@ -2031,7 +2034,7 @@ static int uvc_probe(struct usb_interface *intf,
>  	INIT_LIST_HEAD(&dev->entities);
>  	INIT_LIST_HEAD(&dev->chains);
>  	INIT_LIST_HEAD(&dev->streams);
> -	atomic_set(&dev->nstreams, 0);
> +	kref_init(&dev->ref);
>  	atomic_set(&dev->nmappings, 0);
>  	mutex_init(&dev->lock);
> 
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 15e415e..f157cf7 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -575,7 +575,7 @@ struct uvc_device {
> 
>  	/* Video Streaming interfaces */
>  	struct list_head streams;
> -	atomic_t nstreams;
> +	struct kref ref;
> 
>  	/* Status Interrupt Endpoint */
>  	struct usb_host_endpoint *int_ep;

-- 
Regards,

Laurent Pinchart