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.
* further sg[].address conversions. I've added a macro to facilitate
code sharing between 2.4 and 2.5. What are your plans for that?
* 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.
I've tried to send the patch to the usb-storage list, but it that list
is a member-only list.
I've tested the patch with my SaroTech usb disk (bulk device).
--
Manfred
# This is a BitKeeper generated patch for the following project:
# Project Name: greg k-h's linux 2.5 USB kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.533 -> 1.534
# drivers/usb/storage/datafab.c 1.4 -> 1.5
# drivers/usb/storage/scsiglue.c 1.8 -> 1.9
# drivers/usb/storage/usb.h 1.3 -> 1.4
# drivers/usb/storage/isd200.c 1.4 -> 1.5
# drivers/usb/storage/jumpshot.c 1.6 -> 1.7
# drivers/usb/storage/usb.c 1.13 -> 1.14
# drivers/usb/storage/shuttle_usbat.c 1.5 -> 1.6
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/05/12 [EMAIL PROTECTED] 1.534
# usb-storage cleanup:
# - page address
# - locking
# --------------------------------------------
#
diff -Nru a/drivers/usb/storage/datafab.c b/drivers/usb/storage/datafab.c
--- a/drivers/usb/storage/datafab.c Sun May 12 14:54:14 2002
+++ b/drivers/usb/storage/datafab.c Sun May 12 14:54:14 2002
@@ -265,7 +265,7 @@
while (sg_idx < use_sg && transferred < len) {
if (len - transferred >= sg[sg_idx].length -
current_sg_offset) {
US_DEBUGP("datafab_read_data: adding %d bytes
to %d byte sg buffer\n", sg[sg_idx].length - current_sg_offset, sg[sg_idx].length);
- memcpy(sg[sg_idx].address + current_sg_offset,
+ memcpy(sg_address(&sg[sg_idx]) +
+current_sg_offset,
buffer + transferred,
sg[sg_idx].length - current_sg_offset);
transferred += sg[sg_idx].length -
current_sg_offset;
@@ -274,7 +274,7 @@
++sg_idx;
} else {
US_DEBUGP("datafab_read_data: adding %d bytes
to %d byte sg buffer\n", len - transferred, sg[sg_idx].length);
- memcpy(sg[sg_idx].address + current_sg_offset,
+ memcpy(sg_address(&sg[sg_idx]) +
+current_sg_offset,
buffer + transferred,
len - transferred);
current_sg_offset += len - transferred;
@@ -356,7 +356,7 @@
if (len - transferred >= sg[sg_idx].length -
current_sg_offset) {
US_DEBUGP("datafab_write_data: getting %d
bytes from %d byte sg buffer\n", sg[sg_idx].length - current_sg_offset,
sg[sg_idx].length);
memcpy(ptr + transferred,
- sg[sg_idx].address + current_sg_offset,
+ sg_address(&sg[sg_idx]) +
+current_sg_offset,
sg[sg_idx].length - current_sg_offset);
transferred += sg[sg_idx].length -
current_sg_offset;
current_sg_offset = 0;
@@ -365,7 +365,7 @@
} else {
US_DEBUGP("datafab_write_data: getting %d
bytes from %d byte sg buffer\n", len - transferred, sg[sg_idx].length);
memcpy(ptr + transferred,
- sg[sg_idx].address + current_sg_offset,
+ sg_address(&sg[sg_idx]) +
+current_sg_offset,
len - transferred);
current_sg_offset += len - transferred;
// we only copied part of this sg buffer
@@ -822,7 +822,7 @@
srb->result = SUCCESS;
} else {
info->sense_key = UNIT_ATTENTION;
- srb->result = CHECK_CONDITION;
+ srb->result = CHECK_CONDITION << 1;
}
return rc;
}
diff -Nru a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
--- a/drivers/usb/storage/isd200.c Sun May 12 14:54:14 2002
+++ b/drivers/usb/storage/isd200.c Sun May 12 14:54:14 2002
@@ -864,7 +864,7 @@
* 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 -Nru a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
--- a/drivers/usb/storage/jumpshot.c Sun May 12 14:54:14 2002
+++ b/drivers/usb/storage/jumpshot.c Sun May 12 14:54:14 2002
@@ -341,7 +341,7 @@
while (sg_idx < use_sg && transferred < len) {
if (len - transferred >= sg[sg_idx].length -
current_sg_offset) {
US_DEBUGP("jumpshot_read_data: adding %d
bytes to %d byte sg buffer\n", sg[sg_idx].length - current_sg_offset,
sg[sg_idx].length);
- memcpy(sg[sg_idx].address + current_sg_offset,
+ memcpy(sg_address(&sg[sg_idx]) +
+current_sg_offset,
buffer + transferred,
sg[sg_idx].length - current_sg_offset);
transferred += sg[sg_idx].length -
current_sg_offset;
@@ -350,7 +350,7 @@
++sg_idx;
} else {
US_DEBUGP("jumpshot_read_data: adding %d
bytes to %d byte sg buffer\n", len - transferred, sg[sg_idx].length);
- memcpy(sg[sg_idx].address + current_sg_offset,
+ memcpy(sg_address(&sg[sg_idx]) +
+current_sg_offset,
buffer + transferred,
len - transferred);
current_sg_offset += len - transferred;
@@ -423,7 +423,7 @@
if (len - transferred >= sg[sg_idx].length -
current_sg_offset) {
US_DEBUGP("jumpshot_write_data: getting %d
bytes from %d byte sg buffer\n", sg[sg_idx].length - current_sg_offset,
sg[sg_idx].length);
memcpy(ptr + transferred,
- sg[sg_idx].address + current_sg_offset,
+ sg_address(&sg[sg_idx]) +
+current_sg_offset,
sg[sg_idx].length - current_sg_offset);
transferred += sg[sg_idx].length -
current_sg_offset;
current_sg_offset = 0;
@@ -432,7 +432,7 @@
} else {
US_DEBUGP("jumpshot_write_data: getting %d
bytes from %d byte sg buffer\n", len - transferred, sg[sg_idx].length);
memcpy(ptr + transferred,
- sg[sg_idx].address + current_sg_offset,
+ sg_address(&sg[sg_idx]) +
+current_sg_offset,
len - transferred);
current_sg_offset += len - transferred;
// we only copied part of this sg buffer
@@ -821,7 +821,7 @@
srb->result = SUCCESS;
} else {
info->sense_key = UNIT_ATTENTION;
- srb->result = CHECK_CONDITION;
+ srb->result = CHECK_CONDITION << 1;
}
return rc;
}
diff -Nru a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
--- a/drivers/usb/storage/scsiglue.c Sun May 12 14:54:14 2002
+++ b/drivers/usb/storage/scsiglue.c Sun May 12 14:54:14 2002
@@ -70,7 +70,11 @@
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 @@
/* 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 @@
*
* 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 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 @@
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 @@
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 @@
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 @@
/* unlock the device pointers */
up(&(us->dev_semaphore));
+ scsi_lock(srb->host);
return result;
}
@@ -234,10 +244,13 @@
}
/* 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 @@
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 host_reset( Scsi_Cmnd *srb )
{
printk(KERN_CRIT "usb-storage: host_reset() requested but not implemented\n" );
+ bus_reset(srb);
return FAILED;
}
diff -Nru a/drivers/usb/storage/shuttle_usbat.c b/drivers/usb/storage/shuttle_usbat.c
--- a/drivers/usb/storage/shuttle_usbat.c Sun May 12 14:54:14 2002
+++ b/drivers/usb/storage/shuttle_usbat.c Sun May 12 14:54:14 2002
@@ -743,7 +743,7 @@
if (len - amount >=
sg[sg_segment].length-sg_offset) {
memcpy(page_address(sg[sg_segment].page) +
- sg[sg_sgement].offset + sg_offset,
+ sg[sg_segment].offset + sg_offset,
buffer + amount,
sg[sg_segment].length - sg_offset);
amount +=
diff -Nru a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
--- a/drivers/usb/storage/usb.c Sun May 12 14:54:14 2002
+++ b/drivers/usb/storage/usb.c Sun May 12 14:54:14 2002
@@ -334,9 +334,9 @@
/* 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;
@@ -344,15 +344,16 @@
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:
@@ -362,9 +363,10 @@
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;
}
@@ -377,9 +379,10 @@
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;
}
@@ -388,9 +391,10 @@
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;
}
@@ -400,9 +404,10 @@
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;
}
@@ -463,14 +468,15 @@
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:
@@ -491,9 +497,6 @@
}
} /* for (;;) */
- /* clean up after ourselves */
- set_current_state(TASK_INTERRUPTIBLE);
-
/* notify the exit routine that we're actually exiting now */
complete(&(us->notify));
@@ -770,7 +773,7 @@
/* 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 -Nru a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
--- a/drivers/usb/storage/usb.h Sun May 12 14:54:14 2002
+++ b/drivers/usb/storage/usb.h Sun May 12 14:54:14 2002
@@ -180,7 +180,7 @@
/* 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 */
@@ -196,5 +196,17 @@
/* Function to fill an inquiry response. See usb.c for details */
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
