# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.628 -> 1.629
# drivers/usb/storage/usb.c 1.28 -> 1.29
# drivers/usb/storage/transport.c 1.23 -> 1.24
# drivers/usb/storage/scsiglue.c 1.23 -> 1.24
# drivers/usb/storage/usb.h 1.13 -> 1.14
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/07/07 [EMAIL PROTECTED] 1.629
# [PATCH] PATCH: usb-storage: consolidate, cleanup, etc.
#
# This patch changes how the exit code works to be cleaner, fixes the OOPS on
# rmmod, consolidates some anti-race code from several places to just one,
# and also eliminates some theoretical race conditions.
# --------------------------------------------
#
diff -Nru a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
--- a/drivers/usb/storage/scsiglue.c Sun Jul 7 12:35:47 2002
+++ b/drivers/usb/storage/scsiglue.c Sun Jul 7 12:35:47 2002
@@ -116,9 +116,8 @@
* Enqueue the command, wake up the thread, and wait for
* notification that it's exited.
*/
- US_DEBUGP("-- sending US_ACT_EXIT command to thread\n");
- us->action = US_ACT_EXIT;
-
+ US_DEBUGP("-- sending exit command to thread\n");
+ us->srb = NULL;
up(&(us->sema));
wait_for_completion(&(us->notify));
@@ -138,24 +137,17 @@
}
/* run command */
+/* This is always called with scsi_lock(srb->host) held */
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 */
- spin_lock_irqsave(&us->queue_exclusion, flags);
-
/* enqueue the command */
- us->queue_srb = srb;
srb->scsi_done = done;
- us->action = US_ACT_COMMAND;
-
- /* release the lock on the structure */
- spin_unlock_irqrestore(&us->queue_exclusion, flags);
+ us->srb = srb;
/* wake up the process task */
up(&(us->sema));
@@ -168,28 +160,26 @@
***********************************************************************/
/* Command abort */
+/* This is always called with scsi_lock(srb->host) held */
static int command_abort( Scsi_Cmnd *srb )
{
struct us_data *us = (struct us_data *)srb->host->hostdata[0];
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;
+ /* Is this command still active? */
+ if (us->srb != srb) {
+ US_DEBUGP ("-- nothing to abort\n");
+ return FAILED;
}
- US_DEBUGP ("-- nothing to abort\n");
- return FAILED;
+ usb_stor_abort_transport(us);
+ return SUCCESS;
}
/* This invokes the transport reset mechanism to reset the state of the
* device */
+/* This is always called with scsi_lock(srb->host) held */
static int device_reset( Scsi_Cmnd *srb )
{
struct us_data *us = (struct us_data *)srb->host->hostdata[0];
@@ -197,45 +187,54 @@
US_DEBUGP("device_reset() called\n" );
- /* if the device was removed, then we're already reset */
- if (!(us->flags & US_FL_DEV_ATTACHED))
- return SUCCESS;
-
+ /* set the state and release the lock */
+ atomic_set(&us->sm_state, US_STATE_RESETTING);
scsi_unlock(srb->host);
+
/* lock the device pointers */
down(&(us->dev_semaphore));
- us->srb = srb;
- atomic_set(&us->sm_state, US_STATE_RESETTING);
- result = us->transport_reset(us);
- atomic_set(&us->sm_state, US_STATE_IDLE);
- /* unlock the device pointers */
+ /* if the device was removed, then we're already reset */
+ if (!(us->flags & US_FL_DEV_ATTACHED))
+ result = SUCCESS;
+ else
+ result = us->transport_reset(us);
up(&(us->dev_semaphore));
+
+ /* lock access to the state and clear it */
scsi_lock(srb->host);
+ atomic_set(&us->sm_state, US_STATE_IDLE);
return result;
}
/* This resets the device port, and simulates the device
* disconnect/reconnect for all drivers which have claimed
* interfaces, including ourself. */
+/* This is always called with scsi_lock(srb->host) held */
static int bus_reset( Scsi_Cmnd *srb )
{
struct us_data *us = (struct us_data *)srb->host->hostdata[0];
int i;
int result;
- struct usb_device *pusb_dev_save = us->pusb_dev;
+ struct usb_device *pusb_dev_save;
/* we use the usb_reset_device() function to handle this for us */
US_DEBUGP("bus_reset() called\n");
+ scsi_unlock(srb->host);
+
/* if the device has been removed, this worked */
+ down(&us->dev_semaphore);
if (!(us->flags & US_FL_DEV_ATTACHED)) {
US_DEBUGP("-- device removed already\n");
+ up(&us->dev_semaphore);
+ scsi_lock(srb->host);
return SUCCESS;
}
+ pusb_dev_save = us->pusb_dev;
+ up(&us->dev_semaphore);
/* 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) {
@@ -245,7 +244,7 @@
/* FIXME: This needs to lock out driver probing while it's working
* or we can have race conditions */
- /* Is that still true? I don't see how... AS */
+ /* This functionality really should be provided by the khubd thread */
for (i = 0; i < pusb_dev_save->actconfig->bNumInterfaces; i++) {
struct usb_interface *intf =
&pusb_dev_save->actconfig->interface[i];
diff -Nru a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
--- a/drivers/usb/storage/transport.c Sun Jul 7 12:35:47 2002
+++ b/drivers/usb/storage/transport.c Sun Jul 7 12:35:47 2002
@@ -354,24 +354,35 @@
/*
* This is subtle, so pay attention:
* ---------------------------------
- * We're very concered about races with a command abort. Hanging this code is
- * a sure fire way to hang the kernel.
+ * We're very concerned about races with a command abort. Hanging this code
+ * is a sure fire way to hang the kernel. (Note that this discussion applies
+ * only to transactions resulting from a scsi queued-command, since only
+ * these transactions are subject to a scsi abort. Other transactions, such
+ * as those occurring during device-specific initialization, must be handled
+ * by a separate code path.)
*
- * The abort function first sets the machine state, then tries to acquire the
- * lock on the current_urb to abort it.
+ * The abort function first sets the machine state, then acquires the lock
+ * on the current_urb before checking if it needs to be aborted.
*
- * Once a function grabs the current_urb_sem, then it -MUST- test the state to
- * see if we just got aborted before the lock was grabbed. If so, it's
- * essential to release the lock and return.
+ * 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 idea is that, once the current_urb_sem is held, the state can't really
- * change anymore without also engaging the usb_unlink_urb() call _after_ the
- * URB is actually submitted.
+ * (It's also permissible, but not necessary, to test the state -before-
+ * submitting the URB. Doing so would prevent an unnecessary submission if
+ * the transaction had already been aborted, but this is very unlikely to
+ * happen, because the abort would have to have been requested during actual
+ * kernel processing rather than during an I/O delay.)
*
- * So, we've guaranteed that (after the sm_state test), if we do submit the
- * URB it will get aborted when we release the current_urb_sem. And we've
- * also guaranteed that if the abort code was called before we held the
- * current_urb_sem, we can safely get out before the URB is submitted.
+ * 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() from being called more than once or from being called
+ * during usb_submit_urb().
*/
/* This is the completion handler which will wake us up when an URB
@@ -385,10 +396,10 @@
}
/* This is the common part of the URB message submission code
- * This function expects the current_urb_sem to be held upon entry.
*
- * All URBs from the usb-storage driver _must_ pass through this function
- * (or something like it) for the abort mechanisms to work properly.
+ * All URBs from the usb-storage driver involved in handling a queued scsi
+ * command _must_ pass through this function (or something like it) for the
+ * abort mechanisms to work properly.
*/
static int usb_stor_msg_common(struct us_data *us)
{
@@ -404,17 +415,28 @@
us->current_urb->error_count = 0;
us->current_urb->transfer_flags = USB_ASYNC_UNLINK;
- /* submit the URB */
+ /* lock and submit the URB */
+ down(&(us->current_urb_sem));
status = usb_submit_urb(us->current_urb, GFP_NOIO);
if (status) {
/* something went wrong */
+ up(&(us->current_urb_sem));
return status;
}
- /* wait for the completion of the URB */
+ /* 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) {
+ 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);
- down(&(us->current_urb_sem));
/* return the URB status */
return us->current_urb->status;
@@ -436,29 +458,15 @@
us->dr->wIndex = cpu_to_le16(index);
us->dr->wLength = cpu_to_le16(size);
- /* lock the URB */
- down(&(us->current_urb_sem));
-
- /* has the current command been aborted? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
- up(&(us->current_urb_sem));
- return 0;
- }
-
- /* fill the URB */
+ /* fill and submit the URB */
FILL_CONTROL_URB(us->current_urb, us->pusb_dev, pipe,
(unsigned char*) us->dr, data, size,
usb_stor_blocking_completion, NULL);
-
- /* submit the URB */
status = usb_stor_msg_common(us);
/* return the actual length of the data transferred if no error*/
if (status >= 0)
status = us->current_urb->actual_length;
-
- /* release the lock and return status */
- up(&(us->current_urb_sem));
return status;
}
@@ -470,27 +478,13 @@
{
int status;
- /* lock the URB */
- down(&(us->current_urb_sem));
-
- /* has the current command been aborted? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
- up(&(us->current_urb_sem));
- return 0;
- }
-
- /* fill the URB */
+ /* fill and submit the URB */
FILL_BULK_URB(us->current_urb, us->pusb_dev, pipe, data, len,
usb_stor_blocking_completion, NULL);
-
- /* submit the URB */
status = usb_stor_msg_common(us);
- /* return the actual length of the data transferred */
+ /* store the actual length of the data transferred */
*act_len = us->current_urb->actual_length;
-
- /* release the lock and return status */
- up(&(us->current_urb_sem));
return status;
}
@@ -845,31 +839,27 @@
}
/* Abort the currently running scsi command or device reset.
- */
+ * This must be called with scsi_lock(us->srb->host) held */
void usb_stor_abort_transport(struct us_data *us)
{
int state = atomic_read(&us->sm_state);
US_DEBUGP("usb_stor_abort_transport called\n");
- /* If the current state is wrong or if there's
- * no srb, then there's nothing to do */
- BUG_ON((state != US_STATE_RUNNING && state != US_STATE_RESETTING));
- BUG_ON(!(us->srb));
+ /* Normally the current state is RUNNING. If the control thread
+ * hasn't even started processing this command, the state will be
+ * IDLE. Anything else is a bug. */
- /* set state to abort */
+ /* set state to abort and release the lock */
atomic_set(&us->sm_state, US_STATE_ABORTING);
+ scsi_unlock(us->srb->host);
/* 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 either (a) an URB has just been submitted,
- * or (b) the URB will never get submitted. But, because of the use
- * of us->sm_state and current_urb_sem, we can't get an URB sumbitted
- * _after_ we set the state to US_STATE_ABORTING and this section of
- * code runs. Thus we avoid deadlocks.
- */
+ * the current_urb_sem 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) {
US_DEBUGP("-- cancelling URB\n");
@@ -882,6 +872,9 @@
US_DEBUGP("-- simulating missing IRQ\n");
usb_stor_CBI_irq(us->irq_urb);
}
+
+ /* Reacquire the lock */
+ scsi_lock(us->srb->host);
}
/*
@@ -1397,11 +1390,9 @@
/* return a result code based on the result of the control message */
if (result < 0) {
US_DEBUGP("Soft reset failed: %d\n", result);
- us->srb->result = DID_ERROR << 16;
result = FAILED;
} else {
US_DEBUGP("Soft reset done\n");
- us->srb->result = GOOD << 1;
result = SUCCESS;
}
return result;
diff -Nru a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
--- a/drivers/usb/storage/usb.c Sun Jul 7 12:35:47 2002
+++ b/drivers/usb/storage/usb.c Sun Jul 7 12:35:47 2002
@@ -301,7 +301,6 @@
static int usb_stor_control_thread(void * __us)
{
struct us_data *us = (struct us_data *)__us;
- int action;
lock_kernel();
@@ -334,32 +333,30 @@
for(;;) {
struct Scsi_Host *host;
US_DEBUGP("*** thread sleeping.\n");
- atomic_set(&us->sm_state, US_STATE_IDLE);
if(down_interruptible(&us->sema))
break;
US_DEBUGP("*** thread awakened.\n");
- atomic_set(&us->sm_state, US_STATE_RUNNING);
-
- /* lock access to the queue element */
- spin_lock_irq(&us->queue_exclusion);
- /* take the command off the queue */
- action = us->action;
- us->action = 0;
- us->srb = us->queue_srb;
+ /* if us->srb is NULL, we are being asked to exit */
+ if (us->srb == NULL) {
+ US_DEBUGP("-- exit command received\n");
+ break;
+ }
host = us->srb->host;
- /* release the queue lock as fast as possible */
- spin_unlock_irq(&us->queue_exclusion);
+ /* lock access to the state */
+ scsi_lock(host);
- /* exit if we get a signal to exit */
- if (action == US_ACT_EXIT) {
- US_DEBUGP("-- US_ACT_EXIT command received\n");
- break;
+ /* has the command been aborted *already* ? */
+ if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ us->srb->result = DID_ABORT << 16;
+ goto SkipForAbort;
}
- BUG_ON(action != US_ACT_COMMAND);
+ /* set the state and release the lock */
+ atomic_set(&us->sm_state, US_STATE_RUNNING);
+ scsi_unlock(host);
/* lock the device pointers */
down(&(us->dev_semaphore));
@@ -444,19 +441,29 @@
/* unlock the device pointers */
up(&(us->dev_semaphore));
+ /* lock access to the state */
+ scsi_lock(host);
+
/* indicate that the command is done */
if (us->srb->result != DID_ABORT << 16) {
US_DEBUGP("scsi cmd done, result=0x%x\n",
us->srb->result);
- scsi_lock(host);
us->srb->scsi_done(us->srb);
- us->srb = NULL;
- scsi_unlock(host);
} else {
+ SkipForAbort:
US_DEBUGP("scsi command aborted\n");
- us->srb = NULL;
- complete(&(us->notify));
}
+
+ /* in case an abort request was received after the command
+ * completed, we must use a separate test to see whether
+ * we need to signal that the abort has finished */
+ if (atomic_read(&us->sm_state) == US_STATE_ABORTING)
+ complete(&(us->notify));
+
+ /* empty the queue, reset the state, and release the lock */
+ us->srb = NULL;
+ atomic_set(&us->sm_state, US_STATE_IDLE);
+ scsi_unlock(host);
} /* for (;;) */
/* notify the exit routine that we're actually exiting now */
@@ -478,19 +485,19 @@
int maxp;
int result;
- /* allocate the URB we're going to use */
- US_DEBUGP("Allocating URB\n");
- ss->current_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!ss->current_urb) {
- US_DEBUGP("allocation failed\n");
- return 1;
- }
-
/* allocate the usb_ctrlrequest for control packets */
US_DEBUGP("Allocating usb_ctrlrequest\n");
ss->dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
if (!ss->dr) {
US_DEBUGP("allocation failed\n");
+ return 1;
+ }
+
+ /* allocate the URB we're going to use */
+ US_DEBUGP("Allocating URB\n");
+ ss->current_urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!ss->current_urb) {
+ US_DEBUGP("allocation failed\n");
return 2;
}
@@ -554,12 +561,6 @@
}
up(&(ss->irq_urb_sem));
- /* free the usb_ctrlrequest buffer */
- if (ss->dr) {
- kfree(ss->dr);
- ss->dr = NULL;
- }
-
/* free up the main URB for this device */
if (ss->current_urb) {
US_DEBUGP("-- releasing main URB\n");
@@ -569,6 +570,12 @@
ss->current_urb = NULL;
}
+ /* free the usb_ctrlrequest buffer */
+ if (ss->dr) {
+ kfree(ss->dr);
+ ss->dr = NULL;
+ }
+
/* mark the device as gone */
ss->flags &= ~ US_FL_DEV_ATTACHED;
usb_put_dev(ss->pusb_dev);
@@ -777,10 +784,9 @@
/* Initialize the mutexes only when the struct is new */
init_completion(&(ss->notify));
init_MUTEX_LOCKED(&(ss->ip_waitq));
- spin_lock_init(&ss->queue_exclusion);
init_MUTEX(&(ss->irq_urb_sem));
init_MUTEX(&(ss->current_urb_sem));
- init_MUTEX(&(ss->dev_semaphore));
+ init_MUTEX_LOCKED(&(ss->dev_semaphore));
/* copy over the subclass and protocol data */
ss->subclass = subclass;
@@ -1011,6 +1017,9 @@
/* wait for the thread to start */
wait_for_completion(&(ss->notify));
+ /* unlock the device pointers */
+ up(&(ss->dev_semaphore));
+
/* now register - our detect function will be called */
ss->htmplt.module = THIS_MODULE;
result = scsi_register_host(&(ss->htmplt));
@@ -1019,9 +1028,12 @@
"Unable to register the scsi host\n");
/* tell the control thread to exit */
- ss->action = US_ACT_EXIT;
+ ss->srb = NULL;
up(&ss->sema);
wait_for_completion(&ss->notify);
+
+ /* re-lock the device pointers */
+ down(&ss->dev_semaphore);
goto BadDevice;
}
@@ -1045,13 +1057,13 @@
return ss;
/* we come here if there are any problems */
+ /* ss->dev_semaphore must be locked */
BadDevice:
US_DEBUGP("storage_probe() failed\n");
usb_stor_deallocate_urbs(ss);
+ up(&ss->dev_semaphore);
if (new_device)
kfree(ss);
- else
- up(&ss->dev_semaphore);
return NULL;
}
diff -Nru a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
--- a/drivers/usb/storage/usb.h Sun Jul 7 12:35:47 2002
+++ b/drivers/usb/storage/usb.h Sun Jul 7 12:35:47 2002
@@ -107,10 +107,6 @@
#define US_FLIDX_IP_WANTED 17 /* 0x00020000 is an IRQ expected? */
-/* kernel thread actions */
-#define US_ACT_COMMAND 1
-#define US_ACT_EXIT 5
-
/* processing state machine states */
#define US_STATE_IDLE 1
#define US_STATE_RUNNING 2
@@ -168,8 +164,6 @@
Scsi_Cmnd *srb; /* current srb */
/* thread information */
- Scsi_Cmnd *queue_srb; /* the single queue slot */
- int action; /* what to do */
int pid; /* control thread */
atomic_t sm_state;
@@ -192,7 +186,6 @@
/* mutual exclusion structures */
struct completion notify; /* thread begin/end */
- 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 */
@@ -209,6 +202,8 @@
extern void fill_inquiry_response(struct us_data *us,
unsigned char *data, unsigned int data_len);
+/* The scsi_lock() and scsi_unlock() macros protect the sm_state and the
+ * single queue element srb for write access */
#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)
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
We have stuff for geeks like you.
http://thinkgeek.com/sf
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
