[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.

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 70cac2634164c400ba05cd13b7cbafc230f4d180..f1cded050a09e1c75ce3f41dd014bd0492abbad0 100644 (file)
--- a/drivers/usb/storage/transport.c
+++ b/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)) {

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 67d969a7fa99e467dfcb0096d9c5b8ca82d548e2..ee0ec2a56e98ebd5477d907209dd17b5e46b61b4 100644 (file)
--- a/drivers/usb/storage/usb.c
+++ b/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 */

diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
index 19ecbe4b898eebe9d55a44a306efb13e057f2601..ef6c8ee110596029f1d016451eb91eee5aac235f 100644 (file)
--- a/drivers/usb/storage/usb.h
+++ b/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     */