From 4f6676274fb6303a8e8100d086ea8c2c00c0d8e3 Mon Sep 17 00:00:00 2001 From: David Brownell Date: Sat, 12 Apr 2008 08:32:05 -0700 Subject: USB: ehci: qh_completions cleanup and bugfix Simplify processing of completed qtds, and correct handling of short reads, by removing two state variables: - "qtd_status" wasn't needed. The current URB's status is either OK (-EINPROGRESS) or some fault status. Once a fault appears, the queue halts and any later QTDs are immediately removed, so no temporary status is needed. (Or for typical short reads, it's not treated as a fault, so no queue halt is needed.) - "do_status" was erroneous. Because of how the queue is set up, short control reads can (and should!) be treated like full size reads, and cleaned up the usual way. The status stage will be executed transparently, and usbcore handles the choice of whether to report this status as unexected. The "do_status" problem caused a rather perplexing timing-dependent problem with usbtest case 10. Sometimes it would make the controller skip a dozen transactions while (wrongly) trying to clean up after a short transfer. Fortunately, removing a dcache contention issue made this become trivial to reproduce (on one test rig), so enough clues finally presented themselves ... I think this has been around for a very long time, but was worsened by recent urb->status changes. Signed-off-by: David Brownell Signed-off-by: Greg Kroah-Hartman --- drivers/usb/host/ehci-q.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c index 64942ec584c2..315c7c14aaa8 100644 --- a/drivers/usb/host/ehci-q.c +++ b/drivers/usb/host/ehci-q.c @@ -242,7 +242,8 @@ __acquires(ehci->lock) if (unlikely(urb->unlinked)) { COUNT(ehci->stats.unlink); } else { - if (likely(status == -EINPROGRESS)) + /* report non-error and short read status as zero */ + if (status == -EINPROGRESS || status == -EREMOTEIO) status = 0; COUNT(ehci->stats.complete); } @@ -283,7 +284,6 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh) int last_status = -EINPROGRESS; int stopped; unsigned count = 0; - int do_status = 0; u8 state; u32 halt = HALT_BIT(ehci); @@ -309,7 +309,6 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh) struct ehci_qtd *qtd; struct urb *urb; u32 token = 0; - int qtd_status; qtd = list_entry (entry, struct ehci_qtd, qtd_list); urb = qtd->urb; @@ -377,13 +376,6 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh) else if (last_status == -EINPROGRESS && !urb->unlinked) continue; - /* issue status after short control reads */ - if (unlikely (do_status != 0) - && QTD_PID (token) == 0 /* OUT */) { - do_status = 0; - continue; - } - /* qh unlinked; token in overlay may be most current */ if (state == QH_STATE_IDLE && cpu_to_hc32(ehci, qtd->qtd_dma) @@ -401,15 +393,21 @@ halt: } } - /* remove it from the queue */ - qtd_status = qtd_copy_status(ehci, urb, qtd->length, token); - if (unlikely(qtd_status == -EREMOTEIO)) { - do_status = (!urb->unlinked && - usb_pipecontrol(urb->pipe)); - qtd_status = 0; + /* unless we already know the urb's status, collect qtd status + * and update count of bytes transferred. in common short read + * cases with only one data qtd (including control transfers), + * queue processing won't halt. but with two or more qtds (for + * example, with a 32 KB transfer), when the first qtd gets a + * short read the second must be removed by hand. + */ + if (last_status == -EINPROGRESS) { + last_status = qtd_copy_status(ehci, urb, + qtd->length, token); + if (last_status == -EREMOTEIO + && (qtd->hw_alt_next + & EHCI_LIST_END(ehci))) + last_status = -EINPROGRESS; } - if (likely(last_status == -EINPROGRESS)) - last_status = qtd_status; /* if we're removing something not at the queue head, * patch the hardware queue pointer. -- cgit v1.2.1