ChangeSet 1.823.3.18, 2002/11/14 11:25:53-08:00, david-b@pacbell.net

[PATCH] cleanup usb hcd unlink code

This fixes various minor problems:

- re-orders some tests so that "(no bus?)" diagnostic should
    no longer be appearing (and making folk worry needlessly)

- removes one unreachable test for URB_TIMEOUT_KILLED

- removes the reachable test, since it's never an error on the
    part of the device driver to unlink something the HCD is already
    unlinking.

- gets rid of some comments and code that expected automagic resubmits
    for interrupts (no more!),

- resolves a FIXME for a rather unlikely situation (HCD can't
    perform the unlink, it reports an error)

It also starts to use dev_dbg() macros, which give more concise
(lately) and useful (they have both driver name and device id)
diagnostics than the previous usb-only dbg() macros.  To do this,
DEBUG had to be #defined before <linux/driver.h> is included, but
it can't be #undeffed before <linux/kernel.h> is included.


diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c	Thu Nov 14 14:12:08 2002
+++ b/drivers/usb/core/hcd.c	Thu Nov 14 14:12:08 2002
@@ -26,19 +26,19 @@
 #include <linux/module.h>
 #include <linux/version.h>
 #include <linux/kernel.h>
+
+#ifdef CONFIG_USB_DEBUG
+#	define DEBUG
+#else
+#	undef DEBUG
+#endif
+
 #include <linux/slab.h>
 #include <linux/completion.h>
 #include <linux/uts.h>			/* for UTS_SYSNAME */
 #include <linux/pci.h>			/* for hcd->pdev and dma addressing */
 #include <asm/byteorder.h>
 
-
-#ifdef CONFIG_USB_DEBUG
-	#define DEBUG
-#else
-	#undef DEBUG
-#endif
-
 #include <linux/usb.h>
 #include "hcd.h"
 
@@ -1090,6 +1090,7 @@
 {
 	struct hcd_dev			*dev;
 	struct usb_hcd			*hcd = 0;
+	struct device			*sys = 0;
 	unsigned long			flags;
 	struct completion_splice	splice;
 	int				retval;
@@ -1110,38 +1111,31 @@
 	 */
 	spin_lock_irqsave (&urb->lock, flags);
 	spin_lock (&hcd_data_lock);
-	if (!urb->hcpriv || urb->transfer_flags & URB_TIMEOUT_KILLED) {
-		retval = -EINVAL;
-		goto done;
-	}
 
 	if (!urb->dev || !urb->dev->bus) {
 		retval = -ENODEV;
 		goto done;
 	}
 
-	/* giveback clears dev; non-null means it's linked at this level */
 	dev = urb->dev->hcpriv;
+	sys = &urb->dev->dev;
 	hcd = urb->dev->bus->hcpriv;
 	if (!dev || !hcd) {
 		retval = -ENODEV;
 		goto done;
 	}
 
-	/* Except for interrupt transfers, any status except -EINPROGRESS
-	 * means the HCD already started to unlink this URB from the hardware.
-	 * So there's no more work to do.
-	 *
-	 * For interrupt transfers, this is the only way to trigger unlinking
-	 * from the hardware.  Since we (currently) overload urb->status to
-	 * tell the driver to unlink, error status might get clobbered ...
-	 * unless that transfer hasn't yet restarted.  One such case is when
-	 * the URB gets unlinked from its completion handler.
+	if (!urb->hcpriv) {
+		retval = -EINVAL;
+		goto done;
+	}
+
+	/* Any status except -EINPROGRESS means something already started to
+	 * unlink this URB from the hardware.  So there's no more work to do.
 	 *
-	 * FIXME use an URB_UNLINKED flag to match URB_TIMEOUT_KILLED
+	 * FIXME use better explicit urb state
 	 */
-	if (urb->status != -EINPROGRESS
-			&& usb_pipetype (urb->pipe) != PIPE_INTERRUPT) {
+	if (urb->status != -EINPROGRESS) {
 		retval = -EINVAL;
 		goto done;
 	}
@@ -1150,9 +1144,7 @@
 	 * lower level hcd code is always async, locking on urb->status
 	 * updates; an intercepted completion unblocks us.
 	 */
-	if ((urb->transfer_flags & URB_TIMEOUT_KILLED))
-		urb->status = -ETIMEDOUT;
-	else if (!(urb->transfer_flags & URB_ASYNC_UNLINK)) {
+	if (!(urb->transfer_flags & URB_ASYNC_UNLINK)) {
 		if (in_interrupt ()) {
 			dbg ("non-async unlink in_interrupt");
 			retval = -EWOULDBLOCK;
@@ -1177,29 +1169,34 @@
 		retval = 0;
 	} else {
 		retval = hcd->driver->urb_dequeue (hcd, urb);
-// FIXME:  if retval and we tried to splice, whoa!!
-if (retval && urb->status == -ENOENT) err ("whoa! retval %d", retval);
+
+		/* hcds shouldn't really fail these calls, but... */
+		if (retval) {
+			dev_dbg (*sys, "dequeue %p --> %d\n", urb, retval);
+			if (!(urb->transfer_flags & URB_ASYNC_UNLINK)) {
+				spin_lock_irqsave (&urb->lock, flags);
+				urb->complete = splice.complete;
+				urb->context = splice.context;
+				spin_unlock_irqrestore (&urb->lock, flags);
+			}
+			goto bye;
+		}
 	}
 
     	/* block till giveback, if needed */
-	if (!(urb->transfer_flags & (URB_ASYNC_UNLINK|URB_TIMEOUT_KILLED))
-			&& HCD_IS_RUNNING (hcd->state)
-			&& !retval) {
-		dbg ("%s: wait for giveback urb %p",
-			hcd->self.bus_name, urb);
-		wait_for_completion (&splice.done);
-	} else if ((urb->transfer_flags & URB_ASYNC_UNLINK) && retval == 0) {
+	if (urb->transfer_flags & URB_ASYNC_UNLINK)
 		return -EINPROGRESS;
-	}
-	goto bye;
+
+	dev_dbg (*sys, "wait for giveback urb %p\n", urb);
+	wait_for_completion (&splice.done);
+	return 0;
+
 done:
 	spin_unlock (&hcd_data_lock);
 	spin_unlock_irqrestore (&urb->lock, flags);
 bye:
-	if (retval)
-		dbg ("%s: hcd_unlink_urb fail %d",
-		    hcd ? hcd->self.bus_name : "(no bus?)",
-		    retval);
+	if (retval && sys)
+		dev_dbg (*sys, "hcd_unlink_urb %p fail %d\n", urb, retval);
 	return retval;
 }