[PATCH] PATCH: usb-storage: consolidate, cleanup, etc.

This patch changes how the exit code works to be cleaner, fixes the OOPS on
rmmod, consolidates some anti-race code from several places to just one,
and also eliminates some theoretical race conditions.

drivers/usb/storage/scsiglue.c

@@ -116,9 +116,8 @@ static int release(struct Scsi_Host *psh)
 	 * Enqueue the command, wake up the thread, and wait for 
 	 * notification that it's exited.
 	 */
-	US_DEBUGP("-- sending US_ACT_EXIT command to thread\n");
-	us->action = US_ACT_EXIT;
-	
+	US_DEBUGP("-- sending exit command to thread\n");
+	us->srb = NULL;
 	up(&(us->sema));
 	wait_for_completion(&(us->notify));
 
@@ -138,24 +137,17 @@ static int command( Scsi_Cmnd *srb )
 }
 
 /* run command */
+/* This is always called with scsi_lock(srb->host) held */
 static int queuecommand( Scsi_Cmnd *srb , void (*done)(Scsi_Cmnd *))
 {
 	struct us_data *us = (struct us_data *)srb->host->hostdata[0];
-	unsigned long flags;
 
 	US_DEBUGP("queuecommand() called\n");
 	srb->host_scribble = (unsigned char *)us;
 
-	/* get exclusive access to the structures we want */
-	spin_lock_irqsave(&us->queue_exclusion, flags);
-
 	/* enqueue the command */
-	us->queue_srb = srb;
 	srb->scsi_done = done;
-	us->action = US_ACT_COMMAND;
-
-	/* release the lock on the structure */
-	spin_unlock_irqrestore(&us->queue_exclusion, flags);
+	us->srb = srb;
 
 	/* wake up the process task */
 	up(&(us->sema));
@@ -168,28 +160,26 @@ 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 )
 {
 	struct us_data *us = (struct us_data *)srb->host->hostdata[0];
 
 	US_DEBUGP("command_abort() called\n");
 
-	if (atomic_read(&us->sm_state) == US_STATE_RUNNING) {
- 		scsi_unlock(srb->host);
-		usb_stor_abort_transport(us);
-
-		/* wait for us to be done */
-		wait_for_completion(&(us->notify));
- 		scsi_lock(srb->host);
-		return SUCCESS;
+	/* Is this command still active? */
+	if (us->srb != srb) {
+		US_DEBUGP ("-- nothing to abort\n");
+		return FAILED;
 	}
 
-	US_DEBUGP ("-- nothing to abort\n");
-	return FAILED;
+	usb_stor_abort_transport(us);
+	return SUCCESS;
 }
 
 /* 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 )
 {
 	struct us_data *us = (struct us_data *)srb->host->hostdata[0];
@@ -197,45 +187,54 @@ static int device_reset( Scsi_Cmnd *srb )
 
 	US_DEBUGP("device_reset() called\n" );
 
-	/* if the device was removed, then we're already reset */
-	if (!(us->flags & US_FL_DEV_ATTACHED))
-		return SUCCESS;
-
+	/* set the state and release the lock */
+	atomic_set(&us->sm_state, US_STATE_RESETTING);
 	scsi_unlock(srb->host);
+
 	/* lock the device pointers */
 	down(&(us->dev_semaphore));
-	us->srb = srb;
-	atomic_set(&us->sm_state, US_STATE_RESETTING);
-	result = us->transport_reset(us);
-	atomic_set(&us->sm_state, US_STATE_IDLE);
 
-	/* unlock the device pointers */
+	/* if the device was removed, then we're already reset */
+	if (!(us->flags & US_FL_DEV_ATTACHED))
+		result = SUCCESS;
+	else
+		result = us->transport_reset(us);
 	up(&(us->dev_semaphore));
+
+	/* lock access to the state and clear it */
 	scsi_lock(srb->host);
+	atomic_set(&us->sm_state, US_STATE_IDLE);
 	return result;
 }
 
 /* This resets the device port, and simulates the device
  * 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 )
 {
 	struct us_data *us = (struct us_data *)srb->host->hostdata[0];
 	int i;
 	int result;
-	struct usb_device *pusb_dev_save = us->pusb_dev;
+	struct usb_device *pusb_dev_save;
 
 	/* we use the usb_reset_device() function to handle this for us */
 	US_DEBUGP("bus_reset() called\n");
 
+	scsi_unlock(srb->host);
+
 	/* if the device has been removed, this worked */
+	down(&us->dev_semaphore);
 	if (!(us->flags & US_FL_DEV_ATTACHED)) {
 		US_DEBUGP("-- device removed already\n");
+		up(&us->dev_semaphore);
+		scsi_lock(srb->host);
 		return SUCCESS;
 	}
+	pusb_dev_save = us->pusb_dev;
+	up(&us->dev_semaphore);
 
 	/* attempt to reset the port */
-	scsi_unlock(srb->host);
 	result = usb_reset_device(pusb_dev_save);
 	US_DEBUGP("usb_reset_device returns %d\n", result);
 	if (result < 0) {
@@ -245,7 +244,7 @@ static int bus_reset( Scsi_Cmnd *srb )
 
 	/* FIXME: This needs to lock out driver probing while it's working
 	 * or we can have race conditions */
-	/* Is that still true?  I don't see how...  AS */
+	/* This functionality really should be provided by the khubd thread */
 	for (i = 0; i < pusb_dev_save->actconfig->bNumInterfaces; i++) {
  		struct usb_interface *intf =
 			&pusb_dev_save->actconfig->interface[i];

drivers/usb/storage/transport.c

@@ -354,24 +354,35 @@ unsigned int usb_stor_transfer_length(Scsi_Cmnd *srb)
 /*
  * 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.
+ * We're very concerned about races with a command abort.  Hanging this code
+ * is a sure fire way to hang the kernel.  (Note that this discussion applies
+ * only to transactions resulting from a scsi queued-command, since only
+ * these transactions are subject to a scsi abort.  Other transactions, such
+ * as those occurring during device-specific initialization, must be handled
+ * by a separate code path.)
  *
- * The abort function first sets the machine state, then tries to acquire the
- * lock on the current_urb to abort it.
+ * The abort function first sets the machine state, then acquires the lock
+ * on the current_urb before checking if it needs to be aborted.
  *
- * 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.
+ * When a function submits the current_urb, it must first grab the
+ * current_urb_sem to prevent the abort function from trying to cancel the
+ * URB while the submit call is underway.  After a function submits the
+ * current_urb, it -MUST- test the state to see if we got aborted just before
+ * the submission.  If so, it's essential to abort the URB if it's still in
+ * progress.  Either way, the function must then release the lock and wait
+ * for the URB to finish.
  *
- * 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.
+ * (It's also permissible, but not necessary, to test the state -before-
+ * submitting the URB.  Doing so would prevent an unnecessary submission if
+ * the transaction had already been aborted, but this is very unlikely to
+ * happen, because the abort would have to have been requested during actual
+ * kernel processing rather than during an I/O delay.)
  *
- * 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.
+ * The idea is that (1) once the state is changed to ABORTING, either the
+ * aborting function or the submitting function is guaranteed to call
+ * usb_unlink_urb() for an active URB, and (2) current_urb_sem prevents
+ * usb_unlink_urb() from being called more than once or from being called
+ * during usb_submit_urb().
  */
 
 /* This is the completion handler which will wake us up when an URB
@@ -385,10 +396,10 @@ 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.
+ * All URBs from the usb-storage driver involved in handling a queued scsi
+ * command _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)
 {
@@ -404,17 +415,28 @@ static int usb_stor_msg_common(struct us_data *us)
 	us->current_urb->error_count = 0;
 	us->current_urb->transfer_flags = USB_ASYNC_UNLINK;
 
-	/* submit the URB */
+	/* lock and submit the URB */
+	down(&(us->current_urb_sem));
 	status = usb_submit_urb(us->current_urb, GFP_NOIO);
 	if (status) {
 		/* something went wrong */
+		up(&(us->current_urb_sem));
 		return status;
 	}
 
-	/* wait for the completion of the URB */
+	/* has the current command been aborted? */
+	if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+
+		/* cancel the URB, if it hasn't been cancelled already */
+		if (us->current_urb->status == -EINPROGRESS) {
+			US_DEBUGP("-- cancelling URB\n");
+			usb_unlink_urb(us->current_urb);
+		}
+	}
 	up(&(us->current_urb_sem));
+
+	/* wait for the completion of the URB */
 	wait_for_completion(&urb_done);
-	down(&(us->current_urb_sem));
 
 	/* return the URB status */
 	return us->current_urb->status;
@@ -436,29 +458,15 @@ int usb_stor_control_msg(struct us_data *us, unsigned int pipe,
 	us->dr->wIndex = cpu_to_le16(index);
 	us->dr->wLength = cpu_to_le16(size);
 
-	/* 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 and submit the URB */
 	FILL_CONTROL_URB(us->current_urb, us->pusb_dev, pipe, 
 			 (unsigned char*) us->dr, data, size, 
 			 usb_stor_blocking_completion, NULL);
-
-	/* submit the URB */
 	status = usb_stor_msg_common(us);
 
 	/* return the actual length of the data transferred if no error*/
 	if (status >= 0)
 		status = us->current_urb->actual_length;
-
-	/* release the lock and return status */
-	up(&(us->current_urb_sem));
 	return status;
 }
 
@@ -470,27 +478,13 @@ int usb_stor_bulk_msg(struct us_data *us, void *data, int pipe,
 {
 	int status;
 
-	/* 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 and submit the URB */
 	FILL_BULK_URB(us->current_urb, us->pusb_dev, pipe, data, len,
 		      usb_stor_blocking_completion, NULL);
-
-	/* submit the URB */
 	status = usb_stor_msg_common(us);
 
-	/* return the actual length of the data transferred */
+	/* store the actual length of the data transferred */
 	*act_len = us->current_urb->actual_length;
-
-	/* release the lock and return status */
-	up(&(us->current_urb_sem));
 	return status;
 }
 
@@ -845,31 +839,27 @@ void usb_stor_invoke_transport(Scsi_Cmnd *srb, struct us_data *us)
 }
 
 /* Abort the currently running scsi command or device reset.
- */
+ * This must be called with scsi_lock(us->srb->host) held */
 void usb_stor_abort_transport(struct us_data *us)
 {
 	int state = atomic_read(&us->sm_state);
 
 	US_DEBUGP("usb_stor_abort_transport called\n");
 
-	/* If the current state is wrong or if there's
-	 *  no srb, then there's nothing to do */
-	BUG_ON((state != US_STATE_RUNNING && state != US_STATE_RESETTING));
-	BUG_ON(!(us->srb));
+	/* Normally the current state is RUNNING.  If the control thread
+	 * hasn't even started processing this command, the state will be
+	 * IDLE.  Anything else is a bug. */
 
-	/* set state to abort */
+	/* set state to abort and release the lock */
 	atomic_set(&us->sm_state, US_STATE_ABORTING);
+	scsi_unlock(us->srb->host);
 
 	/* 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.  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.
-	 */
+	 * the current_urb_sem that if a URB has just been submitted, it
+	 * won't be cancelled more than once. */
 	down(&(us->current_urb_sem));
 	if (us->current_urb->status == -EINPROGRESS) {
 		US_DEBUGP("-- cancelling URB\n");
@@ -882,6 +872,9 @@ void usb_stor_abort_transport(struct us_data *us)
 		US_DEBUGP("-- simulating missing IRQ\n");
 		usb_stor_CBI_irq(us->irq_urb);
 	}
+
+	/* Reacquire the lock */
+	scsi_lock(us->srb->host);
 }
 
 /*
@@ -1397,11 +1390,9 @@ static int usb_stor_reset_common(struct us_data *us,
 	/* return a result code based on the result of the control message */
 	if (result < 0) {
 		US_DEBUGP("Soft reset failed: %d\n", result);
-		us->srb->result = DID_ERROR << 16;
 		result = FAILED;
 	} else {
 		US_DEBUGP("Soft reset done\n");
-		us->srb->result = GOOD << 1;
 		result = SUCCESS;
 	}
 	return result;

drivers/usb/storage/usb.c

@@ -301,7 +301,6 @@ void fill_inquiry_response(struct us_data *us, unsigned char *data,
 static int usb_stor_control_thread(void * __us)
 {
 	struct us_data *us = (struct us_data *)__us;
-	int action;
 
 	lock_kernel();
 
@@ -334,32 +333,30 @@ 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);
 
-		/* take the command off the queue */
-		action = us->action;
-		us->action = 0;
-		us->srb = us->queue_srb;
+		/* if us->srb is NULL, we are being asked to exit */
+		if (us->srb == NULL) {
+			US_DEBUGP("-- exit command received\n");
+			break;
+		}
 		host = us->srb->host;
 
-		/* release the queue lock as fast as possible */
-		spin_unlock_irq(&us->queue_exclusion);
+		/* lock access to the state */
+		scsi_lock(host);
 
-		/* exit if we get a signal to exit */
-		if (action == US_ACT_EXIT) {
-			US_DEBUGP("-- US_ACT_EXIT command received\n");
-			break;
+		/* has the command been aborted *already* ? */
+		if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+			us->srb->result = DID_ABORT << 16;
+			goto SkipForAbort;
 		}
 
-		BUG_ON(action != US_ACT_COMMAND);
+		/* set the state and release the lock */
+		atomic_set(&us->sm_state, US_STATE_RUNNING);
+		scsi_unlock(host);
 
 		/* lock the device pointers */
 		down(&(us->dev_semaphore));
@@ -444,19 +441,29 @@ static int usb_stor_control_thread(void * __us)
 		/* unlock the device pointers */
 		up(&(us->dev_semaphore));
 
+		/* lock access to the state */
+		scsi_lock(host);
+
 		/* indicate that the command is done */
 		if (us->srb->result != DID_ABORT << 16) {
 			US_DEBUGP("scsi cmd done, result=0x%x\n", 
 				   us->srb->result);
-			scsi_lock(host);
 			us->srb->scsi_done(us->srb);
-			us->srb = NULL;
-			scsi_unlock(host);
 		} else {
+			SkipForAbort:
 			US_DEBUGP("scsi command aborted\n");
-			us->srb = NULL;
-			complete(&(us->notify));
 		}
+
+		/* in case an abort request was received after the command
+		 * completed, we must use a separate test to see whether
+		 * we need to signal that the abort has finished */
+		if (atomic_read(&us->sm_state) == US_STATE_ABORTING)
+			complete(&(us->notify));
+
+		/* empty the queue, reset the state, and release the lock */
+		us->srb = NULL;
+		atomic_set(&us->sm_state, US_STATE_IDLE);
+		scsi_unlock(host);
 	} /* for (;;) */
 
 	/* notify the exit routine that we're actually exiting now */
@@ -478,18 +485,18 @@ static int usb_stor_allocate_urbs(struct us_data *ss)
 	int maxp;
 	int result;
 
-	/* allocate the URB we're going to use */
-	US_DEBUGP("Allocating URB\n");
-	ss->current_urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!ss->current_urb) {
-		US_DEBUGP("allocation failed\n");
-		return 1;
-	}
-
 	/* allocate the usb_ctrlrequest for control packets */
 	US_DEBUGP("Allocating usb_ctrlrequest\n");
 	ss->dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
 	if (!ss->dr) {
+		US_DEBUGP("allocation failed\n");
+		return 1;
+	}
+
+	/* allocate the URB we're going to use */
+	US_DEBUGP("Allocating URB\n");
+	ss->current_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!ss->current_urb) {
 		US_DEBUGP("allocation failed\n");
 		return 2;
 	}
@@ -554,12 +561,6 @@ static void usb_stor_deallocate_urbs(struct us_data *ss)
 	}
 	up(&(ss->irq_urb_sem));
 
-	/* free the usb_ctrlrequest buffer */
-	if (ss->dr) {
-		kfree(ss->dr);
-		ss->dr = NULL;
-	}
-
 	/* free up the main URB for this device */
 	if (ss->current_urb) {
 		US_DEBUGP("-- releasing main URB\n");
@@ -569,6 +570,12 @@ static void usb_stor_deallocate_urbs(struct us_data *ss)
 		ss->current_urb = NULL;
 	}
 
+	/* free the usb_ctrlrequest buffer */
+	if (ss->dr) {
+		kfree(ss->dr);
+		ss->dr = NULL;
+	}
+
 	/* mark the device as gone */
 	ss->flags &= ~ US_FL_DEV_ATTACHED;
 	usb_put_dev(ss->pusb_dev);
@@ -777,10 +784,9 @@ static void * storage_probe(struct usb_device *dev, unsigned int ifnum,
 		/* Initialize the mutexes only when the struct is new */
 		init_completion(&(ss->notify));
 		init_MUTEX_LOCKED(&(ss->ip_waitq));
-		spin_lock_init(&ss->queue_exclusion);
 		init_MUTEX(&(ss->irq_urb_sem));
 		init_MUTEX(&(ss->current_urb_sem));
-		init_MUTEX(&(ss->dev_semaphore));
+		init_MUTEX_LOCKED(&(ss->dev_semaphore));
 
 		/* copy over the subclass and protocol data */
 		ss->subclass = subclass;
@@ -1011,6 +1017,9 @@ static void * storage_probe(struct usb_device *dev, unsigned int ifnum,
 		/* wait for the thread to start */
 		wait_for_completion(&(ss->notify));
 
+		/* 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));
@@ -1019,9 +1028,12 @@ static void * storage_probe(struct usb_device *dev, unsigned int ifnum,
 				"Unable to register the scsi host\n");
 
 			/* tell the control thread to exit */
-			ss->action = US_ACT_EXIT;
+			ss->srb = NULL;
 			up(&ss->sema);
 			wait_for_completion(&ss->notify);
+
+			/* re-lock the device pointers */
+			down(&ss->dev_semaphore);
 			goto BadDevice;
 		}
 
@@ -1045,13 +1057,13 @@ static void * storage_probe(struct usb_device *dev, unsigned int ifnum,
 	return ss;
 
 	/* we come here if there are any problems */
+	/* ss->dev_semaphore must be locked */
 	BadDevice:
 	US_DEBUGP("storage_probe() failed\n");
 	usb_stor_deallocate_urbs(ss);
+	up(&ss->dev_semaphore);
 	if (new_device)
 		kfree(ss);
-	else
-		up(&ss->dev_semaphore);
 	return NULL;
 }
 

drivers/usb/storage/usb.h

@@ -107,10 +107,6 @@ struct us_unusual_dev {
 #define US_FLIDX_IP_WANTED   17  /* 0x00020000	is an IRQ expected?	    */
 
 
-/* kernel thread actions */
-#define US_ACT_COMMAND		1
-#define US_ACT_EXIT		5
-
 /* processing state machine states */
 #define US_STATE_IDLE		1
 #define US_STATE_RUNNING	2
@@ -168,8 +164,6 @@ struct us_data {
 	Scsi_Cmnd		*srb;		 /* current srb		*/
 
 	/* thread information */
-	Scsi_Cmnd		*queue_srb;	 /* the single queue slot */
-	int			action;		 /* what to do		  */
 	int			pid;		 /* control thread	  */
 	atomic_t		sm_state;
 
@@ -192,7 +186,6 @@ struct us_data {
 
 	/* mutual exclusion structures */
 	struct completion	notify;		 /* thread begin/end	    */
-	spinlock_t		queue_exclusion; /* to protect data structs */
 	struct us_unusual_dev   *unusual_dev;	 /* If unusual device       */
 	void			*extra;		 /* Any extra data          */
 	extra_data_destructor	extra_destructor;/* extra data destructor   */
@@ -209,6 +202,8 @@ extern struct usb_driver usb_storage_driver;
 extern void fill_inquiry_response(struct us_data *us,
 	unsigned char *data, unsigned int data_len);
 
+/* The scsi_lock() and scsi_unlock() macros protect the sm_state and the
+ * single queue element srb for write access */
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,3)
 #define scsi_unlock(host)	spin_unlock_irq(host->host_lock)
 #define scsi_lock(host)		spin_lock_irq(host->host_lock)