[PATCH] ehci locking

I've been chasing problems on a KT333 based system, with
the 8253 southbridge and EHCI 1.0 (!), and this fixes at
least some of them:

   - locking updates:
      * a few routines weren't protected right
      * less irqsave thrashing for schedule lock

   - adds a watchdog timer that should fire when the
     STS_IAA interrupt seems to be missing.

   - gives ports back to companion UHCI/OHCI on rmmod

   - re-enables faulted QH only after all its completion
     callbacks have done their work

   - removes an oops I've seen when usb-storage unlinks
     stuff.  (it seemed confused about error handling, but
     that's not a reason to oops.)

   - minor cleanup:  deadcode rm, etc

Right now the watchdog just barks, and that mechanism might
go away (or into the shared hcd code).  Sometimes the issue
it reports seems to clear up by itself, but sometimes not...

drivers/usb/host/ehci-hcd.c

@@ -87,7 +87,7 @@
  * 2001-June	Works with usb-storage and NEC EHCI on 2.4
  */
 
-#define DRIVER_VERSION "2002-Aug-06"
+#define DRIVER_VERSION "2002-Aug-28"
 #define DRIVER_AUTHOR "David Brownell"
 #define DRIVER_DESC "USB 2.0 'Enhanced' Host Controller (EHCI) Driver"
 
@@ -104,6 +104,8 @@
 #define	EHCI_TUNE_MULT_HS	1	/* 1-3 transactions/uframe; 4.10.3 */
 #define	EHCI_TUNE_MULT_TT	1
 
+#define EHCI_WATCHDOG_JIFFIES	(HZ/10)		/* arbitrary; ~100 msec */
+
 /* Initial IRQ latency:  lower than default */
 static int log2_irq_thresh = 0;		// 0 to 6
 MODULE_PARM (log2_irq_thresh, "i");
@@ -232,6 +234,23 @@ static void ehci_ready (struct ehci_hcd *ehci)
 
 static void ehci_tasklet (unsigned long param);
 
+static void ehci_watchdog (unsigned long param)
+{
+	struct ehci_hcd		*ehci = (struct ehci_hcd *) param;
+	unsigned long		flags;
+
+	/* guard against lost IAA, which wedges everything */
+	spin_lock_irqsave (&ehci->lock, flags);
+	if (ehci->reclaim) {
+		err ("%s watchdog, reclaim qh %p%s", ehci->hcd.self.bus_name,
+			ehci->reclaim, ehci->reclaim_ready ? " ready" : "");
+//		ehci->reclaim_ready = 1;
+		tasklet_schedule (&ehci->tasklet);
+	}
+	spin_unlock_irqrestore (&ehci->lock, flags);
+
+}
+
 /* EHCI 0.96 (and later) section 5.1 says how to kick BIOS/SMM/...
  * off the controller (maybe it can boot from highspeed USB disks).
  */
@@ -372,6 +391,10 @@ static int ehci_start (struct usb_hcd *hcd)
 	ehci->tasklet.func = ehci_tasklet;
 	ehci->tasklet.data = (unsigned long) ehci;
 
+	init_timer (&ehci->watchdog);
+	ehci->watchdog.function = ehci_watchdog;
+	ehci->watchdog.data = (unsigned long) ehci;
+
 	/* wire up the root hub */
 	hcd->self.root_hub = udev = usb_alloc_dev (NULL, &hcd->self);
 	if (!udev) {
@@ -394,7 +417,7 @@ static int ehci_start (struct usb_hcd *hcd)
         /* PCI Serial Bus Release Number is at 0x60 offset */
 	pci_read_config_byte (hcd->pdev, 0x60, &tempbyte);
 	temp = readw (&ehci->caps->hci_version);
-	info ("USB %x.%x support enabled, EHCI rev %x.%2x",
+	info ("USB %x.%x support enabled, EHCI rev %x.%02x",
 	      ((tempbyte & 0xf0)>>4),
 	      (tempbyte & 0x0f),
 	       temp >> 8,
@@ -433,8 +456,15 @@ static void ehci_stop (struct usb_hcd *hcd)
 	/* no more interrupts ... */
 	if (hcd->state == USB_STATE_RUNNING)
 		ehci_ready (ehci);
+	if (in_interrupt ())		/* should not happen!! */
+		err ("stopped %s!", RUN_CONTEXT);
+	else
+		del_timer_sync (&ehci->watchdog);
 	ehci_reset (ehci);
 
+	/* let companion controllers work when we aren't */
+	writel (0, &ehci->regs->configured_flag);
+
 	remove_debug_files (ehci);
 
 	/* root hub is shut down separately (first, when possible) */
@@ -546,12 +576,17 @@ dbg ("%s: resume port %d", hcd->self.bus_name, i);
 static void ehci_tasklet (unsigned long param)
 {
 	struct ehci_hcd		*ehci = (struct ehci_hcd *) param;
+	unsigned long		flags;
+
+	spin_lock_irqsave (&ehci->lock, flags);
 
 	if (ehci->reclaim_ready)
-		end_unlink_async (ehci);
-	scan_async (ehci);
+		flags = end_unlink_async (ehci, flags);
+	flags = scan_async (ehci, flags);
 	if (ehci->next_uframe != -1)
-		scan_periodic (ehci);
+		flags = scan_periodic (ehci, flags);
+
+	spin_unlock_irqrestore (&ehci->lock, flags);
 }
 
 /*-------------------------------------------------------------------------*/
@@ -681,7 +716,13 @@ static int ehci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
 	default:
 		spin_lock_irqsave (&ehci->lock, flags);
 		if (ehci->reclaim) {
-			dbg ("dq: reclaim busy, %s", RUN_CONTEXT);
+			dbg ("dq %p: reclaim = %p, %s",
+				qh, ehci->reclaim, RUN_CONTEXT);
+			if (qh == ehci->reclaim) {
+				/* unlinking qh for another queued urb? */
+				spin_unlock_irqrestore (&ehci->lock, flags);
+				return 0;
+			}
 			if (in_interrupt ()) {
 				spin_unlock_irqrestore (&ehci->lock, flags);
 				return -EAGAIN;
@@ -702,19 +743,19 @@ static int ehci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
 		break;
 
 	case PIPE_INTERRUPT:
+		spin_lock_irqsave (&ehci->lock, flags);
 		if (qh->qh_state == QH_STATE_LINKED) {
 			/* messy, can spin or block a microframe ... */
-			intr_deschedule (ehci, qh, 1);
+			flags = intr_deschedule (ehci, qh, 1, flags);
 			/* qh_state == IDLE */
 		}
-		qh_completions (ehci, qh);
+		flags = qh_completions (ehci, qh, flags);
 
 		/* reschedule QH iff another request is queued */
 		if (!list_empty (&qh->qtd_list)
 				&& HCD_IS_RUNNING (ehci->hcd.state)) {
 			int status;
 
-			spin_lock_irqsave (&ehci->lock, flags);
 			status = qh_schedule (ehci, qh);
 			spin_unlock_irqrestore (&ehci->lock, flags);
 
@@ -726,7 +767,7 @@ static int ehci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
 			}
 			return status;
 		}
-
+		spin_unlock_irqrestore (&ehci->lock, flags);
 		break;
 
 	case PIPE_ISOCHRONOUS:
@@ -805,8 +846,7 @@ static void ehci_free_config (struct usb_hcd *hcd, struct usb_device *udev)
 				start_unlink_async (ehci, qh);
 			while (qh->qh_state != QH_STATE_IDLE
 					&& ehci->hcd.state != USB_STATE_HALT) {
-				spin_unlock_irqrestore (&ehci->lock,
-					flags);
+				spin_unlock_irqrestore (&ehci->lock, flags);
 				wait_ms (1);
 				spin_lock_irqsave (&ehci->lock, flags);
 			}

drivers/usb/host/ehci-q.c

@@ -161,9 +161,10 @@ static inline void qtd_copy_status (struct urb *urb, size_t length, u32 token)
 
 /* urb->lock ignored from here on (hcd is done with urb) */
 
-static void ehci_urb_done (
+static unsigned long ehci_urb_done (
 	struct ehci_hcd		*ehci,
-	struct urb		*urb
+	struct urb		*urb,
+	unsigned long		flags
 ) {
 #ifdef	INTR_AUTOMAGIC
 	struct urb		*resubmit = 0;
@@ -199,6 +200,8 @@ static void ehci_urb_done (
 			urb->status = 0;
 	}
 
+	/* complete() can reenter this HCD */
+	spin_unlock_irqrestore (&ehci->lock, flags);
 	usb_hcd_giveback_urb (&ehci->hcd, urb);
 
 #ifdef	INTR_AUTOMAGIC
@@ -219,27 +222,25 @@ static void ehci_urb_done (
 		usb_put_urb (resubmit);
 	}
 #endif
+
+	spin_lock_irqsave (&ehci->lock, flags);
+	return flags;
 }
 
 
 /*
- * Process completed qtds for a qh, issuing completions if needed.
- * Frees qtds, unmaps buf, returns URB to driver.
- * Races up to qh->hw_current; returns number of urb completions.
+ * Process and free completed qtds for a qh, returning URBs to drivers.
+ * Chases up to qh->hw_current, returns irqsave flags (maybe modified).
  */
-static void
-qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
+static unsigned long
+qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh, unsigned long flags)
 {
 	struct ehci_qtd		*qtd, *last;
 	struct list_head	*next, *qtd_list = &qh->qtd_list;
 	int			unlink = 0, halted = 0;
-	unsigned long		flags;
 
-	spin_lock_irqsave (&ehci->lock, flags);
-	if (unlikely (list_empty (qtd_list))) {
-		spin_unlock_irqrestore (&ehci->lock, flags);
-		return;
-	}
+	if (unlikely (list_empty (qtd_list)))
+		return flags;
 
 	/* scan QTDs till end of list, or we reach an active one */
 	for (qtd = list_entry (qtd_list->next, struct ehci_qtd, qtd_list),
@@ -252,12 +253,8 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
 
 		/* clean up any state from previous QTD ...*/
 		if (last) {
-			if (likely (last->urb != urb)) {
-				/* complete() can reenter this HCD */
-				spin_unlock_irqrestore (&ehci->lock, flags);
-				ehci_urb_done (ehci, last->urb);
-				spin_lock_irqsave (&ehci->lock, flags);
-			}
+			if (likely (last->urb != urb))
+				flags = ehci_urb_done (ehci, last->urb, flags);
 
 			/* qh overlays can have HC's old cached copies of
 			 * next qtd ptrs, if an URB was queued afterwards.
@@ -283,6 +280,9 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
 			|| (ehci->hcd.state == USB_STATE_HALT)
 			|| (qh->qh_state == QH_STATE_IDLE);
 
+		// FIXME Remove the automagic unlink mode.
+		// Drivers can now clean up safely; its' their job.
+
 		/* fault: unlink the rest, since this qtd saw an error? */
 		if (unlikely ((token & QTD_STS_HALT) != 0)) {
 			unlink = 1;
@@ -341,18 +341,19 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
 #endif
 	}
 
-	/* patch up list head? */
+	/* last urb's completion might still need calling */
+	if (likely (last != 0)) {
+		flags = ehci_urb_done (ehci, last->urb, flags);
+		ehci_qtd_free (ehci, last);
+	}
+
+	/* reactivate queue after error and driver's cleanup */
 	if (unlikely (halted && !list_empty (qtd_list))) {
 		qh_update (qh, list_entry (qtd_list->next,
 				struct ehci_qtd, qtd_list));
 	}
-	spin_unlock_irqrestore (&ehci->lock, flags);
 
-	/* last urb's completion might still need calling */
-	if (likely (last != 0)) {
-		ehci_urb_done (ehci, last->urb);
-		ehci_qtd_free (ehci, last);
-	}
+	return flags;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -367,31 +368,12 @@ static void qtd_list_free (
 	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;
-			size_t	size;
-
-			/* for ctrl unmap twice: SETUP and DATA;
-			 * else (bulk, intr) just once: DATA
-			 */
-			if (!unmapped++ && usb_pipecontrol (urb->pipe)) {
-				direction = PCI_DMA_TODEVICE;
-				size = sizeof (struct usb_ctrlrequest);
-			} else {
-				direction = usb_pipein (urb->pipe)
-					? PCI_DMA_FROMDEVICE
-					: PCI_DMA_TODEVICE;
-				size = qtd->urb->transfer_buffer_length;
-				unmapped++;
-			}
-		}
 		ehci_qtd_free (ehci, qtd);
 	}
 }
@@ -886,25 +868,28 @@ submit_async (
 /* the async qh for the qtds being reclaimed are now unlinked from the HC */
 /* caller must not own ehci->lock */
 
-static void end_unlink_async (struct ehci_hcd *ehci)
+static unsigned long
+end_unlink_async (struct ehci_hcd *ehci, unsigned long flags)
 {
 	struct ehci_qh		*qh = ehci->reclaim;
 
+	del_timer (&ehci->watchdog);
+
 	qh->qh_state = QH_STATE_IDLE;
 	qh->qh_next.qh = 0;
 	qh_put (ehci, qh);			// refcount from reclaim 
 	ehci->reclaim = 0;
 	ehci->reclaim_ready = 0;
 
-	qh_completions (ehci, qh);
+	flags = qh_completions (ehci, qh, flags);
 
-	// unlink any urb should now unlink all following urbs, so that
-	// relinking only happens for urbs before the unlinked ones.
 	if (!list_empty (&qh->qtd_list)
 			&& HCD_IS_RUNNING (ehci->hcd.state))
 		qh_link_async (ehci, qh);
 	else
 		qh_put (ehci, qh);		// refcount from async list
+
+	return flags;
 }
 
 
@@ -975,16 +960,17 @@ static void start_unlink_async (struct ehci_hcd *ehci, struct ehci_qh *qh)
 	cmd |= CMD_IAAD;
 	writel (cmd, &ehci->regs->command);
 	/* posted write need not be known to HC yet ... */
+
+	mod_timer (&ehci->watchdog, jiffies + EHCI_WATCHDOG_JIFFIES);
 }
 
 /*-------------------------------------------------------------------------*/
 
-static void scan_async (struct ehci_hcd *ehci)
+static unsigned long
+scan_async (struct ehci_hcd *ehci, unsigned long flags)
 {
 	struct ehci_qh		*qh;
-	unsigned long		flags;
 
-	spin_lock_irqsave (&ehci->lock, flags);
 rescan:
 	qh = ehci->async;
 	if (likely (qh != 0)) {
@@ -993,12 +979,9 @@ static void scan_async (struct ehci_hcd *ehci)
 			if (!list_empty (&qh->qtd_list)) {
 				// dbg_qh ("scan_async", ehci, qh);
 				qh = qh_get (qh);
-				spin_unlock_irqrestore (&ehci->lock, flags);
 
 				/* concurrent unlink could happen here */
-				qh_completions (ehci, qh);
-
-				spin_lock_irqsave (&ehci->lock, flags);
+				flags = qh_completions (ehci, qh, flags);
 				qh_put (ehci, qh);
 			}
 
@@ -1020,6 +1003,5 @@ static void scan_async (struct ehci_hcd *ehci)
 				goto rescan;
 		} while (qh != ehci->async);
 	}
-
-	spin_unlock_irqrestore (&ehci->lock, flags);
+	return flags;
 }

drivers/usb/host/ehci-sched.c

@@ -222,17 +222,15 @@ static int disable_periodic (struct ehci_hcd *ehci)
 
 // FIXME microframe periods not yet handled
 
-static void intr_deschedule (
+static unsigned long intr_deschedule (
 	struct ehci_hcd	*ehci,
 	struct ehci_qh	*qh,
-	int		wait
+	int		wait,
+	unsigned long	flags
 ) {
-	unsigned long	flags;
 	int		status;
 	unsigned	frame = qh->start;
 
-	spin_lock_irqsave (&ehci->lock, flags);
-
 	do {
 		periodic_unlink (ehci, frame, qh);
 		qh_put (ehci, qh);
@@ -251,8 +249,6 @@ static void intr_deschedule (
 		vdbg ("periodic schedule still enabled");
 	}
 
-	spin_unlock_irqrestore (&ehci->lock, flags);
-
 	/*
 	 * If the hc may be looking at this qh, then delay a uframe
 	 * (yeech!) to be sure it's done.
@@ -260,8 +256,10 @@ static void intr_deschedule (
 	 */
 	if (((ehci_get_frame (&ehci->hcd) - frame) % qh->period) == 0) {
 		if (wait) {
+			spin_unlock_irqrestore (&ehci->lock, flags);
 			udelay (125);
 			qh->hw_next = EHCI_LIST_END;
+			spin_lock_irqsave (&ehci->lock, flags);
 		} else {
 			/* we may not be IDLE yet, but if the qh is empty
 			 * the race is very short.  then if qh also isn't
@@ -281,6 +279,7 @@ static void intr_deschedule (
 	vdbg ("descheduled qh %p, per = %d frame = %d count = %d, urbs = %d",
 		qh, qh->period, frame,
 		atomic_read (&qh->refcount), ehci->periodic_sched);
+	return flags;
 }
 
 static int check_period (
@@ -513,12 +512,10 @@ intr_complete (
 	}
 	
 	/* handle any completions */
-	spin_unlock_irqrestore (&ehci->lock, flags);
-	qh_completions (ehci, qh);
-	spin_lock_irqsave (&ehci->lock, flags);
+	flags = qh_completions (ehci, qh, flags);
 
 	if (unlikely (list_empty (&qh->qtd_list)))
-		intr_deschedule (ehci, qh, 0);
+		flags = intr_deschedule (ehci, qh, 0, flags);
 
 	return flags;
 }
@@ -1091,13 +1088,12 @@ static int sitd_submit (struct ehci_hcd *ehci, struct urb *urb, int mem_flags)
 
 /*-------------------------------------------------------------------------*/
 
-static void scan_periodic (struct ehci_hcd *ehci)
+static unsigned long
+scan_periodic (struct ehci_hcd *ehci, unsigned long flags)
 {
 	unsigned	frame, clock, now_uframe, mod;
-	unsigned long	flags;
 
 	mod = ehci->periodic_size << 3;
-	spin_lock_irqsave (&ehci->lock, flags);
 
 	/*
 	 * When running, scan from last scan point up to "now"
@@ -1237,5 +1233,5 @@ static void scan_periodic (struct ehci_hcd *ehci)
 		} else
 			frame = (frame + 1) % ehci->periodic_size;
 	} 
-	spin_unlock_irqrestore (&ehci->lock, flags);
+	return flags;
 }

drivers/usb/host/ehci.h

@@ -69,6 +69,8 @@ struct ehci_hcd {			/* one per controller */
 	struct pci_pool		*qtd_pool;	/* one or more per qh */
 	struct pci_pool		*itd_pool;	/* itd per iso urb */
 	struct pci_pool		*sitd_pool;	/* sitd per split iso urb */
+
+	struct timer_list	watchdog;
 };
 
 /* unwrap an HCD pointer to get an EHCI_HCD pointer */