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.liakhovet...@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.pinch...@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
