[PATCH] USB Storage: Remove unnecessary state testing

This patch started as as405 from Alan Stern.  It has been re-generated
against the current tip of the BK tree.

For quite a while we've had a bunch of state-transition testing code in the
driver, to report if anything bad ever happens (like the SCSI midlayer
trying to queue a second command before the first one finishes).  None of
those tests triggered in a very long time; this aspect of the code appears
to be extremely stable.

So this patch removes all those tests for illegal values of us->sm_state.
It turns out that sm_state was used only for one other purpose: to check
whether a command had timed out and caused a SCSI abort.  That piece of
information can easily be stored in a single new bitflag (which is called
calling US_FLIDX_TIMED_OUT) and doing so makes us->sm_state completely
unused.  Hence the patch removes it from the structure.
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Matthew Dharm <mdharm-usb@one-eyed-alien.net>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

drivers/usb/storage/isd200.c

@@ -555,7 +555,7 @@ static void isd200_invoke_transport( struct us_data *us,
 	/* if the command gets aborted by the higher layers, we need to
 	 * short-circuit all other processing
 	 */
-	if (us->sm_state == US_STATE_ABORTING) {
+	if (test_bit(US_FLIDX_TIMED_OUT, &us->flags)) {
 		US_DEBUGP("-- command was aborted\n");
 		goto Handle_Abort;
 	}
@@ -602,7 +602,7 @@ static void isd200_invoke_transport( struct us_data *us,
 
 	if (need_auto_sense) {
 		result = isd200_read_regs(us);
-		if (us->sm_state == US_STATE_ABORTING) {
+		if (test_bit(US_FLIDX_TIMED_OUT, &us->flags)) {
 			US_DEBUGP("-- auto-sense aborted\n");
 			goto Handle_Abort;
 		}

drivers/usb/storage/scsiglue.c

@@ -173,10 +173,9 @@ static int queuecommand(struct scsi_cmnd *srb,
 	srb->host_scribble = (unsigned char *)us;
 
 	/* check for state-transition errors */
-	if (us->sm_state != US_STATE_IDLE || us->srb != NULL) {
-		printk(KERN_ERR USB_STORAGE "Error in %s: " 
-			"state = %d, us->srb = %p\n",
-			__FUNCTION__, us->sm_state, us->srb);
+	if (us->srb != NULL) {
+		printk(KERN_ERR USB_STORAGE "Error in %s: us->srb = %p\n",
+			__FUNCTION__, us->srb);
 		return SCSI_MLQUEUE_HOST_BUSY;
 	}
 
@@ -200,7 +199,7 @@ static int queuecommand(struct scsi_cmnd *srb,
  * Error handling functions
  ***********************************************************************/
 
-/* Command abort */
+/* Command timeout and abort */
 /* This is always called with scsi_lock(srb->host) held */
 static int command_abort(struct scsi_cmnd *srb )
 {
@@ -215,22 +214,12 @@ static int command_abort(struct scsi_cmnd *srb )
 		return FAILED;
 	}
 
-	/* 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. */
-	if (us->sm_state != US_STATE_RUNNING
-				&& us->sm_state != US_STATE_IDLE) {
-		printk(KERN_ERR USB_STORAGE "Error in %s: "
-			"invalid state %d\n", __FUNCTION__, us->sm_state);
-		return FAILED;
-	}
-
-	/* Set state to ABORTING and set the ABORTING bit, but only if
+	/* Set the TIMED_OUT bit.  Also set the ABORTING bit, but only if
 	 * a device reset isn't already in progress (to avoid interfering
 	 * with the reset).  To prevent races with auto-reset, we must
 	 * stop any ongoing USB transfers while still holding the host
 	 * lock. */
-	us->sm_state = US_STATE_ABORTING;
+	set_bit(US_FLIDX_TIMED_OUT, &us->flags);
 	if (!test_bit(US_FLIDX_RESETTING, &us->flags)) {
 		set_bit(US_FLIDX_ABORTING, &us->flags);
 		usb_stor_stop_transport(us);
@@ -243,6 +232,7 @@ static int command_abort(struct scsi_cmnd *srb )
 	/* Reacquire the lock and allow USB transfers to resume */
 	scsi_lock(host);
 	clear_bit(US_FLIDX_ABORTING, &us->flags);
+	clear_bit(US_FLIDX_TIMED_OUT, &us->flags);
 	return SUCCESS;
 }
 
@@ -255,14 +245,7 @@ static int device_reset(struct scsi_cmnd *srb)
 	int result;
 
 	US_DEBUGP("%s called\n", __FUNCTION__);
-	if (us->sm_state != US_STATE_IDLE) {
-		printk(KERN_ERR USB_STORAGE "Error in %s: "
-			"invalid state %d\n", __FUNCTION__, us->sm_state);
-		return FAILED;
-	}
 
-	/* set the state and release the lock */
-	us->sm_state = US_STATE_RESETTING;
 	scsi_unlock(srb->device->host);
 
 	/* lock the device pointers and do the reset */
@@ -274,9 +257,8 @@ static int device_reset(struct scsi_cmnd *srb)
 		result = us->transport_reset(us);
 	up(&(us->dev_semaphore));
 
-	/* lock access to the state and clear it */
+	/* lock the host for the return */
 	scsi_lock(srb->device->host);
-	us->sm_state = US_STATE_IDLE;
 	return result;
 }
 
@@ -290,14 +272,7 @@ static int bus_reset(struct scsi_cmnd *srb)
 	int result, rc;
 
 	US_DEBUGP("%s called\n", __FUNCTION__);
-	if (us->sm_state != US_STATE_IDLE) {
-		printk(KERN_ERR USB_STORAGE "Error in %s: "
-			"invalid state %d\n", __FUNCTION__, us->sm_state);
-		return FAILED;
-	}
 
-	/* set the state and release the lock */
-	us->sm_state = US_STATE_RESETTING;
 	scsi_unlock(srb->device->host);
 
 	/* The USB subsystem doesn't handle synchronisation between
@@ -325,9 +300,8 @@ static int bus_reset(struct scsi_cmnd *srb)
 	}
 	up(&(us->dev_semaphore));
 
-	/* lock access to the state and clear it */
+	/* lock the host for the return */
 	scsi_lock(srb->device->host);
-	us->sm_state = US_STATE_IDLE;
 	return result < 0 ? FAILED : SUCCESS;
 }
 

drivers/usb/storage/transport.c

@@ -537,7 +537,7 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 	/* if the command gets aborted by the higher layers, we need to
 	 * short-circuit all other processing
 	 */
-	if (us->sm_state == US_STATE_ABORTING) {
+	if (test_bit(US_FLIDX_TIMED_OUT, &us->flags)) {
 		US_DEBUGP("-- command was aborted\n");
 		goto Handle_Abort;
 	}
@@ -665,7 +665,7 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 		srb->cmd_len = old_cmd_len;
 		memcpy(srb->cmnd, old_cmnd, MAX_COMMAND_SIZE);
 
-		if (us->sm_state == US_STATE_ABORTING) {
+		if (test_bit(US_FLIDX_TIMED_OUT, &us->flags)) {
 			US_DEBUGP("-- auto-sense aborted\n");
 			goto Handle_Abort;
 		}

drivers/usb/storage/usb.c

@@ -318,8 +318,8 @@ static int usb_stor_control_thread(void * __us)
 		/* lock access to the state */
 		scsi_lock(host);
 
-		/* has the command been aborted *already* ? */
-		if (us->sm_state == US_STATE_ABORTING) {
+		/* has the command timed out *already* ? */
+		if (test_bit(US_FLIDX_TIMED_OUT, &us->flags)) {
 			us->srb->result = DID_ABORT << 16;
 			goto SkipForAbort;
 		}
@@ -330,8 +330,6 @@ static int usb_stor_control_thread(void * __us)
 			goto SkipForDisconnect;
 		}
 
-		/* set the state and release the lock */
-		us->sm_state = US_STATE_RUNNING;
 		scsi_unlock(host);
 
 		/* reject the command if the direction indicator 
@@ -392,16 +390,15 @@ static int usb_stor_control_thread(void * __us)
 
 		/* If an abort request was received we need to signal that
 		 * the abort has finished.  The proper test for this is
-		 * sm_state == US_STATE_ABORTING, not srb->result == DID_ABORT,
-		 * because an abort request might be received after all the
+		 * the TIMED_OUT flag, not srb->result == DID_ABORT, because
+		 * a timeout/abort request might be received after all the
 		 * USB processing was complete. */
-		if (us->sm_state == US_STATE_ABORTING)
+		if (test_bit(US_FLIDX_TIMED_OUT, &us->flags))
 			complete(&(us->notify));
 
-		/* empty the queue, reset the state, and release the lock */
+		/* finished working on this command */
 SkipForDisconnect:
 		us->srb = NULL;
-		us->sm_state = US_STATE_IDLE;
 		scsi_unlock(host);
 
 		/* unlock the device pointers */
@@ -801,7 +798,6 @@ static int usb_stor_acquire_resources(struct us_data *us)
 	us->host->hostdata[0] = (unsigned long) us;
 
 	/* Start up our control thread */
-	us->sm_state = US_STATE_IDLE;
 	p = kernel_thread(usb_stor_control_thread, us, CLONE_VM);
 	if (p < 0) {
 		printk(KERN_WARNING USB_STORAGE 
@@ -829,7 +825,6 @@ void usb_stor_release_resources(struct us_data *us)
 		/* Wait for the thread to be idle */
 		down(&us->dev_semaphore);
 		US_DEBUGP("-- sending exit command to thread\n");
-		BUG_ON(us->sm_state != US_STATE_IDLE);
 
 		/* If the SCSI midlayer queued a final command just before
 		 * scsi_remove_host() was called, us->srb might not be

drivers/usb/storage/usb.h

@@ -83,14 +83,9 @@ struct us_unusual_dev {
 #define ABORTING_OR_DISCONNECTING	((1UL << US_FLIDX_ABORTING) | \
 					 (1UL << US_FLIDX_DISCONNECTING))
 #define US_FLIDX_RESETTING	22  /* 0x00400000  device reset in progress */
+#define US_FLIDX_TIMED_OUT	23  /* 0x00800000  SCSI midlayer timed out  */
 
 
-/* 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
 
 /*
@@ -148,7 +143,6 @@ struct us_data {
 
 	/* thread information */
 	int			pid;		 /* control thread	 */
-	int			sm_state;	 /* what we are doing	 */
 
 	/* control and bulk communications data */
 	struct urb		*current_urb;	 /* USB requests	 */