[PATCH] USB: ohci, fix iso "bad entry" bug + misc

A while back there were some reports of ohci reporting a "bad entry"
diagnostic, mostly with ISO transfers, which were mysterious until
I recently found an easy way to reproduce it.

This patch:

  - Fixes at least one cause of that "bad entry" diagnostic by
    waiting for INTR_WDH before completing ED unlink processing.
    (Else URB unlinking could free TDs on the donelist, so the
    WDH processing would see those entries as "bad".)

  - Merges the patch from Darwin Rambo <drambo@broadcom.com>,
    coping with CPUs that can't do 16 bit accesses (MIPS).

  - Renames a function as start_ed_unlink(), matching its role.

  - Fixes minor debug output issues, including a FIXME to tell
    more info about TDs on the periodic schedule.  And adding
    some missing newlines (makes this patch seem big).

Nobody's complained much about that "bad entry" issue lately, but
if necessary that part would be particularly easy to split out.

Please merge to the next kernel that gets USB patches.

drivers/usb/host/ohci-dbg.c

@@ -269,18 +269,19 @@ static void ohci_dump (struct ohci_hcd *controller, int verbose)
 	ohci_dump_status (controller, NULL, 0);
 	if (controller->hcca)
 		ohci_dbg (controller,
-			"hcca frame #%04x\n", controller->hcca->frame_no);
+			"hcca frame #%04x\n", OHCI_FRAME_NO(controller->hcca));
 	ohci_dump_roothub (controller, 1, NULL, 0);
 }
 
 static const char data0 [] = "DATA0";
 static const char data1 [] = "DATA1";
 
-static void ohci_dump_td (struct ohci_hcd *ohci, char *label, struct td *td)
+static void ohci_dump_td (const struct ohci_hcd *ohci, const char *label,
+		const struct td *td)
 {
 	u32	tmp = le32_to_cpup (&td->hwINFO);
 
-	ohci_dbg (ohci, "%s td %p%s; urb %p index %d; hw next td %08x",
+	ohci_dbg (ohci, "%s td %p%s; urb %p index %d; hw next td %08x\n",
 		label, td,
 		(tmp & TD_DONE) ? " (DONE)" : "",
 		td->urb, td->index,
@@ -301,28 +302,28 @@ static void ohci_dump_td (struct ohci_hcd *ohci, char *label, struct td *td)
 		case TD_DP_OUT: pid = "OUT"; break;
 		default: pid = "(bad pid)"; break;
 		}
-		ohci_dbg (ohci, "     info %08x CC=%x %s DI=%d %s %s", tmp,
+		ohci_dbg (ohci, "     info %08x CC=%x %s DI=%d %s %s\n", tmp,
 			TD_CC_GET(tmp), /* EC, */ toggle,
 			(tmp & TD_DI) >> 21, pid,
 			(tmp & TD_R) ? "R" : "");
 		cbp = le32_to_cpup (&td->hwCBP);
 		be = le32_to_cpup (&td->hwBE);
-		ohci_dbg (ohci, "     cbp %08x be %08x (len %d)", cbp, be,
+		ohci_dbg (ohci, "     cbp %08x be %08x (len %d)\n", cbp, be,
 			cbp ? (be + 1 - cbp) : 0);
 	} else {
 		unsigned	i;
-		ohci_dbg (ohci, "     info %08x CC=%x FC=%d DI=%d SF=%04x", tmp,
+		ohci_dbg (ohci, "  info %08x CC=%x FC=%d DI=%d SF=%04x\n", tmp,
 			TD_CC_GET(tmp),
 			(tmp >> 24) & 0x07,
 			(tmp & TD_DI) >> 21,
 			tmp & 0x0000ffff);
-		ohci_dbg (ohci, "     bp0 %08x be %08x",
+		ohci_dbg (ohci, "  bp0 %08x be %08x\n",
 			le32_to_cpup (&td->hwCBP) & ~0x0fff,
 			le32_to_cpup (&td->hwBE));
 		for (i = 0; i < MAXPSW; i++) {
 			u16	psw = le16_to_cpup (&td->hwPSW [i]);
 			int	cc = (psw >> 12) & 0x0f;
-			ohci_dbg (ohci, "       psw [%d] = %2x, CC=%x %s=%d", i,
+			ohci_dbg (ohci, "    psw [%d] = %2x, CC=%x %s=%d\n", i,
 				psw, cc,
 				(cc >= 0x0e) ? "OFFSET" : "SIZE",
 				psw & 0x0fff);
@@ -332,12 +333,13 @@ static void ohci_dump_td (struct ohci_hcd *ohci, char *label, struct td *td)
 
 /* caller MUST own hcd spinlock if verbose is set! */
 static void __attribute__((unused))
-ohci_dump_ed (struct ohci_hcd *ohci, char *label, struct ed *ed, int verbose)
+ohci_dump_ed (const struct ohci_hcd *ohci, const char *label,
+		const struct ed *ed, int verbose)
 {
 	u32	tmp = ed->hwINFO;
 	char	*type = "";
 
-	ohci_dbg (ohci, "%s, ed %p state 0x%x type %s; next ed %08x",
+	ohci_dbg (ohci, "%s, ed %p state 0x%x type %s; next ed %08x\n",
 		label,
 		ed, ed->state, edstring (ed->type),
 		le32_to_cpup (&ed->hwNextED));
@@ -347,7 +349,7 @@ ohci_dump_ed (struct ohci_hcd *ohci, char *label, struct ed *ed, int verbose)
 	/* else from TDs ... control */
 	}
 	ohci_dbg (ohci,
-		"  info %08x MAX=%d%s%s%s%s EP=%d%s DEV=%d", le32_to_cpu (tmp),
+		"  info %08x MAX=%d%s%s%s%s EP=%d%s DEV=%d\n", le32_to_cpu (tmp),
 		0x03ff & (le32_to_cpu (tmp) >> 16),
 		(tmp & ED_DEQUEUE) ? " DQ" : "",
 		(tmp & ED_ISO) ? " ISO" : "",
@@ -356,7 +358,7 @@ ohci_dump_ed (struct ohci_hcd *ohci, char *label, struct ed *ed, int verbose)
 		0x000f & (le32_to_cpu (tmp) >> 7),
 		type,
 		0x007f & le32_to_cpu (tmp));
-	ohci_dbg (ohci, "  tds: head %08x %s%s tail %08x%s",
+	ohci_dbg (ohci, "  tds: head %08x %s%s tail %08x%s\n",
 		tmp = le32_to_cpup (&ed->hwHeadP),
 		(ed->hwHeadP & ED_C) ? data1 : data0,
 		(ed->hwHeadP & ED_H) ? " HALT" : "",
@@ -541,24 +543,29 @@ show_periodic (struct class_device *class_dev, char *buf)
 			if (temp == seen_count) {
 				u32	info = ed->hwINFO;
 				u32	scratch = cpu_to_le32p (&ed->hwINFO);
+				struct list_head	*entry;
+				unsigned		qlen = 0;
+
+				/* qlen measured here in TDs, not urbs */
+				list_for_each (entry, &ed->td_list)
+					qlen++;
 
 				temp = snprintf (next, size,
-					" (%cs dev%d%s ep%d%s"
+					" (%cs dev%d ep%d%s-%s qlen %u"
 					" max %d %08x%s%s)",
 					(info & ED_LOWSPEED) ? 'l' : 'f',
 					scratch & 0x7f,
-					(info & ED_ISO) ? " iso" : "",
 					(scratch >> 7) & 0xf,
 					(info & ED_IN) ? "in" : "out",
+					(info & ED_ISO) ? "iso" : "int",
+					qlen,
 					0x03ff & (scratch >> 16),
 					scratch,
-					(info & ED_SKIP) ? " s" : "",
+					(info & ED_SKIP) ? " K" : "",
 					(ed->hwHeadP & ED_H) ? " H" : "");
 				size -= temp;
 				next += temp;
 
-				// FIXME some TD info too
-
 				if (seen_count < DBG_SCHED_LIMIT)
 					seen [seen_count++] = ed;
 
@@ -617,7 +624,7 @@ show_registers (struct class_device *class_dev, char *buf)
 	/* hcca */
 	if (ohci->hcca)
 		ohci_dbg_sw (ohci, &next, &size,
-			"hcca frame 0x%04x\n", ohci->hcca->frame_no);
+			"hcca frame 0x%04x\n", OHCI_FRAME_NO(ohci->hcca));
 
 	/* other registers mostly affect frame timings */
 	rdata = readl (&regs->fminterval);

drivers/usb/host/ohci-hcd.c

@@ -226,7 +226,7 @@ static int ohci_urb_enqueue (
 		if (retval < 0)
 			goto fail;
 		if (ed->type == PIPE_ISOCHRONOUS) {
-			u16	frame = le16_to_cpu (ohci->hcca->frame_no);
+			u16	frame = OHCI_FRAME_NO(ohci->hcca);
 
 			/* delay a few frames before the first TD */
 			frame += max_t (u16, 8, ed->interval);
@@ -281,7 +281,7 @@ static int ohci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
 		urb_priv = urb->hcpriv;
 		if (urb_priv) {
 			if (urb_priv->ed->state == ED_OPER)
-				start_urb_unlink (ohci, urb_priv->ed);
+				start_ed_unlink (ohci, urb_priv->ed);
 		}
 	} else {
 		/*
@@ -363,7 +363,7 @@ static int ohci_get_frame (struct usb_hcd *hcd)
 {
 	struct ohci_hcd		*ohci = hcd_to_ohci (hcd);
 
-	return le16_to_cpu (ohci->hcca->frame_no);
+	return OHCI_FRAME_NO(ohci->hcca);
 }
 
 /*-------------------------------------------------------------------------*
@@ -591,7 +591,7 @@ static void ohci_irq (struct usb_hcd *hcd, struct pt_regs *ptregs)
 	 */
 	spin_lock (&ohci->lock);
 	if (ohci->ed_rm_list)
-		finish_unlinks (ohci, le16_to_cpu (ohci->hcca->frame_no),
+		finish_unlinks (ohci, OHCI_FRAME_NO(ohci->hcca),
 				ptregs);
 	if ((ints & OHCI_INTR_SF) != 0 && !ohci->ed_rm_list
 			&& HCD_IS_RUNNING(ohci->hcd.state))

drivers/usb/host/ohci-q.c

@@ -430,7 +430,7 @@ static struct ed *ed_get (
  * put the ep on the rm_list
  * real work is done at the next start frame (SF) hardware interrupt
  */
-static void start_urb_unlink (struct ohci_hcd *ohci, struct ed *ed)
+static void start_ed_unlink (struct ohci_hcd *ohci, struct ed *ed)
 {    
 	ed->hwINFO |= ED_DEQUEUE;
 	ed->state = ED_UNLINK;
@@ -441,7 +441,7 @@ static void start_urb_unlink (struct ohci_hcd *ohci, struct ed *ed)
 	 * behave.  frame_no wraps every 2^16 msec, and changes right before
 	 * SF is triggered.
 	 */
-	ed->tick = le16_to_cpu (ohci->hcca->frame_no) + 1;
+	ed->tick = OHCI_FRAME_NO(ohci->hcca) + 1;
 
 	/* rm_list is just singly linked, for simplicity */
 	ed->ed_next = ohci->ed_rm_list;
@@ -479,7 +479,8 @@ td_fill (struct ohci_hcd *ohci, u32 info,
 	 * and iso; other urbs rarely need more than one TD per urb.
 	 * this way, only final tds (or ones with an error) cause IRQs.
 	 * at least immediately; use DI=6 in case any control request is
-	 * tempted to die part way through.
+	 * tempted to die part way through.  (and to force the hc to flush
+	 * its donelist soonish, even on unlink paths.)
 	 *
 	 * NOTE: could delay interrupts even for the last TD, and get fewer
 	 * interrupts ... increasing per-urb latency by sharing interrupts.
@@ -879,14 +880,29 @@ finish_unlinks (struct ohci_hcd *ohci, u16 tick, struct pt_regs *regs)
 		u32			*prev;
 
 		/* only take off EDs that the HC isn't using, accounting for
-		 * frame counter wraps.
+		 * frame counter wraps and EDs with partially retired TDs
 		 */
-		if (tick_before (tick, ed->tick)
-				&& HCD_IS_RUNNING(ohci->hcd.state)) {
+		if (likely (HCD_IS_RUNNING(ohci->hcd.state))) {
+			if (tick_before (tick, ed->tick)) {
+skip_ed:
 				last = &ed->ed_next;
 				continue;
 			}
 
+			if (!list_empty (&ed->td_list)) {
+				struct td	*td;
+				u32		head;
+
+				td = list_entry (ed->td_list.next, struct td,
+							td_list);
+				head = cpu_to_le32 (ed->hwHeadP) & TD_MASK;
+
+				/* INTR_WDH may need to clean up first */
+				if (td->td_dma != head)
+					goto skip_ed;
+			}
+		}
+
 		/* reentrancy:  if we drop the schedule lock, someone might
 		 * have modified this list.  normally it's just prepending
 		 * entries (which we'd ignore), but paranoia won't hurt.

drivers/usb/host/ohci.h

@@ -172,8 +172,14 @@ static const int cc_to_error [16] = {
 struct ohci_hcca {
 #define NUM_INTS 32
 	__u32	int_table [NUM_INTS];	/* periodic schedule */
-	__u16	frame_no;		/* current frame number */
-	__u16	pad1;			/* set to 0 on each frame_no change */
+
+	/* 
+	 * OHCI defines u16 frame_no, followed by u16 zero pad.
+	 * Since some processors can't do 16 bit bus accesses,
+	 * portable access must be a 32 bit byteswapped access.
+	 */
+	u32	frame_no;		/* current frame number */
+#define OHCI_FRAME_NO(hccap) ((u16)le32_to_cpup(&(hccap)->frame_no))
 	__u32	done_head;		/* info returned for an interrupt */
 	u8	reserved_for_hc [116];
 	u8	what [4];		/* spec only identifies 252 bytes :) */