ChangeSet 1.1557.49.5, 2004/02/17 16:20:54-08:00, stern@rowland.harvard.edu [PATCH] USB: Remove unneeded and error-provoking variable in UHCI This patch removes an unneeded "status" field from the UHCI driver's URB-private data structure. The driver had been storing the status of completed URBs there rather than in the URBs themselves. This not only is wasteful of space but is also a cause of bugs, since the USB core relies on urb->status for proper synchronization with the driver. The patch 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_ bothered me!) drivers/usb/host/uhci-debug.c | 4 +-- drivers/usb/host/uhci-hcd.c | 50 ++++++++++++------------------------------ drivers/usb/host/uhci-hcd.h | 2 - 3 files changed, 17 insertions(+), 39 deletions(-) diff -Nru a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c --- a/drivers/usb/host/uhci-debug.c Thu Feb 19 17:22:42 2004 +++ b/drivers/usb/host/uhci-debug.c Thu Feb 19 17:22:42 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); diff -Nru a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c --- a/drivers/usb/host/uhci-hcd.c Thu Feb 19 17:22:42 2004 +++ b/drivers/usb/host/uhci-hcd.c Thu Feb 19 17:22:42 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); diff -Nru a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h --- a/drivers/usb/host/uhci-hcd.h Thu Feb 19 17:22:42 2004 +++ b/drivers/usb/host/uhci-hcd.h Thu Feb 19 17:22:42 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 */