On Mon, 9 Feb 2004, Stephen Hemminger wrote:
> The following patch fixes the race where a URB is dequeued but it on the
> complete list. It moves up uhci->list_lock so it overlaps the remove_list
> lock.
>
> This is the important one, the other 9 are just minor improvements that
> reduce code or memory.
Never mind what I said earlier. I think I figured out what's really
happening.
A while back I wrote that an URB can't both be unlinked and complete
normally. Well, that's _supposed_ to be true -- but it isn't quite. This
is actually something I've wanted to fix for a long time.
The urbp structure includes an unnecessary status member, which does
little more than duplicate the information in urb->status. This is a case
where duplication is definitely bad, because the USB core uses urb->status
to determine whether or not the URB has completed or been unlinked, but
the UHCI driver was storing the completion code in urbp->status instead.
As a result, you can get this sequence of events if the timing is right:
URB completes. Driver stores result in urbp->status; see
uhci_transfer_result(). Urbp goes on the complete_list.
URB is unlinked. The core sees urb->status == -EINPROGRESS
still, so it goes ahead and calls uhci_urb_dequeue(). This
puts urbp on the remove_list.
And there you are. That must have been what happened to you.
The patch below should fix this problem. It gets rid of urbp->status. It
also takes care of a number of small things that have been bugging me for
ages:
Close a small loophole by allowing an URB to be unlinked even
before the uhci_urb_enqueue() procedure has started.
Remove some fossil code from back when interrupt URBs were
automagically resubmitted.
Giveback unlinked URBs in the order they were unlinked.
Don't set urb->status for dequeued URBs; the core has already
set it for us.
Rename uhci_remove_pending_qhs to uhci_remove_pending_urbps.
(That has _really_ gnawed at me!)
Try out this patch, which applies to a virgin driver (without your 10
patches) and see if it doesn't cure the addressing violation you were
getting.
BTW, your patches 2 - 10 all looked good, although I'm afraid this will
collide with some of them. Once we get this thing straightened out,
I'll update them and submit them to Greg KH.
Alan Stern
===== uhci-debug.c 1.11 vs edited =====
--- 1.11/drivers/usb/host/uhci-debug.c Mon Oct 6 13:46:12 2003
+++ edited/drivers/usb/host/uhci-debug.c Tue Feb 10 15:48:02 2004
@@ -321,8 +321,8 @@
out += sprintf(out, "%s", (urbp->fsbr ? "FSBR " : ""));
out += sprintf(out, "%s", (urbp->fsbr_timeout ? "FSBR_TO " : ""));
- if (urbp->status != -EINPROGRESS)
- out += sprintf(out, "Status=%d ", urbp->status);
+ if (urbp->urb->status != -EINPROGRESS)
+ out += sprintf(out, "Status=%d ", urbp->urb->status);
//out += sprintf(out, "Inserttime=%lx ",urbp->inserttime);
//out += sprintf(out, "FSBRtime=%lx ",urbp->fsbrtime);
===== uhci-hcd.c 1.80 vs edited =====
--- 1.80/drivers/usb/host/uhci-hcd.c Tue Feb 10 00:33:10 2004
+++ edited/drivers/usb/host/uhci-hcd.c Tue Feb 10 16:29:52 2004
@@ -1462,11 +1462,14 @@
spin_lock_irqsave(&uhci->urb_list_lock, flags);
+ if (urb->status != -EINPROGRESS) /* URB already unlinked! */
+ goto out;
+
eurb = uhci_find_urb_ep(uhci, urb);
if (!uhci_alloc_urb_priv(uhci, urb)) {
- spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
switch (usb_pipetype(urb->pipe)) {
@@ -1514,10 +1517,11 @@
return ret;
}
+ ret = 0;
+out:
spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
-
- return 0;
+ return ret;
}
/*
@@ -1527,7 +1531,7 @@
*/
static void uhci_transfer_result(struct uhci_hcd *uhci, struct urb *urb)
{
- int ret = -EINVAL;
+ int ret = -EINPROGRESS;
unsigned long flags;
struct urb_priv *urbp;
@@ -1535,10 +1539,8 @@
urbp = (struct urb_priv *)urb->hcpriv;
- if (urb->status != -EINPROGRESS) {
- info("uhci_transfer_result: called for URB %p not in flight?", urb);
+ if (urb->status != -EINPROGRESS) /* URB already dequeued */
goto out;
- }
switch (usb_pipetype(urb->pipe)) {
case PIPE_CONTROL:
@@ -1555,10 +1557,9 @@
break;
}
- urbp->status = ret;
-
if (ret == -EINPROGRESS)
goto out;
+ urb->status = ret;
switch (usb_pipetype(urb->pipe)) {
case PIPE_CONTROL:
@@ -1607,10 +1608,6 @@
struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
int prevactive = 1;
- /* We can get called when urbp allocation fails, so check */
- if (!urbp)
- return;
-
uhci_dec_fsbr(uhci, urb); /* Safe since it checks */
/*
@@ -1660,13 +1657,6 @@
unsigned long flags;
struct urb_priv *urbp = urb->hcpriv;
- /* If this is an interrupt URB that is being killed in urb->complete, */
- /* then just set its status and return */
- if (!urbp) {
- urb->status = -ECONNRESET;
- return 0;
- }
-
spin_lock_irqsave(&uhci->urb_list_lock, flags);
list_del_init(&urbp->urb_list);
@@ -1678,7 +1668,7 @@
/* If we're the first, set the next interrupt bit */
if (list_empty(&uhci->urb_remove_list))
uhci_set_next_interrupt(uhci);
- list_add(&urbp->urb_list, &uhci->urb_remove_list);
+ list_add_tail(&urbp->urb_list, &uhci->urb_remove_list);
spin_unlock(&uhci->urb_remove_list_lock);
spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
@@ -1844,17 +1834,11 @@
static void uhci_finish_urb(struct usb_hcd *hcd, struct urb *urb, struct pt_regs
*regs)
{
- struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
struct uhci_hcd *uhci = hcd_to_uhci(hcd);
- int status;
unsigned long flags;
spin_lock_irqsave(&urb->lock, flags);
- status = urbp->status;
uhci_destroy_urb_priv(uhci, urb);
-
- if (urb->status != -ENOENT && urb->status != -ECONNRESET)
- urb->status = status;
spin_unlock_irqrestore(&urb->lock, flags);
usb_hcd_giveback_urb(hcd, urb, regs);
@@ -1885,7 +1869,7 @@
spin_unlock_irqrestore(&uhci->complete_list_lock, flags);
}
-static void uhci_remove_pending_qhs(struct uhci_hcd *uhci)
+static void uhci_remove_pending_urbps(struct uhci_hcd *uhci)
{
struct list_head *tmp, *head;
unsigned long flags;
@@ -1898,11 +1882,7 @@
struct urb *urb = urbp->urb;
tmp = tmp->next;
-
list_del_init(&urbp->urb_list);
-
- urbp->status = urb->status = -ECONNRESET;
-
uhci_add_complete(uhci, urb);
}
spin_unlock_irqrestore(&uhci->urb_remove_list_lock, flags);
@@ -1942,7 +1922,7 @@
uhci_free_pending_tds(uhci);
- uhci_remove_pending_qhs(uhci);
+ uhci_remove_pending_urbps(uhci);
uhci_clear_next_interrupt(uhci);
@@ -2467,7 +2447,7 @@
*/
uhci_free_pending_qhs(uhci);
uhci_free_pending_tds(uhci);
- uhci_remove_pending_qhs(uhci);
+ uhci_remove_pending_urbps(uhci);
reset_hc(uhci);
===== uhci-hcd.h 1.17 vs edited =====
--- 1.17/drivers/usb/host/uhci-hcd.h Tue Feb 10 00:33:10 2004
+++ edited/drivers/usb/host/uhci-hcd.h Tue Feb 10 15:48:36 2004
@@ -383,8 +383,6 @@
/* a control transfer, retrigger */
/* the status phase */
- int status; /* Final status */
-
unsigned long inserttime; /* In jiffies */
unsigned long fsbrtime; /* In jiffies */
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
