[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

@@ -548,10 +548,9 @@ void isd200_invoke_transport( struct us_data *us,
 	/* 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 @@ void isd200_invoke_transport( struct us_data *us,
 		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 @@ void isd200_invoke_transport( struct us_data *us,
 
 	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 @@ void isd200_invoke_transport( struct us_data *us,
 			/* 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 @@ void isd200_invoke_transport( struct us_data *us,
 	 */
 	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

drivers/usb/storage/scsiglue.c

@@ -141,16 +141,15 @@ static int usb_storage_release(struct Scsi_Host *psh)
 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 @@ static int usb_storage_command_abort( Scsi_Cmnd *srb )
 {
 	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 @@ static int usb_storage_command_abort( Scsi_Cmnd *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. */
-	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_command_abort( Scsi_Cmnd *srb )
 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_device_reset( Scsi_Cmnd *srb )
 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 @@ static int usb_storage_proc_info (struct Scsi_Host *hostptr, char *buffer, char
 #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);

drivers/usb/storage/transport.c

@@ -136,9 +136,9 @@ static int usb_stor_msg_common(struct us_data *us, int timeout)
 	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 @@ static int interpret_urb_result(struct us_data *us, unsigned int pipe,
 			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 @@ static int interpret_urb_result(struct us_data *us, unsigned int pipe,
 		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 @@ int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,
 	/* 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 @@ int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,
 		/* 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 @@ void usb_stor_invoke_transport(Scsi_Cmnd *srb, struct us_data *us)
 	/* 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 @@ void usb_stor_invoke_transport(Scsi_Cmnd *srb, struct us_data *us)
 		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 @@ void usb_stor_stop_transport(struct us_data *us)
 	/* 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);
 	}
 }
 

drivers/usb/storage/usb.c

@@ -328,13 +328,13 @@ static int usb_stor_control_thread(void * __us)
 		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 @@ static int usb_stor_control_thread(void * __us)
 		 * 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 @@ static void get_device_info(struct us_data *us,
 	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 @@ static int usb_stor_acquire_resources(struct us_data *us)
 		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 @@ static int usb_stor_acquire_resources(struct us_data *us)
 	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 @@ void usb_stor_release_resources(struct us_data *us)
 	 */
 	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 @@ void usb_stor_release_resources(struct us_data *us)
 	}
 
 	/* 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)

drivers/usb/storage/usb.h

@@ -139,7 +139,7 @@ struct us_data {
 
 	/* 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 @@ struct us_data {
 	/* 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   */