[PATCH] usb-storage abort path cleanup

cleanup of abort mechanism.  This version should be much more crash
resistant (dare I say crash-proof?)

drivers/usb/storage/debug.h

@@ -4,7 +4,7 @@
  * $Id: debug.h,v 1.6 2001/01/12 23:51:04 mdharm Exp $
  *
  * Current development and maintenance by:
- *   (c) 1999, 2000 Matthew Dharm (mdharm-usb@one-eyed-alien.net)
+ *   (c) 1999-2002 Matthew Dharm (mdharm-usb@one-eyed-alien.net)
  *
  * Initial work by:
  *   (c) 1999 Michael Gee (michael@linuxspecific.com)

drivers/usb/storage/scsiglue.c

@@ -4,7 +4,7 @@
  * $Id: scsiglue.c,v 1.26 2002/04/22 03:39:43 mdharm Exp $
  *
  * Current development and maintenance by:
- *   (c) 1999, 2000 Matthew Dharm (mdharm-usb@one-eyed-alien.net)
+ *   (c) 1999-2002 Matthew Dharm (mdharm-usb@one-eyed-alien.net)
  *
  * Developed with the assistance of:
  *   (c) 2000 David L. Brown, Jr. (usb-storage@davidb.org)
@@ -56,9 +56,6 @@
  */
 
 #define US_ACT_COMMAND		1
-#define US_ACT_DEVICE_RESET	2
-#define US_ACT_BUS_RESET	3
-#define US_ACT_HOST_RESET	4
 #define US_ACT_EXIT		5
 
 /***********************************************************************

drivers/usb/storage/transport.c

@@ -351,6 +351,29 @@ unsigned int usb_stor_transfer_length(Scsi_Cmnd *srb)
  * Data transfer routines
  ***********************************************************************/
 
+/*
+ * This is subtle, so pay attention:
+ * ---------------------------------
+ * We're very concered about races with a command abort.  Hanging this code is
+ * a sure fire way to hang the kernel.
+ *
+ * The abort function first sets the machine state, then tries to acquire the
+ * lock on the current_urb to abort it.
+ *
+ * Once a function grabs the current_urb_sem, then it -MUST- test the state to
+ * see if we just got aborted before the lock was grabbed.  If so, it's
+ * essential to release the lock and return.
+ *
+ * The idea is that, once the current_urb_sem is held, the state can't really
+ * change anymore without also engaging the usb_unlink_urb() call _after_ the
+ * URB is actually submitted.
+ *
+ * So, we've guaranteed that (after the sm_state test), if we do submit the
+ * URB it will get aborted when we release the current_urb_sem.  And we've
+ * also guaranteed that if the abort code was called before we held the
+ * current_urb_sem, we can safely get out before the URB is submitted.
+ */
+
 /* This is the completion handler which will wake us up when an URB
  * completes.
  */
@@ -363,6 +386,9 @@ static void usb_stor_blocking_completion(struct urb *urb)
 
 /* This is the common part of the URB message submission code
  * This function expects the current_urb_sem to be held upon entry.
+ *
+ * All URBs from the usb-storage driver _must_ pass through this function
+ * (or something like it) for the abort mechanisms to work properly.
  */
 static int usb_stor_msg_common(struct us_data *us)
 {
@@ -385,16 +411,6 @@ static int usb_stor_msg_common(struct us_data *us)
 		return status;
 	}
 
-	/* has the current command been aborted? */
-	if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
-
-		/* avoid a race with usb_stor_abort_transport():
-		 *  if the abort took place before we submitted
-		 *  the URB, we must cancel it ourselves */
-		if (us->current_urb->status == -EINPROGRESS)
-			usb_unlink_urb(us->current_urb);
-		}
-
 	/* wait for the completion of the URB */
 	up(&(us->current_urb_sem));
 	wait_for_completion(&urb_done);
@@ -428,6 +444,12 @@ int usb_stor_control_msg(struct us_data *us, unsigned int pipe,
 	/* lock the URB */
 	down(&(us->current_urb_sem));
 
+	/* has the current command been aborted? */
+	if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+		up(&(us->current_urb_sem));
+		return 0;
+	}
+
 	/* fill the URB */
 	FILL_CONTROL_URB(us->current_urb, us->pusb_dev, pipe, 
 			 (unsigned char*) dr, data, size, 
@@ -456,6 +478,12 @@ int usb_stor_bulk_msg(struct us_data *us, void *data, int pipe,
 	/* lock the URB */
 	down(&(us->current_urb_sem));
 
+	/* has the current command been aborted? */
+	if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+		up(&(us->current_urb_sem));
+		return 0;
+	}
+
 	/* fill the URB */
 	FILL_BULK_URB(us->current_urb, us->pusb_dev, pipe, data, len,
 		      usb_stor_blocking_completion, NULL);
@@ -831,24 +859,31 @@ void usb_stor_abort_transport(struct us_data *us)
 
 	/* If the current state is wrong or if there's
 	 *  no srb, then there's nothing to do */
-	if ( !(state == US_STATE_RUNNING || state == US_STATE_RESETTING)
-		    || !us->srb) {
-		US_DEBUGP("-- invalid current state\n");
-		return;
-	}
+	BUG_ON((state != US_STATE_RUNNING && state != US_STATE_RESETTING));
+	BUG_ON(!(us->srb));
+
+	/* set state to abort */
 	atomic_set(&us->sm_state, US_STATE_ABORTING);
 
 	/* If the state machine is blocked waiting for an URB or an IRQ,
 	 * let's wake it up */
 
-	/* if we have an URB pending, cancel it */
+	/* If we have an URB pending, cancel it.  Note that we guarantee with
+	 * the current_urb_sem that either (a) an URB has just been submitted,
+	 * or (b) the URB will never get submitted.  But, because of the use
+	 * of us->sm_state and current_urb_sem, we can't get an URB sumbitted
+	 * _after_ we set the state to US_STATE_ABORTING and this section of
+	 * code runs.  Thus we avoid deadlocks.
+	 */
+	down(&(us->current_urb_sem));
 	if (us->current_urb->status == -EINPROGRESS) {
 		US_DEBUGP("-- cancelling URB\n");
 		usb_unlink_urb(us->current_urb);
 	}
+	up(&(us->current_urb_sem));
 
-	/* if we are waiting for an IRQ, simulate it */
-	else if (test_bit(IP_WANTED, &us->bitflags)) {
+	/* If we are waiting for an IRQ, simulate it */
+	if (test_bit(IP_WANTED, &us->bitflags)) {
 		US_DEBUGP("-- simulating missing IRQ\n");
 		usb_stor_CBI_irq(us->irq_urb);
 	}

drivers/usb/storage/usb.c

@@ -104,9 +104,6 @@ static int my_host_number;
  */
 
 #define US_ACT_COMMAND		1
-#define US_ACT_DEVICE_RESET	2
-#define US_ACT_BUS_RESET	3
-#define US_ACT_HOST_RESET	4
 #define US_ACT_EXIT		5
 
 /* The list of structures and the protective lock for them */
@@ -344,10 +341,12 @@ static int usb_stor_control_thread(void * __us)
 	for(;;) {
 		struct Scsi_Host *host;
 		US_DEBUGP("*** thread sleeping.\n");
+		atomic_set(&us->sm_state, US_STATE_IDLE);
 		if(down_interruptible(&us->sema))
 			break;
 			
 		US_DEBUGP("*** thread awakened.\n");
+		atomic_set(&us->sm_state, US_STATE_RUNNING);
 
 		/* lock access to the queue element */
 		spin_lock_irq(&us->queue_exclusion);
@@ -361,8 +360,14 @@ static int usb_stor_control_thread(void * __us)
 		/* release the queue lock as fast as possible */
 		spin_unlock_irq(&us->queue_exclusion);
 
-		switch (action) {
-		case US_ACT_COMMAND:
+		/* exit if we get a signal to exit */
+		if (action == US_ACT_EXIT) {
+			US_DEBUGP("-- US_ACT_EXIT command received\n");
+			break;
+		}
+
+		BUG_ON(action != US_ACT_COMMAND);
+
 		/* reject the command if the direction indicator 
 		 * is UNKNOWN
 		 */
@@ -373,7 +378,7 @@ static int usb_stor_control_thread(void * __us)
 			us->srb->scsi_done(us->srb);
 			us->srb = NULL;
 			scsi_unlock(host);
-				break;
+			continue;
 		}
 
 		/* reject if target != 0 or if LUN is higher than
@@ -389,7 +394,7 @@ static int usb_stor_control_thread(void * __us)
 			us->srb->scsi_done(us->srb);
 			us->srb = NULL;
 			scsi_unlock(host);
-				break;
+			continue;
 		}
 
 		if (us->srb->lun > us->max_lun) {
@@ -401,7 +406,7 @@ static int usb_stor_control_thread(void * __us)
 			us->srb->scsi_done(us->srb);
 			us->srb = NULL;
 			scsi_unlock(host);
-				break;
+			continue;
 		}
 
 		/* handle those devices which can't do a START_STOP */
@@ -414,14 +419,14 @@ static int usb_stor_control_thread(void * __us)
 			us->srb->scsi_done(us->srb);
 			us->srb = NULL;
 			scsi_unlock(host);
-				break;
+			continue;
 		}
 
 		/* lock the device pointers */
 		down(&(us->dev_semaphore));
 
 		/* our device has gone - pretend not ready */
-			if (atomic_read(&us->sm_state) == US_STATE_DETACHED) {
+		if (atomic_read(&us->device_state) == US_STATE_DETACHED) {
 			US_DEBUGP("Request is for removed device\n");
 			/* For REQUEST_SENSE, it's the data.  But
 			 * for anything else, it should look like
@@ -445,7 +450,7 @@ static int usb_stor_control_thread(void * __us)
 				       sizeof(usb_stor_sense_notready));
 				us->srb->result = CHECK_CONDITION << 1;
 			}
-			} else { /* atomic_read(&us->sm_state) == STATE_DETACHED */
+		} else { /* atomic_read(&us->device_state) == STATE_DETACHED */
 
 			/* Handle those devices which need us to fake 
 			 * their inquiry data */
@@ -461,9 +466,7 @@ static int usb_stor_control_thread(void * __us)
 			} else {
 				/* we've got a command, let's do it! */
 				US_DEBUG(usb_stor_show_command(us->srb));
-					atomic_set(&us->sm_state, US_STATE_RUNNING);
 				us->proto_handler(us->srb, us);
-					atomic_set(&us->sm_state, US_STATE_IDLE);
 			}
 		}
 
@@ -483,24 +486,6 @@ static int usb_stor_control_thread(void * __us)
 			us->srb = NULL;
 			complete(&(us->notify));
 		}
-			break;
-
-		case US_ACT_DEVICE_RESET:
-			break;
-
-		case US_ACT_BUS_RESET:
-			break;
-
-		case US_ACT_HOST_RESET:
-			break;
-
-		} /* end switch on action */
-
-		/* exit if we get a signal to exit */
-		if (action == US_ACT_EXIT) {
-			US_DEBUGP("-- US_ACT_EXIT command received\n");
-			break;
-		}
 	} /* for (;;) */
 
 	/* notify the exit routine that we're actually exiting now */
@@ -725,7 +710,7 @@ static void * storage_probe(struct usb_device *dev, unsigned int ifnum,
 		/* establish the connection to the new device upon reconnect */
 		ss->ifnum = ifnum;
 		ss->pusb_dev = dev;
-		atomic_set(&ss->sm_state, US_STATE_IDLE);
+		atomic_set(&ss->device_state, US_STATE_ATTACHED);
 
 		/* copy over the endpoint data */
 		if (ep_in)
@@ -1016,6 +1001,7 @@ static void * storage_probe(struct usb_device *dev, unsigned int ifnum,
 
 		/* start up our control thread */
 		atomic_set(&ss->sm_state, US_STATE_IDLE);
+		atomic_set(&ss->device_state, US_STATE_ATTACHED);
 		ss->pid = kernel_thread(usb_stor_control_thread, ss,
 					CLONE_VM);
 		if (ss->pid < 0) {

drivers/usb/storage/usb.h

@@ -4,7 +4,7 @@
  * $Id: usb.h,v 1.21 2002/04/21 02:57:59 mdharm Exp $
  *
  * Current development and maintenance by:
- *   (c) 1999, 2000 Matthew Dharm (mdharm-usb@one-eyed-alien.net)
+ *   (c) 1999-2002 Matthew Dharm (mdharm-usb@one-eyed-alien.net)
  *
  * Initial work by:
  *   (c) 1999 Michael Gee (michael@linuxspecific.com)
@@ -103,11 +103,15 @@ struct us_unusual_dev {
 #define US_FL_SCM_MULT_TARG   0x00000020 /* supports multiple targets */
 #define US_FL_FIX_INQUIRY     0x00000040 /* INQUIRY response needs fixing */
 
-#define US_STATE_DETACHED	1	/* State machine states */
-#define US_STATE_IDLE		2
-#define US_STATE_RUNNING	3
-#define US_STATE_RESETTING	4
-#define US_STATE_ABORTING	5
+/* device attached/detached states */
+#define US_STATE_DETACHED	1
+#define US_STATE_ATTACHED	2
+
+/* processing state machine states */
+#define US_STATE_IDLE		1
+#define US_STATE_RUNNING	2
+#define US_STATE_RESETTING	3
+#define US_STATE_ABORTING	4
 
 #define USB_STOR_STRING_LEN 32
 
@@ -120,8 +124,13 @@ typedef void (*extra_data_destructor)(void *);	 /* extra data destructor   */
 struct us_data {
 	struct us_data		*next;		 /* next device */
 
-	/* the device we're working with */
+	/* The device we're working with
+	 * It's important to note:
+	 *    (o) you must hold dev_semaphore to change pusb_dev
+	 *    (o) device_state should change whenever pusb_dev does
+	 */
 	struct semaphore	dev_semaphore;	 /* protect pusb_dev */
+	atomic_t		device_state;	 /* attached or detached */
 	struct usb_device	*pusb_dev;	 /* this usb_device */
 
 	unsigned int		flags;		 /* from filter initially */