ChangeSet 1.1318.4.3, 2003/06/16 14:53:28-07:00, mdharm-usb@one-eyed-alien.net [PATCH] USB storage: cleanups Some minor cleanups. First, some locking in the bus-reset. Next, we move current_sg into struct us_data (why make more memory allocation issues for ourselves?). Next, we change sm_state into a normal variable, since it shouldn't require atomic_t anytmore. Finally, we remove some references to a couple of flags that don't do anything anymore. # Fix device locking during the bus-reset routine. # # Embed current_sg in struct us_data. # # Make us->sm_state a regular int instead of an atomic_t. # # Remove a couple of references to the START_STOP and IGNORE_SER # flag bits. drivers/usb/storage/isd200.c | 31 +++++++++++++----- drivers/usb/storage/scsiglue.c | 68 ++++++++++++++++------------------------ drivers/usb/storage/transport.c | 35 +++++++++++--------- drivers/usb/storage/usb.c | 23 ++++--------- drivers/usb/storage/usb.h | 4 +- 5 files changed, 80 insertions(+), 81 deletions(-) diff -Nru a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c --- a/drivers/usb/storage/isd200.c Wed Jun 18 11:15:27 2003 +++ b/drivers/usb/storage/isd200.c Wed Jun 18 11:15:27 2003 @@ -548,10 +548,9 @@ /* if the command gets aborted by the higher layers, we need to * short-circuit all other processing */ - if (atomic_read(&us->sm_state) == US_STATE_ABORTING) { - US_DEBUGP("-- transport indicates command was aborted\n"); - srb->result = DID_ABORT << 16; - return; + if (us->sm_state == US_STATE_ABORTING) { + US_DEBUGP("-- command was aborted\n"); + goto Handle_Abort; } switch (transferStatus) { @@ -561,6 +560,11 @@ srb->result = SAM_STAT_GOOD; break; + case USB_STOR_TRANSPORT_NO_SENSE: + US_DEBUGP("-- transport indicates protocol failure\n"); + srb->result = SAM_STAT_CHECK_CONDITION; + return; + case USB_STOR_TRANSPORT_FAILED: US_DEBUGP("-- transport indicates command failure\n"); need_auto_sense = 1; @@ -591,10 +595,9 @@ if (need_auto_sense) { result = isd200_read_regs(us); - if (atomic_read(&us->sm_state) == US_STATE_ABORTING) { + if (us->sm_state == US_STATE_ABORTING) { US_DEBUGP("-- auto-sense aborted\n"); - srb->result = DID_ABORT << 16; - return; + goto Handle_Abort; } if (result == ISD200_GOOD) { isd200_build_sense(us, srb); @@ -603,8 +606,10 @@ /* If things are really okay, then let's show that */ if ((srb->sense_buffer[2] & 0xf) == 0x0) srb->result = SAM_STAT_GOOD; - } else + } else { srb->result = DID_ERROR << 16; + /* Need reset here */ + } } /* Regardless of auto-sense, if we _know_ we have an error @@ -612,6 +617,16 @@ */ if (transferStatus == USB_STOR_TRANSPORT_FAILED) srb->result = SAM_STAT_CHECK_CONDITION; + return; + + /* abort processing: the bulk-only transport requires a reset + * following an abort */ + Handle_Abort: + srb->result = DID_ABORT << 16; + + /* permit the reset transfer to take place */ + clear_bit(US_FLIDX_ABORTING, &us->flags); + /* Need reset here */ } #ifdef CONFIG_USB_STORAGE_DEBUG diff -Nru a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c --- a/drivers/usb/storage/scsiglue.c Wed Jun 18 11:15:27 2003 +++ b/drivers/usb/storage/scsiglue.c Wed Jun 18 11:15:27 2003 @@ -141,16 +141,15 @@ static int usb_storage_queuecommand( Scsi_Cmnd *srb , void (*done)(Scsi_Cmnd *)) { struct us_data *us = (struct us_data *)srb->device->host->hostdata[0]; - int state = atomic_read(&us->sm_state); US_DEBUGP("queuecommand() called\n"); srb->host_scribble = (unsigned char *)us; /* enqueue the command */ - if (state != US_STATE_IDLE || us->srb != NULL) { + if (us->sm_state != US_STATE_IDLE || us->srb != NULL) { printk(KERN_ERR USB_STORAGE "Error in %s: " "state = %d, us->srb = %p\n", - __FUNCTION__, state, us->srb); + __FUNCTION__, us->sm_state, us->srb); return SCSI_MLQUEUE_HOST_BUSY; } @@ -173,7 +172,6 @@ { struct Scsi_Host *host = srb->device->host; struct us_data *us = (struct us_data *) host->hostdata[0]; - int state = atomic_read(&us->sm_state); US_DEBUGP("%s called\n", __FUNCTION__); @@ -186,20 +184,20 @@ /* 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. */ - if (state != US_STATE_RUNNING && state != US_STATE_IDLE) { + if (us->sm_state != US_STATE_RUNNING + && us->sm_state != US_STATE_IDLE) { printk(KERN_ERR USB_STORAGE "Error in %s: " - "invalid state %d\n", __FUNCTION__, state); + "invalid state %d\n", __FUNCTION__, us->sm_state); return FAILED; } /* Set state to ABORTING, set the ABORTING bit, and release the lock */ - atomic_set(&us->sm_state, US_STATE_ABORTING); + us->sm_state = US_STATE_ABORTING; set_bit(US_FLIDX_ABORTING, &us->flags); scsi_unlock(host); - /* If the state was RUNNING, stop an ongoing USB transfer */ - if (state == US_STATE_RUNNING) - usb_stor_stop_transport(us); + /* Stop an ongoing USB transfer */ + usb_stor_stop_transport(us); /* Wait for the aborted command to finish */ wait_for_completion(&us->notify); @@ -216,32 +214,27 @@ static int usb_storage_device_reset( Scsi_Cmnd *srb ) { struct us_data *us = (struct us_data *)srb->device->host->hostdata[0]; - int state = atomic_read(&us->sm_state); int result; US_DEBUGP("%s called\n", __FUNCTION__); - if (state != US_STATE_IDLE) { + if (us->sm_state != US_STATE_IDLE) { printk(KERN_ERR USB_STORAGE "Error in %s: " - "invalid state %d\n", __FUNCTION__, state); + "invalid state %d\n", __FUNCTION__, us->sm_state); return FAILED; } /* set the state and release the lock */ - atomic_set(&us->sm_state, US_STATE_RESETTING); + us->sm_state = US_STATE_RESETTING; scsi_unlock(srb->device->host); - /* lock the device pointers */ + /* lock the device pointers and do the reset */ down(&(us->dev_semaphore)); - - /* do the reset */ result = us->transport_reset(us); - - /* unlock */ up(&(us->dev_semaphore)); /* lock access to the state and clear it */ scsi_lock(srb->device->host); - atomic_set(&us->sm_state, US_STATE_IDLE); + us->sm_state = US_STATE_IDLE; return result; } @@ -252,42 +245,39 @@ static int usb_storage_bus_reset( Scsi_Cmnd *srb ) { struct us_data *us = (struct us_data *)srb->device->host->hostdata[0]; - int state = atomic_read(&us->sm_state); int result; US_DEBUGP("%s called\n", __FUNCTION__); - if (state != US_STATE_IDLE) { + if (us->sm_state != US_STATE_IDLE) { printk(KERN_ERR USB_STORAGE "Error in %s: " - "invalid state %d\n", __FUNCTION__, state); + "invalid state %d\n", __FUNCTION__, us->sm_state); return FAILED; } /* set the state and release the lock */ - atomic_set(&us->sm_state, US_STATE_RESETTING); + us->sm_state = US_STATE_RESETTING; scsi_unlock(srb->device->host); /* The USB subsystem doesn't handle synchronisation between - a device's several drivers. Therefore we reset only devices - with just one interface, which we of course own. - */ - - //FIXME: needs locking against config changes + * a device's several drivers. Therefore we reset only devices + * with just one interface, which we of course own. */ - if (us->pusb_dev->actconfig->desc.bNumInterfaces == 1) { - - /* lock the device and attempt to reset the port */ - down(&(us->dev_semaphore)); - result = usb_reset_device(us->pusb_dev); - up(&(us->dev_semaphore)); - US_DEBUGP("usb_reset_device returns %d\n", result); - } else { + down(&(us->dev_semaphore)); + if (test_bit(US_FLIDX_DISCONNECTING, &us->flags)) { + result = -EIO; + US_DEBUGP("Attempt to reset during disconnect\n"); + } else if (us->pusb_dev->actconfig->desc.bNumInterfaces != 1) { result = -EBUSY; US_DEBUGP("Refusing to reset a multi-interface device\n"); + } else { + result = usb_reset_device(us->pusb_dev); + US_DEBUGP("usb_reset_device returns %d\n", result); } + up(&(us->dev_semaphore)); /* lock access to the state and clear it */ scsi_lock(srb->device->host); - atomic_set(&us->sm_state, US_STATE_IDLE); + us->sm_state = US_STATE_IDLE; return result < 0 ? FAILED : SUCCESS; } @@ -333,8 +323,6 @@ #define DO_FLAG(a) if (f & US_FL_##a) pos += sprintf(pos, " " #a) DO_FLAG(SINGLE_LUN); DO_FLAG(MODE_XLATE); - DO_FLAG(START_STOP); - DO_FLAG(IGNORE_SER); DO_FLAG(SCM_MULT_TARG); DO_FLAG(FIX_INQUIRY); DO_FLAG(FIX_CAPACITY); diff -Nru a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c --- a/drivers/usb/storage/transport.c Wed Jun 18 11:15:27 2003 +++ b/drivers/usb/storage/transport.c Wed Jun 18 11:15:27 2003 @@ -136,9 +136,9 @@ struct timer_list to_timer; int status; - /* don't submit URBS during abort/disconnect processing */ + /* don't submit URBs during abort/disconnect processing */ if (us->flags & DONT_SUBMIT) - return -ECONNRESET; + return -EIO; /* set up data structures for the wakeup system */ init_completion(&urb_done); @@ -299,17 +299,17 @@ return USB_STOR_XFER_ERROR; return USB_STOR_XFER_STALLED; - /* NAK - that means we've retried this a few times already */ + /* timeout or excessively long NAK */ case -ETIMEDOUT: - US_DEBUGP("-- device NAKed\n"); + US_DEBUGP("-- timeout or NAK\n"); return USB_STOR_XFER_ERROR; /* babble - the device tried to send more than we wanted to read */ case -EOVERFLOW: - US_DEBUGP("-- Babble\n"); + US_DEBUGP("-- babble\n"); return USB_STOR_XFER_LONG; - /* the transfer was cancelled, presumably by an abort */ + /* the transfer was cancelled by abort, disconnect, or timeout */ case -ECONNRESET: US_DEBUGP("-- transfer cancelled\n"); return USB_STOR_XFER_ERROR; @@ -319,6 +319,11 @@ US_DEBUGP("-- short read transfer\n"); return USB_STOR_XFER_SHORT; + /* abort or disconnect in progress */ + case -EIO: + US_DEBUGP("-- abort or disconnect in progress\n"); + return USB_STOR_XFER_ERROR; + /* the catch-all error case */ default: US_DEBUGP("-- unknown error\n"); @@ -430,7 +435,7 @@ /* initialize the scatter-gather request block */ US_DEBUGP("%s: xfer %u bytes, %d entries\n", __FUNCTION__, length, num_sg); - result = usb_sg_init(us->current_sg, us->pusb_dev, pipe, 0, + result = usb_sg_init(&us->current_sg, us->pusb_dev, pipe, 0, sg, num_sg, length, SLAB_NOIO); if (result) { US_DEBUGP("usb_sg_init returned %d\n", result); @@ -447,19 +452,19 @@ /* cancel the request, if it hasn't been cancelled already */ if (test_and_clear_bit(US_FLIDX_SG_ACTIVE, &us->flags)) { US_DEBUGP("-- cancelling sg request\n"); - usb_sg_cancel(us->current_sg); + usb_sg_cancel(&us->current_sg); } } /* wait for the completion of the transfer */ - usb_sg_wait(us->current_sg); + usb_sg_wait(&us->current_sg); clear_bit(US_FLIDX_SG_ACTIVE, &us->flags); - result = us->current_sg->status; + result = us->current_sg.status; if (act_len) - *act_len = us->current_sg->bytes; + *act_len = us->current_sg.bytes; return interpret_urb_result(us, pipe, length, result, - us->current_sg->bytes); + us->current_sg.bytes); } /* @@ -518,7 +523,7 @@ /* if the command gets aborted by the higher layers, we need to * short-circuit all other processing */ - if (atomic_read(&us->sm_state) == US_STATE_ABORTING) { + if (us->sm_state == US_STATE_ABORTING) { US_DEBUGP("-- command was aborted\n"); goto Handle_Abort; } @@ -650,7 +655,7 @@ srb->cmd_len = old_cmd_len; memcpy(srb->cmnd, old_cmnd, MAX_COMMAND_SIZE); - if (atomic_read(&us->sm_state) == US_STATE_ABORTING) { + if (us->sm_state == US_STATE_ABORTING) { US_DEBUGP("-- auto-sense aborted\n"); goto Handle_Abort; } @@ -734,7 +739,7 @@ /* If we are waiting for a scatter-gather operation, cancel it. */ if (test_and_clear_bit(US_FLIDX_SG_ACTIVE, &us->flags)) { US_DEBUGP("-- cancelling sg request\n"); - usb_sg_cancel(us->current_sg); + usb_sg_cancel(&us->current_sg); } } diff -Nru a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c --- a/drivers/usb/storage/usb.c Wed Jun 18 11:15:27 2003 +++ b/drivers/usb/storage/usb.c Wed Jun 18 11:15:27 2003 @@ -328,13 +328,13 @@ scsi_lock(host); /* has the command been aborted *already* ? */ - if (atomic_read(&us->sm_state) == US_STATE_ABORTING) { + if (us->sm_state == US_STATE_ABORTING) { us->srb->result = DID_ABORT << 16; goto SkipForAbort; } /* set the state and release the lock */ - atomic_set(&us->sm_state, US_STATE_RUNNING); + us->sm_state = US_STATE_RUNNING; scsi_unlock(host); /* lock the device pointers */ @@ -404,12 +404,12 @@ * sm_state == US_STATE_ABORTING, not srb->result == DID_ABORT, * because an abort request might be received after all the * USB processing was complete. */ - if (atomic_read(&us->sm_state) == US_STATE_ABORTING) + if (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); + us->sm_state = US_STATE_IDLE; scsi_unlock(host); } /* for (;;) */ @@ -447,7 +447,7 @@ if (dev->descriptor.iProduct) usb_string(dev, dev->descriptor.iProduct, us->product, sizeof(us->product)); - if (dev->descriptor.iSerialNumber && !(us->flags & US_FL_IGNORE_SER)) + if (dev->descriptor.iSerialNumber) usb_string(dev, dev->descriptor.iSerialNumber, us->serial, sizeof(us->serial)); @@ -698,13 +698,6 @@ return -ENOMEM; } - US_DEBUGP("Allocating scatter-gather request block\n"); - us->current_sg = kmalloc(sizeof(*us->current_sg), GFP_KERNEL); - if (!us->current_sg) { - US_DEBUGP("allocation failed\n"); - return -ENOMEM; - } - /* Lock the device while we carry out the next two operations */ down(&us->dev_semaphore); @@ -720,7 +713,7 @@ up(&us->dev_semaphore); /* Start up our control thread */ - atomic_set(&us->sm_state, US_STATE_IDLE); + us->sm_state = US_STATE_IDLE; p = kernel_thread(usb_stor_control_thread, us, CLONE_VM); if (p < 0) { printk(KERN_WARNING USB_STORAGE @@ -782,7 +775,7 @@ */ if (us->pid) { US_DEBUGP("-- sending exit command to thread\n"); - BUG_ON(atomic_read(&us->sm_state) != US_STATE_IDLE); + BUG_ON(us->sm_state != US_STATE_IDLE); us->srb = NULL; up(&(us->sema)); wait_for_completion(&(us->notify)); @@ -800,8 +793,6 @@ } /* Free the USB control blocks */ - if (us->current_sg) - kfree(us->current_sg); if (us->current_urb) usb_free_urb(us->current_urb); if (us->dr) diff -Nru a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h --- a/drivers/usb/storage/usb.h Wed Jun 18 11:15:27 2003 +++ b/drivers/usb/storage/usb.h Wed Jun 18 11:15:27 2003 @@ -139,7 +139,7 @@ /* thread information */ int pid; /* control thread */ - atomic_t sm_state; /* what we are doing */ + int sm_state; /* what we are doing */ /* interrupt communications data */ unsigned char irqdata[2]; /* data from USB IRQ */ @@ -147,7 +147,7 @@ /* control and bulk communications data */ struct urb *current_urb; /* non-int USB requests */ struct usb_ctrlrequest *dr; /* control requests */ - struct usb_sg_request *current_sg; /* scatter-gather USB */ + struct usb_sg_request current_sg; /* scatter-gather USB */ /* the semaphore for sleeping the control thread */ struct semaphore sema; /* to sleep thread on */