USB fixes for 2.6.7-rc3

Hi,

Here are three bugfixes for the USB code against 2.6.7-rc3. They do the following:
- fix deadlock in usbfs
- fix bug and enable the pwc USB driver to build
- fix bugs in cyberjack driver to enable device to work.

The first one is the most serious, but the others are good things to have fixed in the final 2.6.7 release.

Please pull from:

	bk://kernel.bkbits.net/gregkh/linux/usb-fix-2.6

Patches will be posted to linux-usb-devel as a follow-up thread for those who want to see them.

thanks,

greg k-h

 drivers/usb/core/devio.c       |    2 +-
 drivers/usb/media/Kconfig      |    2 +-
 drivers/usb/media/pwc-if.c     |    9 +--------
 drivers/usb/serial/cyberjack.c |   21 ++++++---------------
 include/linux/usb.h            |    1 +
 5 files changed, 10 insertions(+), 25 deletions(-)

-----

<kaie:kuix.de>:
o USB: enable pwc usb camera driver

<siegfried.hildebrand:fernuni-hagen.de>:
o USB: Fix problems with cyberjack usb-serial-module since kernel 2.6.2

Duncan Sands:
o USB devio.c: deadlock fix

Hi,

Ok, thanks to both of you posting bug reports that seemed quite different, I think I've finally fixed both of your issues. The patch below is what I've just added to my trees and will send to Linus in a bit, and should solve both problems.

Basically the issue was 2 things:
- Zwane correctly found that we shouldn't have been calling the usb_driver_release_interface() call on disconnect, but if you didn't make this call, we leaked memory. This was because of the next piece...
- Harald noticed that if you unloaded a usb-serial driver with the device still plugged in, and then removed it, the kernel oopsed. He also noticed double calls to the disconnect function. This was because we were incorrectly binding the device to the usb serial generic driver instead of the one that was controlling it.

So, by fixing the usb-serial generic issue, that fixed the fact that we shouldn't have been calling the release_interface() call, as it isn't necessary (the usb core will take care of it.)

Thanks to everyone for helping out here, and if with this patch, you still have problems, please let me know...

/me crosses his fingers

greg k-h

USB: fix bug where removing usb-serial modules or usb serial devices could oops

This fixes the issue where the Generic driver would bind to all usb-serial devices, so the disconnect would not properly go to the real driver that controlled the device. This was very bad when unloading the module with the device still connected.

diff -Nru a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
@@ -53,6 +53,32 @@
 	.num_ports =		1,
 	.shutdown =		usb_serial_generic_shutdown,
 };
+
+/* we want to look at all devices, as the vendor/product id can change
+ * depending on the command line argument */
+static struct usb_device_id generic_serial_ids[] = {
+	{.driver_info = 42},
+	{}
+};
+
+static int generic_probe(struct usb_interface *interface,
+			       const struct usb_device_id *id)
+{
+	const struct usb_device_id *id_pattern;
+
+	id_pattern = usb_match_id(interface, generic_device_ids);
+	if (id_pattern != NULL)
+		return usb_serial_probe(interface, id);
+	return -ENODEV;
+}
+
+static struct usb_driver generic_driver = {
+	.owner =	THIS_MODULE,
+	.name =		"usbserial_generic",
+	.probe =	generic_probe,
+	.disconnect =	usb_serial_disconnect,
+	.id_table =	generic_serial_ids,
+};
 #endif
 
 int usb_serial_generic_register (int _debug)

@@ -67,6 +93,12 @@
 
 	/* register our generic driver with ourselves */
 	retval = usb_serial_register (&usb_serial_generic_device);
+	if (retval)
+		goto exit;
+	retval = usb_register(&generic_driver);
+	if (retval)
+		usb_serial_deregister(&usb_serial_generic_device);
+exit:
 #endif
 	return retval;
 }

@@ -75,6 +107,7 @@
 {
 #ifdef CONFIG_USB_SERIAL_GENERIC
 	/* remove our generic driver */
+	usb_deregister(&generic_driver);
 	usb_serial_deregister (&usb_serial_generic_device);
 #endif
 }

diff -Nru a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
@@ -355,25 +355,12 @@
 #define DRIVER_DESC "USB Serial Driver core"
 
 
-#ifdef CONFIG_USB_SERIAL_GENERIC
-/* we want to look at all devices, as the vendor/product id can change
- * depending on the command line argument */
-static struct usb_device_id generic_serial_ids[] = {
-	{.driver_info = 42},
-	{}
-};
-
-#endif /* CONFIG_USB_SERIAL_GENERIC */
-
 /* Driver structure we register with the USB core */
 static struct usb_driver usb_serial_driver = {
 	.owner =	THIS_MODULE,
 	.name =		"usbserial",
 	.probe =	usb_serial_probe,
 	.disconnect =	usb_serial_disconnect,
-#ifdef CONFIG_USB_SERIAL_GENERIC
-	.id_table =	generic_serial_ids,
-#endif
 };
 
 /* There is no MODULE_DEVICE_TABLE for usbserial.c.  Instead

@@ -1383,22 +1370,9 @@
 
 void usb_serial_deregister(struct usb_serial_device_type *device)
 {
-	struct usb_serial *serial;
-	int i;
-
 	info("USB Serial deregistering driver %s", device->name);
-
-	/* clear out the serial_table if the device is attached to a port */
-	for(i = 0; i < SERIAL_TTY_MINORS; ++i) {
-		serial = serial_table[i];
-		if ((serial != NULL) && (serial->type == device)) {
-			usb_driver_release_interface (&usb_serial_driver, serial->interface);
-			usb_serial_disconnect (serial->interface);
-		}
-	}
-
 	list_del(&device->driver_list);
-	usb_serial_bus_deregister (device);
+	usb_serial_bus_deregister(device);
 }
 
 

Great, thanks Greg, this one works for me.

Loading the ipaq module, connecting a device and then unloading ipaq.ko oopses.

PocketPC PDA ttyUSB0: PocketPC PDA converter now disconnected from ttyUSB0 usbserial 1-2:1.0: device disconnected

Unable to handle kernel NULL pointer dereference at virtual address 00000085
 printing eip:
f8990a6f
*pde = 00000000
Oops: 0000 [#1]
SMP
Modules linked in: ppp_async ppp_generic slhc ipaq usbserial vmnet vmmon
dm_mod e100 e1000 3c59x
CPU:    0
EIP:    0060:[<f8990a6f>]    Tainted: P   VLI
EFLAGS: 00210246   (2.6.7-rc3-mm2-slock)

EIP is at usb_serial_disconnect+0x1b/0x86 [usbserial]
eax: 00000000 ebx: 00000011 ecx: 00000002 edx: 00000000
esi: 00000000 edi: 00000001 ebp: d9d0c000 esp: d9d0cf38
ds: 007b es: 007b ss: 0068
Process rmmod (pid: 19148, threadinfo=d9d0c000 task=e62ced10)

Stack: f8993f40 c02ef8f6 c1d61b48 c1ec46d8 00000000 f8998220 f8990e04 f89924c0
       f8991e8c f8996cc8 f8998480 c04631a4 00000000 c0128b94 00000000 71617069
       00000000 d5d33a80 d5d33a80 c013cac9 40001000 40000000 c013ce17 40000000
Call Trace:
 [<c02ef8f6>] device_release_driver+0x56/0x58
 [<f8990e04>] usb_serial_deregister+0x9b/0x9f [usbserial]
 [<c0128b94>] sys_delete_module+0x132/0x180
 [<c013cac9>] unmap_vma_list+0xe/0x17
 [<c013ce17>] do_munmap+0x10a/0x144
 [<c0110df4>] do_page_fault+0x0/0x501
 [<c0103a71>] sysenter_past_esp+0x52/0x71

Code: 04 24 b3 1d 99 f8 e8 89 65 78 c7 e9 8a f5 ff ff 83 ec 18 89 5c 24 0c

89 7c 24 14 89 74 24 10 8d 58 10 89 c7 a1 04 46 99 f8 85 c0 <8b> 73 74 75 48 85 f6 c7 43 74 00 00 00 00 74 08 8d 46 38 e8 47

(gdb) list * usb_serial_disconnect+0x1b 0x1a6f is in usb_serial_disconnect (device.h:298).
293 }
294

295     static inline void *
296     dev_get_drvdata (struct device *dev)
297     {

298 return dev->driver_data;
299 }

The problem is due to the following;

void usb_serial_deregister(struct usb_serial_device_type *device)

{
	struct usb_serial *serial;
	int i;

	for(i = 0; i < SERIAL_TTY_MINORS; ++i) {
		serial = serial_table[i];
		if ((serial != NULL) && (serial->type == device)) {
			printk("usb_serial_deregister: %p %p\n", serial, serial->interface);
			usb_driver_release_interface (&usb_serial_driver, serial->interface);
			usb_serial_disconnect (serial->interface); <===
		}
	}

...
}

It's not safe to use serial->interface after that usb_driver_release_interface(). The following patch works as a workaround but i don't trust it as there may be a leak.

Index: linux-2.6.7/drivers/usb/serial/usb-serial.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.7/drivers/usb/serial/usb-serial.c,v retrieving revision 1.1.1.1

diff -u -p -B -r1.1.1.1 usb-serial.c
@@ -1393,7 +1393,6 @@ void usb_serial_deregister(struct usb_se
 		serial = serial_table[i];
 		if ((serial != NULL) && (serial->type == device)) {
 			usb_driver_release_interface (&usb_serial_driver, serial->interface);
-			usb_serial_disconnect (serial->interface);
 		}
 	}
 

Why not? The ->interface pointer is still valid, only thing is that device does not have a driver bound to it anymore, so the later call to dev_info() in the usb_serial_disconnect() call might cause the oops.

How about just switching those two calls around (usb_serial_disconnect() before calling usb_driver_release_interface())? Will that solve the problem for you? If you just comment out usb_serial_disconnect() you will leak memory :(

thanks,

greg k-h

void usb_driver_release_interface(struct usb_driver *driver,
struct usb_interface *iface)

{
	struct device *dev = &iface->dev;

...

	dev->driver = NULL;
	usb_set_intfdata(iface, NULL);
}

usb_set_intfdata() sets usb_interface->dev.driver_data to NULL, whilst in usbserial_disconnect() we try and retrieve and use it;

void usb_serial_disconnect(struct usb_interface *interface)

{
	struct usb_serial *serial = usb_get_intfdata (interface);
	struct device *dev = &interface->dev;

...
}

I tried it last night, switching around will cause an oops in usb_driver_release_interface()

Unable to handle kernel paging request at virtual address 6b6b6beb
 printing eip:
c042da9a
*pde = 00000000
Oops: 0000 [#1]
PREEMPT SMP DEBUG_PAGEALLOC
Modules linked in: ipaq usbserial
CPU:    1
EIP:    0060:[<c042da9a>]    Not tainted
EFLAGS: 00010246   (2.6.7)

EIP is at usb_driver_release_interface+0xa/0x50
eax: 6b6b6b6b ebx: 6b6b6b7b ecx: 6b6b6b6b edx: de0a5000
esi: 00000000 edi: e0ddda60 ebp: de0a5f34 esp: de0a5f30
ds: 007b es: 007b ss: 0068
Process rmmod (pid: 1414, threadinfo=de0a5000 task=d5c85a60)

Stack: dee0bec0 de0a5f54 e0e1b11c e0e1f1c0 6b6b6b6b d72baef8 e0dddcc0 c05dd3a8
       00000000 de0a5f64 e0ddbea7 e0ddda60 e0ddd9c0 de0a5fbc c0139c9e 00000000
       71617069 40000000 de0a5fa0 c0156f3d d647cdc8 de801a30 d647cdfc 40001000
Call Trace:
 [<c0107675>] show_stack+0x75/0x90
 [<c01077d5>] show_registers+0x125/0x180
 [<c010796b>] die+0xab/0x170
 [<c01185d8>] do_page_fault+0x1e8/0x535
 [<c01072cd>] error_code+0x2d/0x40
 [<e0e1b11c>] usb_serial_deregister+0x7c/0x90 [usbserial]
 [<e0ddbea7>] ipaq_exit+0x17/0x1b [ipaq]
 [<c0139c9e>] sys_delete_module+0x12e/0x190
 [<c0106139>] sysenter_past_esp+0x52/0x79

(gdb) list *usb_driver_release_interface+0xa
0x31a is in usb_driver_release_interface (drivers/usb/core/usb.c:347).
342 struct usb_interface *iface)
343 {
344 struct device *dev = &iface->dev;
345
346 /* this should never happen, don't release something that's not ours */
347 if (!dev->driver || dev->driver != &driver->driver)
348 return;
349
350 /* don't disconnect from disconnect(), or before dev_add() */
351 if (!list_empty (&dev->driver_list) && !list_empty (&dev->bus_list))

0x00000310 <usb_driver_release_interface+0>:    push   %ebp
0x00000311 <usb_driver_release_interface+1>:    mov    %esp,%ebp
0x00000313 <usb_driver_release_interface+3>:    push   %ebx
0x00000314 <usb_driver_release_interface+4>:    mov    0xc(%ebp),%ecx
0x00000317 <usb_driver_release_interface+7>:    lea    0x10(%ecx),%ebx
0x0000031a <usb_driver_release_interface+10>:   mov    0x70(%ebx),%edx
0x0000031d <usb_driver_release_interface+13>:   test   %edx,%edx

hmm, iface = %ebp, dev = %ecx = 0x6b6b6b6b, how did that get freed/poisoned since it's encapsulated by iface?

Ta,
Zwane