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

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;
         }

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

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;
         }

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;
 }
 

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));

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