[PATCH RFT]: stk-webcam: release via video_device release callback

[PATCH RFT]: stk-webcam: release via video_device release callback

[David Ellingsworth]

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

With the recent patch to v4l2 titled "v4l2: use register_chrdev_region
instead of register_chrdev", the internal reference count is no longer
necessary in order to free the internal stk_webcam struct. This patch
removes the reference counter from the stk_webcam struct and frees the
struct via the video_device release callback. It also fixes an
associated bug in stk_camera_probe which could result from
video_unregister_device being called before video_register_device.
Lastly, it simplifies access to the stk_webcam struct in several
places. This patch should apply cleanly against the "working" branch
of the v4l-dvb git repository.

This patch is identical to the patch I sent a couple of months back
titled "stk-webcam: Fix video_device handling" except that it has been
rebased against current modifications to stk-webcam and it no longer
depends on any other outstanding patches.

Regards,

David Ellingsworth

=============================================================
[PATCH] stk-webcam: release via video_device release callback


Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/video/stk-webcam.c |   84 ++++++++++---------------------------
 drivers/media/video/stk-webcam.h |    2 -
 2 files changed, 23 insertions(+), 63 deletions(-)

diff --git a/drivers/media/video/stk-webcam.c b/drivers/media/video/stk-webcam.c
index 8dda568..db69bc5 100644
--- a/drivers/media/video/stk-webcam.c
+++ b/drivers/media/video/stk-webcam.c
@@ -65,22 +65,6 @@ static struct usb_device_id stkwebcam_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, stkwebcam_table);

-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);
-	video_set_drvdata(&dev->vdev, NULL);
-
-	if (dev->sio_bufs != NULL || dev->isobufs != NULL)
-		STK_ERROR("We are leaking memory\n");
-	usb_put_intf(dev->interface);
-	kfree(dev);
-}
-
-
 /*
  * Basic stuff
  */
@@ -694,8 +678,7 @@ static int v4l_stk_open(struct inode *inode,
struct file *fp)
 		unlock_kernel();
 		return -ENXIO;
 	}
-	fp->private_data = vdev;
-	kref_get(&dev->kref);
+	fp->private_data = dev;
 	usb_autopm_get_interface(dev->interface);
 	unlock_kernel();

@@ -704,23 +687,10 @@ 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);
-		kref_put(&dev->kref, stk_camera_cleanup);
 		return 0;
 	}

@@ -731,7 +701,6 @@ static int v4l_stk_release(struct inode *inode,
struct file *fp)
 	dev->owner = NULL;

 	usb_autopm_put_interface(dev->interface);
-	kref_put(&dev->kref, stk_camera_cleanup);

 	return 0;
 }
@@ -742,14 +711,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;
@@ -808,15 +771,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;

@@ -854,16 +810,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;
@@ -1359,6 +1311,12 @@ static const struct v4l2_ioctl_ops v4l_stk_ioctl_ops = {

 static void stk_v4l_dev_release(struct video_device *vd)
 {
+	struct stk_camera *dev = vdev_to_camera(vd);
+
+	if (dev->sio_bufs != NULL || dev->isobufs != NULL)
+		STK_ERROR("We are leaking memory\n");
+	usb_put_intf(dev->interface);
+	kfree(dev);
 }

 static struct video_device stk_v4l_data = {
@@ -1379,7 +1337,6 @@ static int stk_register_video_device(struct
stk_camera *dev)
 	dev->vdev = stk_v4l_data;
 	dev->vdev.debug = debug;
 	dev->vdev.parent = &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");
@@ -1396,7 +1353,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);
@@ -1409,7 +1366,6 @@ static int stk_camera_probe(struct usb_interface
*interface,
 		return -ENOMEM;
 	}

-	kref_init(&dev->kref);
 	spin_lock_init(&dev->spinlock);
 	init_waitqueue_head(&dev->wait_frame);

@@ -1442,8 +1398,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;
@@ -1457,14 +1413,17 @@ static int stk_camera_probe(struct
usb_interface *interface,

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

 	stk_create_sysfs_files(&dev->vdev);
 	usb_autopm_enable(dev->interface);

 	return 0;
+
+error:
+	kfree(dev);
+	return err;
 }

 static void stk_camera_disconnect(struct usb_interface *interface)
@@ -1477,7 +1436,10 @@ static void stk_camera_disconnect(struct
usb_interface *interface)
 	wake_up_interruptible(&dev->wait_frame);
 	stk_remove_sysfs_files(&dev->vdev);

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

 #ifdef CONFIG_PM
diff --git a/drivers/media/video/stk-webcam.h b/drivers/media/video/stk-webcam.h
index df4dfef..084a85b 100644
--- a/drivers/media/video/stk-webcam.h
+++ b/drivers/media/video/stk-webcam.h
@@ -99,7 +99,6 @@ struct stk_camera {

 	u8 isoc_ep;

-	struct kref kref;
 	/* Not sure if this is right */
 	atomic_t urbs_used;

@@ -121,7 +120,6 @@ struct stk_camera {
 	unsigned sequence;
 };

-#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 *);
-- 
1.5.6

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

[PATCH] stk-webcam: release via video_device release callback


Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/video/stk-webcam.c |   84 ++++++++++---------------------------
 drivers/media/video/stk-webcam.h |    2 -
 2 files changed, 23 insertions(+), 63 deletions(-)

diff --git a/drivers/media/video/stk-webcam.c b/drivers/media/video/stk-webcam.c
index 8dda568..db69bc5 100644
--- a/drivers/media/video/stk-webcam.c
+++ b/drivers/media/video/stk-webcam.c
@@ -65,22 +65,6 @@ static struct usb_device_id stkwebcam_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, stkwebcam_table);
 
-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);
-	video_set_drvdata(&dev->vdev, NULL);
-
-	if (dev->sio_bufs != NULL || dev->isobufs != NULL)
-		STK_ERROR("We are leaking memory\n");
-	usb_put_intf(dev->interface);
-	kfree(dev);
-}
-
-
 /*
  * Basic stuff
  */
@@ -694,8 +678,7 @@ static int v4l_stk_open(struct inode *inode, struct file *fp)
 		unlock_kernel();
 		return -ENXIO;
 	}
-	fp->private_data = vdev;
-	kref_get(&dev->kref);
+	fp->private_data = dev;
 	usb_autopm_get_interface(dev->interface);
 	unlock_kernel();
 
@@ -704,23 +687,10 @@ 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);
-		kref_put(&dev->kref, stk_camera_cleanup);
 		return 0;
 	}
 
@@ -731,7 +701,6 @@ static int v4l_stk_release(struct inode *inode, struct file *fp)
 	dev->owner = NULL;
 
 	usb_autopm_put_interface(dev->interface);
-	kref_put(&dev->kref, stk_camera_cleanup);
 
 	return 0;
 }
@@ -742,14 +711,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;
@@ -808,15 +771,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;
 
@@ -854,16 +810,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;
@@ -1359,6 +1311,12 @@ static const struct v4l2_ioctl_ops v4l_stk_ioctl_ops = {
 
 static void stk_v4l_dev_release(struct video_device *vd)
 {
+	struct stk_camera *dev = vdev_to_camera(vd);
+
+	if (dev->sio_bufs != NULL || dev->isobufs != NULL)
+		STK_ERROR("We are leaking memory\n");
+	usb_put_intf(dev->interface);
+	kfree(dev);
 }
 
 static struct video_device stk_v4l_data = {
@@ -1379,7 +1337,6 @@ static int stk_register_video_device(struct stk_camera *dev)
 	dev->vdev = stk_v4l_data;
 	dev->vdev.debug = debug;
 	dev->vdev.parent = &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");
@@ -1396,7 +1353,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);
@@ -1409,7 +1366,6 @@ static int stk_camera_probe(struct usb_interface *interface,
 		return -ENOMEM;
 	}
 
-	kref_init(&dev->kref);
 	spin_lock_init(&dev->spinlock);
 	init_waitqueue_head(&dev->wait_frame);
 
@@ -1442,8 +1398,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;
@@ -1457,14 +1413,17 @@ static int stk_camera_probe(struct usb_interface *interface,
 
 	err = stk_register_video_device(dev);
 	if (err) {
-		kref_put(&dev->kref, stk_camera_cleanup);
-		return err;
+		goto error;
 	}
 
 	stk_create_sysfs_files(&dev->vdev);
 	usb_autopm_enable(dev->interface);
 
 	return 0;
+
+error:
+	kfree(dev);
+	return err;
 }
 
 static void stk_camera_disconnect(struct usb_interface *interface)
@@ -1477,7 +1436,10 @@ static void stk_camera_disconnect(struct usb_interface *interface)
 	wake_up_interruptible(&dev->wait_frame);
 	stk_remove_sysfs_files(&dev->vdev);
 
-	kref_put(&dev->kref, stk_camera_cleanup);
+	STK_INFO("Syntek USB2.0 Camera release resources"
+		"video device /dev/video%d\n", dev->vdev.minor);
+
+	video_unregister_device(&dev->vdev);
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/media/video/stk-webcam.h b/drivers/media/video/stk-webcam.h
index df4dfef..084a85b 100644
--- a/drivers/media/video/stk-webcam.h
+++ b/drivers/media/video/stk-webcam.h
@@ -99,7 +99,6 @@ struct stk_camera {
 
 	u8 isoc_ep;
 
-	struct kref kref;
 	/* Not sure if this is right */
 	atomic_t urbs_used;
 
@@ -121,7 +120,6 @@ struct stk_camera {
 	unsigned sequence;
 };
 
-#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 *);
-- 
1.5.6


[-- 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: [PATCH RFT]: stk-webcam: release via video_device release callback

[Jaime Velasco Juan]

Hi David, sorry for the delay and thanks for working in this.

I've done some testing and everything seemed to work, but I finally
managed to trigger a bug. I think it's unrelated to this patch.

The testing goes as follow:
1 - xawtv&
2 - rmmod ehci-hcd && modprobe ehci-hcd
3 - repeat...

As I cannot unplug the camera, I unload the ehci driver to simulate the
unplugging-while-in-use case. This normally works as expected, but after
some iterations of the test I got this:

[...]
usb 4-4: new high speed USB device using ehci_hcd and address 2
usb 4-4: configuration #1 chosen from 1 choice
stkwebcam: Syntek USB2.0 Camera is now controlling video device /dev/video2
stkwebcam: OmniVision sensor detected, id 9652 at address 60
stkwebcam: frame 3, bytesused=2461154, skipping
stkwebcam: frame 11, bytesused=1157638, skipping
stkwebcam: frame 4, bytesused=2586972, skipping
stkwebcam: frame 10, bytesused=2343188, skipping
ehci_hcd 0000:00:03.3: remove, state 1
usb usb4: USB disconnect, address 1
usb 4-4: USB disconnect, address 2
stkwebcam: Error (-19) re-submitting urb in stk_isoc_handler.
stkwebcam: Syntek USB2.0 Camera release resourcesvideo device /dev/video2
ehci_hcd 0000:00:03.3: USB bus 4 deregistered
ehci_hcd 0000:00:03.3: PCI INT D disabled
BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
IP: [<ffffffff803e0660>] unlink1+0x30/0x170
PGD 2b886067 PUD 2b875067 PMD 0 
Oops: 0000 [1] PREEMPT 
CPU 0 
Modules linked in: nouveau drm ppdev lp ipv6 fuse ipt_LOG ipt_recent nf_conntrack_ipv4 xt_state nf_conntrack xt_tcpudp iptable_filter ip_tables x_tables loop firewire_sbp2 joydev stkwebcam compat_ioctl32 videodev v4l1_compat arc4 ecb crypto_blkcipher cryptomgr crypto_algapi b43 rfkill rng_core mac80211 cfg80211 input_polldev irtty_sir sir_dev irda crc_ccitt pcspkr psmouse serio_raw k8temp r8169 bitrev snd_intel8x0 video output battery sdhci_pci sdhci snd_ac97_codec ac ac97_bus snd_pcm snd_timer mmc_core snd firewire_ohci firewire_core soundcore snd_page_alloc ssb crc_itu_t button ohci_hcd sg pcmcia sr_mod cdrom crc32 ext3 jbd mbcache sd_mod [last unloaded: ehci_hcd]
Pid: 3422, comm: cheese Not tainted 2.6.27-rc5-video0 #1
RIP: 0010:[<ffffffff803e0660>]  [<ffffffff803e0660>] unlink1+0x30/0x170
RSP: 0018:ffff88002b8a3e58  EFLAGS: 00010206
RAX: ffff88002ca0b800 RBX: ffff880032cf3a00 RCX: ffff880032cf3a00
RDX: 00000000fffffffe RSI: ffff880032cf3a00 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: ffff88003eeacef0
R10: 0000000000000001 R11: 0000000000000206 R12: 00000000fffffffe
R13: ffff88003f73f000 R14: ffff88003ed9db00 R15: 00007fd222fff940
FS:  00007fd2284077e0(0000) GS:ffffffff805a8e80(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00000000000000e8 CR3: 000000002b931000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process cheese (pid: 3422, threadinfo ffff88002b8a2000, task ffff8800365d38e0)
Stack:  ffff880032cf3a00 0000000000000000 ffff88003ec24000 ffff88003f73f000
 ffff88003ed9db00 ffffffff803e0949 ffff88002a4833f0 ffffffff803e1dd0
 0000000000000246 ffff88002a462dc8 ffff88002a462dc8 ffff88002a4833f0
Call Trace:
 [<ffffffff803e0949>] usb_hcd_unlink_urb+0x19/0x30
 [<ffffffff803e1dd0>] usb_kill_urb+0x50/0xe0
 [<ffffffffa0204ab7>] stk_clean_iso+0x87/0xc0 [stkwebcam]
 [<ffffffffa0206538>] v4l_stk_release+0x38/0x50 [stkwebcam]
 [<ffffffff802a0bc1>] __fput+0xa1/0x1c0
 [<ffffffff8029d7bb>] filp_close+0x5b/0x90
 [<ffffffff8029d8a1>] sys_close+0xb1/0x140
 [<ffffffff8020c37b>] system_call_fastpath+0x16/0x1b


Code: 89 1c 24 48 89 6c 24 08 48 89 f3 4c 89 64 24 10 4c 89 6c 24 18 48 89 fd 4c 89 74 24 20 48 8b 46 48 41 89 d4 48 83 78 38 00 74 30 <48> 8b 87 e8 00 00 00 48 8b 1c 24 48 8b 6c 24 08 4c 8b 64 24 10 
RIP  [<ffffffff803e0660>] unlink1+0x30/0x170
 RSP <ffff88002b8a3e58>
CR2: 00000000000000e8
---[ end trace c57f522e2b519e93 ]---
usb 1-2: new full speed USB device using ohci_hcd and address 8
usb 1-2: configuration #1 chosen from 1 choice
stkwebcam: Syntek USB2.0 Camera is now controlling video device /dev/video0
ehci_hcd 0000:00:03.3: PCI INT D -> GSI 23 (level, low) -> IRQ 23
ehci_hcd 0000:00:03.3: EHCI Host Controller
ehci_hcd 0000:00:03.3: new USB bus registered, assigned bus number 4
ehci_hcd 0000:00:03.3: cache line size of 64 is not supported
ehci_hcd 0000:00:03.3: irq 23, io mem 0xfebfc000
ehci_hcd 0000:00:03.3: USB 2.0 started, EHCI 1.00, driver 10 Dec 2004
usb usb4: configuration #1 chosen from 1 choice
hub 4-0:1.0: USB hub found
hub 4-0:1.0: 8 ports detected
usb 1-2: USB disconnect, address 8
stkwebcam: Syntek USB2.0 Camera release resourcesvideo device /dev/video0
usb 4-4: new high speed USB device using ehci_hcd and address 2
usb 4-4: configuration #1 chosen from 1 choice
stkwebcam: Syntek USB2.0 Camera is now controlling video device /dev/video0

The difference between this run of the test and the previous (working)
one is the line 
stkwebcam: Error (-19) re-submitting urb in stk_isoc_handler.

I think what happens is that the ehci driver is unloaded while stkwebcam
is inside its ISOC handler, it fails resubmitting the URB and then it oopses
when finally cleaning up and trying to kill an unsubmitted URB. So I think
the bug is in the URB handling code in stk-webcam (or maybe in the usb core?).

I think your patch is correct, and will try to find some time and fix the
broken URB handling code in stk-webcam. I'd appreciate comments if someone
thinks I'm mistaken here, or any hints about fixing the bug.

In short,
Acked-by: Jaime Velasco Juan <jsagarribay@gmail.com>

Kind regards.

El dom. 21 de sep. de 2008, a las 00:12:03 -0400, David Ellingsworth escribió:
> With the recent patch to v4l2 titled "v4l2: use register_chrdev_region
> instead of register_chrdev", the internal reference count is no longer
> necessary in order to free the internal stk_webcam struct. This patch
> removes the reference counter from the stk_webcam struct and frees the
> struct via the video_device release callback. It also fixes an
> associated bug in stk_camera_probe which could result from
> video_unregister_device being called before video_register_device.
> Lastly, it simplifies access to the stk_webcam struct in several
> places. This patch should apply cleanly against the "working" branch
> of the v4l-dvb git repository.
> 
> This patch is identical to the patch I sent a couple of months back
> titled "stk-webcam: Fix video_device handling" except that it has been
> rebased against current modifications to stk-webcam and it no longer
> depends on any other outstanding patches.
> 
> 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: [PATCH RFT]: stk-webcam: release via video_device release callback

[David Ellingsworth]

On Tue, Sep 23, 2008 at 3:51 PM, Jaime Velasco Juan
<jsagarribay@gmail.com> wrote:
> Hi David, sorry for the delay and thanks for working in this.
>
> I've done some testing and everything seemed to work, but I finally
> managed to trigger a bug. I think it's unrelated to this patch.
>
> The testing goes as follow:
> 1 - xawtv&
> 2 - rmmod ehci-hcd && modprobe ehci-hcd
> 3 - repeat...
>
> As I cannot unplug the camera, I unload the ehci driver to simulate the
> unplugging-while-in-use case. This normally works as expected, but after
> some iterations of the test I got this:
>
> [...]
> usb 4-4: new high speed USB device using ehci_hcd and address 2
> usb 4-4: configuration #1 chosen from 1 choice
> stkwebcam: Syntek USB2.0 Camera is now controlling video device /dev/video2
> stkwebcam: OmniVision sensor detected, id 9652 at address 60
> stkwebcam: frame 3, bytesused=2461154, skipping
> stkwebcam: frame 11, bytesused=1157638, skipping
> stkwebcam: frame 4, bytesused=2586972, skipping
> stkwebcam: frame 10, bytesused=2343188, skipping
> ehci_hcd 0000:00:03.3: remove, state 1
> usb usb4: USB disconnect, address 1
> usb 4-4: USB disconnect, address 2
> stkwebcam: Error (-19) re-submitting urb in stk_isoc_handler.
> stkwebcam: Syntek USB2.0 Camera release resourcesvideo device /dev/video2
> ehci_hcd 0000:00:03.3: USB bus 4 deregistered
> ehci_hcd 0000:00:03.3: PCI INT D disabled
> BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
> IP: [<ffffffff803e0660>] unlink1+0x30/0x170
> PGD 2b886067 PUD 2b875067 PMD 0
> Oops: 0000 [1] PREEMPT
> CPU 0
> Modules linked in: nouveau drm ppdev lp ipv6 fuse ipt_LOG ipt_recent nf_conntrack_ipv4 xt_state nf_conntrack xt_tcpudp iptable_filter ip_tables x_tables loop firewire_sbp2 joydev stkwebcam compat_ioctl32 videodev v4l1_compat arc4 ecb crypto_blkcipher cryptomgr crypto_algapi b43 rfkill rng_core mac80211 cfg80211 input_polldev irtty_sir sir_dev irda crc_ccitt pcspkr psmouse serio_raw k8temp r8169 bitrev snd_intel8x0 video output battery sdhci_pci sdhci snd_ac97_codec ac ac97_bus snd_pcm snd_timer mmc_core snd firewire_ohci firewire_core soundcore snd_page_alloc ssb crc_itu_t button ohci_hcd sg pcmcia sr_mod cdrom crc32 ext3 jbd mbcache sd_mod [last unloaded: ehci_hcd]
> Pid: 3422, comm: cheese Not tainted 2.6.27-rc5-video0 #1
> RIP: 0010:[<ffffffff803e0660>]  [<ffffffff803e0660>] unlink1+0x30/0x170
> RSP: 0018:ffff88002b8a3e58  EFLAGS: 00010206
> RAX: ffff88002ca0b800 RBX: ffff880032cf3a00 RCX: ffff880032cf3a00
> RDX: 00000000fffffffe RSI: ffff880032cf3a00 RDI: 0000000000000000
> RBP: 0000000000000000 R08: 0000000000000000 R09: ffff88003eeacef0
> R10: 0000000000000001 R11: 0000000000000206 R12: 00000000fffffffe
> R13: ffff88003f73f000 R14: ffff88003ed9db00 R15: 00007fd222fff940
> FS:  00007fd2284077e0(0000) GS:ffffffff805a8e80(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 00000000000000e8 CR3: 000000002b931000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process cheese (pid: 3422, threadinfo ffff88002b8a2000, task ffff8800365d38e0)
> Stack:  ffff880032cf3a00 0000000000000000 ffff88003ec24000 ffff88003f73f000
>  ffff88003ed9db00 ffffffff803e0949 ffff88002a4833f0 ffffffff803e1dd0
>  0000000000000246 ffff88002a462dc8 ffff88002a462dc8 ffff88002a4833f0
> Call Trace:
>  [<ffffffff803e0949>] usb_hcd_unlink_urb+0x19/0x30
>  [<ffffffff803e1dd0>] usb_kill_urb+0x50/0xe0
>  [<ffffffffa0204ab7>] stk_clean_iso+0x87/0xc0 [stkwebcam]
>  [<ffffffffa0206538>] v4l_stk_release+0x38/0x50 [stkwebcam]
>  [<ffffffff802a0bc1>] __fput+0xa1/0x1c0
>  [<ffffffff8029d7bb>] filp_close+0x5b/0x90
>  [<ffffffff8029d8a1>] sys_close+0xb1/0x140
>  [<ffffffff8020c37b>] system_call_fastpath+0x16/0x1b

The above stack trace is a result of a _close_ after _disconnect_.
Before my patch, it looks as if stk-webcam was able to detect that the
camera was already disconnected in the release function by examining
the stk_camera pointer and return an error. The above patch removed
this check as the pointer is still valid. If there really is nothing
for stk-webcam to do in this case, adding an if statement that
examines the state of the device using the is_present macro and
returning in v4l_stk_release should fix the above crash. Is this the
right thing to do though? Or does stk-webcam still have cleanup that
it needs to do before returning?

Under the circumstances, the urb error here is expected. It is a
result of the device physically being removed while stk-webcam is
trying to send it something. We know this to be the case as it is
immediately followed by a message that the video device has been
unregistered, which only happens once the device is disconnected.

I hope this clears up the issue for you. I noticed a small formatting
bug in the other output. I will correct it in the next patch. For
simplicity purposes, I'm going to split the above patch into 2. The
first will remove the reference count and the second will simplify
access to the stk_camera struct. Making this division, I believe the
second patch if kept the same as it is now would trigger the above
crash and requires a fix. How it should be fixed has yet to be
determined.

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: [PATCH RFT]: stk-webcam: release via video_device release callback

[David Ellingsworth]

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

Jamie,

After re-examining stk-webcam, I believe the crash you experienced
would occur despite having applied my patch. In the current version of
stk-webcam.c there are two if conditions at the very beginning of the
v4l_stk_release function that checks for NULL. In the case of a close
occuring after disconnect, neither of these variable will be NULL. The
sequence would be as follows:

v4l_stk_release
  stk_free_buffers
    stk_clean_iso
      usb_kill_urb
        (crash)

After the camera has been disconnected, usb_kill_urb should not be
called since usb_disconnect already takes care of killing all urbs
associated with a disconnected device. usb_free_urb however still
needs to be called for every urb allocated. Therefore I believe the
proper fix for this should be to checke that the camera is present
before calling usb_kill_urb.

Attached are a series of patches, the first of which should correct
the crash you experienced.

Regards,

David Ellingsworth

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-stkwebcam-fix-crash-on-disconnect-before-close.patch --]
[-- Type: text/x-diff; name=0001-stkwebcam-fix-crash-on-disconnect-before-close.patch, Size: 849 bytes --]

From 2caae22f93469976bfd1266895da39bcf7bfbbe2 Mon Sep 17 00:00:00 2001
From: David Ellingsworth <david@identd.dyndns.org>
Date: Thu, 25 Sep 2008 20:59:34 -0400
Subject: [PATCH] stkwebcam: fix crash on disconnect before close


Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/video/stk-webcam.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/stk-webcam.c b/drivers/media/video/stk-webcam.c
index 8dda568..bbf3677 100644
--- a/drivers/media/video/stk-webcam.c
+++ b/drivers/media/video/stk-webcam.c
@@ -576,7 +576,7 @@ static void stk_clean_iso(struct stk_camera *dev)
 
 		urb = dev->isobufs[i].urb;
 		if (urb) {
-			if (atomic_read(&dev->urbs_used))
+			if (atomic_read(&dev->urbs_used) && is_present(dev))
 				usb_kill_urb(urb);
 			usb_free_urb(urb);
 		}
-- 
1.5.6


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-stkwebcam-release-via-video_device-release-callback.patch --]
[-- Type: text/x-diff; name=0002-stkwebcam-release-via-video_device-release-callback.patch, Size: 4468 bytes --]

From 30088fc71cab5af069668a5629fc749d5713cac8 Mon Sep 17 00:00:00 2001
From: David Ellingsworth <david@identd.dyndns.org>
Date: Thu, 25 Sep 2008 21:00:21 -0400
Subject: [PATCH] stkwebcam: release via video_device release callback


Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/video/stk-webcam.c |   44 +++++++++++++++----------------------
 drivers/media/video/stk-webcam.h |    2 -
 2 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/media/video/stk-webcam.c b/drivers/media/video/stk-webcam.c
index bbf3677..7ad97c7 100644
--- a/drivers/media/video/stk-webcam.c
+++ b/drivers/media/video/stk-webcam.c
@@ -65,22 +65,6 @@ static struct usb_device_id stkwebcam_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, stkwebcam_table);
 
-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);
-	video_set_drvdata(&dev->vdev, NULL);
-
-	if (dev->sio_bufs != NULL || dev->isobufs != NULL)
-		STK_ERROR("We are leaking memory\n");
-	usb_put_intf(dev->interface);
-	kfree(dev);
-}
-
-
 /*
  * Basic stuff
  */
@@ -695,7 +679,6 @@ static int v4l_stk_open(struct inode *inode, struct file *fp)
 		return -ENXIO;
 	}
 	fp->private_data = vdev;
-	kref_get(&dev->kref);
 	usb_autopm_get_interface(dev->interface);
 	unlock_kernel();
 
@@ -720,7 +703,6 @@ static int v4l_stk_release(struct inode *inode, struct file *fp)
 
 	if (dev->owner != fp) {
 		usb_autopm_put_interface(dev->interface);
-		kref_put(&dev->kref, stk_camera_cleanup);
 		return 0;
 	}
 
@@ -731,7 +713,6 @@ static int v4l_stk_release(struct inode *inode, struct file *fp)
 	dev->owner = NULL;
 
 	usb_autopm_put_interface(dev->interface);
-	kref_put(&dev->kref, stk_camera_cleanup);
 
 	return 0;
 }
@@ -1359,6 +1340,12 @@ static const struct v4l2_ioctl_ops v4l_stk_ioctl_ops = {
 
 static void stk_v4l_dev_release(struct video_device *vd)
 {
+	struct stk_camera *dev = vdev_to_camera(vd);
+
+	if (dev->sio_bufs != NULL || dev->isobufs != NULL)
+		STK_ERROR("We are leaking memory\n");
+	usb_put_intf(dev->interface);
+	kfree(dev);
 }
 
 static struct video_device stk_v4l_data = {
@@ -1396,7 +1383,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);
@@ -1409,7 +1396,6 @@ static int stk_camera_probe(struct usb_interface *interface,
 		return -ENOMEM;
 	}
 
-	kref_init(&dev->kref);
 	spin_lock_init(&dev->spinlock);
 	init_waitqueue_head(&dev->wait_frame);
 
@@ -1442,8 +1428,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;
@@ -1457,14 +1443,17 @@ static int stk_camera_probe(struct usb_interface *interface,
 
 	err = stk_register_video_device(dev);
 	if (err) {
-		kref_put(&dev->kref, stk_camera_cleanup);
-		return err;
+		goto error;
 	}
 
 	stk_create_sysfs_files(&dev->vdev);
 	usb_autopm_enable(dev->interface);
 
 	return 0;
+
+error:
+	kfree(dev);
+	return err;
 }
 
 static void stk_camera_disconnect(struct usb_interface *interface)
@@ -1477,7 +1466,10 @@ static void stk_camera_disconnect(struct usb_interface *interface)
 	wake_up_interruptible(&dev->wait_frame);
 	stk_remove_sysfs_files(&dev->vdev);
 
-	kref_put(&dev->kref, stk_camera_cleanup);
+	STK_INFO("Syntek USB2.0 Camera release resources"
+		"video device /dev/video%d\n", dev->vdev.minor);
+
+	video_unregister_device(&dev->vdev);
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/media/video/stk-webcam.h b/drivers/media/video/stk-webcam.h
index df4dfef..084a85b 100644
--- a/drivers/media/video/stk-webcam.h
+++ b/drivers/media/video/stk-webcam.h
@@ -99,7 +99,6 @@ struct stk_camera {
 
 	u8 isoc_ep;
 
-	struct kref kref;
 	/* Not sure if this is right */
 	atomic_t urbs_used;
 
@@ -121,7 +120,6 @@ struct stk_camera {
 	unsigned sequence;
 };
 
-#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 *);
-- 
1.5.6


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-stkwebcam-simplify-access-to-stk_camera-struct.patch --]
[-- Type: text/x-diff; name=0003-stkwebcam-simplify-access-to-stk_camera-struct.patch, Size: 3169 bytes --]

From b5886307fe11ed3e7684137cfef2766dc6b11ec4 Mon Sep 17 00:00:00 2001
From: David Ellingsworth <david@identd.dyndns.org>
Date: Thu, 25 Sep 2008 21:00:57 -0400
Subject: [PATCH] stkwebcam: simplify access to stk_camera struct


Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/video/stk-webcam.c |   40 ++++---------------------------------
 1 files changed, 5 insertions(+), 35 deletions(-)

diff --git a/drivers/media/video/stk-webcam.c b/drivers/media/video/stk-webcam.c
index 7ad97c7..c3e7d66 100644
--- a/drivers/media/video/stk-webcam.c
+++ b/drivers/media/video/stk-webcam.c
@@ -678,7 +678,7 @@ static int v4l_stk_open(struct inode *inode, struct file *fp)
 		unlock_kernel();
 		return -ENXIO;
 	}
-	fp->private_data = vdev;
+	fp->private_data = dev;
 	usb_autopm_get_interface(dev->interface);
 	unlock_kernel();
 
@@ -687,19 +687,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);
@@ -723,14 +711,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;
@@ -789,15 +771,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;
 
@@ -835,16 +810,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;
@@ -1366,7 +1337,6 @@ static int stk_register_video_device(struct stk_camera *dev)
 	dev->vdev = stk_v4l_data;
 	dev->vdev.debug = debug;
 	dev->vdev.parent = &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");
-- 
1.5.6


[-- Attachment #5: 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: [PATCH RFT]: stk-webcam: release via video_device release callback

[David Ellingsworth]

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

I forgot to fix the typo that I saw in your output. Attached is the
updated patch with the typo corrected. It replaces the second patch in
the previous series.

Regards,

David Ellingsworth

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-stkwebcam-free-via-video_device-release-callback.patch --]
[-- Type: text/x-diff; name=0002-stkwebcam-free-via-video_device-release-callback.patch, Size: 4466 bytes --]

From bd669a31bc62761efd6c981887b61ecba6389850 Mon Sep 17 00:00:00 2001
From: David Ellingsworth <david@identd.dyndns.org>
Date: Fri, 26 Sep 2008 18:25:14 -0400
Subject: [PATCH] stkwebcam: free via video_device release callback


Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/video/stk-webcam.c |   44 +++++++++++++++----------------------
 drivers/media/video/stk-webcam.h |    2 -
 2 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/media/video/stk-webcam.c b/drivers/media/video/stk-webcam.c
index bbf3677..0a3a2de 100644
--- a/drivers/media/video/stk-webcam.c
+++ b/drivers/media/video/stk-webcam.c
@@ -65,22 +65,6 @@ static struct usb_device_id stkwebcam_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, stkwebcam_table);
 
-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);
-	video_set_drvdata(&dev->vdev, NULL);
-
-	if (dev->sio_bufs != NULL || dev->isobufs != NULL)
-		STK_ERROR("We are leaking memory\n");
-	usb_put_intf(dev->interface);
-	kfree(dev);
-}
-
-
 /*
  * Basic stuff
  */
@@ -695,7 +679,6 @@ static int v4l_stk_open(struct inode *inode, struct file *fp)
 		return -ENXIO;
 	}
 	fp->private_data = vdev;
-	kref_get(&dev->kref);
 	usb_autopm_get_interface(dev->interface);
 	unlock_kernel();
 
@@ -720,7 +703,6 @@ static int v4l_stk_release(struct inode *inode, struct file *fp)
 
 	if (dev->owner != fp) {
 		usb_autopm_put_interface(dev->interface);
-		kref_put(&dev->kref, stk_camera_cleanup);
 		return 0;
 	}
 
@@ -731,7 +713,6 @@ static int v4l_stk_release(struct inode *inode, struct file *fp)
 	dev->owner = NULL;
 
 	usb_autopm_put_interface(dev->interface);
-	kref_put(&dev->kref, stk_camera_cleanup);
 
 	return 0;
 }
@@ -1359,6 +1340,12 @@ static const struct v4l2_ioctl_ops v4l_stk_ioctl_ops = {
 
 static void stk_v4l_dev_release(struct video_device *vd)
 {
+	struct stk_camera *dev = vdev_to_camera(vd);
+
+	if (dev->sio_bufs != NULL || dev->isobufs != NULL)
+		STK_ERROR("We are leaking memory\n");
+	usb_put_intf(dev->interface);
+	kfree(dev);
 }
 
 static struct video_device stk_v4l_data = {
@@ -1396,7 +1383,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);
@@ -1409,7 +1396,6 @@ static int stk_camera_probe(struct usb_interface *interface,
 		return -ENOMEM;
 	}
 
-	kref_init(&dev->kref);
 	spin_lock_init(&dev->spinlock);
 	init_waitqueue_head(&dev->wait_frame);
 
@@ -1442,8 +1428,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;
@@ -1457,14 +1443,17 @@ static int stk_camera_probe(struct usb_interface *interface,
 
 	err = stk_register_video_device(dev);
 	if (err) {
-		kref_put(&dev->kref, stk_camera_cleanup);
-		return err;
+		goto error;
 	}
 
 	stk_create_sysfs_files(&dev->vdev);
 	usb_autopm_enable(dev->interface);
 
 	return 0;
+
+error:
+	kfree(dev);
+	return err;
 }
 
 static void stk_camera_disconnect(struct usb_interface *interface)
@@ -1477,7 +1466,10 @@ static void stk_camera_disconnect(struct usb_interface *interface)
 	wake_up_interruptible(&dev->wait_frame);
 	stk_remove_sysfs_files(&dev->vdev);
 
-	kref_put(&dev->kref, stk_camera_cleanup);
+	STK_INFO("Syntek USB2.0 Camera release resources "
+		"video device /dev/video%d\n", dev->vdev.minor);
+
+	video_unregister_device(&dev->vdev);
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/media/video/stk-webcam.h b/drivers/media/video/stk-webcam.h
index df4dfef..084a85b 100644
--- a/drivers/media/video/stk-webcam.h
+++ b/drivers/media/video/stk-webcam.h
@@ -99,7 +99,6 @@ struct stk_camera {
 
 	u8 isoc_ep;
 
-	struct kref kref;
 	/* Not sure if this is right */
 	atomic_t urbs_used;
 
@@ -121,7 +120,6 @@ struct stk_camera {
 	unsigned sequence;
 };
 
-#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 *);
-- 
1.5.6


[-- 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: [PATCH RFT]: stk-webcam: release via video_device release callback

[Jaime Velasco Juan]

Hi,

El jue. 25 de sep. de 2008, a las 21:49:14 -0400, David Ellingsworth escribió:
> Jamie,
> 
> After re-examining stk-webcam, I believe the crash you experienced
> would occur despite having applied my patch. In the current version of
> stk-webcam.c there are two if conditions at the very beginning of the
> v4l_stk_release function that checks for NULL. In the case of a close
> occuring after disconnect, neither of these variable will be NULL. The
> sequence would be as follows:
> 
> v4l_stk_release
>   stk_free_buffers
>     stk_clean_iso
>       usb_kill_urb
>         (crash)
> 
> After the camera has been disconnected, usb_kill_urb should not be
> called since usb_disconnect already takes care of killing all urbs
> associated with a disconnected device. usb_free_urb however still
> needs to be called for every urb allocated. Therefore I believe the
> proper fix for this should be to checke that the camera is present
> before calling usb_kill_urb.
> 

Yes, I also think that the bug was present in the driver before your patch.
With these new patches, however, it still crashes at a different point:
(sorry, the log is not complete)

[...]
stkwebcam: Syntek USB2.0 Camera is now controlling video device /dev/video3
stkwebcam: OmniVision sensor detected, id 9652 at address 60
stkwebcam: frame 9, bytesused=586798, skipping
stkwebcam: frame 12, bytesused=379780, skipping
stkwebcam: Frame buffer overflow, lost sync
stkwebcam: Frame buffer overflow, lost sync
stkwebcam: frame 13, bytesused=613382, skipping
ehci_hcd 0000:00:03.3: remove, state 1
usb usb2: USB disconnect, address 1
usb 2-4: USB disconnect, address 2
stkwebcam: Syntek USB2.0 Camera release resources video device /dev/video3
ehci_hcd 0000:00:03.3: USB bus 2 deregistered
ehci_hcd 0000:00:03.3: PCI INT D disabled
usb 1-2: new full speed USB device using ohci_hcd and address 6
usb 1-2: configuration #1 chosen from 1 choice
stkwebcam: Syntek USB2.0 Camera is now controlling video device /dev/video4
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff80492aca>] __mutex_lock_slowpath+0x3a/0x100
PGD 3ee2e067 PUD 2d1a5067 PMD 0 
Oops: 0002 [2] PREEMPT 
CPU 0 
Modules linked in: stkwebcam act_police sch_ingress cls_u32 sch_sfq sch_htb ppp_deflate zlib_deflate zlib_inflate bsd_comp ppp_async nouveau drm ppdev lp ipv6 ppp_generic slhc option usbserial fuse ipt_LOG ipt_recent nf_conntrack_ipv4 xt_state nf_conntrack xt_tcpudp iptable_filter ip_tables x_tables loop firewire_sbp2 usb_storage compat_ioctl32 videodev v4l1_compat arc4 ecb crypto_blkcipher cryptomgr crypto_algapi b43 rfkill rng_core joydev mac80211 cfg80211 input_polldev irtty_sir sir_dev irda crc_ccitt pcspkr psmouse serio_raw k8temp r8169 bitrev video output battery ac sdhci_pci sdhci mmc_core snd_intel8x0 snd_ac97_codec ac97_bus snd_pcm snd_timer button firewire_ohci firewire_core snd crc_itu_t ssb ohci_hcd soundcore snd_page_alloc sg pcmcia sr_mod cdrom crc32 ext3 jbd mbcache sd_mod [last unloaded: ehci_hcd]
Pid: 5303, comm: xawtv.bin Tainted: G      D   2.6.27-rc5-video0 #1
RIP: 0010:[<ffffffff80492aca>]  [<ffffffff80492aca>] __mutex_lock_slowpath+0x3a/0x100
RSP: 0018:ffff88002b161e70  EFLAGS: 00010202
RAX: ffff88002b161fd8 RBX: ffff880033e77d80 RCX: ffffffffa0213080
RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffff880033e77d80
RBP: ffff880033c045b0 R08: 0000000000000000 R09: ffff880001101040
R10: 0000000000000002 R11: 0000000000000000 R12: 00000000ffffffed
R13: ffff880033e77d88 R14: 00000000ffffffff R15: ffff880033e77d80
FS:  00007fc629032740(0000) GS:ffffffff805a8e80(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 0000000025af4000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process xawtv.bin (pid: 5303, threadinfo ffff88002b160000, task ffff880033c045b0)
Stack:  ffff880033e77d88 0000000000000000 ffff88002d173fc0 ffff88003f6af258
 0000000000000282 ffff88003e94d000 ffff880033e77888 00000000ffffffed
 ffff880033e77800 ffffffff804928b9 ffffffff803e548b 0000000000000100
Call Trace:
 [<ffffffff804928b9>] ? mutex_lock+0x9/0x10
 [<ffffffff803e548b>] ? usb_autopm_do_interface+0x4b/0x120
 [<ffffffffa021450d>] ? v4l_stk_release+0x1d/0x50 [stkwebcam]
 [<ffffffff802a0bc1>] ? __fput+0xa1/0x1c0
 [<ffffffff8029d7bb>] ? filp_close+0x5b/0x90
 [<ffffffff8029d8a1>] ? sys_close+0xb1/0x140
 [<ffffffff8020c37b>] ? system_call_fastpath+0x16/0x1b


Code: 8b 2c 25 00 00 00 00 65 48 8b 04 25 10 00 00 00 ff 80 48 e0 ff ff 48 8b 57 10 4c 8d 6f 08 48 89 63 10 4c 89 2c 24 48 89 54 24 08 <48> 89 22 48 c7 c2 ff ff ff ff 48 89 6c 24 10 48 89 d0 87 07 ff 
RIP  [<ffffffff80492aca>] __mutex_lock_slowpath+0x3a/0x100
 RSP <ffff88002b161e70>
CR2: 0000000000000000
---[ end trace bfdcde72f302e116 ]---
note: xawtv.bin[5303] exited with preempt_count 1
BUG: scheduling while atomic: xawtv.bin/5303/0x10000002
Modules linked in: stkwebcam act_police sch_ingress cls_u32 sch_sfq sch_htb ppp_deflate zlib_deflate zlib_inflate bsd_comp ppp_async nouveau drm ppdev lp ipv6 ppp_generic slhc option usbserial fuse ipt_LOG ipt_recent nf_conntrack_ipv4 xt_state nf_conntrack xt_tcpudp iptable_filter ip_tables x_tables loop firewire_sbp2 usb_storage compat_ioctl32 videodev v4l1_compat arc4 ecb crypto_blkcipher cryptomgr crypto_algapi b43 rfkill rng_core joydev mac80211 cfg80211 input_polldev irtty_sir sir_dev irda crc_ccitt pcspkr psmouse serio_raw k8temp r8169 bitrev video output battery ac sdhci_pci sdhci mmc_core snd_intel8x0 snd_ac97_codec ac97_bus snd_pcm snd_timer button firewire_ohci firewire_core snd crc_itu_t ssb ohci_hcd soundcore snd_page_alloc sg pcmcia sr_mod cdrom crc32 ext3 jbd mbcache sd_mod [last unloaded: ehci_hcd]
Pid: 5303, comm: xawtv.bin Tainted: G      D   2.6.27-rc5-video0 #1

Call Trace:
 [<ffffffff80491da0>] thread_return+0xd8/0x268
 [<ffffffff802b3be7>] d_kill+0x47/0x70
 [<ffffffff802b46d6>] dput+0xa6/0x210
 [<ffffffff8022c600>] __cond_resched+0x20/0x50
 [<ffffffff80491ff5>] _cond_resched+0x35/0x50
 [<ffffffff8023415a>] put_files_struct+0x7a/0xe0
 [<ffffffff80236299>] do_exit+0x789/0x970
 [<ffffffff80494574>] oops_end+0x74/0x80
 [<ffffffff804962bf>] do_page_fault+0x2df/0xa60
 [<ffffffff8040950e>] sock_aio_read+0x19e/0x1b0
 [<ffffffff80493fa9>] error_exit+0x0/0x51
 [<ffffffffa0213080>] stk_free_sio_buffers+0x110/0x160 [stkwebcam]
 [<ffffffff80492aca>] __mutex_lock_slowpath+0x3a/0x100
 [<ffffffff804928b9>] mutex_lock+0x9/0x10
 [<ffffffff803e548b>] usb_autopm_do_interface+0x4b/0x120
 [<ffffffffa021450d>] v4l_stk_release+0x1d/0x50 [stkwebcam]
 [<ffffffff802a0bc1>] __fput+0xa1/0x1c0
 [<ffffffff8029d7bb>] filp_close+0x5b/0x90
 [<ffffffff8029d8a1>] sys_close+0xb1/0x140
 [<ffffffff8020c37b>] system_call_fastpath+0x16/0x1b

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff80492aca>] __mutex_lock_slowpath+0x3a/0x100
PGD 2d2a0067 PUD 2d17e067 PMD 0 
Oops: 0002 [3] PREEMPT 
CPU 0 
Modules linked in: stkwebcam act_police sch_ingress cls_u32 sch_sfq sch_htb ppp_deflate zlib_deflate zlib_inflate bsd_comp ppp_async nouveau drm ppdev lp ipv6 ppp_generic slhc option usbserial fuse ipt_LOG ipt_recent nf_conntrack_ipv4 xt_state nf_conntrack xt_tcpudp iptable_filter ip_tables x_tables loop firewire_sbp2 usb_storage compat_ioctl32 videodev v4l1_compat arc4 ecb crypto_blkcipher cryptomgr crypto_algapi b43 rfkill rng_core joydev mac80211 cfg80211 input_polldev irtty_sir sir_dev irda crc_ccitt pcspkr psmouse serio_raw k8temp r8169 bitrev video output battery ac sdhci_pci sdhci mmc_core snd_intel8x0 snd_ac97_codec ac97_bus snd_pcm snd_timer button firewire_ohci firewire_core snd crc_itu_t ssb ohci_hcd soundcore snd_page_alloc sg pcmcia sr_mod cdrom crc32 ext3 jbd mbcache sd_mod [last unloaded: ehci_hcd]
Pid: 5177, comm: xawtv.bin Tainted: G      D   2.6.27-rc5-video0 #1
RIP: 0010:[<ffffffff80492aca>]  [<ffffffff80492aca>] __mutex_lock_slowpath+0x3a/0x100
RSP: 0018:ffff880026131e70  EFLAGS: 00010202
RAX: ffff880026131fd8 RBX: ffff88003fbc7d80 RCX: ffffffffa0213080
RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffff88003fbc7d80
RBP: ffff88002d1b9c70 R08: 0000000000000000 R09: ffff880001101040
R10: 0000000000000002 R11: 0000000000000000 R12: 00000000ffffffed
R13: ffff88003fbc7d88 R14: 00000000ffffffff R15: ffff88003fbc7d80
FS:  00007f1eb5e84740(0000) GS:ffffffff805a8e80(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 000000002d17b000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process xawtv.bin (pid: 5177, threadinfo ffff880026130000, task ffff88002d1b9c70)
Stack:  ffff88003fbc7d88 0000000000000000 ffff88003705c280 ffff88003c3e3578
 0000000000000282 ffff88003e951800 ffff88003fbc7888 00000000ffffffed
 ffff88003fbc7800 ffffffff804928b9 ffffffff803e548b 0000000000000100
Call Trace:
 [<ffffffff804928b9>] ? mutex_lock+0x9/0x10
 [<ffffffff803e548b>] ? usb_autopm_do_interface+0x4b/0x120
 [<ffffffffa021450d>] ? v4l_stk_release+0x1d/0x50 [stkwebcam]
 [<ffffffff802a0bc1>] ? __fput+0xa1/0x1c0
 [<ffffffff8029d7bb>] ? filp_close+0x5b/0x90
 [<ffffffff8029d8a1>] ? sys_close+0xb1/0x140
 [<ffffffff8020c37b>] ? system_call_fastpath+0x16/0x1b


Code: 8b 2c 25 00 00 00 00 65 48 8b 04 25 10 00 00 00 ff 80 48 e0 ff ff 48 8b 57 10 4c 8d 6f 08 48 89 63 10 4c 89 2c 24 48 89 54 24 08 <48> 89 22 48 c7 c2 ff ff ff ff 48 89 6c 24 10 48 89 d0 87 07 ff 
RIP  [<ffffffff80492aca>] __mutex_lock_slowpath+0x3a/0x100
 RSP <ffff880026131e70>
CR2: 0000000000000000
---[ end trace bfdcde72f302e116 ]---

The following patch seems to fix this crash. What do you think about it?

Regards.

>From 3f8ffca543dfb32b011888a7cd1659efb5036556 Mon Sep 17 00:00:00 2001
From: Jaime Velasco Juan <jsagarribay@gmail.com>
Date: Sun, 28 Sep 2008 16:19:54 +0100
Subject: [PATCH] stkwebcam: Check device is present before modifying autopm status

Signed-off-by: Jaime Velasco Juan <jsagarribay@gmail.com>
---
 drivers/media/video/stk-webcam.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/media/video/stk-webcam.c b/drivers/media/video/stk-webcam.c
index 5c62280..1a1b1d5 100644
--- a/drivers/media/video/stk-webcam.c
+++ b/drivers/media/video/stk-webcam.c
@@ -689,18 +689,14 @@ static int v4l_stk_release(struct inode *inode, struct file *fp)
 {
 	struct stk_camera *dev = fp->private_data;
 
-	if (dev->owner != fp) {
-		usb_autopm_put_interface(dev->interface);
-		return 0;
+	if (dev->owner == fp) {
+		stk_stop_stream(dev);
+		stk_free_buffers(dev);
+		dev->owner = NULL;
 	}
 
-	stk_stop_stream(dev);
-
-	stk_free_buffers(dev);
-
-	dev->owner = NULL;
-
-	usb_autopm_put_interface(dev->interface);
+	if (is_present(dev))
+		usb_autopm_put_interface(dev->interface);
 
 	return 0;
 }
-- 
1.5.6.5

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