[PATCH] usb-storage: make CBI interrupt URBs one-shot

Change interrupt used for CBI from periodic to one-shot.  This (a) reduces
consumed bandwidth, (b) makes the logic clearer, and (c) makes the abort
mechanism more uniform.

drivers/usb/storage/transport.c

@@ -503,6 +503,35 @@ int usb_stor_bulk_msg(struct us_data *us, void *data, unsigned int pipe,
 	return status;
 }
 
+/* This is our function to submit interrupt URBs with enough control
+ * to make aborts/resets/timeouts work
+ *
+ * This routine always uses us->recv_intr_pipe as the pipe and
+ * us->ep_int->bInterval as the interrupt interval.
+ */
+int usb_stor_interrupt_msg(struct us_data *us, void *data,
+			unsigned int len, unsigned int *act_len)
+{
+	unsigned int pipe = us->recv_intr_pipe;
+	unsigned int maxp;
+	int status;
+
+	/* calculate the max packet size */
+	maxp = usb_maxpacket(us->pusb_dev, pipe, usb_pipeout(pipe));
+	if (maxp > len)
+		maxp = len;
+
+	/* fill and submit the URB */
+	usb_fill_int_urb(us->current_urb, us->pusb_dev, pipe, data,
+			maxp, usb_stor_blocking_completion, NULL,
+			us->ep_int->bInterval);
+	status = usb_stor_msg_common(us);
+
+	/* store the actual length of the data transferred */
+	*act_len = us->current_urb->actual_length;
+	return status;
+}
+
 /* This is a version of usb_clear_halt() that doesn't read the status from
  * the device -- this is because some devices crash their internal firmware
  * when the status is requested after a halt.
@@ -626,6 +655,29 @@ int usb_stor_ctrl_transfer(struct us_data *us, unsigned int pipe,
 	return interpret_urb_result(us, pipe, size, result, partial);
 }
 
+/*
+ * Receive one buffer via interrupt transfer
+ *
+ * This function does basically the same thing as usb_stor_interrupt_msg()
+ * above, except that return codes are USB_STOR_XFER_xxx rather than the
+ * urb status.
+ */
+int usb_stor_intr_transfer(struct us_data *us, void *buf,
+		unsigned int length, unsigned int *act_len)
+{
+	int result;
+	unsigned int partial;
+
+	/* transfer the data */
+	US_DEBUGP("usb_stor_intr_transfer(): xfer %u bytes\n", length);
+	result = usb_stor_interrupt_msg(us, buf, length, &partial);
+	if (act_len)
+		*act_len = partial;
+
+	return interpret_urb_result(us, us->recv_intr_pipe,
+			length, result, partial);
+}
+
 /*
  * Transfer one buffer via bulk transfer
  *
@@ -947,7 +999,7 @@ void usb_stor_abort_transport(struct us_data *us)
 	host = us->srb->host;
 	scsi_unlock(host);
 
-	/* If the state machine is blocked waiting for an URB or an IRQ,
+	/* If the state machine is blocked waiting for an URB,
 	 * let's wake it up */
 
 	/* If we have an URB pending, cancel it.  The test_and_clear_bit()
@@ -964,12 +1016,6 @@ void usb_stor_abort_transport(struct us_data *us)
 		usb_sg_cancel(us->current_sg);
 	}
 
-	/* If we are waiting for an IRQ, simulate it */
-	if (test_bit(US_FLIDX_IP_WANTED, &us->flags)) {
-		US_DEBUGP("-- simulating missing IRQ\n");
-		usb_stor_CBI_irq(us->irq_urb, NULL);
-	}
-
 	/* Wait for the aborted command to finish */
 	wait_for_completion(&us->notify);
 
@@ -981,94 +1027,11 @@ void usb_stor_abort_transport(struct us_data *us)
  * Control/Bulk/Interrupt transport
  */
 
-/* The interrupt handler for CBI devices */
-void usb_stor_CBI_irq(struct urb *urb, struct pt_regs *regs)
-{
-	struct us_data *us = (struct us_data *)urb->context;
-	int status;
-
-	US_DEBUGP("USB IRQ received for device on host %d\n", us->host_no);
-	US_DEBUGP("-- IRQ data length is %d\n", urb->actual_length);
-	US_DEBUGP("-- IRQ state is %d\n", urb->status);
-	US_DEBUGP("-- Interrupt Status (0x%x, 0x%x)\n",
-			us->irqbuf[0], us->irqbuf[1]);
-
-	/* has the current command been aborted? */
-	if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
-
-		/* was this a wanted interrupt? */
-		if (!test_and_clear_bit(US_FLIDX_IP_WANTED, &us->flags)) {
-			US_DEBUGP("ERROR: Unwanted interrupt received!\n");
-			goto exit;
-		}
-		US_DEBUGP("-- command aborted\n");
-
-		/* wake up the command thread */
-		up(&us->ip_waitq);
-		goto exit;
-	}
-
-	/* is the device removed? */
-	if (urb->status == -ENOENT) {
-		US_DEBUGP("-- device has been removed\n");
-
-		/* was this a wanted interrupt? */
-		if (!test_and_clear_bit(US_FLIDX_IP_WANTED, &us->flags))
-			return;
-
-		/* indicate a transport error -- this is the best we can do */
-		us->irqdata[0] = us->irqdata[1] = 0xFF;
-
-		/* wake up the command thread */
-		up(&us->ip_waitq);
-		return;
-	}
-
-	/* reject improper IRQs */
-	if (urb->actual_length != 2) {
-		US_DEBUGP("-- IRQ too short\n");
-		goto exit;
-	}
-
-	/* was this a command-completion interrupt? */
-	if (us->irqbuf[0] && (us->subclass != US_SC_UFI)) {
-		US_DEBUGP("-- not a command-completion IRQ\n");
-		goto exit;
-	}
-
-	/* was this a wanted interrupt? */
-	if (!test_and_clear_bit(US_FLIDX_IP_WANTED, &us->flags)) {
-		US_DEBUGP("ERROR: Unwanted interrupt received!\n");
-		goto exit;
-	}
-		
-	/* copy the valid data */
-	us->irqdata[0] = us->irqbuf[0];
-	us->irqdata[1] = us->irqbuf[1];
-
-	/* wake up the command thread */
-	up(&(us->ip_waitq));
-
-exit:
-	/* resubmit the urb */
-	status = usb_submit_urb (urb, GFP_ATOMIC);
-	if (status)
-		err ("%s - usb_submit_urb failed with result %d",
-		     __FUNCTION__, status);
-}
-
 int usb_stor_CBI_transport(Scsi_Cmnd *srb, struct us_data *us)
 {
 	unsigned int transfer_length = usb_stor_transfer_length(srb);
 	int result;
 
-	/* re-initialize the mutex so that we avoid any races with
-	 * early/late IRQs from previous commands */
-	init_MUTEX_LOCKED(&(us->ip_waitq));
-
-	/* Set up for status notification */
-	set_bit(US_FLIDX_IP_WANTED, &us->flags);
-
 	/* COMMAND STAGE */
 	/* let's send the command via the control pipe */
 	result = usb_stor_ctrl_transfer(us, us->send_ctrl_pipe,
@@ -1079,8 +1042,6 @@ int usb_stor_CBI_transport(Scsi_Cmnd *srb, struct us_data *us)
 	/* check the return code for the command */
 	US_DEBUGP("Call to usb_stor_ctrl_transfer() returned %d\n", result);
 	if (result != USB_STOR_XFER_GOOD) {
-		/* Reset flag for status notification */
-		clear_bit(US_FLIDX_IP_WANTED, &us->flags);
 		/* Uh oh... serious problem here */
 		return USB_STOR_TRANSPORT_ERROR;
 	}
@@ -1093,25 +1054,17 @@ int usb_stor_CBI_transport(Scsi_Cmnd *srb, struct us_data *us)
 		result = usb_stor_bulk_transfer_srb(us, pipe, srb,
 					transfer_length);
 		US_DEBUGP("CBI data stage result is 0x%x\n", result);
-		if (result == USB_STOR_XFER_ERROR) {
-			clear_bit(US_FLIDX_IP_WANTED, &us->flags);
+		if (result == USB_STOR_XFER_ERROR)
 			return USB_STOR_TRANSPORT_ERROR;
-		}
 	}
 
 	/* STATUS STAGE */
-
-	/* go to sleep until we get this interrupt */
-	down(&(us->ip_waitq));
-
-	/* has the current command been aborted? */
-	if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
-		US_DEBUGP("CBI interrupt aborted\n");
-		return USB_STOR_TRANSPORT_ERROR;
-	}
-
+	result = usb_stor_intr_transfer(us, us->irqdata,
+					sizeof(us->irqdata), NULL);
 	US_DEBUGP("Got interrupt data (0x%x, 0x%x)\n", 
 			us->irqdata[0], us->irqdata[1]);
+	if (result != USB_STOR_XFER_GOOD)
+		return USB_STOR_TRANSPORT_ERROR;
 
 	/* UFI gives us ASC and ASCQ, like a request sense
 	 *

drivers/usb/storage/transport.h

@@ -145,7 +145,6 @@ struct bulk_cs_wrap {
 
 #define US_CBI_ADSC		0
 
-extern void usb_stor_CBI_irq(struct urb*, struct pt_regs *);
 extern int usb_stor_CBI_transport(Scsi_Cmnd*, struct us_data*);
 
 extern int usb_stor_CB_transport(Scsi_Cmnd*, struct us_data*);
@@ -164,11 +163,15 @@ extern int usb_stor_bulk_msg(struct us_data *us, void *data,
 extern int usb_stor_control_msg(struct us_data *us, unsigned int pipe,
 		u8 request, u8 requesttype, u16 value, u16 index,
 		void *data, u16 size);
+extern int usb_stor_interrupt_msg(struct us_data *us, void *data,
+		unsigned int len, unsigned int *act_len);
 
 extern int usb_stor_clear_halt(struct us_data*, unsigned int pipe);
 extern int usb_stor_ctrl_transfer(struct us_data *us, unsigned int pipe,
 		u8 request, u8 requesttype, u16 value, u16 index,
 		void *data, u16 size);
+extern int usb_stor_intr_transfer(struct us_data *us, void *buf,
+		unsigned int length, unsigned int *act_len);
 extern int usb_stor_bulk_transfer_buf(struct us_data *us, unsigned int pipe,
 		void *buf, unsigned int length, unsigned int *act_len);
 extern int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,

drivers/usb/storage/usb.c

@@ -477,24 +477,21 @@ static int usb_stor_control_thread(void * __us)
 	return 0;
 }	
 
-/* Set up the URB, the usb_ctrlrequest, and the IRQ pipe and handler.
+/* Set up the URB and the usb_ctrlrequest.
  * ss->dev_semaphore must already be locked.
  * Note that this function assumes that all the data in the us_data
- * strucuture is current.  This includes the ep_int field, which gives us
- * the endpoint for the interrupt.
+ * structure is current.
  * Returns non-zero on failure, zero on success
  */ 
 static int usb_stor_allocate_urbs(struct us_data *ss)
 {
-	unsigned int pipe;
-	int maxp;
-	int result;
-
 	/* calculate and store the pipe values */
 	ss->send_bulk_pipe = usb_sndbulkpipe(ss->pusb_dev, ss->ep_out);
 	ss->recv_bulk_pipe = usb_rcvbulkpipe(ss->pusb_dev, ss->ep_in);
 	ss->send_ctrl_pipe = usb_sndctrlpipe(ss->pusb_dev, 0);
 	ss->recv_ctrl_pipe = usb_rcvctrlpipe(ss->pusb_dev, 0);
+	ss->recv_intr_pipe = usb_rcvintpipe(ss->pusb_dev,
+		    ss->ep_int->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK);
 
 	/* allocate the usb_ctrlrequest for control packets */
 	US_DEBUGP("Allocating usb_ctrlrequest\n");
@@ -519,45 +516,6 @@ static int usb_stor_allocate_urbs(struct us_data *ss)
 		return 5;
 	}
 
-	/* allocate the IRQ URB, if it is needed */
-	if (ss->protocol == US_PR_CBI) {
-		US_DEBUGP("Allocating IRQ for CBI transport\n");
-
-		/* lock access to the data structure */
-		down(&(ss->irq_urb_sem));
-
-		/* allocate the URB */
-		ss->irq_urb = usb_alloc_urb(0, GFP_KERNEL);
-		if (!ss->irq_urb) {
-			up(&(ss->irq_urb_sem));
-			US_DEBUGP("couldn't allocate interrupt URB");
-			return 3;
-		}
-
-		/* calculate the pipe and max packet size */
-		pipe = usb_rcvintpipe(ss->pusb_dev,
-		    ss->ep_int->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK);
-		maxp = usb_maxpacket(ss->pusb_dev, pipe, usb_pipeout(pipe));
-		if (maxp > sizeof(ss->irqbuf))
-			maxp = sizeof(ss->irqbuf);
-
-		/* fill in the URB with our data */
-		usb_fill_int_urb(ss->irq_urb, ss->pusb_dev, pipe, ss->irqbuf,
-			maxp, usb_stor_CBI_irq, ss, ss->ep_int->bInterval); 
-
-		/* submit the URB for processing */
-		result = usb_submit_urb(ss->irq_urb, GFP_KERNEL);
-		US_DEBUGP("usb_submit_urb() returns %d\n", result);
-		if (result) {
-			up(&(ss->irq_urb_sem));
-			return 4;
-		}
-
-		/* unlock the data structure */
-		up(&(ss->irq_urb_sem));
-
-	} /* ss->protocol == US_PR_CBI */
-
 	return 0;	/* success */
 }
 
@@ -568,17 +526,6 @@ static void usb_stor_deallocate_urbs(struct us_data *ss)
 {
 	int result;
 
-	/* release the IRQ, if we have one */
-	down(&(ss->irq_urb_sem));
-	if (ss->irq_urb) {
-		US_DEBUGP("-- releasing irq URB\n");
-		result = usb_unlink_urb(ss->irq_urb);
-		US_DEBUGP("-- usb_unlink_urb() returned %d\n", result);
-		usb_free_urb(ss->irq_urb);
-		ss->irq_urb = NULL;
-	}
-	up(&(ss->irq_urb_sem));
-
 	/* free the scatter-gather request block */
 	if (ss->current_sg) {
 		kfree(ss->current_sg);
@@ -810,8 +757,6 @@ static int storage_probe(struct usb_interface *intf,
 
 		/* Initialize the mutexes only when the struct is new */
 		init_completion(&(ss->notify));
-		init_MUTEX_LOCKED(&(ss->ip_waitq));
-		init_MUTEX(&(ss->irq_urb_sem));
 		init_MUTEX_LOCKED(&(ss->dev_semaphore));
 
 		/* copy over the subclass and protocol data */

drivers/usb/storage/usb.h

@@ -105,7 +105,6 @@ struct us_unusual_dev {
 #define US_FL_FIX_CAPACITY    0x00000080 /* READ CAPACITY response too big  */
 
 #define US_FL_DEV_ATTACHED    0x00010000 /* is the device attached?	    */
-#define US_FLIDX_IP_WANTED   17  /* 0x00020000	is an IRQ expected?	    */
 #define US_FLIDX_CAN_CANCEL  18  /* 0x00040000  okay to cancel current_urb? */
 #define US_FLIDX_CANCEL_SG   19  /* 0x00080000	okay to cancel current_sg?  */
 
@@ -139,6 +138,7 @@ struct us_data {
 	unsigned int		recv_bulk_pipe;
 	unsigned int		send_ctrl_pipe;
 	unsigned int		recv_ctrl_pipe;
+	unsigned int		recv_intr_pipe;
 
 	/* information about the device -- always good */
 	char			vendor[USB_STOR_STRING_LEN];
@@ -173,13 +173,7 @@ struct us_data {
 	int			pid;		 /* control thread	 */
 	atomic_t		sm_state;	 /* what we are doing	 */
 
-	/* interrupt info for CBI devices -- only good if attached */
-	struct semaphore	ip_waitq;	 /* for CBI interrupts	 */
-
 	/* interrupt communications data */
-	struct semaphore	irq_urb_sem;	 /* to protect irq_urb	 */
-	struct urb		*irq_urb;	 /* for USB int requests */
-	unsigned char		irqbuf[2];	 /* buffer for USB IRQ	 */
 	unsigned char		irqdata[2];	 /* data from USB IRQ	 */
 
 	/* control and bulk communications data */