[PATCH] usb-storage: move to SCSI hotplugging

The attached patch is my first implementation of SCSI hotplugging.

It's only been tested that it compiles, as I can't get the current
linux-2.5 tree from linuxusb to boot.  It dies _very_ early.  Greg, I'm not
sure if you'll want to apply this.  Linus seemed to want this very much,
and it is 2.5.x... I say go for it, but I can understand if you have
reservations.

I would definately like to see this tested by anyone who can get a kernel
to boot.

This patch is quite large.  Lots of things had to be changed.  Among them:

(o) The proc interface now uses the host number to look up the SCSI host
    structure, and then finds the usb-storage structure from that.
(o) The SCSI interface has been changed.  The code flow is now much
    clearer, as more work is done from the USB probe/detach functions than
    from auxillary functions.
(o) Names have been changed for newer conventions
(o) GUIDs have been removed
(o) The linked-list of devices has been removed, and it's associated
    semaphore
(o) All code dealing with re-attaching a device to it's old association has
    been removed
(o) Some spaces changed to tabs
(o) usb-storage now takes one directory under /proc/scsi instead of
    one per virtual-HBA
(o) All control threads now have the same name.  This could be changed back
    to the old behavior, if enough people want it.

Known problems:
(o) Testing, testing, testing
(o) More dead code needs to be cut
(o) It's a unclear how a LLD is supposed to cut off the flow of
    commands, so that the unregister() call always succeeds.  SCSI folks
    need to work on this.
(o) Probing needs to be broken down into smaller functions, probably.

drivers/usb/storage/scsiglue.c

@@ -50,23 +50,25 @@
 #include "transport.h"
 
 #include <linux/slab.h>
+#include <linux/module.h>
 
 
 /***********************************************************************
  * Host functions 
  ***********************************************************************/
 
-static const char* host_info(struct Scsi_Host *host)
+static const char* usb_storage_info(struct Scsi_Host *host)
 {
 	return "SCSI emulation for USB Mass Storage devices";
 }
 
+#if 0
 /* detect a virtual adapter (always works)
  * Synchronization: 2.4: with the io_request_lock
  * 			2.5: no locks.
  * fortunately we don't care.
  * */
-static int detect(struct SHT *sht)
+static int usb_storage_detect(struct SHT *sht)
 {
 	struct us_data *us;
 	char local_name[32];
@@ -109,7 +111,7 @@ static int detect(struct SHT *sht)
  * the driver and we're doing each virtual host in turn, not in parallel
  * Synchronization: BKL, no spinlock.
  */
-static int release(struct Scsi_Host *psh)
+static int usb_storage_release(struct Scsi_Host *psh)
 {
 	struct us_data *us = (struct us_data *)psh->hostdata[0];
 
@@ -132,18 +134,11 @@ static int release(struct Scsi_Host *psh)
 	/* we always have a successful release */
 	return 0;
 }
-
-/* run command */
-static int command( Scsi_Cmnd *srb )
-{
-	US_DEBUGP("Bad use of us_command\n");
-
-	return DID_BAD_TARGET << 16;
-}
+#endif
 
 /* queue a command */
 /* This is always called with scsi_lock(srb->host) held */
-static int queuecommand( Scsi_Cmnd *srb , void (*done)(Scsi_Cmnd *))
+static int usb_storage_queuecommand( Scsi_Cmnd *srb , void (*done)(Scsi_Cmnd *))
 {
 	struct us_data *us = (struct us_data *)srb->host->hostdata[0];
 
@@ -168,7 +163,7 @@ static int queuecommand( Scsi_Cmnd *srb , void (*done)(Scsi_Cmnd *))
 
 /* Command abort */
 /* This is always called with scsi_lock(srb->host) held */
-static int command_abort( Scsi_Cmnd *srb )
+static int usb_storage_command_abort( Scsi_Cmnd *srb )
 {
 	struct us_data *us = (struct us_data *)srb->host->hostdata[0];
 
@@ -187,7 +182,7 @@ static int command_abort( Scsi_Cmnd *srb )
 /* This invokes the transport reset mechanism to reset the state of the
  * device */
 /* This is always called with scsi_lock(srb->host) held */
-static int device_reset( Scsi_Cmnd *srb )
+static int usb_storage_device_reset( Scsi_Cmnd *srb )
 {
 	struct us_data *us = (struct us_data *)srb->host->hostdata[0];
 	int result;
@@ -219,7 +214,7 @@ static int device_reset( Scsi_Cmnd *srb )
  * disconnect/reconnect for all drivers which have claimed
  * interfaces, including ourself. */
 /* This is always called with scsi_lock(srb->host) held */
-static int bus_reset( Scsi_Cmnd *srb )
+static int usb_storage_bus_reset( Scsi_Cmnd *srb )
 {
 	struct us_data *us = (struct us_data *)srb->host->hostdata[0];
 	int i;
@@ -274,14 +269,6 @@ static int bus_reset( Scsi_Cmnd *srb )
 	return SUCCESS;
 }
 
-/* FIXME: This doesn't do anything right now */
-static int host_reset( Scsi_Cmnd *srb )
-{
-	printk(KERN_CRIT "usb-storage: host_reset() requested but not implemented\n" );
-	bus_reset(srb);
-	return FAILED;
-}
-
 /***********************************************************************
  * /proc/scsi/ functions
  ***********************************************************************/
@@ -291,29 +278,24 @@ static int host_reset( Scsi_Cmnd *srb )
 #define SPRINTF(args...) \
 	do { if (pos < buffer+length) pos += sprintf(pos, ## args); } while (0)
 
-static int proc_info (char *buffer, char **start, off_t offset, int length,
-		int hostno, int inout)
+static int usb_storage_proc_info (char *buffer, char **start, off_t offset,
+		int length, int hostno, int inout)
 {
 	struct us_data *us;
 	char *pos = buffer;
+	struct Scsi_Host *hostptr;
 
 	/* if someone is sending us data, just throw it away */
 	if (inout)
 		return length;
 
-	/* lock the data structures */
-	down(&us_list_semaphore);
-
-	/* find our data from hostno */
-	us = us_list;
-	while (us) {
-		if (us->host_no == hostno)
-			break;
-		us = us->next;
+	/* find our data from the given hostno */
+	hostptr = scsi_host_hn_get(hostno);
+	if (!hostptr) {	 /* if we couldn't find it, we return an error */
+		return -ESRCH;
 	}
-
-	/* release our lock on the data structures */
-	up(&us_list_semaphore);
+	us = (struct us_data*)hostptr->hostdata[0];
+	scsi_host_put(hostptr);
 
 	/* if we couldn't find it, we return an error */
 	if (!us) {
@@ -332,8 +314,7 @@ static int proc_info (char *buffer, char **start, off_t offset, int length,
 	SPRINTF("     Protocol: %s\n", us->protocol_name);
 	SPRINTF("    Transport: %s\n", us->transport_name);
 
-	/* show the GUID of the device */
-	SPRINTF("         GUID: " GUID_FORMAT "\n", GUID_ARGS(us->guid));
+	/* show attached status of the device */
 	SPRINTF("     Attached: %s\n", (us->flags & US_FL_DEV_ATTACHED ?
 			"Yes" : "No"));
 
@@ -351,33 +332,69 @@ static int proc_info (char *buffer, char **start, off_t offset, int length,
 }
 
 /*
- * this defines our 'host'
+ * this defines our host template, with which we'll allocate hosts
  */
 
-Scsi_Host_Template usb_stor_host_template = {
+struct SHT usb_stor_host_template = {
+	/* basic userland interface stuff */
 	.name =				"usb-storage",
-	.proc_info =		proc_info,
-	.info =			host_info,
+	.proc_name =			"usb-storage",
+	.proc_info =			usb_storage_proc_info,
+	.proc_dir =			NULL,
+	.info =				usb_storage_info,
+	.ioctl =			NULL,
+
+	/* old-style detect and release */
+	.detect =			NULL,
+	.release =			NULL,
+
+	/* command interface -- queued only */
+	.command =			NULL,
+	.queuecommand =			usb_storage_queuecommand,
+
+	/* error and abort handlers */
+	.eh_abort_handler =		usb_storage_command_abort,
+	.eh_device_reset_handler =	usb_storage_device_reset,
+	.eh_bus_reset_handler =		usb_storage_bus_reset,
+	.eh_host_reset_handler =	NULL,
+	.eh_strategy_handler =		NULL,
+
+	/* queue commands only, only one command per LUN */
+	.can_queue =			1,
+	.cmd_per_lun =			1,
 
-	.detect =		detect,
-	.release =		release,
-	.command =		command,
-	.queuecommand =		queuecommand,
+	/* unknown initiator id */
+	.this_id =			-1,
 
-	.eh_abort_handler =	command_abort,
-	.eh_device_reset_handler =device_reset,
-	.eh_bus_reset_handler =	bus_reset,
-	.eh_host_reset_handler =host_reset,
+	/* no limit on commands */
+	.max_sectors =			0,
 	
-	.can_queue =		1,
-	.this_id =		-1,
+	/* pre- and post- device scan functions */
+	.slave_alloc =			NULL,
+	.slave_configure =		NULL,
+	.slave_destroy =		NULL,
 
+	/* lots of sg segments can be handled */
 	.sg_tablesize =			SG_ALL,
-	.cmd_per_lun =		1,
-	.present =		0,
+
+	/* use 32-bit address space for DMA */
 	.unchecked_isa_dma =		FALSE,
+	.highmem_io =			FALSE,
+
+	/* merge commands... this seems to help performance, but
+	 * periodically someone should test to see which setting is more
+	 * optimal.
+	 */
 	.use_clustering =		TRUE,
-	.emulated =		TRUE
+
+	/* emulated HBA */
+	.emulated =			TRUE,
+
+	/* sorry, no BIOS to help us */
+	.bios_param =			NULL,
+
+	/* module management */
+	.module =			THIS_MODULE
 };
 
 /* For a device that is "Not Ready" */

drivers/usb/storage/scsiglue.h

@@ -47,7 +47,7 @@
 
 extern unsigned char usb_stor_sense_notready[18];
 extern unsigned char usb_stor_sense_invalidCDB[18];
-extern Scsi_Host_Template usb_stor_host_template;
+extern struct SHT usb_stor_host_template;
 extern int usb_stor_scsiSense10to6(Scsi_Cmnd*);
 extern int usb_stor_scsiSense6to10(Scsi_Cmnd*);
 

drivers/usb/storage/usb.c

@@ -93,16 +93,6 @@ MODULE_AUTHOR("Matthew Dharm <mdharm-usb@one-eyed-alien.net>");
 MODULE_DESCRIPTION("USB Mass Storage driver for Linux");
 MODULE_LICENSE("GPL");
 
-/*
- * Per device data
- */
-
-static int my_host_number;
-
-/* The list of structures and the protective lock for them */
-struct us_data *us_list;
-struct semaphore us_list_semaphore;
-
 static int storage_probe(struct usb_interface *iface,
 			 const struct usb_device_id *id);
 
@@ -319,7 +309,7 @@ static int usb_stor_control_thread(void * __us)
 	spin_unlock_irq(&current->sig->siglock);
 
 	/* set our name for identification purposes */
-	sprintf(current->comm, "usb-storage-%d", us->host_number);
+	sprintf(current->comm, "usb-storage");
 
 	unlock_kernel();
 
@@ -563,12 +553,10 @@ static int storage_probe(struct usb_interface *intf,
 	char mf[USB_STOR_STRING_LEN];		     /* manufacturer */
 	char prod[USB_STOR_STRING_LEN];		     /* product */
 	char serial[USB_STOR_STRING_LEN];	     /* serial number */
-	GUID(guid);			   /* Global Unique Identifier */
 	unsigned int flags;
 	struct us_unusual_dev *unusual_dev;
 	struct us_data *ss = NULL;
 	int result;
-	int new_device = 0;
 
 	/* these are temporary copies -- we test on these, then put them
 	 * in the us-data structure 
@@ -676,8 +664,7 @@ static int storage_probe(struct usb_interface *intf,
 	/* At this point, we've decided to try to use the device */
 	usb_get_dev(dev);
 
-	/* clear the GUID and fetch the strings */
-	GUID_CLEAR(guid);
+	/* fetch the strings */
 	if (dev->descriptor.iManufacturer)
 		usb_string(dev, dev->descriptor.iManufacturer, 
 			   mf, sizeof(mf));
@@ -688,69 +675,7 @@ static int storage_probe(struct usb_interface *intf,
 		usb_string(dev, dev->descriptor.iSerialNumber, 
 			   serial, sizeof(serial));
 
-	/* Create a GUID for this device */
-	if (dev->descriptor.iSerialNumber && serial[0]) {
-		/* If we have a serial number, and it's a non-NULL string */
-		make_guid(guid, dev->descriptor.idVendor, 
-			  dev->descriptor.idProduct, serial);
-	} else {
-		/* We don't have a serial number, so we use 0 */
-		make_guid(guid, dev->descriptor.idVendor, 
-			  dev->descriptor.idProduct, "0");
-	}
-
-	/*
-	 * Now check if we have seen this GUID before
-	 * We're looking for a device with a matching GUID that isn't
-	 * already on the system
-	 */
-	ss = us_list;
-	while ((ss != NULL) && 
-	           ((ss->flags & US_FL_DEV_ATTACHED) ||
-		    !GUID_EQUAL(guid, ss->guid)))
-		ss = ss->next;
-
-	if (ss != NULL) {
-		/* Existing device -- re-connect */
-		US_DEBUGP("Found existing GUID " GUID_FORMAT "\n",
-			  GUID_ARGS(guid));
-
-		/* lock the device pointers */
-		down(&(ss->dev_semaphore));
-
-		/* establish the connection to the new device upon reconnect */
-		ss->ifnum = ifnum;
-		ss->pusb_dev = dev;
-		ss->flags |= US_FL_DEV_ATTACHED;
-
-		/* copy over the endpoint data */
-		ss->ep_in = ep_in->bEndpointAddress & 
-			USB_ENDPOINT_NUMBER_MASK;
-		ss->ep_out = ep_out->bEndpointAddress & 
-			USB_ENDPOINT_NUMBER_MASK;
-		if (ep_int) {
-			ss->ep_int = ep_int->bEndpointAddress & 
-				USB_ENDPOINT_NUMBER_MASK;
-			ss->ep_bInterval = ep_int->bInterval;
-		}
-		else
-			ss->ep_int = ss->ep_bInterval = 0;
-
-		/* allocate the URB, the usb_ctrlrequest, and the IRQ URB */
-		if (usb_stor_allocate_urbs(ss))
-			goto BadDevice;
-
-                /* Re-Initialize the device if it needs it */
-		if (unusual_dev && unusual_dev->initFunction)
-			(unusual_dev->initFunction)(ss);
-
-		/* unlock the device pointers */
-		up(&(ss->dev_semaphore));
-
-	} else { 
 	/* New device -- allocate memory and initialize */
-		US_DEBUGP("New GUID " GUID_FORMAT "\n", GUID_ARGS(guid));
-
 	if ((ss = (struct us_data *)kmalloc(sizeof(struct us_data), 
 					    GFP_KERNEL)) == NULL) {
 		printk(KERN_WARNING USB_STORAGE "Out of memory\n");
@@ -758,7 +683,6 @@ static int storage_probe(struct usb_interface *intf,
 		return -ENOMEM;
 	}
 	memset(ss, 0, sizeof(struct us_data));
-		new_device = 1;
 
 	/* Initialize the mutexes only when the struct is new */
 	init_completion(&(ss->notify));
@@ -810,9 +734,6 @@ static int storage_probe(struct usb_interface *intf,
 	if (strlen(ss->serial) == 0)
 		strncpy(ss->serial, "None", USB_STOR_STRING_LEN);
 
-		/* copy the GUID we created before */
-		memcpy(ss->guid, guid, sizeof(guid));
-
 	/* 
 	 * Set the handler pointers based on the protocol
 	 * Again, this data is persistant across reattachments
@@ -968,19 +889,6 @@ static int storage_probe(struct usb_interface *intf,
 	 * host definition, and register with the higher SCSI layers
 	 */
 
-		/* Initialize the host template based on the default one */
-		memcpy(&(ss->htmplt), &usb_stor_host_template, 
-		       sizeof(usb_stor_host_template));
-
-		/* Grab the next host number */
-		ss->host_number = my_host_number++;
-
-		/* We abuse this pointer so we can pass the ss pointer to 
-		 * the host controller thread in us_detect.  But how else are
-		 * we to do it?
-		 */
-		(struct us_data *)ss->htmplt.proc_dir = ss; 
-
 	/* Just before we start our control thread, initialize
 	 * the device if it needs initialization */
 	if (unusual_dev && unusual_dev->initFunction)
@@ -1002,10 +910,9 @@ static int storage_probe(struct usb_interface *intf,
 	/* unlock the device pointers */
 	up(&(ss->dev_semaphore));
 
-		/* now register	 - our detect function will be called */
-		ss->htmplt.module = THIS_MODULE;
-		result = scsi_register_host(&(ss->htmplt));
-		if (result) {
+	/* now register	*/
+	ss->host = scsi_register(&usb_stor_host_template, sizeof(ss));
+	if (ss->host) {
 		printk(KERN_WARNING USB_STORAGE
 			"Unable to register the scsi host\n");
 
@@ -1019,17 +926,25 @@ static int storage_probe(struct usb_interface *intf,
 		goto BadDevice;
 	}
 
-		/* lock access to the data structures */
-		down(&us_list_semaphore);
+	/* now add the host */
+	result = scsi_add_host(ss->host, NULL);
+	if (result) {
+		printk(KERN_WARNING USB_STORAGE
+			"Unable to add the scsi host\n");
 
-		/* put us in the list */
-		ss->next = us_list;
-		us_list = ss;
+		/* tell the control thread to exit */
+		ss->srb = NULL;
+		up(&ss->sema);
+		wait_for_completion(&ss->notify);
 
-		/* release the data structure lock */
-		up(&us_list_semaphore);
+		/* re-lock the device pointers */
+		down(&ss->dev_semaphore);
+		goto BadDevice;
 	}
 
+	ss->host->hostdata[0] = (unsigned long)ss;
+	scsi_set_device(ss->host, &intf->dev);
+
 	printk(KERN_DEBUG 
 	       "WARNING: USB Mass Storage data integrity not assured\n");
 	printk(KERN_DEBUG 
@@ -1041,11 +956,10 @@ static int storage_probe(struct usb_interface *intf,
 
 	/* we come here if there are any problems */
 	/* ss->dev_semaphore must be locked */
-	BadDevice:
+BadDevice:
 	US_DEBUGP("storage_probe() failed\n");
 	usb_stor_deallocate_urbs(ss);
 	up(&ss->dev_semaphore);
-	if (new_device)
 	kfree(ss);
 	return -EIO;
 }
@@ -1065,9 +979,52 @@ static void storage_disconnect(struct usb_interface *intf)
 		return;
 	}
 
+	/* lock device access -- no need to unlock, as we're going away */
 	down(&(ss->dev_semaphore));
+
+	/* remove the pointer to the data structure we were using */
+	(struct us_data*)ss->host->hostdata[0] = NULL;
+
+	/* begin SCSI host removal sequence */
+	if(scsi_remove_host(ss->host)) {
+		US_DEBUGP("-- SCSI refused to unregister\n");
+		BUG();
+		return;
+	};
+
+	/* finish SCSI host removal sequence */
+	scsi_unregister(ss->host);
+
+	/* Kill the control threads
+	 *
+	 * Enqueue the command, wake up the thread, and wait for 
+	 * notification that it has exited.
+	 */
+	US_DEBUGP("-- sending exit command to thread\n");
+	BUG_ON(atomic_read(&ss->sm_state) != US_STATE_IDLE);
+	ss->srb = NULL;
+	up(&(ss->sema));
+	wait_for_completion(&(ss->notify));
+
+	/* free allocated urbs */
 	usb_stor_deallocate_urbs(ss);
-	up(&(ss->dev_semaphore));
+
+	/* If there's extra data in the us_data structure then
+	 * free that first */
+	if (ss->extra) {
+		/* call the destructor routine, if it exists */
+		if (ss->extra_destructor) {
+			US_DEBUGP("-- calling extra_destructor()\n");
+			ss->extra_destructor(ss->extra);
+		}
+
+		/* destroy the extra data */
+		US_DEBUGP("-- freeing the data structure\n");
+		kfree(ss->extra);
+	}
+
+	/* free the structure itself */
+	kfree (ss);
 }
 
 /***********************************************************************
@@ -1078,11 +1035,6 @@ int __init usb_stor_init(void)
 {
 	printk(KERN_INFO "Initializing USB Mass Storage driver...\n");
 
-	/* initialize internal global data elements */
-	us_list = NULL;
-	init_MUTEX(&us_list_semaphore);
-	my_host_number = 0;
-
 	/* register the driver, return -1 if error */
 	if (usb_register(&usb_storage_driver) < 0)
 		return -1;
@@ -1094,16 +1046,16 @@ int __init usb_stor_init(void)
 
 void __exit usb_stor_exit(void)
 {
-	struct us_data *next;
-
 	US_DEBUGP("usb_stor_exit() called\n");
 
 	/* Deregister the driver
-	 * This eliminates races with probes and disconnects 
+	 * This will cause disconnect() to be called for each
+	 * attached unit
 	 */
 	US_DEBUGP("-- calling usb_deregister()\n");
 	usb_deregister(&usb_storage_driver) ;
 
+#if 0
 	/* While there are still virtual hosts, unregister them
 	 * Note that it's important to do this completely before removing
 	 * the structures because of possible races with the /proc
@@ -1111,7 +1063,7 @@ void __exit usb_stor_exit(void)
 	 */
 	for (next = us_list; next; next = next->next) {
 		US_DEBUGP("-- calling scsi_unregister_host()\n");
-		scsi_unregister_host(&(next->htmplt));
+		scsi_unregister_host(&usb_stor_host_template);
 	}
 
 	/* While there are still structures, free them.  Note that we are
@@ -1142,6 +1094,7 @@ void __exit usb_stor_exit(void)
 		/* advance the list pointer */
 		us_list = next;
 	}
+#endif
 }
 
 module_init(usb_stor_init);

drivers/usb/storage/usb.h

@@ -52,33 +52,6 @@
 #include "scsi.h"
 #include "hosts.h"
 
-/* 
- * GUID definitions
- */
-
-#define GUID(x) __u32 x[3]
-#define GUID_EQUAL(x, y) (x[0] == y[0] && x[1] == y[1] && x[2] == y[2])
-#define GUID_CLEAR(x) x[0] = x[1] = x[2] = 0;
-#define GUID_NONE(x) (!x[0] && !x[1] && !x[2])
-#define GUID_FORMAT "%08x%08x%08x"
-#define GUID_ARGS(x) x[0], x[1], x[2]
-
-static inline void make_guid( __u32 *pg, __u16 vendor, __u16 product, char *serial)
-{
-	pg[0] = (vendor << 16) | product;
-	pg[1] = pg[2] = 0;
-	while (*serial) {
-		pg[1] <<= 4;
-		pg[1] |= pg[2] >> 28;
-		pg[2] <<= 4;
-		if (*serial >= 'a')
-			*serial -= 'a' - 'A';
-		pg[2] |= (*serial <= '9' && *serial >= '0') ? *serial - '0'
-			: *serial - 'A' + 10;
-		serial++;
-	}
-}
-
 struct us_data;
 
 /*
@@ -124,8 +97,6 @@ typedef void (*extra_data_destructor)(void *);	 /* extra data destructor   */
 
 /* we allocate one of these for every device that we remember */
 struct us_data {
-	struct us_data		*next;		 /* next device */
-
 	/* The device we're working with
 	 * It's important to note:
 	 *    (o) you must hold dev_semaphore to change pusb_dev
@@ -163,11 +134,7 @@ struct us_data {
 	proto_cmnd		proto_handler;	 /* protocol handler	   */
 
 	/* SCSI interfaces */
-	GUID(guid);				 /* unique dev id	*/
 	struct Scsi_Host	*host;		 /* our dummy host data */
-	Scsi_Host_Template	htmplt;		 /* own host template	*/
-	int			host_number;	 /* to find us		*/
-	int			host_no;	 /* allocated by scsi	*/
 	Scsi_Cmnd		*srb;		 /* current srb		*/
 
 	/* thread information */
@@ -192,10 +159,6 @@ struct us_data {
 	extra_data_destructor	extra_destructor;/* extra data destructor   */
 };
 
-/* The list of structures and the protective lock for them */
-extern struct us_data *us_list;
-extern struct semaphore us_list_semaphore;
-
 /* The structure which defines our driver */
 extern struct usb_driver usb_storage_driver;