# 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 mdharm-usb@one-eyed-alien.net 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)