[PATCH] ohci unlink cleanups

Attached is a patch that cleans up a few more issues in the OHCI unlink
code.

There may still be an ISO-IN data problem, I'll look at that separately
since it seems unrelated to unlink issues.

- Simplify/correct ED lifecycle
	* UNLINK is now for real: descheduled and waiting for SOF
	* finish_unlinks() expects descheduled EDs (may reschedule)
	* only ed_deschedule() turns off hardware schedule processing
	* no more NEW state
	* no more ED_URB_DEL flag (it added extra states)
	* new IDLE state, "not scheduled" (replaces previous UNLINKing)
- Bugfixes
	* ed_get(), potential memleak is now gone
	* urb_enqueue(), won't submit to dead/sleeping hc
	* free_config(), rescans after SOF when needed
	* ed_schedule(), use wmb()
	* ed_schedule() and finish_unlinks(), more thorough about
	  restarting control or bulk processing
	* finish_unlinks(), more cautious about reentering
- General:
	* ed->ed_rm_list renamed ed_next; to be used more later
	* slightly shrink object code
	* rename some functions

This leaves one notable issue in the unlink paths:  the driver never waits
for SOF after descheduling (empty) EDs.  That's racey in most cases, though
there are a few light-traffic cases where that's correct (in part because
the ED is empty).  Easy to fix once the rest of this is known to behave.

drivers/usb/host/ohci-hcd.c

@@ -10,8 +10,15 @@
  * [ (C) Copyright 1999 Gregory P. Smith]
  * 
  * 
+ * OHCI is the main "non-Intel/VIA" standard for USB 1.1 host controller
+ * interfaces (though some non-x86 Intel chips use it).  It supports
+ * smarter hardware than UHCI.  A download link for the spec available
+ * through the http://www.usb.org website.
+ *
  * History:
  * 
+ * 2002/07/19 fixes to management of ED and schedule state.
+ * 2002/06/09 SA-1111 support (Christopher Hoover)
  * 2002/06/01 remember frame when HC won't see EDs any more; use that info
  *	to fix urb unlink races caused by interrupt latency assumptions;
  *	minor ED field and function naming updates
@@ -95,12 +102,12 @@
 /*
  * TO DO:
  *
- *	- "disabled" should be the hcd state
+ *	- "disabled" and "sleeping" should be in hcd->state
  *	- bandwidth alloc to generic code
  *	- lots more testing!!
  */
 
-#define DRIVER_VERSION "2002-Jun-15"
+#define DRIVER_VERSION "2002-Jul-19"
 #define DRIVER_AUTHOR "Roman Weissgaerber <weissg@vienna.at>, David Brownell"
 #define DRIVER_DESC "USB 1.1 'Open' Host Controller (OHCI) Driver"
 
@@ -140,6 +147,7 @@ static int ohci_urb_enqueue (
 	int		i, size = 0;
 	unsigned long	flags;
 	int		bustime = 0;
+	int		retval = 0;
 	
 #ifdef OHCI_VERBOSE_DEBUG
 	urb_print (urb, "SUB", usb_pipein (pipe));
@@ -191,19 +199,25 @@ static int ohci_urb_enqueue (
 		return -ENOMEM;
 	memset (urb_priv, 0, sizeof (urb_priv_t) + size * sizeof (struct td *));
 	
+	spin_lock_irqsave (&ohci->lock, flags);
+
+	/* don't submit to a dead HC */
+	if (ohci->disabled || ohci->sleeping) {
+		retval = -ENODEV;
+		goto fail;
+	}
+
 	/* fill the private part of the URB */
 	urb_priv->length = size;
 	urb_priv->ed = ed;	
 
 	/* allocate the TDs (updating hash chains) */
-	spin_lock_irqsave (&ohci->lock, flags);
 	for (i = 0; i < size; i++) {
 		urb_priv->td [i] = td_alloc (ohci, SLAB_ATOMIC);
 		if (!urb_priv->td [i]) {
 			urb_priv->length = i;
-			urb_free_priv (ohci, urb_priv);
-			spin_unlock_irqrestore (&ohci->lock, flags);
-			return -ENOMEM;
+			retval = -ENOMEM;
+			goto fail;
 		}
 	}	
 
@@ -217,7 +231,7 @@ static int ohci_urb_enqueue (
 	switch (usb_pipetype (pipe)) {
 		case PIPE_ISOCHRONOUS:
 			if (urb->transfer_flags & USB_ISO_ASAP) { 
-				urb->start_frame = ( (ed->state == ED_OPER)
+				urb->start_frame = ((ed->state != ED_IDLE)
 					? (ed->intriso.last_iso + 1)
 					: (le16_to_cpu (ohci->hcca->frame_no)
 						+ 10)) & 0xffff;
@@ -238,18 +252,20 @@ static int ohci_urb_enqueue (
 
 	urb->hcpriv = urb_priv;
 
-	/* link the ed into a chain if is not already */
-	if (ed->state != ED_OPER)
-		ep_link (ohci, ed);
+	/* schedule the ed if needed */
+	if (ed->state == ED_IDLE)
+		ed_schedule (ohci, ed);
 
 	/* fill the TDs and link them to the ed; and
 	 * enable that part of the schedule, if needed
 	 */
 	td_submit_urb (urb);
 
+fail:
+	if (retval)
+		urb_free_priv (ohci, urb_priv);
 	spin_unlock_irqrestore (&ohci->lock, flags);
-
-	return 0;
+	return retval;
 }
 
 /*
@@ -270,19 +286,17 @@ static int ohci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
 	if (!ohci->disabled) {
 		urb_priv_t  *urb_priv;
 
-		/* flag the urb's data for deletion in some upcoming
-		 * SF interrupt's delete list processing
+		/* Unless an IRQ completed the unlink while it was being
+		 * handed to us, flag it for unlink and giveback, and force
+		 * some upcoming INTR_SF to call finish_unlinks()
 		 */
 		spin_lock_irqsave (&ohci->lock, flags);
 		urb_priv = urb->hcpriv;
-
-		if (!urb_priv || (urb_priv->state == URB_DEL)) {
-			spin_unlock_irqrestore (&ohci->lock, flags);
-			return 0;
-		}
-			
+		if (urb_priv) {
 			urb_priv->state = URB_DEL; 
+			if (urb_priv->ed->state == ED_OPER)
 				start_urb_unlink (ohci, urb_priv->ed);
+		}
 		spin_unlock_irqrestore (&ohci->lock, flags);
 	} else {
 		/*
@@ -296,6 +310,10 @@ static int ohci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
 
 /*-------------------------------------------------------------------------*/
 
+/* frees config/altsetting state for endpoints,
+ * including ED memory, dummy TD, and bulk/intr data toggle
+ */
+
 static void
 ohci_free_config (struct usb_hcd *hcd, struct usb_device *udev)
 {
@@ -303,7 +321,11 @@ ohci_free_config (struct usb_hcd *hcd, struct usb_device *udev)
 	struct hcd_dev		*dev = (struct hcd_dev *) udev->hcpriv;
 	int			i;
 	unsigned long		flags;
+#ifdef DEBUG
+	int			rescans = 0;
+#endif
 
+rescan:
 	/* free any eds, and dummy tds, still hanging around */
 	spin_lock_irqsave (&ohci->lock, flags);
 	for (i = 0; i < 32; i++) {
@@ -312,27 +334,47 @@ ohci_free_config (struct usb_hcd *hcd, struct usb_device *udev)
 		if (!ed)
 			continue;
 
-		ed->state &= ~ED_URB_DEL;
-		if (ohci->disabled && ed->state == ED_OPER)
-			ed->state = ED_UNLINK;
+		if (ohci->disabled && ed->state != ED_IDLE)
+			ed->state = ED_IDLE;
 		switch (ed->state) {
-		case ED_NEW:
-			break;
-		case ED_UNLINK:
+		case ED_UNLINK:		/* wait a frame? */
+			goto do_rescan;
+		case ED_IDLE:		/* fully unlinked */
 			td_free (ohci, ed->dummy);
 			break;
-
-		case ED_OPER:
 		default:
+#ifdef DEBUG
 			err ("illegal ED %d state in free_config, %d",
 				i, ed->state);
-#ifdef DEBUG
-			BUG ();
 #endif
+			/* ED_OPER: some driver disconnect() is broken,
+			 * it didn't even start its unlinks much less wait
+			 * for their completions.
+			 * OTHERWISE:  hcd bug, ed is garbage
+			 */
+			BUG ();
 		}
 		ed_free (ohci, ed);
 	}
 	spin_unlock_irqrestore (&ohci->lock, flags);
+	return;
+
+do_rescan:
+#ifdef DEBUG
+	/* a driver->disconnect() returned before its unlinks completed? */
+	if (in_interrupt ()) {
+		dbg ("WARNING: spin in interrupt; driver->disconnect() bug");
+		dbg ("dev usb-%s-%s ep 0x%x", 
+			ohci->hcd.self.bus_name, udev->devpath, i);
+	}
+	BUG_ON (!(readl (&ohci->regs->intrenable) & OHCI_INTR_SF));
+	BUG_ON (rescans >= 2);	/* HWBUG */
+	rescans++;
+#endif
+
+	spin_unlock_irqrestore (&ohci->lock, flags);
+	wait_ms (1);
+	goto rescan;
 }
 
 static int ohci_get_frame (struct usb_hcd *hcd)

drivers/usb/host/ohci.h

@@ -31,15 +31,21 @@ struct ed {
 
 	/* rest are purely for the driver's use */
 	dma_addr_t		dma;		/* addr of ED */
+	struct td		*dummy;		/* next TD to activate */
+
+	/* host's view of schedule */
+	struct ed		*ed_next;	/* on schedule or rm_list */
 	struct ed		*ed_prev;	/* for non-interrupt EDs */
-	struct td		*dummy;
 	struct list_head	td_list;	/* "shadow list" of our TDs */
 
-	u8			state;		/* ED_{NEW,UNLINK,OPER} */
-#define ED_NEW 		0x00		/* unused, no dummy td */
-#define ED_UNLINK 	0x01		/* dummy td, maybe linked to hc */
-#define ED_OPER		0x02		/* dummy td, _is_ linked to hc */
-#define ED_URB_DEL  	0x08		/* for unlinking; masked in */
+	/* create --> IDLE --> OPER --> ... --> IDLE --> destroy
+	 * usually:  OPER --> UNLINK --> (IDLE | OPER) --> ...
+	 * some special cases :  OPER --> IDLE ...
+	 */
+	u8			state;		/* ED_{IDLE,UNLINK,OPER} */
+#define ED_IDLE 	0x00		/* NOT linked to HC */
+#define ED_UNLINK 	0x01		/* being unlinked from hc */
+#define ED_OPER		0x02		/* IS linked to hc */
 
 	u8			type; 		/* PIPE_{BULK,...} */
 	u16			interval;	/* interrupt, isochronous */
@@ -53,7 +59,6 @@ struct ed {
 
 	/* HC may see EDs on rm_list until next frame (frame_no == tick) */
 	u16			tick;
-	struct ed		*ed_rm_list;
 } __attribute__ ((aligned(16)));
 
 #define ED_MASK	((u32)~0x0f)		/* strip hw status in low addr bits */