[PATCH] PATCH: 2.5.8 ehci, submit errors

It fixes problems with interrupt transfers, which I think that
nobody else has run into (or I'd surely have heard of it :).
Looks like not many folk are using USB 2.0 hubs yet.

    - wasn't checking enough of the periodic schedule to
      detect bandwidth overcommit (would BUG out)
    - frames to uframes is rightshift 3, not 8 :)
    - properly cleans up (no oops!) after certain rare errors
      in the interrupt submit path (just my luck to hit one)
    - use that cleanup to bypass some old implementation
      shortcuts in the control and bulk submit paths
    - there are also some other minor updates/cleanups

drivers/usb/host/ehci-hcd.c

@@ -70,6 +70,8 @@
  *
  * HISTORY:
  *
+ * 2002-04-19	Control/bulk/interrupt submit no longer uses giveback() on
+ *	errors in submit path.  Bugfixes to interrupt scheduling/processing.
  * 2002-03-05	Initial high-speed ISO support; reduce ITD memory; shift
  *	more checking to generic hcd framework (db).  Make it work with
  *	Philips EHCI; reduce PCI traffic; shorten IRQ path (Rory Bolt).
@@ -81,7 +83,7 @@
  * 2001-June	Works with usb-storage and NEC EHCI on 2.4
  */
 
-#define DRIVER_VERSION "$Revision: 0.27 $"
+#define DRIVER_VERSION "$Revision: 0.31 $"
 #define DRIVER_AUTHOR "David Brownell"
 #define DRIVER_DESC "USB 2.0 'Enhanced' Host Controller (EHCI) Driver"
 
@@ -512,8 +514,7 @@ static int ehci_urb_enqueue (
 	case PIPE_BULK:
 		if (!qh_urb_transaction (ehci, urb, &qtd_list, mem_flags))
 			return -ENOMEM;
-		submit_async (ehci, urb, &qtd_list, mem_flags);
-		break;
+		return submit_async (ehci, urb, &qtd_list, mem_flags);
 
 	case PIPE_INTERRUPT:
 		if (!qh_urb_transaction (ehci, urb, &qtd_list, mem_flags))
@@ -531,8 +532,9 @@ static int ehci_urb_enqueue (
 		return -ENOSYS;
 #endif /* have_split_iso */
 
+	default:	/* can't happen */
+		return -ENOSYS;
 	}
-	return 0;
 }
 
 /* remove from hardware lists
@@ -587,7 +589,7 @@ dbg ("wait for dequeue: state %d, reclaim %p, hcd state %d",
 		// itd or sitd ...
 
 		// wait till next completion, do it then.
-		// completion irqs can wait up to 128 msec,
+		// completion irqs can wait up to 1024 msec,
 		urb->transfer_flags |= EHCI_STATE_UNLINK;
 		return 0;
 	}
@@ -614,10 +616,7 @@ static void ehci_free_config (struct usb_hcd *hcd, struct usb_device *udev)
 		if (dev->ep [i]) {
 			struct ehci_qh		*qh;
 
-			// FIXME:  this might be an itd/sitd too ...
-			// or an interrupt urb (not on async list)
-			// can use "union ehci_shadow"
-
+			/* dev->ep never has ITDs or SITDs */
 			qh = (struct ehci_qh *) dev->ep [i];
 			vdbg ("free_config, ep 0x%02x qh %p", i, qh);
 			if (!list_empty (&qh->qtd_list)) {

drivers/usb/host/ehci-q.c

@@ -280,6 +280,8 @@ qh_completions (
 
 		/* if these qtds were queued to the HC, some may be active.
 		 * else we're cleaning up after a failed URB submission.
+		 *
+		 * FIXME can simplify: cleanup case is gone now.
 		 */
 		if (likely (qh != 0)) {
 			int		qh_halted;
@@ -405,6 +407,47 @@ qh_completions (
 
 /*-------------------------------------------------------------------------*/
 
+/*
+ * reverse of qh_urb_transaction:  free a list of TDs.
+ * used for cleanup after errors, before HC sees an URB's TDs.
+ */
+static void qtd_list_free (
+	struct ehci_hcd		*ehci,
+	struct urb		*urb,
+	struct list_head	*qtd_list
+) {
+	struct list_head	*entry, *temp;
+	int			unmapped = 0;
+
+	list_for_each_safe (entry, temp, qtd_list) {
+		struct ehci_qtd	*qtd;
+
+		qtd = list_entry (entry, struct ehci_qtd, qtd_list);
+		list_del (&qtd->qtd_list);
+		if (unmapped != 2) {
+			int	direction;
+
+			/* for ctrl unmap twice: SETUP and DATA;
+			 * else (bulk, intr) just once: DATA
+			 */
+			if (!unmapped++ && usb_pipecontrol (urb->pipe))
+				direction = PCI_DMA_TODEVICE;
+			else {
+				direction = usb_pipein (urb->pipe)
+					? PCI_DMA_FROMDEVICE
+					: PCI_DMA_TODEVICE;
+				unmapped++;
+			}
+			if (qtd->buf_dma)
+				pci_unmap_single (ehci->hcd.pdev,
+					qtd->buf_dma,
+					qtd->urb->transfer_buffer_length,
+					direction);
+		}
+		ehci_qtd_free (ehci, qtd);
+	}
+}
+
 /*
  * create a list of filled qtds for this URB; won't link into qh.
  */
@@ -547,8 +590,7 @@ qh_urb_transaction (
 	return head;
 
 cleanup:
-	urb->status = -ENOMEM;
-	qh_completions (ehci, head, 1);
+	qtd_list_free (ehci, urb, head);
 	return 0;
 }
 
@@ -713,7 +755,7 @@ static void qh_link_async (struct ehci_hcd *ehci, struct ehci_qh *qh)
 
 /*-------------------------------------------------------------------------*/
 
-static void
+static int
 submit_async (
 	struct ehci_hcd		*ehci,
 	struct urb		*urb,
@@ -807,8 +849,7 @@ submit_async (
 		if (likely (qh != 0)) {
 			// dbg_qh ("new qh", ehci, qh);
 			dev->ep [epnum] = qh;
-		} else
-			urb->status = -ENOMEM;
+		}
 	}
 
 	/* Control/bulk operations through TTs don't need scheduling,
@@ -820,8 +861,11 @@ submit_async (
 			qh_link_async (ehci, qh_get (qh));
 	}
 	spin_unlock_irqrestore (&ehci->lock, flags);
-	if (unlikely (!qh))
-		qh_completions (ehci, qtd_list, 1);
+	if (unlikely (qh == 0)) {
+		qtd_list_free (ehci, urb, qtd_list);
+		return -ENOMEM;
+	}
+	return 0;
 }
 
 /*-------------------------------------------------------------------------*/

drivers/usb/host/ehci-sched.c

@@ -220,6 +220,8 @@ static void intr_deschedule (
 ) {
 	unsigned long	flags;
 
+	period >>= 3;		// FIXME microframe periods not handled yet
+
 	spin_lock_irqsave (&ehci->lock, flags);
 
 	do {
@@ -256,6 +258,38 @@ static void intr_deschedule (
 		atomic_read (&qh->refcount), ehci->periodic_urbs);
 }
 
+static int check_period (
+	struct ehci_hcd *ehci, 
+	unsigned	frame,
+	int		uframe,
+	unsigned	period,
+	unsigned	usecs
+) {
+	/*
+	 * 80% periodic == 100 usec/uframe available
+	 * convert "usecs we need" to "max already claimed" 
+	 */
+	usecs = 100 - usecs;
+
+	do {
+		int	claimed;
+
+// FIXME delete when intr_submit handles non-empty queues
+// this gives us a one intr/frame limit (vs N/uframe)
+		if (ehci->pshadow [frame].ptr)
+			return 0;
+
+		claimed = periodic_usecs (ehci, frame, uframe);
+		if (claimed > usecs)
+			return 0;
+
+// FIXME update to handle sub-frame periods
+	} while ((frame += period) < ehci->periodic_size);
+
+	// success!
+	return 1;
+}
+
 static int intr_submit (
 	struct ehci_hcd		*ehci,
 	struct urb		*urb,
@@ -263,7 +297,6 @@ static int intr_submit (
 	int			mem_flags
 ) {
 	unsigned		epnum, period;
-	unsigned		temp;
 	unsigned short		usecs;
 	unsigned long		flags;
 	struct ehci_qh		*qh;
@@ -272,11 +305,8 @@ static int intr_submit (
 
 	/* get endpoint and transfer data */
 	epnum = usb_pipeendpoint (urb->pipe);
-	if (usb_pipein (urb->pipe)) {
-		temp = urb->dev->epmaxpacketin [epnum];
+	if (usb_pipein (urb->pipe))
 		epnum |= 0x10;
-	} else
-		temp = urb->dev->epmaxpacketout [epnum];
 	if (urb->dev->speed != USB_SPEED_HIGH) {
 		dbg ("no intr/tt scheduling yet"); 
 		status = -ENOSYS;
@@ -287,6 +317,13 @@ static int intr_submit (
 	 * NOTE: current completion/restart logic doesn't handle more than
 	 * one qtd in a periodic qh ... 16-20 KB/urb is pretty big for this.
 	 * such big requests need many periods to transfer.
+	 *
+	 * FIXME want to change hcd core submit model to expect queuing
+	 * for all transfer types ... not just ISO and (with flag) BULK.
+	 * that means: getting rid of this check; handling the "interrupt
+	 * urb already queued" case below like bulk queuing is handled (no
+	 * errors possible!); and completly getting rid of that annoying
+	 * qh restart logic.  simpler/smaller overall, and more flexible.
 	 */
 	if (unlikely (qtd_list->next != qtd_list->prev)) {
 		dbg ("only one intr qtd per urb allowed"); 
@@ -297,10 +334,12 @@ static int intr_submit (
 	usecs = HS_USECS (urb->transfer_buffer_length);
 
 	/* FIXME handle HS periods of less than 1 frame. */
-	if (urb->interval < 8)
-		period = 1;
-	else
-		period = urb->interval >> 8;
+	period = urb->interval >> 3;
+	if (period < 1) {
+		dbg ("intr period %d uframes, NYET!", urb->interval);
+		status = -EINVAL;
+		goto done;
+	}
 
 	spin_lock_irqsave (&ehci->lock, flags);
 
@@ -326,21 +365,22 @@ static int intr_submit (
 			list_splice (qtd_list, &qh->qtd_list);
 			qh_update (qh, list_entry (qtd_list->next,
 						struct ehci_qtd, qtd_list));
+			qtd_list = &qh->qtd_list;
 		}
 	} else {
 		/* can't sleep here, we have ehci->lock... */
 		qh = ehci_qh_make (ehci, urb, qtd_list, SLAB_ATOMIC);
-		qtd_list = &qh->qtd_list;
 		if (likely (qh != 0)) {
 			// dbg ("new INTR qh %p", qh);
 			dev->ep [epnum] = qh;
+			qtd_list = &qh->qtd_list;
 		} else
 			status = -ENOMEM;
 	}
 
 	/* Schedule this periodic QH. */
 	if (likely (status == 0)) {
-		unsigned	frame = urb->interval;
+		unsigned	frame = period;
 
 		qh->hw_next = EHCI_LIST_END;
 		qh->usecs = usecs;
@@ -352,28 +392,20 @@ static int intr_submit (
 		do {
 			int	uframe;
 
-			/* Select some frame 0..(urb->interval - 1) with a
-			 * microframe that can hold this transaction.
+			/* pick a set of slots such that all uframes have
+			 * enough periodic bandwidth available.
 			 *
 			 * FIXME for TT splits, need uframes for start and end.
 			 * FSTNs can put end into next frame (uframes 0 or 1).
 			 */
 			frame--;
 			for (uframe = 0; uframe < 8; uframe++) {
-				int	claimed;
-				claimed = periodic_usecs (ehci, frame, uframe);
-				/* 80% periodic == 100 usec max committed */
-				if ((claimed + usecs) <= 100) {
-					vdbg ("frame %d.%d: %d usecs, plus %d",
-						frame, uframe, claimed, usecs);
+				if (check_period (ehci, frame, uframe,
+						period, usecs) != 0)
 					break;
 			}
-			}
 			if (uframe == 8)
 				continue;
-// FIXME delete when code below handles non-empty queues
-			if (ehci->pshadow [frame].ptr)
-				continue;
 
 			/* QH will run once each period, starting there  */
 			urb->start_frame = frame;
@@ -389,8 +421,9 @@ static int intr_submit (
 				qh, qh->usecs, period, frame, uframe);
 			do {
 				if (unlikely (ehci->pshadow [frame].ptr != 0)) {
-// FIXME -- just link to the end, before any qh with a shorter period,
+// FIXME -- just link toward the end, before any qh with a shorter period,
 // AND handle it already being (implicitly) linked into this frame
+// AS WELL AS updating the check_period() logic
 					BUG ();
 				} else {
 					ehci->pshadow [frame].qh = qh_get (qh);
@@ -401,7 +434,7 @@ static int intr_submit (
 			} while (frame < ehci->periodic_size);
 
 			/* update bandwidth utilization records (for usbfs) */
-			usb_claim_bandwidth (urb->dev, urb, usecs, 0);
+			usb_claim_bandwidth (urb->dev, urb, usecs/period, 0);
 
 			/* maybe enable periodic schedule processing */
 			if (!ehci->periodic_urbs++)
@@ -412,14 +445,9 @@ static int intr_submit (
 	}
 	spin_unlock_irqrestore (&ehci->lock, flags);
 done:
-	if (status) {
-		usb_complete_t	complete = urb->complete;
+	if (status)
+		qtd_list_free (ehci, urb, qtd_list);
 
-		urb->complete = 0;
-		urb->status = status;
-		qh_completions (ehci, qtd_list, 1);
-		urb->complete = complete;
-	}
 	return status;
 }
 
@@ -438,6 +466,10 @@ intr_complete (
 	if (likely ((qh->hw_token & __constant_cpu_to_le32 (QTD_STS_ACTIVE))
 			!= 0))
 		return flags;
+	if (unlikely (list_empty (&qh->qtd_list))) {
+		dbg ("intr qh %p no TDs?", qh);
+		return flags;
+	}
 	
 	qtd = list_entry (qh->qtd_list.next, struct ehci_qtd, qtd_list);
 	urb = qtd->urb;