[PATCH] usb-storage locking fixes

I found several SMP and UP locking errors in usb-storage, attached is a
patch:

Changes:
* srb->result is a bitfield, several << 1 were missing.
* add scsi_lock calls around midlayer calls, release the lock before
  calling usb functions that might sleep.
* replace the queue semaphore with a queue spinlocks, queuecommand is
  called from bh context.

diff --git a/drivers/usb/storage/datafab.c b/drivers/usb/storage/datafab.c
index 539722a92d5bd164e0fa6786c1a2971a9c1d9764..bb915868853cc31cdb393f6e7834b932574d189f 100644 (file)
--- a/drivers/usb/storage/datafab.c
+++ b/drivers/usb/storage/datafab.c
@@ -822,7 +822,7 @@ int datafab_transport(Scsi_Cmnd * srb, struct us_data *us)
                        srb->result = SUCCESS;
                } else {
                        info->sense_key = UNIT_ATTENTION;
-                       srb->result = CHECK_CONDITION;
+                       srb->result = CHECK_CONDITION << 1;
                }
                return rc;
         }

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 6258520240c0aafc8781069a42f2003a8e2dbbcb..ba87e2285d7e38016bc664601ce65ac7204a9232 100644 (file)
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -864,7 +864,7 @@ void isd200_invoke_transport( struct us_data *us,
         * condition, show that in the result code
         */
        if (transferStatus == ISD200_TRANSPORT_FAILED)
-               srb->result = CHECK_CONDITION;
+               srb->result = CHECK_CONDITION << 1;
 }
 
 #ifdef CONFIG_USB_STORAGE_DEBUG

diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
index 3e39e05389e20cb1fddf6648864b219500bab2d6..faaa0417da4293f4ec6c0066811076ae0faaab97 100644 (file)
--- a/drivers/usb/storage/jumpshot.c
+++ b/drivers/usb/storage/jumpshot.c
@@ -821,7 +821,7 @@ int jumpshot_transport(Scsi_Cmnd * srb, struct us_data *us)
                        srb->result = SUCCESS;
                } else {
                        info->sense_key = UNIT_ATTENTION;
-                       srb->result = CHECK_CONDITION;
+                       srb->result = CHECK_CONDITION << 1;
                }
                return rc;
         }

diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 0ffdcf90ac998fc103b2b9ec9eae7c22a160a307..8aedbe34aba37d0b1977d5b373f5f7f0361f14a0 100644 (file)
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -70,7 +70,11 @@ static const char* host_info(struct Scsi_Host *host)
        return "SCSI emulation for USB Mass Storage devices";
 }
 
-/* detect a virtual adapter (always works) */
+/* detect a virtual adapter (always works)
+ * Synchronization: 2.4: with the io_request_lock
+ *                     2.5: no locks.
+ * fortunately we don't care.
+ * */
 static int detect(struct SHT *sht)
 {
        struct us_data *us;
@@ -82,7 +86,7 @@ static int detect(struct SHT *sht)
 
        /* set up the name of our subdirectory under /proc/scsi/ */
        sprintf(local_name, "usb-storage-%d", us->host_number);
-       sht->proc_name = kmalloc (strlen(local_name) + 1, GFP_KERNEL);
+       sht->proc_name = kmalloc (strlen(local_name) + 1, GFP_ATOMIC);
        if (!sht->proc_name)
                return 0;
        strcpy(sht->proc_name, local_name);
@@ -108,6 +112,7 @@ static int detect(struct SHT *sht)
  *
  * NOTE: There is no contention here, because we're already deregistered
  * the driver and we're doing each virtual host in turn, not in parallel
+ * Synchronization: BLK, no spinlock.
  */
 static int release(struct Scsi_Host *psh)
 {
@@ -145,12 +150,13 @@ static int command( Scsi_Cmnd *srb )
 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 */
-       down(&(us->queue_exclusion));
+       spin_lock_irqsave(&us->queue_exclusion, flags);
 
        /* enqueue the command */
        us->queue_srb = srb;
@@ -158,7 +164,7 @@ static int queuecommand( Scsi_Cmnd *srb , void (*done)(Scsi_Cmnd *))
        us->action = US_ACT_COMMAND;
 
        /* release the lock on the structure */
-       up(&(us->queue_exclusion));
+       spin_unlock_irqrestore(&us->queue_exclusion, flags);
 
        /* wake up the process task */
        up(&(us->sema));
@@ -178,10 +184,12 @@ static int command_abort( Scsi_Cmnd *srb )
        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;
        }
 
@@ -202,6 +210,7 @@ static int device_reset( Scsi_Cmnd *srb )
        if (atomic_read(&us->sm_state) == US_STATE_DETACHED)
                return SUCCESS;
 
+       scsi_unlock(srb->host);
        /* lock the device pointers */
        down(&(us->dev_semaphore));
        us->srb = srb;
@@ -211,6 +220,7 @@ static int device_reset( Scsi_Cmnd *srb )
 
        /* unlock the device pointers */
        up(&(us->dev_semaphore));
+       scsi_lock(srb->host);
        return result;
 }
 
@@ -234,10 +244,13 @@ static int bus_reset( Scsi_Cmnd *srb )
        }
 
        /* 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)
+       if (result < 0) {
+               scsi_lock(srb->host);
                return FAILED;
+       }
 
        /* FIXME: This needs to lock out driver probing while it's working
         * or we can have race conditions */
@@ -262,8 +275,8 @@ static int bus_reset( Scsi_Cmnd *srb )
                intf->driver->probe(pusb_dev_save, i, id);
                up(&intf->driver->serialize);
        }
-
        US_DEBUGP("bus_reset() complete\n");
+       scsi_lock(srb->host);
        return SUCCESS;
 }
 
@@ -271,6 +284,7 @@ static int bus_reset( Scsi_Cmnd *srb )
 static int host_reset( Scsi_Cmnd *srb )
 {
        printk(KERN_CRIT "usb-storage: host_reset() requested but not implemented\n" );
+       bus_reset(srb);
        return FAILED;
 }
 

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 121f1679d13ea0648da6a2e31a0e3c6bf01a10dc..fc1f5593740f009370363c3a0b20866d9958a1f8 100644 (file)
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -337,9 +337,9 @@ static int usb_stor_control_thread(void * __us)
 
        /* signal that we've started the thread */
        complete(&(us->notify));
-       set_current_state(TASK_INTERRUPTIBLE);
 
        for(;;) {
+               struct Scsi_Host *host;
                US_DEBUGP("*** thread sleeping.\n");
                if(down_interruptible(&us->sema))
                        break;
@@ -347,15 +347,16 @@ static int usb_stor_control_thread(void * __us)
                US_DEBUGP("*** thread awakened.\n");
 
                /* lock access to the queue element */
-               down(&(us->queue_exclusion));
+               spin_lock_irq(&us->queue_exclusion);
 
                /* take the command off the queue */
                action = us->action;
                us->action = 0;
                us->srb = us->queue_srb;
+               host = us->srb->host;
 
                /* release the queue lock as fast as possible */
-               up(&(us->queue_exclusion));
+               spin_unlock_irq(&us->queue_exclusion);
 
                switch (action) {
                case US_ACT_COMMAND:
@@ -365,9 +366,10 @@ static int usb_stor_control_thread(void * __us)
                        if (us->srb->sc_data_direction == SCSI_DATA_UNKNOWN) {
                                US_DEBUGP("UNKNOWN data direction\n");
                                us->srb->result = DID_ERROR << 16;
-                               set_current_state(TASK_INTERRUPTIBLE);
+                               scsi_lock(host);
                                us->srb->scsi_done(us->srb);
                                us->srb = NULL;
+                               scsi_unlock(host);
                                break;
                        }
 
@@ -380,9 +382,10 @@ static int usb_stor_control_thread(void * __us)
                                          us->srb->target, us->srb->lun);
                                us->srb->result = DID_BAD_TARGET << 16;
 
-                               set_current_state(TASK_INTERRUPTIBLE);
+                               scsi_lock(host);
                                us->srb->scsi_done(us->srb);
                                us->srb = NULL;
+                               scsi_unlock(host);
                                break;
                        }
 
@@ -391,9 +394,10 @@ static int usb_stor_control_thread(void * __us)
                                          us->srb->target, us->srb->lun);
                                us->srb->result = DID_BAD_TARGET << 16;
 
-                               set_current_state(TASK_INTERRUPTIBLE);
+                               scsi_lock(host);
                                us->srb->scsi_done(us->srb);
                                us->srb = NULL;
+                               scsi_unlock(host);
                                break;
                        }
 
@@ -403,9 +407,10 @@ static int usb_stor_control_thread(void * __us)
                                US_DEBUGP("Skipping START_STOP command\n");
                                us->srb->result = GOOD << 1;
 
-                               set_current_state(TASK_INTERRUPTIBLE);
+                               scsi_lock(host);
                                us->srb->scsi_done(us->srb);
                                us->srb = NULL;
+                               scsi_unlock(host);
                                break;
                        }
 
@@ -466,14 +471,15 @@ static int usb_stor_control_thread(void * __us)
                        if (us->srb->result != DID_ABORT << 16) {
                                US_DEBUGP("scsi cmd done, result=0x%x\n", 
                                           us->srb->result);
-                               set_current_state(TASK_INTERRUPTIBLE);
+                               scsi_lock(host);
                                us->srb->scsi_done(us->srb);
+                               us->srb = NULL;
+                               scsi_unlock(host);
                        } else {
                                US_DEBUGP("scsi command aborted\n");
-                               set_current_state(TASK_INTERRUPTIBLE);
+                               us->srb = NULL;
                                complete(&(us->notify));
                        }
-                       us->srb = NULL;
                        break;
 
                case US_ACT_DEVICE_RESET:
@@ -494,9 +500,6 @@ static int usb_stor_control_thread(void * __us)
                }
        } /* for (;;) */
 
-       /* clean up after ourselves */
-       set_current_state(TASK_INTERRUPTIBLE);
-
        /* notify the exit routine that we're actually exiting now */
        complete(&(us->notify));
 
@@ -773,7 +776,7 @@ 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));
-               init_MUTEX(&(ss->queue_exclusion));
+               spin_lock_init(&ss->queue_exclusion);
                init_MUTEX(&(ss->irq_urb_sem));
                init_MUTEX(&(ss->current_urb_sem));
                init_MUTEX(&(ss->dev_semaphore));

diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
index 0763b157e4af2d85ea3b22dd6f06b82e8d72450a..1136449b3913b3cdb7d1c2291d620875cb6f78c1 100644 (file)
--- a/drivers/usb/storage/usb.h
+++ b/drivers/usb/storage/usb.h
@@ -180,7 +180,7 @@ struct us_data {
 
        /* mutual exclusion structures */
        struct completion       notify;          /* thread begin/end        */
-       struct semaphore        queue_exclusion; /* to protect data structs */
+       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   */
@@ -197,4 +197,16 @@ extern struct usb_driver usb_storage_driver;
 extern void fill_inquiry_response(struct us_data *us,
        unsigned char *data, unsigned int data_len);
 
+#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)
+
+#define sg_address(psg)                (page_address((psg)->page) + (psg)->offset)
+#else
+#define scsi_unlock(host)      spin_unlock_irq(&io_request_lock)
+#define scsi_lock(host)                spin_lock_irq(&io_request_lock)
+
+#define sg_address(psg)                ((psg)->address)
+#endif
+
 #endif