# 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.588.1.6 -> 1.588.1.7 # drivers/usb/storage/usb.c 1.22 -> 1.23 # drivers/usb/storage/transport.c 1.18 -> 1.19 # drivers/usb/storage/scsiglue.c 1.19 -> 1.20 # drivers/usb/storage/debug.h 1.4 -> 1.5 # drivers/usb/storage/usb.h 1.9 -> 1.10 # # The following is the BitKeeper ChangeSet Log # -------------------------------------------- # 02/05/28 mdharm@one-eyed-alien.net 1.588.1.7 # [PATCH] usb-storage abort path cleanup # # cleanup of abort mechanism. This version should be much more crash # resistant (dare I say crash-proof?) # -------------------------------------------- # diff -Nru a/drivers/usb/storage/debug.h b/drivers/usb/storage/debug.h --- a/drivers/usb/storage/debug.h Tue May 28 23:48:36 2002 +++ b/drivers/usb/storage/debug.h Tue May 28 23:48:36 2002 @@ -4,7 +4,7 @@ * $Id: debug.h,v 1.6 2001/01/12 23:51:04 mdharm Exp $ * * Current development and maintenance by: - * (c) 1999, 2000 Matthew Dharm (mdharm-usb@one-eyed-alien.net) + * (c) 1999-2002 Matthew Dharm (mdharm-usb@one-eyed-alien.net) * * Initial work by: * (c) 1999 Michael Gee (michael@linuxspecific.com) diff -Nru a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c --- a/drivers/usb/storage/scsiglue.c Tue May 28 23:48:36 2002 +++ b/drivers/usb/storage/scsiglue.c Tue May 28 23:48:36 2002 @@ -4,7 +4,7 @@ * $Id: scsiglue.c,v 1.26 2002/04/22 03:39:43 mdharm Exp $ * * Current development and maintenance by: - * (c) 1999, 2000 Matthew Dharm (mdharm-usb@one-eyed-alien.net) + * (c) 1999-2002 Matthew Dharm (mdharm-usb@one-eyed-alien.net) * * Developed with the assistance of: * (c) 2000 David L. Brown, Jr. (usb-storage@davidb.org) @@ -56,9 +56,6 @@ */ #define US_ACT_COMMAND 1 -#define US_ACT_DEVICE_RESET 2 -#define US_ACT_BUS_RESET 3 -#define US_ACT_HOST_RESET 4 #define US_ACT_EXIT 5 /*********************************************************************** diff -Nru a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c --- a/drivers/usb/storage/transport.c Tue May 28 23:48:36 2002 +++ b/drivers/usb/storage/transport.c Tue May 28 23:48:36 2002 @@ -351,6 +351,29 @@ * Data transfer routines ***********************************************************************/ +/* + * 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. + * + * The abort function first sets the machine state, then tries to acquire the + * lock on the current_urb to abort it. + * + * 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. + * + * 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. + * + * 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. + */ + /* This is the completion handler which will wake us up when an URB * completes. */ @@ -363,6 +386,9 @@ /* 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. */ static int usb_stor_msg_common(struct us_data *us) { @@ -385,16 +411,6 @@ return status; } - /* has the current command been aborted? */ - if (atomic_read(&us->sm_state) == US_STATE_ABORTING) { - - /* avoid a race with usb_stor_abort_transport(): - * if the abort took place before we submitted - * the URB, we must cancel it ourselves */ - if (us->current_urb->status == -EINPROGRESS) - usb_unlink_urb(us->current_urb); - } - /* wait for the completion of the URB */ up(&(us->current_urb_sem)); wait_for_completion(&urb_done); @@ -428,6 +444,12 @@ /* 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_CONTROL_URB(us->current_urb, us->pusb_dev, pipe, (unsigned char*) dr, data, size, @@ -456,6 +478,12 @@ /* 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_BULK_URB(us->current_urb, us->pusb_dev, pipe, data, len, usb_stor_blocking_completion, NULL); @@ -831,24 +859,31 @@ /* If the current state is wrong or if there's * no srb, then there's nothing to do */ - if ( !(state == US_STATE_RUNNING || state == US_STATE_RESETTING) - || !us->srb) { - US_DEBUGP("-- invalid current state\n"); - return; - } + BUG_ON((state != US_STATE_RUNNING && state != US_STATE_RESETTING)); + BUG_ON(!(us->srb)); + + /* set state to abort */ atomic_set(&us->sm_state, US_STATE_ABORTING); /* If the state machine is blocked waiting for an URB or an IRQ, - * let's wake it up */ + * let's wake it up */ - /* if we have an URB pending, cancel it */ + /* 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. + */ + down(&(us->current_urb_sem)); if (us->current_urb->status == -EINPROGRESS) { US_DEBUGP("-- cancelling URB\n"); usb_unlink_urb(us->current_urb); } + up(&(us->current_urb_sem)); - /* if we are waiting for an IRQ, simulate it */ - else if (test_bit(IP_WANTED, &us->bitflags)) { + /* If we are waiting for an IRQ, simulate it */ + if (test_bit(IP_WANTED, &us->bitflags)) { US_DEBUGP("-- simulating missing IRQ\n"); usb_stor_CBI_irq(us->irq_urb); } diff -Nru a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c --- a/drivers/usb/storage/usb.c Tue May 28 23:48:36 2002 +++ b/drivers/usb/storage/usb.c Tue May 28 23:48:36 2002 @@ -104,9 +104,6 @@ */ #define US_ACT_COMMAND 1 -#define US_ACT_DEVICE_RESET 2 -#define US_ACT_BUS_RESET 3 -#define US_ACT_HOST_RESET 4 #define US_ACT_EXIT 5 /* The list of structures and the protective lock for them */ @@ -344,10 +341,12 @@ 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); @@ -361,145 +360,131 @@ /* release the queue lock as fast as possible */ spin_unlock_irq(&us->queue_exclusion); - switch (action) { - case US_ACT_COMMAND: - /* reject the command if the direction indicator - * is UNKNOWN - */ - if (us->srb->sc_data_direction == SCSI_DATA_UNKNOWN) { - US_DEBUGP("UNKNOWN data direction\n"); - us->srb->result = DID_ERROR << 16; - scsi_lock(host); - us->srb->scsi_done(us->srb); - us->srb = NULL; - scsi_unlock(host); - break; - } + /* exit if we get a signal to exit */ + if (action == US_ACT_EXIT) { + US_DEBUGP("-- US_ACT_EXIT command received\n"); + break; + } - /* reject if target != 0 or if LUN is higher than - * the maximum known LUN - */ - if (us->srb->target && - !(us->flags & US_FL_SCM_MULT_TARG)) { - US_DEBUGP("Bad target number (%d/%d)\n", - us->srb->target, us->srb->lun); - us->srb->result = DID_BAD_TARGET << 16; - - scsi_lock(host); - us->srb->scsi_done(us->srb); - us->srb = NULL; - scsi_unlock(host); - break; - } + BUG_ON(action != US_ACT_COMMAND); - if (us->srb->lun > us->max_lun) { - US_DEBUGP("Bad LUN (%d/%d)\n", - us->srb->target, us->srb->lun); - us->srb->result = DID_BAD_TARGET << 16; - - scsi_lock(host); - us->srb->scsi_done(us->srb); - us->srb = NULL; - scsi_unlock(host); - break; - } - - /* handle those devices which can't do a START_STOP */ - if ((us->srb->cmnd[0] == START_STOP) && - (us->flags & US_FL_START_STOP)) { - US_DEBUGP("Skipping START_STOP command\n"); - us->srb->result = GOOD << 1; + /* reject the command if the direction indicator + * is UNKNOWN + */ + if (us->srb->sc_data_direction == SCSI_DATA_UNKNOWN) { + US_DEBUGP("UNKNOWN data direction\n"); + us->srb->result = DID_ERROR << 16; + scsi_lock(host); + us->srb->scsi_done(us->srb); + us->srb = NULL; + scsi_unlock(host); + continue; + } - scsi_lock(host); - us->srb->scsi_done(us->srb); - us->srb = NULL; - scsi_unlock(host); - break; - } + /* reject if target != 0 or if LUN is higher than + * the maximum known LUN + */ + if (us->srb->target && + !(us->flags & US_FL_SCM_MULT_TARG)) { + US_DEBUGP("Bad target number (%d/%d)\n", + us->srb->target, us->srb->lun); + us->srb->result = DID_BAD_TARGET << 16; + + scsi_lock(host); + us->srb->scsi_done(us->srb); + us->srb = NULL; + scsi_unlock(host); + continue; + } + + if (us->srb->lun > us->max_lun) { + US_DEBUGP("Bad LUN (%d/%d)\n", + us->srb->target, us->srb->lun); + us->srb->result = DID_BAD_TARGET << 16; + + scsi_lock(host); + us->srb->scsi_done(us->srb); + us->srb = NULL; + scsi_unlock(host); + continue; + } + + /* handle those devices which can't do a START_STOP */ + if ((us->srb->cmnd[0] == START_STOP) && + (us->flags & US_FL_START_STOP)) { + US_DEBUGP("Skipping START_STOP command\n"); + us->srb->result = GOOD << 1; + + scsi_lock(host); + us->srb->scsi_done(us->srb); + us->srb = NULL; + scsi_unlock(host); + continue; + } - /* lock the device pointers */ - down(&(us->dev_semaphore)); + /* lock the device pointers */ + down(&(us->dev_semaphore)); - /* our device has gone - pretend not ready */ - if (atomic_read(&us->sm_state) == US_STATE_DETACHED) { - US_DEBUGP("Request is for removed device\n"); - /* For REQUEST_SENSE, it's the data. But - * for anything else, it should look like - * we auto-sensed for it. - */ - if (us->srb->cmnd[0] == REQUEST_SENSE) { - memcpy(us->srb->request_buffer, - usb_stor_sense_notready, - sizeof(usb_stor_sense_notready)); - us->srb->result = GOOD << 1; - } else if(us->srb->cmnd[0] == INQUIRY) { - unsigned char data_ptr[36] = { - 0x20, 0x80, 0x02, 0x02, - 0x1F, 0x00, 0x00, 0x00}; - US_DEBUGP("Faking INQUIRY command for disconnected device\n"); - fill_inquiry_response(us, data_ptr, 36); - us->srb->result = GOOD << 1; - } else { - memcpy(us->srb->sense_buffer, - usb_stor_sense_notready, - sizeof(usb_stor_sense_notready)); - us->srb->result = CHECK_CONDITION << 1; - } - } else { /* atomic_read(&us->sm_state) == STATE_DETACHED */ - - /* Handle those devices which need us to fake - * their inquiry data */ - if ((us->srb->cmnd[0] == INQUIRY) && - (us->flags & US_FL_FIX_INQUIRY)) { - unsigned char data_ptr[36] = { - 0x00, 0x80, 0x02, 0x02, - 0x1F, 0x00, 0x00, 0x00}; - - US_DEBUGP("Faking INQUIRY command\n"); - fill_inquiry_response(us, data_ptr, 36); - us->srb->result = GOOD << 1; - } else { - /* we've got a command, let's do it! */ - US_DEBUG(usb_stor_show_command(us->srb)); - atomic_set(&us->sm_state, US_STATE_RUNNING); - us->proto_handler(us->srb, us); - atomic_set(&us->sm_state, US_STATE_IDLE); - } + /* our device has gone - pretend not ready */ + if (atomic_read(&us->device_state) == US_STATE_DETACHED) { + US_DEBUGP("Request is for removed device\n"); + /* For REQUEST_SENSE, it's the data. But + * for anything else, it should look like + * we auto-sensed for it. + */ + if (us->srb->cmnd[0] == REQUEST_SENSE) { + memcpy(us->srb->request_buffer, + usb_stor_sense_notready, + sizeof(usb_stor_sense_notready)); + us->srb->result = GOOD << 1; + } else if(us->srb->cmnd[0] == INQUIRY) { + unsigned char data_ptr[36] = { + 0x20, 0x80, 0x02, 0x02, + 0x1F, 0x00, 0x00, 0x00}; + US_DEBUGP("Faking INQUIRY command for disconnected device\n"); + fill_inquiry_response(us, data_ptr, 36); + us->srb->result = GOOD << 1; + } else { + memcpy(us->srb->sense_buffer, + usb_stor_sense_notready, + sizeof(usb_stor_sense_notready)); + us->srb->result = CHECK_CONDITION << 1; } + } else { /* atomic_read(&us->device_state) == STATE_DETACHED */ - /* unlock the device pointers */ - up(&(us->dev_semaphore)); + /* Handle those devices which need us to fake + * their inquiry data */ + if ((us->srb->cmnd[0] == INQUIRY) && + (us->flags & US_FL_FIX_INQUIRY)) { + unsigned char data_ptr[36] = { + 0x00, 0x80, 0x02, 0x02, + 0x1F, 0x00, 0x00, 0x00}; - /* 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); + US_DEBUGP("Faking INQUIRY command\n"); + fill_inquiry_response(us, data_ptr, 36); + us->srb->result = GOOD << 1; } else { - US_DEBUGP("scsi command aborted\n"); - us->srb = NULL; - complete(&(us->notify)); + /* we've got a command, let's do it! */ + US_DEBUG(usb_stor_show_command(us->srb)); + us->proto_handler(us->srb, us); } - break; - - case US_ACT_DEVICE_RESET: - break; - - case US_ACT_BUS_RESET: - break; - - case US_ACT_HOST_RESET: - break; + } - } /* end switch on action */ + /* unlock the device pointers */ + up(&(us->dev_semaphore)); - /* exit if we get a signal to exit */ - if (action == US_ACT_EXIT) { - US_DEBUGP("-- US_ACT_EXIT command received\n"); - break; + /* 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 { + US_DEBUGP("scsi command aborted\n"); + us->srb = NULL; + complete(&(us->notify)); } } /* for (;;) */ @@ -725,7 +710,7 @@ /* establish the connection to the new device upon reconnect */ ss->ifnum = ifnum; ss->pusb_dev = dev; - atomic_set(&ss->sm_state, US_STATE_IDLE); + atomic_set(&ss->device_state, US_STATE_ATTACHED); /* copy over the endpoint data */ if (ep_in) @@ -1016,6 +1001,7 @@ /* start up our control thread */ atomic_set(&ss->sm_state, US_STATE_IDLE); + atomic_set(&ss->device_state, US_STATE_ATTACHED); ss->pid = kernel_thread(usb_stor_control_thread, ss, CLONE_VM); if (ss->pid < 0) { diff -Nru a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h --- a/drivers/usb/storage/usb.h Tue May 28 23:48:36 2002 +++ b/drivers/usb/storage/usb.h Tue May 28 23:48:36 2002 @@ -4,7 +4,7 @@ * $Id: usb.h,v 1.21 2002/04/21 02:57:59 mdharm Exp $ * * Current development and maintenance by: - * (c) 1999, 2000 Matthew Dharm (mdharm-usb@one-eyed-alien.net) + * (c) 1999-2002 Matthew Dharm (mdharm-usb@one-eyed-alien.net) * * Initial work by: * (c) 1999 Michael Gee (michael@linuxspecific.com) @@ -103,11 +103,15 @@ #define US_FL_SCM_MULT_TARG 0x00000020 /* supports multiple targets */ #define US_FL_FIX_INQUIRY 0x00000040 /* INQUIRY response needs fixing */ -#define US_STATE_DETACHED 1 /* State machine states */ -#define US_STATE_IDLE 2 -#define US_STATE_RUNNING 3 -#define US_STATE_RESETTING 4 -#define US_STATE_ABORTING 5 +/* device attached/detached states */ +#define US_STATE_DETACHED 1 +#define US_STATE_ATTACHED 2 + +/* processing state machine states */ +#define US_STATE_IDLE 1 +#define US_STATE_RUNNING 2 +#define US_STATE_RESETTING 3 +#define US_STATE_ABORTING 4 #define USB_STOR_STRING_LEN 32 @@ -120,8 +124,13 @@ struct us_data { struct us_data *next; /* next device */ - /* the device we're working with */ + /* The device we're working with + * It's important to note: + * (o) you must hold dev_semaphore to change pusb_dev + * (o) device_state should change whenever pusb_dev does + */ struct semaphore dev_semaphore; /* protect pusb_dev */ + atomic_t device_state; /* attached or detached */ struct usb_device *pusb_dev; /* this usb_device */ unsigned int flags; /* from filter initially */