PROBLEM: usb not working with 2.4.8-ac8

Looks like the change int he usb_start_wait_urb code is a problem.

I suspect either you need to add wmb() and rmb() to stop misoptimisations on done (along with making it (!timeout && !awd.done)

there is a signal related problem - if so making it set the state to TASK_UNINTERRUPTIBLE might cure it

Quoted 2 lines

Looks like the change int he usb_start_wait_urb code is a problem.

Yes, something was not right with my change.
I am trying to coax people into testing this (on top of -ac8):

drivers/usb/usb.c Tue Aug 21 16:02:01 2001 renamed from drivers/usb/usb.c Tue Aug 21 14:39:55 2001 +11 -4

@@ -1080,10 +1080,17 @@
        current->state = TASK_RUNNING;
        remove_wait_queue(&awd.wqh, &wait);
 
-       if (!timeout) {
-               printk("usb_control/bulk_msg: timeout\n");
-               usb_unlink_urb(urb);  // remove urb safely
-               status = -ETIMEDOUT;
+       if (!awd.done) {
+               if (urb->status != -EINPROGRESS) {      /* No callback?!! */
+                       printk(KERN_ERR "usb: raced timeout, "
+                           "pipe 0x%x status %d time left %d\n",
+                           urb->pipe, urb->status, timeout);
+                       status = urb->status;
+               } else {
+                       printk("usb_control/bulk_msg: timeout\n");
+                       usb_unlink_urb(urb);  // remove urb safely
+                       status = -ETIMEDOUT;
+               }
        } else
                status = urb->status;
 

Quoted 3 lines

I suspect either you need to add wmb() and rmb() to stop misoptimisations on done (along with making it (!timeout && !awd.done)

Arjan said so too - I'll add them later just in case. About (!timeout && !awd.done) - it's not going to be enough, I think. I suspect that callback is not delivered at all in some circumstances, or something really screwy like that.

Quoted 2 lines

there is a signal related problem - if so making it set the state to TASK_UNINTERRUPTIBLE might cure it

I tried that first and it worked, but someone disagreed. Dunno...

-- Pete
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Date: Wed, 22 Aug 2001 01:48:57 -0500
From: Jordan Breeding <[email protected]>
To: Pete Zaitcev <[email protected]>,
Linux Kernel <[email protected]>

Running 2.4.8-ac7 using the alternate uhci driver both my USB keyboard and mouse work fine. I can't use the regular uhci driver because it aparently has SMP issues with keyboards and like to hard lock the entire system, no oops or anything, if any LED key (numlock,capslock,scrolllock) is pressed enough times to turn an LED on and then off.

Using 2.4.8-ac8 with the patch from below at least works
more than stock 2.4.8-ac8 but I still get failures: specifically the regular uhci driver seems to work perfectly except for the issue noted above.

But my uchi controller using the alternate uhci driver on
2.4.8-ac8 results in an unusable system, and with the patch below kind of works. By kind of I mean that it doesn't oops and there aren't any of the couldn't assign id type messages but I get a ton of the "raced timeout" messages and the mouse and keyboard only sporadically work,

The usb-uhci and ohci worked fine, so I poked uhci a little bit and made it all work, at least with my setup. Apparently, sometimes uhci returns a success (0) from submit_urb, but forgets to deliver a completion callback.

The fix is a Johannes' territory and I not so sure of my way there, so perhaps a better one may be forthcoming.

diff -ur -X dontdiff linux-2.4.8-ac9/drivers/usb/uhci.c linux-2.4.8-ac9-niph/drivers/usb/uhci.c

drivers/usb/uhci.c +13 -9

@@ -1471,6 +1471,7 @@
 
 static int uhci_submit_urb(struct urb *urb)
 {
+	struct usb_device *dev;
 	int ret = -EINVAL;
 	struct uhci *uhci;
 	unsigned long flags;

@@ -1480,15 +1481,16 @@
 	if (!urb)
 		return -EINVAL;
 
-	if (!urb->dev || !urb->dev->bus || !urb->dev->bus->hcpriv) {
+	dev = urb->dev;
+	if (!dev || !dev->bus || !dev->bus->hcpriv) {
 		warn("uhci_submit_urb: urb %p belongs to disconnected device or bus?", urb);
 		return -ENODEV;
 	}
 
-	uhci = (struct uhci *)urb->dev->bus->hcpriv;
+	uhci = (struct uhci *)dev->bus->hcpriv;
 
 	INIT_LIST_HEAD(&urb->urb_list);
-	usb_inc_dev_use(urb->dev);
+	usb_inc_dev_use(dev);
 
 	spin_lock_irqsave(&urb->lock, flags);
 

@@ -1497,7 +1499,7 @@
 		dbg("uhci_submit_urb: urb not available to submit (status = %d)", urb->status);
 		/* Since we can have problems on the out path */
 		spin_unlock_irqrestore(&urb->lock, flags);
-		usb_dec_dev_use(urb->dev);
+		usb_dec_dev_use(dev);
 
 		return ret;
 	}

@@ -1516,7 +1518,7 @@
 	}
 
 	/* Short circuit the virtual root hub */
-	if (urb->dev == uhci->rh.dev) {
+	if (dev == uhci->rh.dev) {
 		ret = rh_submit_urb(urb);
 
 		goto out;

@@ -1528,13 +1530,13 @@
 		break;
 	case PIPE_INTERRUPT:
 		if (urb->bandwidth == 0) {	/* not yet checked/allocated */
-			bustime = usb_check_bandwidth(urb->dev, urb);
+			bustime = usb_check_bandwidth(dev, urb);
 			if (bustime < 0)
 				ret = bustime;
 			else {
 				ret = uhci_submit_interrupt(urb);
 				if (ret == -EINPROGRESS)
-					usb_claim_bandwidth(urb->dev, urb, bustime, 0);
+					usb_claim_bandwidth(dev, urb, bustime, 0);
 			}
 		} else		/* bandwidth is already set */
 			ret = uhci_submit_interrupt(urb);

@@ -1548,7 +1550,7 @@
 				ret = -EINVAL;
 				break;
 			}
-			bustime = usb_check_bandwidth(urb->dev, urb);
+			bustime = usb_check_bandwidth(dev, urb);
 			if (bustime < 0) {
 				ret = bustime;
 				break;

@@ -1556,7 +1558,7 @@
 
 			ret = uhci_submit_isochronous(urb);
 			if (ret == -EINPROGRESS)
-				usb_claim_bandwidth(urb->dev, urb, bustime, 1);
+				usb_claim_bandwidth(dev, urb, bustime, 1);
 		} else		/* bandwidth is already set */
 			ret = uhci_submit_isochronous(urb);
 		break;

@@ -1578,8 +1580,14 @@
 
 	uhci_unlink_generic(uhci, urb);
 	uhci_destroy_urb_priv(urb);
+	if (ret == 0) {				/* N.B. Done, must notify */
+		/* uhci_call_completion(urb); */ /* ->> uhci_destroy_urb_priv */

Quoted 1 line

What was you test scenario where you didn't see a completion callback?

The root hub emulation never posts one.

Quoted 7 lines

Quoted 7 lines

(quoted diff) +6 -0

+	if (ret == 0) {				/* N.B. Done, must notify */
+		/* uhci_call_completion(urb); */ /* ->> uhci_destroy_urb_priv */
+		urb->dev = NULL;
+		if (urb->complete)
+			urb->complete(urb);
+	}

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +1 -1

-	usb_dec_dev_use(urb->dev);
+	usb_dec_dev_use(dev);

What's all of this for? Protecting against an urb->dev race condition?

No race, but we cannot use urb after callback.