stk-webcam: [RFT] Fix video_device handling

stk-webcam: [RFT] Fix video_device handling

[David Ellingsworth]

[-- Attachment #1: Type: text/plain, Size: 10941 bytes --]

This patch replaces the internal video_device struct of the stk_camera
struct with a pointer. video_device_alloc() is the used to fill this
pointer and video_device_release to free it. This prevents freeing of
the video_device struct before the release callback of the
video_device has been called per the V4L2 api specs.

This patch also relocates the call to video_unregister_device to the
disconnect callback of the usb_driver. This prevents calling
video_unregister_device from the usb probe callback in error paths.
Doing so removes the /dev device from the system when the physical
device no longer exists thus preventing future opens. Careful
attention has been paid to close after disconnect as well.

- David

================cut here==============================
---
 drivers/media/video/stk-webcam.c |  107 ++++++++++++++-----------------------
 drivers/media/video/stk-webcam.h |    3 +-
 2 files changed, 42 insertions(+), 68 deletions(-)

diff --git a/drivers/media/video/stk-webcam.c b/drivers/media/video/stk-webcam.c
index b12c60c..db39995 100644
--- a/drivers/media/video/stk-webcam.c
+++ b/drivers/media/video/stk-webcam.c
@@ -68,11 +68,6 @@ static void stk_camera_cleanup(struct kref *kref)
 {
        struct stk_camera *dev = to_stk_camera(kref);

-       STK_INFO("Syntek USB2.0 Camera release resources"
-               " video device /dev/video%d\n", dev->vdev.minor);
-       video_unregister_device(&dev->vdev);
-       dev->vdev.priv = NULL;
-
        if (dev->sio_bufs != NULL || dev->isobufs != NULL)
                STK_ERROR("We are leaking memory\n");
        usb_put_intf(dev->interface);
@@ -255,7 +250,7 @@ static ssize_t show_brightness(struct device *class,
                        struct device_attribute *attr, char *buf)
 {
        struct video_device *vdev = to_video_device(class);
-       struct stk_camera *dev = vdev_to_camera(vdev);
+       struct stk_camera *dev = video_get_drvdata(vdev);

        return sprintf(buf, "%X\n", dev->vsettings.brightness);
 }
@@ -268,7 +263,7 @@ static ssize_t store_brightness(struct device *class,
        int ret;

        struct video_device *vdev = to_video_device(class);
-       struct stk_camera *dev = vdev_to_camera(vdev);
+       struct stk_camera *dev = video_get_drvdata(vdev);

        value = simple_strtoul(buf, &endp, 16);

@@ -285,7 +280,7 @@ static ssize_t show_hflip(struct device *class,
                struct device_attribute *attr, char *buf)
 {
        struct video_device *vdev = to_video_device(class);
-       struct stk_camera *dev = vdev_to_camera(vdev);
+       struct stk_camera *dev = video_get_drvdata(vdev);

        return sprintf(buf, "%d\n", dev->vsettings.hflip);
 }
@@ -294,7 +289,7 @@ static ssize_t store_hflip(struct device *class,
                struct device_attribute *attr, const char *buf, size_t count)
 {
        struct video_device *vdev = to_video_device(class);
-       struct stk_camera *dev = vdev_to_camera(vdev);
+       struct stk_camera *dev = video_get_drvdata(vdev);

        if (strncmp(buf, "1", 1) == 0)
                dev->vsettings.hflip = 1;
@@ -310,7 +305,7 @@ static ssize_t show_vflip(struct device *class,
                struct device_attribute *attr, char *buf)
 {
        struct video_device *vdev = to_video_device(class);
-       struct stk_camera *dev = vdev_to_camera(vdev);
+       struct stk_camera *dev = video_get_drvdata(vdev);

        return sprintf(buf, "%d\n", dev->vsettings.vflip);
 }
@@ -319,7 +314,7 @@ static ssize_t store_vflip(struct device *class,
                struct device_attribute *attr, const char *buf, size_t count)
 {
        struct video_device *vdev = to_video_device(class);
-       struct stk_camera *dev = vdev_to_camera(vdev);
+       struct stk_camera *dev = video_get_drvdata(vdev);

        if (strncmp(buf, "1", 1) == 0)
                dev->vsettings.vflip = 1;
@@ -683,11 +678,11 @@ static int v4l_stk_open(struct inode *inode,
struct file *fp)
        struct video_device *vdev;

        vdev = video_devdata(fp);
-       dev = vdev_to_camera(vdev);
+       dev = video_get_drvdata(vdev);

        if (dev == NULL || !is_present(dev))
                return -ENXIO;
-       fp->private_data = vdev;
+       fp->private_data = dev;
        kref_get(&dev->kref);
        usb_autopm_get_interface(dev->interface);

@@ -696,19 +691,7 @@ static int v4l_stk_open(struct inode *inode,
struct file *fp)

 static int v4l_stk_release(struct inode *inode, struct file *fp)
 {
-       struct stk_camera *dev;
-       struct video_device *vdev;
-
-       vdev = video_devdata(fp);
-       if (vdev == NULL) {
-               STK_ERROR("v4l_release called w/o video devdata\n");
-               return -EFAULT;
-       }
-       dev = vdev_to_camera(vdev);
-       if (dev == NULL) {
-               STK_ERROR("v4l_release called on removed device\n");
-               return -ENODEV;
-       }
+       struct stk_camera *dev = fp->private_data;

        if (dev->owner != fp) {
                usb_autopm_put_interface(dev->interface);
@@ -734,14 +717,8 @@ static ssize_t v4l_stk_read(struct file *fp, char
__user *buf,
        int i;
        int ret;
        unsigned long flags;
-       struct stk_camera *dev;
-       struct video_device *vdev;
        struct stk_sio_buffer *sbuf;
-
-       vdev = video_devdata(fp);
-       if (vdev == NULL)
-               return -EFAULT;
-       dev = vdev_to_camera(vdev);
+       struct stk_camera *dev = fp->private_data;

        if (dev == NULL)
                return -EIO;
@@ -800,15 +777,8 @@ static ssize_t v4l_stk_read(struct file *fp, char
__user *buf,

 static unsigned int v4l_stk_poll(struct file *fp, poll_table *wait)
 {
-       struct stk_camera *dev;
-       struct video_device *vdev;
-
-       vdev = video_devdata(fp);
-
-       if (vdev == NULL)
-               return -EFAULT;
+       struct stk_camera *dev = fp->private_data;

-       dev = vdev_to_camera(vdev);
        if (dev == NULL)
                return -ENODEV;

@@ -846,16 +816,12 @@ static int v4l_stk_mmap(struct file *fp, struct
vm_area_struct *vma)
        unsigned int i;
        int ret;
        unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
-       struct stk_camera *dev;
-       struct video_device *vdev;
+       struct stk_camera *dev = fp->private_data;
        struct stk_sio_buffer *sbuf = NULL;

        if (!(vma->vm_flags & VM_WRITE) || !(vma->vm_flags & VM_SHARED))
                return -EINVAL;

-       vdev = video_devdata(fp);
-       dev = vdev_to_camera(vdev);
-
        for (i = 0; i < dev->n_sbufs; i++) {
                if (dev->sio_bufs[i].v4lbuf.m.offset == offset) {
                        sbuf = dev->sio_bufs + i;
@@ -1327,10 +1293,6 @@ static struct file_operations v4l_stk_fops = {
        .llseek = no_llseek
 };

-static void stk_v4l_dev_release(struct video_device *vd)
-{
-}
-
 static struct video_device stk_v4l_data = {
        .name = "stkwebcam",
        .type = VFL_TYPE_GRABBER,
@@ -1339,7 +1301,7 @@ static struct video_device stk_v4l_data = {
        .tvnorms = V4L2_STD_UNKNOWN,
        .current_norm = V4L2_STD_UNKNOWN,
        .fops = &v4l_stk_fops,
-       .release = stk_v4l_dev_release,
+       .release = video_device_release,

        .vidioc_querycap = stk_vidioc_querycap,
        .vidioc_enum_fmt_cap = stk_vidioc_enum_fmt_cap,
@@ -1367,16 +1329,16 @@ static int stk_register_video_device(struct
stk_camera *dev)
 {
        int err;

-       dev->vdev = stk_v4l_data;
-       dev->vdev.debug = debug;
-       dev->vdev.dev = &dev->interface->dev;
-       dev->vdev.priv = dev;
-       err = video_register_device(&dev->vdev, VFL_TYPE_GRABBER, -1);
+       *dev->vdev = stk_v4l_data;
+       dev->vdev->debug = debug;
+       dev->vdev->dev = &dev->interface->dev;
+       video_set_drvdata(dev->vdev, dev);
+       err = video_register_device(dev->vdev, VFL_TYPE_GRABBER, -1);
        if (err)
                STK_ERROR("v4l registration failed\n");
        else
                STK_INFO("Syntek USB2.0 Camera is now controlling video device"
-                       " /dev/video%d\n", dev->vdev.minor);
+                       " /dev/video%d\n", dev->vdev->minor);
        return err;
 }

@@ -1387,7 +1349,7 @@ static int stk_camera_probe(struct usb_interface
*interface,
                const struct usb_device_id *id)
 {
        int i;
-       int err;
+       int err = 0;

        struct stk_camera *dev = NULL;
        struct usb_device *udev = interface_to_usbdev(interface);
@@ -1433,8 +1395,8 @@ static int stk_camera_probe(struct usb_interface
*interface,
        }
        if (!dev->isoc_ep) {
                STK_ERROR("Could not find isoc-in endpoint");
-               kref_put(&dev->kref, stk_camera_cleanup);
-               return -ENODEV;
+               err = -ENODEV;
+               goto error;
        }
        dev->vsettings.brightness = 0x7fff;
        dev->vsettings.palette = V4L2_PIX_FMT_RGB565;
@@ -1446,16 +1408,24 @@ static int stk_camera_probe(struct
usb_interface *interface,

        usb_set_intfdata(interface, dev);

-       err = stk_register_video_device(dev);
-       if (err) {
-               kref_put(&dev->kref, stk_camera_cleanup);
-               return err;
+       if((dev->vdev = video_device_alloc()) == NULL) {
+               err = -ENOMEM;
+               goto error;
        }

-       stk_create_sysfs_files(&dev->vdev);
+       if((err = stk_register_video_device(dev)))
+               goto error_vdev;
+
+       stk_create_sysfs_files(dev->vdev);
        usb_autopm_enable(dev->interface);

        return 0;
+
+error_vdev:
+       video_device_release(dev->vdev);
+error:
+       kref_put(&dev->kref, stk_camera_cleanup);
+       return err;
 }

 static void stk_camera_disconnect(struct usb_interface *interface)
@@ -1466,7 +1436,12 @@ static void stk_camera_disconnect(struct
usb_interface *interface)
        unset_present(dev);

        wake_up_interruptible(&dev->wait_frame);
-       stk_remove_sysfs_files(&dev->vdev);
+       stk_remove_sysfs_files(dev->vdev);
+
+       STK_INFO("Syntek USB2.0 Camera release resources"
+               " video device /dev/video%d\n", dev->vdev->minor);
+
+       video_unregister_device(dev->vdev);

        kref_put(&dev->kref, stk_camera_cleanup);
 }
diff --git a/drivers/media/video/stk-webcam.h b/drivers/media/video/stk-webcam.h
index df4dfef..84257fe 100644
--- a/drivers/media/video/stk-webcam.h
+++ b/drivers/media/video/stk-webcam.h
@@ -91,7 +91,7 @@ struct regval {
 };

 struct stk_camera {
-       struct video_device vdev;
+       struct video_device *vdev;
        struct usb_device *udev;
        struct usb_interface *interface;
        int webcam_model;
@@ -122,7 +122,6 @@ struct stk_camera {
 };

 #define to_stk_camera(d) container_of(d, struct stk_camera, kref)
-#define vdev_to_camera(d) container_of(d, struct stk_camera, vdev)

 void stk_camera_delete(struct kref *);
 int stk_camera_write_reg(struct stk_camera *, u16, u8);
--
1.5.5.1

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: stk-webcam.patch --]
[-- Type: text/x-diff; name=stk-webcam.patch, Size: 8854 bytes --]

---
 drivers/media/video/stk-webcam.c |  107 ++++++++++++++-----------------------
 drivers/media/video/stk-webcam.h |    3 +-
 2 files changed, 42 insertions(+), 68 deletions(-)

diff --git a/drivers/media/video/stk-webcam.c b/drivers/media/video/stk-webcam.c
index b12c60c..db39995 100644
--- a/drivers/media/video/stk-webcam.c
+++ b/drivers/media/video/stk-webcam.c
@@ -68,11 +68,6 @@ static void stk_camera_cleanup(struct kref *kref)
 {
 	struct stk_camera *dev = to_stk_camera(kref);
 
-	STK_INFO("Syntek USB2.0 Camera release resources"
-		" video device /dev/video%d\n", dev->vdev.minor);
-	video_unregister_device(&dev->vdev);
-	dev->vdev.priv = NULL;
-
 	if (dev->sio_bufs != NULL || dev->isobufs != NULL)
 		STK_ERROR("We are leaking memory\n");
 	usb_put_intf(dev->interface);
@@ -255,7 +250,7 @@ static ssize_t show_brightness(struct device *class,
 			struct device_attribute *attr, char *buf)
 {
 	struct video_device *vdev = to_video_device(class);
-	struct stk_camera *dev = vdev_to_camera(vdev);
+	struct stk_camera *dev = video_get_drvdata(vdev);
 
 	return sprintf(buf, "%X\n", dev->vsettings.brightness);
 }
@@ -268,7 +263,7 @@ static ssize_t store_brightness(struct device *class,
 	int ret;
 
 	struct video_device *vdev = to_video_device(class);
-	struct stk_camera *dev = vdev_to_camera(vdev);
+	struct stk_camera *dev = video_get_drvdata(vdev);
 
 	value = simple_strtoul(buf, &endp, 16);
 
@@ -285,7 +280,7 @@ static ssize_t show_hflip(struct device *class,
 		struct device_attribute *attr, char *buf)
 {
 	struct video_device *vdev = to_video_device(class);
-	struct stk_camera *dev = vdev_to_camera(vdev);
+	struct stk_camera *dev = video_get_drvdata(vdev);
 
 	return sprintf(buf, "%d\n", dev->vsettings.hflip);
 }
@@ -294,7 +289,7 @@ static ssize_t store_hflip(struct device *class,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct video_device *vdev = to_video_device(class);
-	struct stk_camera *dev = vdev_to_camera(vdev);
+	struct stk_camera *dev = video_get_drvdata(vdev);
 
 	if (strncmp(buf, "1", 1) == 0)
 		dev->vsettings.hflip = 1;
@@ -310,7 +305,7 @@ static ssize_t show_vflip(struct device *class,
 		struct device_attribute *attr, char *buf)
 {
 	struct video_device *vdev = to_video_device(class);
-	struct stk_camera *dev = vdev_to_camera(vdev);
+	struct stk_camera *dev = video_get_drvdata(vdev);
 
 	return sprintf(buf, "%d\n", dev->vsettings.vflip);
 }
@@ -319,7 +314,7 @@ static ssize_t store_vflip(struct device *class,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct video_device *vdev = to_video_device(class);
-	struct stk_camera *dev = vdev_to_camera(vdev);
+	struct stk_camera *dev = video_get_drvdata(vdev);
 
 	if (strncmp(buf, "1", 1) == 0)
 		dev->vsettings.vflip = 1;
@@ -683,11 +678,11 @@ static int v4l_stk_open(struct inode *inode, struct file *fp)
 	struct video_device *vdev;
 
 	vdev = video_devdata(fp);
-	dev = vdev_to_camera(vdev);
+	dev = video_get_drvdata(vdev);
 
 	if (dev == NULL || !is_present(dev))
 		return -ENXIO;
-	fp->private_data = vdev;
+	fp->private_data = dev;
 	kref_get(&dev->kref);
 	usb_autopm_get_interface(dev->interface);
 
@@ -696,19 +691,7 @@ static int v4l_stk_open(struct inode *inode, struct file *fp)
 
 static int v4l_stk_release(struct inode *inode, struct file *fp)
 {
-	struct stk_camera *dev;
-	struct video_device *vdev;
-
-	vdev = video_devdata(fp);
-	if (vdev == NULL) {
-		STK_ERROR("v4l_release called w/o video devdata\n");
-		return -EFAULT;
-	}
-	dev = vdev_to_camera(vdev);
-	if (dev == NULL) {
-		STK_ERROR("v4l_release called on removed device\n");
-		return -ENODEV;
-	}
+	struct stk_camera *dev = fp->private_data;
 
 	if (dev->owner != fp) {
 		usb_autopm_put_interface(dev->interface);
@@ -734,14 +717,8 @@ static ssize_t v4l_stk_read(struct file *fp, char __user *buf,
 	int i;
 	int ret;
 	unsigned long flags;
-	struct stk_camera *dev;
-	struct video_device *vdev;
 	struct stk_sio_buffer *sbuf;
-
-	vdev = video_devdata(fp);
-	if (vdev == NULL)
-		return -EFAULT;
-	dev = vdev_to_camera(vdev);
+	struct stk_camera *dev = fp->private_data;
 
 	if (dev == NULL)
 		return -EIO;
@@ -800,15 +777,8 @@ static ssize_t v4l_stk_read(struct file *fp, char __user *buf,
 
 static unsigned int v4l_stk_poll(struct file *fp, poll_table *wait)
 {
-	struct stk_camera *dev;
-	struct video_device *vdev;
-
-	vdev = video_devdata(fp);
-
-	if (vdev == NULL)
-		return -EFAULT;
+	struct stk_camera *dev = fp->private_data;
 
-	dev = vdev_to_camera(vdev);
 	if (dev == NULL)
 		return -ENODEV;
 
@@ -846,16 +816,12 @@ static int v4l_stk_mmap(struct file *fp, struct vm_area_struct *vma)
 	unsigned int i;
 	int ret;
 	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
-	struct stk_camera *dev;
-	struct video_device *vdev;
+	struct stk_camera *dev = fp->private_data;
 	struct stk_sio_buffer *sbuf = NULL;
 
 	if (!(vma->vm_flags & VM_WRITE) || !(vma->vm_flags & VM_SHARED))
 		return -EINVAL;
 
-	vdev = video_devdata(fp);
-	dev = vdev_to_camera(vdev);
-
 	for (i = 0; i < dev->n_sbufs; i++) {
 		if (dev->sio_bufs[i].v4lbuf.m.offset == offset) {
 			sbuf = dev->sio_bufs + i;
@@ -1327,10 +1293,6 @@ static struct file_operations v4l_stk_fops = {
 	.llseek = no_llseek
 };
 
-static void stk_v4l_dev_release(struct video_device *vd)
-{
-}
-
 static struct video_device stk_v4l_data = {
 	.name = "stkwebcam",
 	.type = VFL_TYPE_GRABBER,
@@ -1339,7 +1301,7 @@ static struct video_device stk_v4l_data = {
 	.tvnorms = V4L2_STD_UNKNOWN,
 	.current_norm = V4L2_STD_UNKNOWN,
 	.fops = &v4l_stk_fops,
-	.release = stk_v4l_dev_release,
+	.release = video_device_release,
 
 	.vidioc_querycap = stk_vidioc_querycap,
 	.vidioc_enum_fmt_cap = stk_vidioc_enum_fmt_cap,
@@ -1367,16 +1329,16 @@ static int stk_register_video_device(struct stk_camera *dev)
 {
 	int err;
 
-	dev->vdev = stk_v4l_data;
-	dev->vdev.debug = debug;
-	dev->vdev.dev = &dev->interface->dev;
-	dev->vdev.priv = dev;
-	err = video_register_device(&dev->vdev, VFL_TYPE_GRABBER, -1);
+	*dev->vdev = stk_v4l_data;
+	dev->vdev->debug = debug;
+	dev->vdev->dev = &dev->interface->dev;
+	video_set_drvdata(dev->vdev, dev);
+	err = video_register_device(dev->vdev, VFL_TYPE_GRABBER, -1);
 	if (err)
 		STK_ERROR("v4l registration failed\n");
 	else
 		STK_INFO("Syntek USB2.0 Camera is now controlling video device"
-			" /dev/video%d\n", dev->vdev.minor);
+			" /dev/video%d\n", dev->vdev->minor);
 	return err;
 }
 
@@ -1387,7 +1349,7 @@ static int stk_camera_probe(struct usb_interface *interface,
 		const struct usb_device_id *id)
 {
 	int i;
-	int err;
+	int err = 0;
 
 	struct stk_camera *dev = NULL;
 	struct usb_device *udev = interface_to_usbdev(interface);
@@ -1433,8 +1395,8 @@ static int stk_camera_probe(struct usb_interface *interface,
 	}
 	if (!dev->isoc_ep) {
 		STK_ERROR("Could not find isoc-in endpoint");
-		kref_put(&dev->kref, stk_camera_cleanup);
-		return -ENODEV;
+		err = -ENODEV;
+		goto error;
 	}
 	dev->vsettings.brightness = 0x7fff;
 	dev->vsettings.palette = V4L2_PIX_FMT_RGB565;
@@ -1446,16 +1408,24 @@ static int stk_camera_probe(struct usb_interface *interface,
 
 	usb_set_intfdata(interface, dev);
 
-	err = stk_register_video_device(dev);
-	if (err) {
-		kref_put(&dev->kref, stk_camera_cleanup);
-		return err;
+	if((dev->vdev = video_device_alloc()) == NULL) {
+		err = -ENOMEM;
+		goto error;
 	}
 
-	stk_create_sysfs_files(&dev->vdev);
+	if((err = stk_register_video_device(dev)))
+		goto error_vdev;
+
+	stk_create_sysfs_files(dev->vdev);
 	usb_autopm_enable(dev->interface);
 
 	return 0;
+
+error_vdev:
+	video_device_release(dev->vdev);
+error:
+	kref_put(&dev->kref, stk_camera_cleanup);
+	return err;
 }
 
 static void stk_camera_disconnect(struct usb_interface *interface)
@@ -1466,7 +1436,12 @@ static void stk_camera_disconnect(struct usb_interface *interface)
 	unset_present(dev);
 
 	wake_up_interruptible(&dev->wait_frame);
-	stk_remove_sysfs_files(&dev->vdev);
+	stk_remove_sysfs_files(dev->vdev);
+
+	STK_INFO("Syntek USB2.0 Camera release resources"
+		" video device /dev/video%d\n", dev->vdev->minor);
+
+	video_unregister_device(dev->vdev);
 
 	kref_put(&dev->kref, stk_camera_cleanup);
 }
diff --git a/drivers/media/video/stk-webcam.h b/drivers/media/video/stk-webcam.h
index df4dfef..84257fe 100644
--- a/drivers/media/video/stk-webcam.h
+++ b/drivers/media/video/stk-webcam.h
@@ -91,7 +91,7 @@ struct regval {
 };
 
 struct stk_camera {
-	struct video_device vdev;
+	struct video_device *vdev;
 	struct usb_device *udev;
 	struct usb_interface *interface;
 	int webcam_model;
@@ -122,7 +122,6 @@ struct stk_camera {
 };
 
 #define to_stk_camera(d) container_of(d, struct stk_camera, kref)
-#define vdev_to_camera(d) container_of(d, struct stk_camera, vdev)
 
 void stk_camera_delete(struct kref *);
 int stk_camera_write_reg(struct stk_camera *, u16, u8);
-- 
1.5.5.1


[-- Attachment #3: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: stk-webcam: [RFT] Fix video_device handling

[Jaime Velasco Juan]

Hi David,

El vie. 27 de jun. de 2008, a las 19:36:03 -0400, David Ellingsworth
escribió:
> This patch replaces the internal video_device struct of the stk_camera
> struct with a pointer. video_device_alloc() is the used to fill this
> pointer and video_device_release to free it. This prevents freeing of
> the video_device struct before the release callback of the
> video_device has been called per the V4L2 api specs.
> 
The driver did that initially, but as the video_device struct is always
needed, it was embedded in the main stk_camera struct to avoid doing
another allocation and check if it succeded, etc. Also, I didn't want to
use video_get_drvdata, because it's defined inside an
#ifdef OBSOLETE_OWNER /* to be removed soon */
which didn't sound well to me.

In any case, the data should never be freed before all users of the
device have closed it, because it is protected with a kref.

> This patch also relocates the call to video_unregister_device to the
> disconnect callback of the usb_driver. This prevents calling
> video_unregister_device from the usb probe callback in error paths.
> Doing so removes the /dev device from the system when the physical
> device no longer exists thus preventing future opens. Careful
> attention has been paid to close after disconnect as well.

This sounds good to me, but I remember having problems when the camera was
disconnected while in use, so that's what I tried with your patch.  My
camera is integrated but I test disconnection removing the ehci_hcd
module (I'm sure there are better ways...).

This happens when xawtv is running while the camera is disconnected:

ehci_hcd 0000:00:03.3: remove, state 1
usb usb4: USB disconnect, address 1
usb 4-4: USB disconnect, address 2
stkwebcam: Syntek USB2.0 Camera release resources video device /dev/video0
ehci_hcd 0000:00:03.3: USB bus 4 deregistered
ACPI: PCI interrupt for device 0000:00:03.3 disabled
BUG: unable to handle kernel NULL pointer dereference at 000000000000024c
IP: [<ffffffffa0205172>] :videodev:__video_do_ioctl+0x62/0x2ff0
PGD 33faf067 PUD 2e9d9067 PMD 0 
Oops: 0000 [1] PREEMPT 
CPU 0 
Modules linked in: act_police sch_ingress cls_u32 sch_sfq sch_htb af_packet nouveau drm ppdev lp ipv6 ipt_LOG ipt_recent nf_conntrack_ipv4 xt_state nf_conntrack xt_tcpudp iptable_filter ip_tables x_tables fuse loop firewire_sbp2 stkwebcam compat_ioctl32 videodev v4l1_compat arc4 ecb crypto_blkcipher cryptomgr crypto_algapi b43 rng_core mac80211 cfg80211 joydev irtty_sir sir_dev irda crc_ccitt pcspkr psmouse serio_raw sdhci r8169 mmc_core bitrev firewire_ohci firewire_core crc_itu_t video output battery ac snd_intel8x0 snd_ac97_codec ac97_bus snd_pcm_oss snd_mixer_oss snd_pcm snd_timer button snd ssb soundcore ohci_hcd snd_page_alloc sg pcmcia sr_mod firmware_class cdrom crc32 ext3 jbd mbcache sd_mod [last unloaded: ehci_hcd]
Pid: 3516, comm: xawtv.bin Not tainted 2.6.26-rc8-po0 #2
RIP: 0010:[<ffffffffa0205172>]  [<ffffffffa0205172>] :videodev:__video_do_ioctl+0x62/0x2ff0
RSP: 0018:ffff81001f0b3d38  EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff81003fafc940 RCX: ffff81001f0b3e28
RDX: 00000000c0585611 RSI: ffff81002e9a5b40 RDI: ffff81003fafc940
RBP: 00000000c0585611 R08: 0000000000000001 R09: 0000000000000000
R10: ffff81003f9d3900 R11: 0000000000000000 R12: ffff81002e9a5b40
R13: 00000000c0585611 R14: 0000000000000000 R15: ffff81001f0b3e28
FS:  00007f59284966e0(0000) GS:ffffffff80591000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 000000000000024c CR3: 000000002ea64000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process xawtv.bin (pid: 3516, threadinfo ffff81001f0b2000, task ffff81002e8ed140)
Stack:  0000100000000040 ffff810033a73180 0000000000000246 ffffffff80304431
 ffff81001f0b3d60 ffff81001f0b3e08 0000000100000000 0000000100000003
 00007fff305bc6c0 ffff81002e8ed140 0000000000000000 ffffffff802ab18f
Call Trace:
 [<ffffffff80304431>] ? avc_has_perm_noaudit+0x181/0x510
 [<ffffffff802ab18f>] ? core_sys_select+0x26f/0x320
 [<ffffffffa0208267>] ? :videodev:video_ioctl2+0x167/0x344
 [<ffffffff802a979d>] ? vfs_ioctl+0x7d/0xa0
 [<ffffffff802a9a33>] ? do_vfs_ioctl+0x273/0x2e0
 [<ffffffff802a9b41>] ? sys_ioctl+0xa1/0xb0
 [<ffffffff8020c33b>] ? system_call_after_swapgs+0x7b/0x80


Code: ac 24 a0 00 00 00 41 89 d5 48 8b 46 18 49 89 cf 4c 8b 96 90 00 00 00 48 8b 40 08 8b 40 58 25 ff ff 0f 00 4c 8b 34 c5 00 c1 20 a0 <41> 8b 86 4c 02 00 00 83 e0 03 ff c8 0f 84 0a 1f 00 00 41 81 fd 
RIP  [<ffffffffa0205172>] :videodev:__video_do_ioctl+0x62/0x2ff0
 RSP <ffff81001f0b3d38>
CR2: 000000000000024c
---[ end trace f1332077cdef4f39 ]---


I recall working around a similar bug in the past, but the workaround
don't seem very good now ;), and I haven't touch the driver in a while,
so I'm not sure where is the problem. It seems that
video_unregister_device cannot be called when the device is open.

Regards,
Jaime

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: stk-webcam: [RFT] Fix video_device handling

[David Ellingsworth]

[snip]
>
> I recall working around a similar bug in the past, but the workaround
> don't seem very good now ;), and I haven't touch the driver in a while,
> so I'm not sure where is the problem. It seems that
> video_unregister_device cannot be called when the device is open.
>
> Regards,
> Jaime

Thanks for testing, I believe the the error you reported above is
indeed a result of an issue I reported on list a few days ago.
According to the API, the video_device struct is not to be freed until
it is no longer being used, thus the reason for the release callback
in the video_device struct. Currently, video_unregister_device always
causes the video_device struct to be freed despite the fact that it
may still in use. To me, this is a serious bug in the videodev driver,
since it doesn't behave as expected. The videodev driver should
reference count the video_device struct and call the release callback
only once it is no longer being used. I can work on this if no one
objects.

You are right, video_{get|set}_drvdata is obsolete as is
video_device->priv. I will replace all references to
video_{get|set}_drvdata with usb_{get|set}_drvdata(video_device->dev)
instead as suggested in v4l2-dev.h

I will hold off on this patch until videodev has been corrected to
call the callback at the appropriate time.

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: stk-webcam: [RFT] Fix video_device handling

[David Ellingsworth]

On Sat, Jun 28, 2008 at 11:00 AM, David Ellingsworth
<david@identd.dyndns.org> wrote:
[snip]
> According to the API, the video_device struct is not to be freed until
> it is no longer being used, thus the reason for the release callback
> in the video_device struct. Currently, video_unregister_device always
> causes the video_device struct to be freed despite the fact that it
> may still in use. To me, this is a serious bug in the videodev driver,
> since it doesn't behave as expected. The videodev driver should
> reference count the video_device struct and call the release callback
> only once it is no longer being used. I can work on this if no one
> objects.

Upon looking at video_open in videodev.c I noticed that the file_ops
were being replaced with those provided by modules calling
video_register_device. Since most modules use a const static
file_operations structure, it is impossible to overwrite the release
callback to call one within the videodev module. Thus in it's current
state, we cannot properly reference count the video_device struct. At
the moment, I see two possibilities. (1)  return an err in
__video_do_ioctl when video_devdata returns NULL or (2) implement all
possible file_operations in the videodev driver which then call the
associated file_operations in video_device.fops where defined.

Option 1 is easier to implement, but does not reference count the
video_device struct and therefore causes the release callback to occur
at inappropriate times. Option 2 requires some serious work to be done
in the videodev driver, but allows proper reference counting of the
video_device struct. By reference counting the video_device struct,
the free of the struct and the setting of video_device[minor]=NULL can
be delayed until the device is no longer in use, which may occur
during video_unregister_device or in the file_operations release
callback.

What are your opinions on this matter?

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: stk-webcam: [RFT] Fix video_device handling

[David Ellingsworth]

On Sat, Jun 28, 2008 at 4:55 PM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> On Sat, Jun 28, 2008 at 11:00 AM, David Ellingsworth
> <david@identd.dyndns.org> wrote:
> [snip]
>> According to the API, the video_device struct is not to be freed until
>> it is no longer being used, thus the reason for the release callback
>> in the video_device struct. Currently, video_unregister_device always
>> causes the video_device struct to be freed despite the fact that it
>> may still in use. To me, this is a serious bug in the videodev driver,
>> since it doesn't behave as expected. The videodev driver should
>> reference count the video_device struct and call the release callback
>> only once it is no longer being used. I can work on this if no one
>> objects.
>
> Upon looking at video_open in videodev.c I noticed that the file_ops
> were being replaced with those provided by modules calling
> video_register_device. Since most modules use a const static
> file_operations structure, it is impossible to overwrite the release
> callback to call one within the videodev module. Thus in it's current
> state, we cannot properly reference count the video_device struct. At
> the moment, I see two possibilities. (1)  return an err in
> __video_do_ioctl when video_devdata returns NULL or (2) implement all
> possible file_operations in the videodev driver which then call the
> associated file_operations in video_device.fops where defined.
>
> Option 1 is easier to implement, but does not reference count the
> video_device struct and therefore causes the release callback to occur
> at inappropriate times. Option 2 requires some serious work to be done
> in the videodev driver, but allows proper reference counting of the
> video_device struct. By reference counting the video_device struct,
> the free of the struct and the setting of video_device[minor]=NULL can
> be delayed until the device is no longer in use, which may occur
> during video_unregister_device or in the file_operations release
> callback.
>
> What are your opinions on this matter?
>
> Regards,
>
> David Ellingsworth
>
I just thought of another alternative... Option 3, add a private
file_operations structure to the video_device structure. During
video_register_device we initialize this structure with the one
pointed to by fops and set the release callback to a function in the
videodev module. Then during every open we would replace the current
file_operations with the private one in the video_device structure.

Option 3 requires less code than option 2, while still allowing proper
reference counting of the video_device structure between
video_register_device, video_unregister_device, and the open and
release callbacks from file_operations.

What are your thoughts?

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list