Hi Jayakrishnan,
Thank you for the patch.
On Thursday 27 October 2011 16:05:28 Jayakrishnan wrote:
> This patch is a fix for uvcvideo crash observed on device hotplug. If
> the camera is removed when streaming is going on, close(fd) call by the
> streaming application could lead to a crash.
>
> The issue is that memory allocated to instances of uvc_streaming
> structure is freed in the handler for device disconnect notification.
I'm surprised by that. When the device is disconnected the USB subsystem will
call the driver's disconnection handler, uvc_disconnect(). This in turn calls
uvc_unregister_video(), which calls video_unregister_device().
video_unregister_device() will call the uvcvideo release handler
(uvc_release()) back when the last reference to the video device is released.
This happens when all file handles are closed, so the uvc_streaming instance
should not get freed until all uvcvideo file handles are closed by
applications.
Could you elaborate on how you produced the crash, and send me a backtrace ?
> But these instances will be accessed when device is closed. The
> instances of data structure is protected by reference count to avoid
> this crash. Stream count is removed to avoid redundancy.
>
> This logic is quite similar to what was present in uvcvideo earlier.
> Commit 716fdee110ceb816cca8c46c0890d08c5a1addb9 had removed kref.
The kref was required back then as the videodev was racy. This has been fixed
in videodev a while ago, so the kref got removed from uvcvideo.
> Signed-off-by: Jayakrishnan Memana <jayakrishnan.mem...@maxim-ic.com>
> ---
>
> diff --git a/drivers/media/video/uvc/uvc_driver.c
> b/drivers/media/video/uvc/uvc_driver.c
> index 656d4c9..af50624 100644
> --- a/drivers/media/video/uvc/uvc_driver.c
> +++ b/drivers/media/video/uvc/uvc_driver.c
> @@ -1593,8 +1593,9 @@ static int uvc_scan_device(struct uvc_device *dev)
> * already been canceled by the USB core. There is no need to kill the
> * interrupt URB manually.
> */
> -static void uvc_delete(struct uvc_device *dev)
> +void uvc_delete(struct kref *kref)
> {
> + struct uvc_device *dev = container_of(kref, struct uvc_device, kref);
> struct list_head *p, *n;
>
> usb_put_intf(dev->intf);
> @@ -1648,11 +1649,8 @@ 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);
> + /* Delete the device when all references are removed */
> + kref_put(&dev->kref, uvc_delete);
> }
>
> /*
> @@ -1664,10 +1662,9 @@ 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, take a reference now and remove it after the loop
> */
> - atomic_inc(&dev->nstreams);
> + kref_get(&dev->kref);
>
> list_for_each_entry(stream, &dev->streams, list) {
> if (stream->vdev == NULL)
> @@ -1677,11 +1674,10 @@ static void uvc_unregister_video(struct
> uvc_device *dev)
> stream->vdev = NULL;
> }
>
> - /* Decrement the stream count and call uvc_delete explicitly if there
> - * are no stream left.
> + /* Remove the reference and delete the device if all references are
> + * removed
> */
> - if (atomic_dec_and_test(&dev->nstreams))
> - uvc_delete(dev);
> + kref_put(&dev->kref, uvc_delete);
> }
>
> static int uvc_register_video(struct uvc_device *dev,
> @@ -1732,7 +1728,6 @@ static int uvc_register_video(struct uvc_device *dev,
> return ret;
> }
>
> - atomic_inc(&dev->nstreams);
> return 0;
> }
>
> @@ -1816,9 +1811,9 @@ 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);
> atomic_set(&dev->users, 0);
> atomic_set(&dev->nmappings, 0);
> + kref_init(&dev->kref);
>
> dev->udev = usb_get_dev(udev);
> dev->intf = usb_get_intf(intf);
> diff --git a/drivers/media/video/uvc/uvc_v4l2.c
> b/drivers/media/video/uvc/uvc_v4l2.c
> index dadf11f..0c21a56 100644
> --- a/drivers/media/video/uvc/uvc_v4l2.c
> +++ b/drivers/media/video/uvc/uvc_v4l2.c
> @@ -500,6 +500,8 @@ static int uvc_v4l2_open(struct file *file)
> handle->state = UVC_HANDLE_PASSIVE;
> file->private_data = handle;
>
> + kref_get(&stream->dev->kref);
> +
> return 0;
> }
>
> @@ -528,6 +530,7 @@ static int uvc_v4l2_release(struct file *file)
> uvc_status_stop(stream->dev);
>
> usb_autopm_put_interface(stream->dev->intf);
> + kref_put(&stream->dev->kref, uvc_delete);
> return 0;
> }
>
> diff --git a/drivers/media/video/uvc/uvcvideo.h
> b/drivers/media/video/uvc/uvcvideo.h
> index 4c1392e..0925642 100644
> --- a/drivers/media/video/uvc/uvcvideo.h
> +++ b/drivers/media/video/uvc/uvcvideo.h
> @@ -423,6 +423,7 @@ struct uvc_device {
> char name[32];
>
> enum uvc_device_state state;
> + struct kref kref;
> atomic_t users;
> atomic_t nmappings;
>
> @@ -439,7 +440,6 @@ struct uvc_device {
>
> /* Video Streaming interfaces */
> struct list_head streams;
> - atomic_t nstreams;
>
> /* Status Interrupt Endpoint */
> struct usb_host_endpoint *int_ep;
> @@ -509,6 +509,7 @@ extern unsigned int uvc_timeout_param;
>
> /* Core driver */
> extern struct uvc_driver uvc_driver;
> +extern void uvc_delete(struct kref *kref);
>
> extern struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int
> id);
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-uvc-devel mailing list
Linux-uvc-devel@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
