[PATCH] USB storage: remove tests against EINPROGRESS

This patch removes tests of urb->status for EINPROGRESS.  As was pointed
out, that's not such a good idea, for a variety of reasons.

In the process, a semaphore became useless.

drivers/usb/storage/transport.c

@@ -370,16 +370,20 @@ unsigned int usb_stor_transfer_length(Scsi_Cmnd *srb)
  * as those occurring during device-specific initialization, must be handled
  * by a separate code path.)
  *
- * The abort function first sets the machine state, then acquires the lock
- * on the current_urb before checking if it needs to be aborted.
+ * The abort function first sets the machine state, then atomically
+ * tests-and-clears the CAN_CANCEL bit in us->flags to see if the current_urb
+ * needs to be aborted.
  *
- * 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 submit function first verifies that the submission completed without
+ * errors, and only then sets the CAN_CANCEL bit.  This prevents the abort
+ * function from trying to cancel the URB while the submit call is underway.
+ * Next, the submit function must test the state to see if we got aborted
+ * before the submission or before setting the CAN_CANCEL bit.  If so, it's
+ * essential to abort the URB if it hasn't been cancelled already (i.e.,
+ * if the CAN_CANCEL bit is still set).  Either way, the function must then
+ * wait for the URB to finish.  Note that because the USB_ASYNC_UNLINK flag
+ * is set, the URB can still be in progress even after a call to
+ * usb_unlink_urb() returns.
  *
  * (It's also permissible, but not necessary, to test the state -before-
  * submitting the URB.  Doing so would prevent an unnecessary submission if
@@ -389,7 +393,7 @@ unsigned int usb_stor_transfer_length(Scsi_Cmnd *srb)
  *
  * 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() for an active URB, and (2) test_and_clear_bit() prevents
  * usb_unlink_urb() from being called more than once or from being called
  * during usb_submit_urb().
  */
@@ -424,28 +428,30 @@ static int usb_stor_msg_common(struct us_data *us)
 	us->current_urb->error_count = 0;
 	us->current_urb->transfer_flags = USB_ASYNC_UNLINK;
 
-	/* lock and submit the URB */
-	down(&(us->current_urb_sem));
+	/* submit the URB */
 	status = usb_submit_urb(us->current_urb, GFP_NOIO);
 	if (status) {
 		/* something went wrong */
-		up(&(us->current_urb_sem));
 		return status;
 	}
 
+	/* since the URB has been submitted successfully, it's now okay
+	 * to cancel it */
+	set_bit(US_FLIDX_CAN_CANCEL, &us->flags);
+
 	/* 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) {
+		if (test_and_clear_bit(US_FLIDX_CAN_CANCEL, &us->flags)) {
 			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);
+	clear_bit(US_FLIDX_CAN_CANCEL, &us->flags);
 
 	/* return the URB status */
 	return us->current_urb->status;
@@ -867,15 +873,13 @@ void usb_stor_abort_transport(struct us_data *us)
 	/* 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 if a URB has just been submitted, it
+	/* If we have an URB pending, cancel it.  The test_and_clear_bit()
+	 * call guarantees 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) {
+	if (test_and_clear_bit(US_FLIDX_CAN_CANCEL, &us->flags)) {
 		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 */
 	if (test_bit(US_FLIDX_IP_WANTED, &us->flags)) {

drivers/usb/storage/usb.c

@@ -825,7 +825,6 @@ static void * storage_probe(struct usb_device *dev, unsigned int ifnum,
 		init_completion(&(ss->notify));
 		init_MUTEX_LOCKED(&(ss->ip_waitq));
 		init_MUTEX(&(ss->irq_urb_sem));
-		init_MUTEX(&(ss->current_urb_sem));
 		init_MUTEX_LOCKED(&(ss->dev_semaphore));
 
 		/* copy over the subclass and protocol data */

drivers/usb/storage/usb.h

@@ -106,6 +106,7 @@ struct us_unusual_dev {
 
 #define US_FL_DEV_ATTACHED    0x00010000 /* is the device attached?	    */
 #define US_FLIDX_IP_WANTED   17  /* 0x00020000	is an IRQ expected?	    */
+#define US_FLIDX_CAN_CANCEL  18  /* 0x00040000  okay to cancel current_urb? */
 
 
 /* processing state machine states */
@@ -177,7 +178,6 @@ struct us_data {
 	unsigned char		irqdata[2];	 /* data from USB IRQ	 */
 
 	/* control and bulk communications data */
-	struct semaphore	current_urb_sem; /* protect current_urb  */
 	struct urb		*current_urb;	 /* non-int USB requests */
 	struct usb_ctrlrequest	*dr;		 /* control requests	 */