[PATCH] USB: Don't add/del interfaces, register/unregister them

On Fri, 27 Feb 2004, Greg KH wrote:

> On Wed, Feb 25, 2004 at 10:05:37AM -0500, Alan Stern wrote:
>
> > Why would anyone want to do this, you ask?  Well the USB subsystem does it
> > already.  Each USB device can have several configurations, only one of
> > which is active at any time.  Corresponding to each configuration is a set
> > of struct devices, and they (together with their embedded kobjects) are
> > allocated and initialized when the USB device is first detected.  The
> > struct devices are add()'ed and del()'ed as configurations are activated
> > and deactivated, leading to just the sort of call sequence shown above.
>
> Then we need to fix this.

The driver model does not support repeated device_add(), device_del(),
device_add(), device_del(), ... calls for the same device.  But that's
what happens to an interface's embedded struct device when we change
configurations.

Accordingly, this patch changes the device_add()/device_del() calls for
interfaces to device_register()/device_unregister().  When the interface
is unregistered the new code waits for the release method to run, so that
it will be safe to re-register the interface should the former
configuration be reinstated.

Greg, please check this out to make sure I haven't made some dumb mistake.
It works on my system and it fixes a memory leak in the USB system.

drivers/usb/core/config.c

@@ -72,13 +72,10 @@ static int usb_parse_endpoint(struct usb_host_endpoint *endpoint, unsigned char
 	return buffer - buffer0;
 }
 
-static void usb_release_intf(struct device *dev)
+static void usb_free_intf(struct usb_interface *intf)
 {
-	struct usb_interface *intf;
 	int j;
 
-	intf = to_usb_interface(dev);
-
 	if (intf->altsetting) {
 		for (j = 0; j < intf->num_altsetting; j++) {
 			struct usb_host_interface *as = &intf->altsetting[j];
@@ -235,8 +232,6 @@ int usb_parse_configuration(struct usb_host_config *config, char *buffer, int si
 			return -ENOMEM;
 		}
 		memset(interface, 0, sizeof(struct usb_interface));
-		interface->dev.release = usb_release_intf;
-		device_initialize(&interface->dev);
 	}
 
 	/* Go through the descriptors, checking their length and counting the
@@ -374,7 +369,7 @@ void usb_destroy_configuration(struct usb_device *dev)
 			struct usb_interface *ifp = cf->interface[i];
 
 			if (ifp)
-				put_device(&ifp->dev);
+				usb_free_intf(ifp);
 		}
 	}
 	kfree(dev->config);

drivers/usb/core/message.c

@@ -793,6 +793,13 @@ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf)
 	}
 }
 
+static void release_interface(struct device *dev)
+{
+	struct usb_interface *interface = to_usb_interface(dev);
+
+	complete(interface->released);
+}
+
 /*
  * usb_disable_device - Disable all the endpoints for a USB device
  * @dev: the device whose endpoints are being disabled
@@ -822,12 +829,16 @@ void usb_disable_device(struct usb_device *dev, int skip_ep0)
 	if (dev->actconfig) {
 		for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
 			struct usb_interface	*interface;
+			struct completion	intf_completion;
 
 			/* remove this interface */
 			interface = dev->actconfig->interface[i];
 			dev_dbg (&dev->dev, "unregistering interface %s\n",
 				interface->dev.bus_id);
-			device_del(&interface->dev);
+			init_completion (&intf_completion);
+			interface->released = &intf_completion;
+			device_unregister (&interface->dev);
+			wait_for_completion (&intf_completion);
 		}
 		dev->actconfig = 0;
 		if (dev->state == USB_STATE_CONFIGURED)
@@ -1145,6 +1156,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 			intf->dev.driver = NULL;
 			intf->dev.bus = &usb_bus_type;
 			intf->dev.dma_mask = dev->dev.dma_mask;
+			intf->dev.release = release_interface;
 			sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d",
 				 dev->bus->busnum, dev->devpath,
 				 configuration,
@@ -1153,7 +1165,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 				"registering %s (config #%d, interface %d)\n",
 				intf->dev.bus_id, configuration,
 				desc->bInterfaceNumber);
-			device_add (&intf->dev);
+			device_register (&intf->dev);
 			usb_create_driverfs_intf_files (intf);
 		}
 	}

include/linux/usb.h

@@ -89,6 +89,8 @@ struct usb_host_interface {
  *	number from the USB core by calling usb_register_dev().
  * @dev: driver model's view of this device
  * @class_dev: driver model's class view of this device.
+ * @released: wait for the interface to be released when changing
+ *	configurations.
  *
  * USB device drivers attach to interfaces on a physical device.  Each
  * interface encapsulates a single high level function, such as feeding
@@ -122,6 +124,7 @@ struct usb_interface {
 	int minor;			/* minor number this interface is bound to */
 	struct device dev;		/* interface specific device info */
 	struct class_device *class_dev;
+	struct completion *released;	/* wait for release */
 };
 #define	to_usb_interface(d) container_of(d, struct usb_interface, dev)
 #define	interface_to_usbdev(intf) \