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);
