small patch: enable pwc usb camera driver

PATCH Kai Engert 3 participants 6 messages Jun 7, 2004 – Jun 17, 2004 USB

Share & tools

Kai Engert

author

2004-06-07 13:05 UTC #0 lore raw

The attached patch enables the pwc driver included with kernel 2.6.7-rc2

It also removes the warnings during compilation.
However, note that I blindly duplicated the release approach used by other usb camera drivers, replacing the current no-op.

The driver works for me with a Logitech QuickCam Notebook Pro and GnomeMeeting.

Regards,
Kai

drivers/usb/media/Kconfig renamed from linux-2.6.7-rc2/drivers/usb/media/Kconfig +1 -1

diff -ruw org/linux-2.6.7-rc2/drivers/usb/media/Kconfig linux-2.6.7-rc2/drivers/usb/media/Kconfig
@@ -108,7 +108,7 @@
 
 config USB_PWC
 	tristate "USB Philips Cameras"
-	depends on USB && VIDEO_DEV && BROKEN
+	depends on USB && VIDEO_DEV
 	---help---
 	  Say Y or M here if you want to use one of these Philips & OEM
           webcams:

drivers/usb/media/pwc-if.c renamed from linux-2.6.7-rc2/drivers/usb/media/pwc-if.c +1 -8

diff -ruw org/linux-2.6.7-rc2/drivers/usb/media/pwc-if.c linux-2.6.7-rc2/drivers/usb/media/pwc-if.c
@@ -129,7 +129,6 @@
 
 static int pwc_video_open(struct inode *inode, struct file *file);
 static int pwc_video_close(struct inode *inode, struct file *file);
-static int pwc_video_release(struct video_device *);			  
 static ssize_t pwc_video_read(struct file *file, char *buf,
 			  size_t count, loff_t *ppos);
 static unsigned int pwc_video_poll(struct file *file, poll_table *wait);

@@ -1121,12 +1120,6 @@
 	return 0;
 }
 
-static int pwc_video_release(struct video_device *vfd)
-{
-	Trace(TRACE_OPEN, "pwc_video_release() called. Now what?\n");
-}
-		
-
 /*
  *	FIXME: what about two parallel reads ????
  *      ANSWER: Not supported. You can't open the device more than once,

@@ -1855,7 +1848,7 @@
 		}
 	}
 
-	pdev->vdev.release = pwc_video_release;
+	pdev->vdev.release = video_device_release;
 	i = video_register_device(&pdev->vdev, VFL_TYPE_GRABBER, video_nr);
 	if (i < 0) {
 		Err("Failed to register as video device (%d).\n", i);

Greg KH

2004-06-07 20:37 UTC #1 lore raw

Quoted 8 lines · Kai Engert

The attached patch enables the pwc driver included with kernel 2.6.7-rc2
It also removes the warnings during compilation.
However, note that I blindly duplicated the release approach used by other usb camera drivers, replacing the current no-op.
The driver works for me with a Logitech QuickCam Notebook Pro and GnomeMeeting.

Nice, thanks, I've applied this. It's amazing how long it took for this to be fixed... :(

Again, thanks a lot for doing this.

greg k-h

Nemosoft Unv.

2004-06-07 22:27 UTC #2 lore raw

Quoted 4 lines · Greg KH

Quoted 3 lines · Kai Engert
The attached patch enables the pwc driver included with kernel 2.6.7-rc2

Quoted 8 lines

Quoted 6 lines
It also removes the warnings during compilation.
However, note that I blindly duplicated the release approach used by other usb camera drivers, replacing the current no-op.
The driver works for me with a Logitech QuickCam Notebook Pro and GnomeMeeting.
Nice, thanks, I've applied this.

Don't use this. It will BUG() your kernel hard, because of a double free().

Quoted 1 line

It's amazing how long it took for this to be fixed... :(

I could start a big *bleep*ing rant about this, but I´ll save that for some other time.

- Nemosoft

Greg KH

2004-06-07 22:40 UTC #3 lore raw

Quoted 9 lines · Nemosoft Unv.

Quoted 8 lines · Greg KH
Quoted 7 lines · Kai Engert
The attached patch enables the pwc driver included with kernel 2.6.7-rc2
It also removes the warnings during compilation.
However, note that I blindly duplicated the release approach used by other usb camera drivers, replacing the current no-op.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
The driver works for me with a Logitech QuickCam Notebook Pro and GnomeMeeting.
Nice, thanks, I've applied this.
Don't use this. It will BUG() your kernel hard, because of a double free().

Oops, oh well, the pwc driver is really messed up then...
I already sent this to Linus, and as no one has fixed this issue up in a long time, I'll wait for someone to send another patch fixing this one as it's obvious that this driver does not have many users :(

Quoted 4 lines

Quoted 1 line
It's amazing how long it took for this to be fixed... :(
I could start a big *bleep*ing rant about this, but I?ll save that for some other time.

Heh, we've discussed this already in private, I'll be glad to discuss it again, any time, in public on any mailing list.

thanks,

greg k-h

Kai Engert

author

2004-06-17 12:37 UTC #4 lore raw

Quoted 2 lines · Greg KH

Quoted 1 line · Nemosoft Unv.
... Don't use this. It will BUG() your kernel hard, because of a double free().

I confirm, if you disconnect the device / unload the driver, it doesn't take long and the kernel reports an exception. My fault, sorry.

Quoted 1 line

... I'll wait for someone to send another patch fixing this one ...

I'm attaching a new patch:

- revert video device release code to original no-op, using correct function signature.

With this patch applied to 2.6.7, I am able to connect/disconnect the camera, load/unload the driver 20 times and everything still works fine.

No warnings during compilation (which was the original reason to mark the driver broken, IIUC).

Thanks and Regards,
Kai

drivers/usb/media/pwc-if.c +8 -1

diff -ruw org-267/drivers/usb/media/pwc-if.c linux-2.6.7/drivers/usb/media/pwc-if.c
@@ -129,6 +129,7 @@
 
 static int pwc_video_open(struct inode *inode, struct file *file);
 static int pwc_video_close(struct inode *inode, struct file *file);
+static void pwc_video_release(struct video_device *);
 static ssize_t pwc_video_read(struct file *file, char *buf,
 			  size_t count, loff_t *ppos);
 static unsigned int pwc_video_poll(struct file *file, poll_table *wait);

@@ -1120,6 +1121,12 @@
 	return 0;
 }
 
+static void pwc_video_release(struct video_device *vfd)
+{
+	/* Do nothing to prevent a double free */
+	Trace(TRACE_OPEN, "pwc_video_release() called.\n");
+}
+
 /*
  *	FIXME: what about two parallel reads ????
  *      ANSWER: Not supported. You can't open the device more than once,

@@ -1848,7 +1855,7 @@
 		}
 	}
 
-	pdev->vdev.release = video_device_release;
+	pdev->vdev.release = pwc_video_release;
 	i = video_register_device(&pdev->vdev, VFL_TYPE_GRABBER, video_nr);
 	if (i < 0) {
 		Err("Failed to register as video device (%d).\n", i);

Greg KH

2004-06-17 20:23 UTC #5 lore raw

Quoted 9 lines · Kai Engert

Quoted 4 lines · Greg KH
Quoted 3 lines · Nemosoft Unv.
...
Don't use this. It will BUG() your kernel hard, because of a double free().
I confirm, if you disconnect the device / unload the driver, it doesn't take long and the kernel reports an exception. My fault, sorry.

Quoted 6 lines

Quoted 1 line
... I'll wait for someone to send another patch fixing this one ...
I'm attaching a new patch:
- revert video device release code to original no-op, using correct function signature.

Ick ick ick! No, that's not the way to fix this.

Bah, time to mark the driver broken again...

greg k-h