[PATCH] USB: Add locking support for USB device resets

This patch reintroduces the usb_lock_device_for_reset() routine, which
is specially tailored to meet the needs of drivers that have to reset a
device either during probe() or during normal operations.  It updates a
few drivers that do device resets, to make them use the new routine.  It
also adds a new field to struct usb_interface, to keep track of whether
the interface is in the process of being bound to or unbound from a
driver.  This is necessary, because during binding we know the device is
already locked so we don't want to try to acquire the lock again!

With this patch in place, USB device resets should finally become
reliable.
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

drivers/net/irda/stir4200.c

@@ -168,6 +168,7 @@ enum StirTestMask {
 
 struct stir_cb {
         struct usb_device *usbdev;      /* init: probe_irda */
+	struct usb_interface *usbintf;
         struct net_device *netdev;      /* network layer */
         struct irlap_cb   *irlap;       /* The link layer we are binded to */
         struct net_device_stats stats;	/* network statistics */
@@ -508,6 +509,7 @@ static int change_speed(struct stir_cb *stir, unsigned speed)
 {
 	int i, err;
 	__u8 mode;
+	int rc;
 
 	for (i = 0; i < ARRAY_SIZE(stir_modes); ++i) {
 		if (speed == stir_modes[i].speed)
@@ -521,7 +523,14 @@ static int change_speed(struct stir_cb *stir, unsigned speed)
 	pr_debug("speed change from %d to %d\n", stir->speed, speed);
 
 	/* sometimes needed to get chip out of stuck state */
+	rc = usb_lock_device_for_reset(stir->usbdev, stir->usbintf);
+	if (rc < 0) {
+		err = rc;
+		goto out;
+	}
 	err = usb_reset_device(stir->usbdev);
+	if (rc)
+		usb_unlock_device(stir->usbdev);
 	if (err)
 		goto out;
 
@@ -1066,6 +1075,7 @@ static int stir_probe(struct usb_interface *intf,
 	stir = net->priv;
 	stir->netdev = net;
 	stir->usbdev = dev;
+	stir->usbintf = intf;
 
 	ret = usb_reset_configuration(dev);
 	if (ret != 0) {

drivers/usb/core/devio.c

@@ -735,7 +735,7 @@ static int proc_connectinfo(struct dev_state *ps, void __user *arg)
 
 static int proc_resetdevice(struct dev_state *ps)
 {
-	return __usb_reset_device(ps->dev);
+	return usb_reset_device(ps->dev);
 
 }
 

drivers/usb/core/hub.c

@@ -826,7 +826,7 @@ static int hub_reset(struct usb_hub *hub)
 	else
 		return -1;
 
-	if (__usb_reset_device(hdev))
+	if (usb_reset_device(hdev))
 		return -1;
 
 	hub->urb->dev = hdev;                                                    
@@ -2759,8 +2759,10 @@ static int config_descriptors_changed(struct usb_device *udev)
  *
  * The caller must own the device lock.  For example, it's safe to use
  * this from a driver probe() routine after downloading new firmware.
+ * For calls that might not occur during probe(), drivers should lock
+ * the device using usb_lock_device_for_reset().
  */
-int __usb_reset_device(struct usb_device *udev)
+int usb_reset_device(struct usb_device *udev)
 {
 	struct usb_device *parent = udev->parent;
 	struct usb_device_descriptor descriptor = udev->descriptor;
@@ -2795,6 +2797,11 @@ int __usb_reset_device(struct usb_device *udev)
 		return -ENOENT;
 	}
 
+	/* ep0 maxpacket size may change; let the HCD know about it.
+	 * Other endpoints will be handled by re-enumeration. */
+	usb_disable_endpoint(udev, 0);
+	usb_disable_endpoint(udev, 0 + USB_DIR_IN);
+
 	ret = hub_port_init(parent, udev, port);
 	if (ret < 0)
 		goto re_enumerate;
@@ -2848,15 +2855,3 @@ int __usb_reset_device(struct usb_device *udev)
 	hub_port_logical_disconnect(parent, port);
 	return -ENODEV;
 }
-EXPORT_SYMBOL(__usb_reset_device);
-
-int usb_reset_device(struct usb_device *udev)
-{
-	int r;
-	
-	down(&udev->serialize);
-	r = __usb_reset_device(udev);
-	up(&udev->serialize);
-
-	return r;
-}

drivers/usb/core/usb.c

@@ -102,7 +102,10 @@ int usb_probe_interface(struct device *dev)
 	id = usb_match_id (intf, driver->id_table);
 	if (id) {
 		dev_dbg (dev, "%s - got id\n", __FUNCTION__);
+		intf->condition = USB_INTERFACE_BINDING;
 		error = driver->probe (intf, id);
+		intf->condition = error ? USB_INTERFACE_UNBOUND :
+				USB_INTERFACE_BOUND;
 	}
 
 	return error;
@@ -114,6 +117,8 @@ int usb_unbind_interface(struct device *dev)
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_driver *driver = to_usb_driver(intf->dev.driver);
 
+	intf->condition = USB_INTERFACE_UNBINDING;
+
 	/* release all urbs for this interface */
 	usb_disable_interface(interface_to_usbdev(intf), intf);
 
@@ -125,6 +130,7 @@ int usb_unbind_interface(struct device *dev)
 			intf->altsetting[0].desc.bInterfaceNumber,
 			0);
 	usb_set_intfdata(intf, NULL);
+	intf->condition = USB_INTERFACE_UNBOUND;
 
 	return 0;
 }
@@ -324,6 +330,7 @@ int usb_driver_claim_interface(struct usb_driver *driver,
 
 	dev->driver = &driver->driver;
 	usb_set_intfdata(iface, priv);
+	iface->condition = USB_INTERFACE_BOUND;
 
 	/* if interface was already added, bind now; else let
 	 * the future device_add() bind it, bypassing probe()
@@ -363,6 +370,7 @@ void usb_driver_release_interface(struct usb_driver *driver,
 
 	dev->driver = NULL;
 	usb_set_intfdata(iface, NULL);
+	iface->condition = USB_INTERFACE_UNBOUND;
 }
 
 /**
@@ -910,6 +918,54 @@ int usb_trylock_device(struct usb_device *udev)
 	return 1;
 }
 
+/**
+ * usb_lock_device_for_reset - cautiously acquire the lock for a
+ *	usb device structure
+ * @udev: device that's being locked
+ * @iface: interface bound to the driver making the request (optional)
+ *
+ * Attempts to acquire the device lock, but fails if the device is
+ * NOTATTACHED or SUSPENDED, or if iface is specified and the interface
+ * is neither BINDING nor BOUND.  Rather than sleeping to wait for the
+ * lock, the routine polls repeatedly.  This is to prevent deadlock with
+ * disconnect; in some drivers (such as usb-storage) the disconnect()
+ * callback will block waiting for a device reset to complete.
+ *
+ * Returns a negative error code for failure, otherwise 1 or 0 to indicate
+ * that the device will or will not have to be unlocked.  (0 can be
+ * returned when an interface is given and is BINDING, because in that
+ * case the driver already owns the device lock.)
+ */
+int usb_lock_device_for_reset(struct usb_device *udev,
+		struct usb_interface *iface)
+{
+	if (udev->state == USB_STATE_NOTATTACHED)
+		return -ENODEV;
+	if (udev->state == USB_STATE_SUSPENDED)
+		return -EHOSTUNREACH;
+	if (iface) {
+		switch (iface->condition) {
+		  case USB_INTERFACE_BINDING:
+			return 0;
+		  case USB_INTERFACE_BOUND:
+			break;
+		  default:
+			return -EINTR;
+		}
+	}
+
+	while (!usb_trylock_device(udev)) {
+		msleep(15);
+		if (udev->state == USB_STATE_NOTATTACHED)
+			return -ENODEV;
+		if (udev->state == USB_STATE_SUSPENDED)
+			return -EHOSTUNREACH;
+		if (iface && iface->condition != USB_INTERFACE_BOUND)
+			return -EINTR;
+	}
+	return 1;
+}
+
 /**
  * usb_unlock_device - release the lock for a usb device structure
  * @udev: device that's being unlocked
@@ -1493,6 +1549,7 @@ EXPORT_SYMBOL(usb_hub_tt_clear_buffer);
 
 EXPORT_SYMBOL(usb_lock_device);
 EXPORT_SYMBOL(usb_trylock_device);
+EXPORT_SYMBOL(usb_lock_device_for_reset);
 EXPORT_SYMBOL(usb_unlock_device);
 
 EXPORT_SYMBOL(usb_driver_claim_interface);

drivers/usb/image/microtek.c

@@ -341,12 +341,18 @@ static int mts_scsi_abort (Scsi_Cmnd *srb)
 static int mts_scsi_host_reset (Scsi_Cmnd *srb)
 {
 	struct mts_desc* desc = (struct mts_desc*)(srb->device->host->hostdata[0]);
+	int result, rc;
 
 	MTS_DEBUG_GOT_HERE();
 	mts_debug_dump(desc);
 
-	usb_reset_device(desc->usb_dev); /*FIXME: untested on new reset code */
-	return 0;  /* RANT why here 0 and not SUCCESS */
+	rc = usb_lock_device_for_reset(desc->usb_dev, desc->usb_intf);
+	if (rc < 0)
+		return FAILED;
+	result = usb_reset_device(desc->usb_dev);;
+	if (rc)
+		usb_unlock_device(desc->usb_dev);
+	return result ? FAILED : SUCCESS;
 }
 
 static
@@ -777,6 +783,7 @@ static int mts_usb_probe(struct usb_interface *intf,
 		goto out_kfree;
 
 	new_desc->usb_dev = dev;
+	new_desc->usb_intf = intf;
 	init_MUTEX(&new_desc->lock);
 
 	/* endpoints */

drivers/usb/image/microtek.h

@@ -31,6 +31,7 @@ struct mts_desc {
 	struct mts_desc *prev;
 
 	struct usb_device *usb_dev;
+	struct usb_interface *usb_intf;
 
 	/* Endpoint addresses */
 	u8 ep_out;

drivers/usb/storage/scsiglue.c

@@ -283,7 +283,7 @@ static int device_reset(struct scsi_cmnd *srb)
 static int bus_reset(struct scsi_cmnd *srb)
 {
 	struct us_data *us = (struct us_data *)srb->device->host->hostdata[0];
-	int result;
+	int result, rc;
 
 	US_DEBUGP("%s called\n", __FUNCTION__);
 	if (us->sm_state != US_STATE_IDLE) {
@@ -308,8 +308,16 @@ static int bus_reset(struct scsi_cmnd *srb)
 		result = -EBUSY;
 		US_DEBUGP("Refusing to reset a multi-interface device\n");
 	} else {
-		result = usb_reset_device(us->pusb_dev);
-		US_DEBUGP("usb_reset_device returns %d\n", result);
+		rc = usb_lock_device_for_reset(us->pusb_dev, us->pusb_intf);
+		if (rc < 0) {
+			US_DEBUGP("unable to lock device for reset: %d\n", rc);
+			result = rc;
+		} else {
+			result = usb_reset_device(us->pusb_dev);
+			if (rc)
+				usb_unlock_device(us->pusb_dev);
+			US_DEBUGP("usb_reset_device returns %d\n", result);
+		}
 	}
 	up(&(us->dev_semaphore));
 

include/linux/usb.h

@@ -61,6 +61,13 @@ struct usb_host_interface {
 	int extralen;
 };
 
+enum usb_interface_condition {
+	USB_INTERFACE_UNBOUND = 0,
+	USB_INTERFACE_BINDING,
+	USB_INTERFACE_BOUND,
+	USB_INTERFACE_UNBINDING,
+};
+
 /**
  * struct usb_interface - what usb device drivers talk to
  * @altsetting: array of interface structures, one for each alternate
@@ -75,6 +82,8 @@ struct usb_host_interface {
  *	be unused.  The driver should set this value in the probe()
  *	function of the driver, after it has been assigned a minor
  *	number from the USB core by calling usb_register_dev().
+ * @condition: binding state of the interface: not bound, binding
+ *	(in probe()), bound to a driver, or unbinding (in disconnect())
  * @dev: driver model's view of this device
  * @class_dev: driver model's class view of this device.
  *
@@ -113,6 +122,7 @@ struct usb_interface {
 	unsigned num_altsetting;	/* number of alternate settings */
 
 	int minor;			/* minor number this interface is bound to */
+	enum usb_interface_condition condition;		/* state of binding */
 	struct device dev;		/* interface specific device info */
 	struct class_device *class_dev;
 };
@@ -341,11 +351,12 @@ extern void usb_put_dev(struct usb_device *dev);
 
 extern void usb_lock_device(struct usb_device *udev);
 extern int usb_trylock_device(struct usb_device *udev);
+extern int usb_lock_device_for_reset(struct usb_device *udev,
+		struct usb_interface *iface);
 extern void usb_unlock_device(struct usb_device *udev);
 
-/* mostly for devices emulating SCSI over USB */
+/* USB port reset for device reinitialization */
 extern int usb_reset_device(struct usb_device *dev);
-extern int __usb_reset_device(struct usb_device *dev);
 
 extern struct usb_device *usb_find_device(u16 vendor_id, u16 product_id);