[PATCH] USB: lots of locking and other SMP race fixes

This is a merge of a bunch of SMP and locking fixes for the USB code
that Oliver has sent me (greg k-h) over the past few weeks.

drivers/usb/class/printer.c

@@ -655,7 +655,10 @@ static ssize_t usblp_write(struct file *file, const char *buffer, size_t count,
 							  (count - writecount) : USBLP_BUF_SIZE;
 
 		if (copy_from_user(usblp->writeurb->transfer_buffer, buffer + writecount,
-				usblp->writeurb->transfer_buffer_length)) return -EFAULT;
+				usblp->writeurb->transfer_buffer_length)) {
+			up(&usblp->sem);
+			return writecount ? writecount : -EFAULT;
+		}
 
 		usblp->writeurb->dev = usblp->dev;
 		usblp->wcomplete = 0;

drivers/usb/core/devices.c

@@ -152,8 +152,8 @@ static const struct class_info clas_info[] =
 
 void usbdevfs_conn_disc_event(void)
 {
-	wake_up(&deviceconndiscwq);
 	conndiscevcnt++;
+	wake_up(&deviceconndiscwq);
 }
 
 static const char *class_decode(const int class)
@@ -239,6 +239,7 @@ static char *usb_dump_interface_descriptor(char *start, char *end, const struct
 
 	if (start > end)
 		return start;
+	lock_kernel(); /* driver might be unloaded */
 	start += sprintf(start, format_iface,
 			 desc->bInterfaceNumber,
 			 desc->bAlternateSetting,
@@ -248,6 +249,7 @@ static char *usb_dump_interface_descriptor(char *start, char *end, const struct
 			 desc->bInterfaceSubClass,
 			 desc->bInterfaceProtocol,
 			 iface->driver ? iface->driver->name : "(none)");
+	unlock_kernel();
 	return start;
 }
 
@@ -597,6 +599,13 @@ static unsigned int usb_device_poll(struct file *file, struct poll_table_struct
 			unlock_kernel();
 			return POLLIN;
 		}
+		
+		/* we may have dropped BKL - need to check for having lost the race */
+		if (file->private_data) {
+			kfree(st);
+			goto lost_race;
+		}
+
 		/*
 		 * need to prevent the module from being unloaded, since
 		 * proc_unregister does not call the release method and
@@ -606,6 +615,7 @@ static unsigned int usb_device_poll(struct file *file, struct poll_table_struct
 		file->private_data = st;
 		mask = POLLIN;
 	}
+lost_race:
 	if (file->f_mode & FMODE_READ)
                 poll_wait(file, &deviceconndiscwq, wait);
 	if (st->lastev != conndiscevcnt)

drivers/usb/core/devio.c

@@ -361,14 +361,14 @@ static int releaseintf(struct dev_state *ps, unsigned int intf)
 	if (intf >= 8*sizeof(ps->ifclaimed))
 		return -EINVAL;
 	err = -EINVAL;
-	lock_kernel();
 	dev = ps->dev;
+	down(&dev->serialize);
 	if (dev && test_and_clear_bit(intf, &ps->ifclaimed)) {
 		iface = &dev->actconfig->interface[intf];
 		usb_driver_release_interface(&usbdevfs_driver, iface);
 		err = 0;
 	}
-	unlock_kernel();
+	up(&dev->serialize);
 	return err;
 }
 
@@ -722,14 +722,11 @@ static int proc_resetdevice(struct dev_state *ps)
 		if (test_bit(i, &ps->ifclaimed))
 			continue;
 
-		if (intf->driver) {
-			const struct usb_device_id *id;
-			down(&intf->driver->serialize);
-			intf->driver->disconnect(ps->dev, intf->private_data);
-			id = usb_match_id(ps->dev,intf,intf->driver->id_table);
-			intf->driver->probe(ps->dev, i, id);
-			up(&intf->driver->serialize);
+		lock_kernel();
+		if (intf->driver && ps->dev) {
+			usb_bind_driver(intf->driver,ps->dev, i);
 		}
+		unlock_kernel();
 	}
 
 	return 0;
@@ -1092,16 +1089,17 @@ static int proc_ioctl (struct dev_state *ps, void *arg)
 
        /* disconnect kernel driver from interface, leaving it unbound.  */
        case USBDEVFS_DISCONNECT:
+       	/* this function is voodoo. without locking it is a maybe thing */
+		lock_kernel();
                driver = ifp->driver;
                if (driver) {
-                       down (&driver->serialize);
                        dbg ("disconnect '%s' from dev %d interface %d",
                                driver->name, ps->dev->devnum, ctrl.ifno);
-                       driver->disconnect (ps->dev, ifp->private_data);
+		       usb_unbind_driver(ps->dev, ifp);
                        usb_driver_release_interface (driver, ifp);
-                       up (&driver->serialize);
                } else
 			retval = -EINVAL;
+		unlock_kernel();
                break;
 
        /* let kernel drivers try to (re)bind to the interface */
@@ -1111,18 +1109,28 @@ static int proc_ioctl (struct dev_state *ps, void *arg)
 
        /* talk directly to the interface's driver */
        default:
+		lock_kernel(); /* against module unload */
                driver = ifp->driver;
-               if (driver == 0 || driver->ioctl == 0)
-                       retval = -ENOSYS;
-		else {
-			if (ifp->driver->owner)
+               if (driver == 0 || driver->ioctl == 0) {
+			unlock_kernel();
+			retval = -ENOSYS;
+		} else {
+			if (ifp->driver->owner) {
 				__MOD_INC_USE_COUNT(ifp->driver->owner);
+				unlock_kernel();
+			}
 			/* ifno might usefully be passed ... */
                        retval = driver->ioctl (ps->dev, ctrl.ioctl_code, buf);
 			/* size = min_t(int, size, retval)? */
-			if (ifp->driver->owner)
+			if (ifp->driver->owner) {
 				__MOD_DEC_USE_COUNT(ifp->driver->owner);
+			} else {
+				unlock_kernel();
+			}
 		}
+		
+		if (retval == -ENOIOCTLCMD)
+			retval = -ENOTTY;
 	}
 
 	/* cleanup and return */
@@ -1139,7 +1147,7 @@ static int proc_ioctl (struct dev_state *ps, void *arg)
 static int usbdev_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct dev_state *ps = (struct dev_state *)file->private_data;
-	int ret = -ENOIOCTLCMD;
+	int ret = -ENOTTY;
 
 	if (!(file->f_mode & FMODE_WRITE))
 		return -EPERM;

drivers/usb/core/drivers.c

@@ -66,6 +66,7 @@ static ssize_t usb_driver_read(struct file *file, char *buf, size_t nbytes, loff
 	start = page;
 	end = page + (PAGE_SIZE - 100);
 	pos = *ppos;
+	lock_kernel(); /* else drivers might be unloaded */
 	for (; tmp != &usb_driver_list; tmp = tmp->next) {
 		struct usb_driver *driver = list_entry(tmp, struct usb_driver, driver_list);
 		int minor = driver->fops ? driver->minor : -1;
@@ -80,6 +81,7 @@ static ssize_t usb_driver_read(struct file *file, char *buf, size_t nbytes, loff
 			break;
 		}
 	}
+	unlock_kernel();
 	if (start == page)
 		start += sprintf(start, "(none)\n");
 	len = start - page;

drivers/usb/core/file.c

@@ -44,10 +44,13 @@ static int usb_open(struct inode * inode, struct file * file)
 
 	spin_lock (&minor_lock);
 	c = usb_minors[minor];
-	spin_unlock (&minor_lock);
 
-	if (!c || !(new_fops = fops_get(c)))
+	if (!c || !(new_fops = fops_get(c))) {
+		spin_unlock(&minor_lock);
 		return err;
+	}
+	spin_unlock(&minor_lock);
+
 	old_fops = file->f_op;
 	file->f_op = new_fops;
 	/* Curiouser and curiouser... NULL ->open() as "no device" ? */

drivers/usb/core/hub.c

@@ -1046,8 +1046,6 @@ static void usb_hub_events(void)
 
 static int usb_hub_thread(void *__hub)
 {
-	lock_kernel();
-
 	/*
 	 * This thread doesn't need any user-level access,
 	 * so get rid of all our resources
@@ -1067,8 +1065,6 @@ static int usb_hub_thread(void *__hub)
 	} while (!signal_pending(current));
 
 	dbg("usb_hub_thread exiting");
-
-	unlock_kernel();
 	complete_and_exit(&khubd_exited, 0);
 }
 

drivers/usb/core/usb.c

@@ -32,6 +32,7 @@
 #include <linux/init.h>
 #include <linux/spinlock.h>
 #include <linux/errno.h>
+#include <linux/smp_lock.h>
 
 #ifdef CONFIG_USB_DEBUG
 	#define DEBUG
@@ -117,6 +118,108 @@ void usb_scan_devices(void)
 	up (&usb_bus_list_lock);
 }
 
+/**
+ *	usb_unbind_driver - disconnects a driver from a device
+ *	@device: usb device to be disconnected
+ *	@intf: interface of the device to be disconnected
+ *	Context: BKL held
+ *
+ *	Handles module usage count correctly
+ */
+
+void usb_unbind_driver(struct usb_device *device, struct usb_interface *intf)
+{
+	struct usb_driver *driver;
+	void *priv;
+	int m;
+	
+
+	driver = intf->driver;
+	priv = intf->private_data;
+	
+	if (!driver)
+		return;
+
+	/* as soon as we increase the module use count we drop the BKL
+	   before that we must not sleep */
+	if (driver->owner) {
+		m = try_inc_mod_count(driver->owner);
+		if (m == 0) {
+			err("Dieing driver still bound to device.\n");
+			return;
+		}
+		unlock_kernel();
+	}
+	down(&driver->serialize); 	/* if we sleep here on an umanaged driver
+					   the holder of the lock guards against
+					   module unload */
+
+	driver->disconnect(device, priv);
+
+	up(&driver->serialize);
+	if (driver->owner) {
+		lock_kernel();
+		__MOD_DEC_USE_COUNT(driver->owner);
+	}
+}
+
+/**
+ *	usb_bind_driver - connect a driver to a device's interface
+ *	@driver: device driver to be bound to a devices interface
+ *	@dev: device to be bound
+ *	@ifnum: index number of the interface to be used
+ *
+ *	Does a save binding of a driver to a device's interface
+ *	Returns a pointer to the drivers private description of the binding
+ */
+
+void *usb_bind_driver(struct usb_driver *driver, struct usb_device *dev, unsigned int ifnum)
+{
+	int i,m;
+	void *private = NULL;
+	const struct usb_device_id *id;
+	struct usb_interface *interface;
+
+	if (driver->owner) {
+		m = try_inc_mod_count(driver->owner);
+		if (m == 0)
+			return NULL; /* this horse is dead - don't ride*/
+		unlock_kernel();
+	}
+
+	interface = &dev->actconfig->interface[ifnum];
+
+	id = driver->id_table;
+	/* new style driver? */
+	if (id) {
+		for (i = 0; i < interface->num_altsetting; i++) {
+		  	interface->act_altsetting = i;
+			id = usb_match_id(dev, interface, id);
+			if (id) {
+				down(&driver->serialize);
+				private = driver->probe(dev,ifnum,id);
+				up(&driver->serialize);
+				if (private != NULL)
+					break;
+			}
+		}
+
+		/* if driver not bound, leave defaults unchanged */
+		if (private == NULL)
+			interface->act_altsetting = 0;
+	} else { /* "old style" driver */
+		down(&driver->serialize);
+		private = driver->probe(dev, ifnum, NULL);
+		up(&driver->serialize);
+	}
+	if (driver->owner) {
+		lock_kernel();
+		__MOD_DEC_USE_COUNT(driver->owner);
+	}
+
+	return private;
+}
+
 /*
  * This function is part of a depth-first search down the device tree,
  * removing any instances of a device driver.
@@ -136,18 +239,12 @@ static void usb_drivers_purge(struct usb_driver *driver,struct usb_device *dev)
 
 	if (!dev->actconfig)
 		return;
-			
+
 	for (i = 0; i < dev->actconfig->bNumInterfaces; i++) {
 		struct usb_interface *interface = &dev->actconfig->interface[i];
-		
+
 		if (interface->driver == driver) {
-			if (driver->owner)
-				__MOD_INC_USE_COUNT(driver->owner);
-			down(&driver->serialize);
-			driver->disconnect(dev, interface->private_data);
-			up(&driver->serialize);
-			if (driver->owner)
-				__MOD_DEC_USE_COUNT(driver->owner);
+			usb_unbind_driver(dev, interface);
 			/* if driver->disconnect didn't release the interface */
 			if (interface->driver)
 				usb_driver_release_interface(driver, interface);
@@ -163,7 +260,7 @@ static void usb_drivers_purge(struct usb_driver *driver,struct usb_device *dev)
 /**
  * usb_deregister - unregister a USB driver
  * @driver: USB operations of the driver to unregister
- * Context: !in_interrupt ()
+ * Context: !in_interrupt (), must be called with BKL held
  *
  * Unlinks the specified driver from the internal USB driver list.
  * 
@@ -528,9 +625,7 @@ static int usb_find_interface_driver(struct usb_device *dev, unsigned ifnum)
 	struct list_head *tmp;
 	struct usb_interface *interface;
 	void *private;
-	const struct usb_device_id *id;
 	struct usb_driver *driver;
-	int i;
 	
 	if ((!dev) || (ifnum >= dev->actconfig->bNumInterfaces)) {
 		err("bad find_interface_driver params");
@@ -545,37 +640,12 @@ static int usb_find_interface_driver(struct usb_device *dev, unsigned ifnum)
 		goto out_err;
 
 	private = NULL;
+	lock_kernel();
 	for (tmp = usb_driver_list.next; tmp != &usb_driver_list;) {
 		driver = list_entry(tmp, struct usb_driver, driver_list);
 		tmp = tmp->next;
 
-		if (driver->owner)
-			__MOD_INC_USE_COUNT(driver->owner);
-		id = driver->id_table;
-		/* new style driver? */
-		if (id) {
-			for (i = 0; i < interface->num_altsetting; i++) {
-			  	interface->act_altsetting = i;
-				id = usb_match_id(dev, interface, id);
-				if (id) {
-					down(&driver->serialize);
-					private = driver->probe(dev,ifnum,id);
-					up(&driver->serialize);
-					if (private != NULL)
-						break;
-				}
-			}
-
-			/* if driver not bound, leave defaults unchanged */
-			if (private == NULL)
-				interface->act_altsetting = 0;
-		} else { /* "old style" driver */
-			down(&driver->serialize);
-			private = driver->probe(dev, ifnum, NULL);
-			up(&driver->serialize);
-		}
-		if (driver->owner)
-			__MOD_DEC_USE_COUNT(driver->owner);
+		private = usb_bind_driver(driver, dev, ifnum);
 
 		/* probe() may have changed the config on us */
 		interface = dev->actconfig->interface + ifnum;
@@ -583,9 +653,11 @@ static int usb_find_interface_driver(struct usb_device *dev, unsigned ifnum)
 		if (private) {
 			usb_driver_claim_interface(driver, interface, private);
 			up(&dev->serialize);
+			unlock_kernel();
 			return 0;
 		}
 	}
+	unlock_kernel();
 
 out_err:
 	up(&dev->serialize);
@@ -1121,27 +1193,22 @@ void usb_disconnect(struct usb_device **pdev)
 
 	info("USB disconnect on device %d", dev->devnum);
 
+	lock_kernel();
 	if (dev->actconfig) {
 		for (i = 0; i < dev->actconfig->bNumInterfaces; i++) {
 			struct usb_interface *interface = &dev->actconfig->interface[i];
 			struct usb_driver *driver = interface->driver;
 			if (driver) {
-				if (driver->owner)
-					__MOD_INC_USE_COUNT(driver->owner);
-				down(&driver->serialize);
-				driver->disconnect(dev, interface->private_data);
-				up(&driver->serialize);
+				usb_unbind_driver(dev, interface);
 				/* if driver->disconnect didn't release the interface */
 				if (interface->driver)
 					usb_driver_release_interface(driver, interface);
-				/* we don't need the driver any longer */
-				if (driver->owner)
-					__MOD_DEC_USE_COUNT(driver->owner);
 			}
 			/* remove our device node for this interface */
 			put_device(&interface->dev);
 		}
 	}
+	unlock_kernel();
 
 	/* Free up all the children.. */
 	for (i = 0; i < USB_MAXCHILDREN; i++) {
@@ -1475,6 +1542,8 @@ EXPORT_SYMBOL(usb_new_device);
 EXPORT_SYMBOL(usb_reset_device);
 EXPORT_SYMBOL(usb_connect);
 EXPORT_SYMBOL(usb_disconnect);
+EXPORT_SYMBOL(usb_bind_driver);
+EXPORT_SYMBOL(usb_unbind_driver);
 
 EXPORT_SYMBOL(__usb_get_extra_descriptor);
 

drivers/usb/media/pwc-if.c

@@ -1756,40 +1756,40 @@ static void usb_pwc_disconnect(struct usb_device *udev, void *ptr)
 	pdev = (struct pwc_device *)ptr;
 	if (pdev == NULL) {
 		Err("pwc_disconnect() Called without private pointer.\n");
-		return;
+		goto out_err;
 	}
 	if (pdev->udev == NULL) {
 		Err("pwc_disconnect() already called for %p\n", pdev);
-		return;
+		goto out_err;
 	}
 	if (pdev->udev != udev) {
 		Err("pwc_disconnect() Woops: pointer mismatch udev/pdev.\n");
-		return;
+		goto out_err;
 	}
 #ifdef PWC_MAGIC	
 	if (pdev->magic != PWC_MAGIC) {
 		Err("pwc_disconnect() Magic number failed. Consult your scrolls and try again.\n");
-		return;
+		goto out_err;
 	}
-#endif	
-	
+#endif
+
 	pdev->unplugged = 1;
 	if (pdev->vdev != NULL) {
-		video_unregister_device(pdev->vdev); 
+		video_unregister_device(pdev->vdev);
 		if (pdev->vopen) {
 			Info("Disconnected while device/video is open!\n");
-			
+
 			/* Wake up any processes that might be waiting for
 			   a frame, let them return an error condition
 			 */
 			wake_up(&pdev->frameq);
-			
+
 			/* Wait until we get a 'go' from _close(). This used
 			   to have a gigantic race condition, since we kfree()
-			   stuff here, but we have to wait until close() 
-			   is finished. 
+			   stuff here, but we have to wait until close()
+			   is finished.
 			 */
-			   
+
 			Trace(TRACE_PROBE, "Sleeping on remove_ok.\n");
 			add_wait_queue(&pdev->remove_ok, &wait);
 			set_current_state(TASK_UNINTERRUPTIBLE);
@@ -1815,6 +1815,7 @@ static void usb_pwc_disconnect(struct usb_device *udev, void *ptr)
 			device_hint[hint].pdev = NULL;
 
 	pdev->udev = NULL;
+out_err:
 	unlock_kernel();
 	kfree(pdev);
 }

include/linux/usb.h

@@ -431,6 +431,10 @@ extern void usb_free_dev(struct usb_device *);
 /* for when layers above USB add new non-USB drivers */
 extern void usb_scan_devices(void);
 
+/* for probe/disconnect with correct module usage counting */
+void *usb_bind_driver(struct usb_driver *driver, struct usb_device *dev, unsigned int ifnum);
+void usb_unbind_driver(struct usb_device *device, struct usb_interface *intf);
+
 /* mostly for devices emulating SCSI over USB */
 extern int usb_reset_device(struct usb_device *dev);