diff options
author | David Brownell <david-b@pacbell.net> | 2008-04-12 11:32:05 -0400 |
---|---|---|
committer | Greg Kroah-Hartman <gregkh@suse.de> | 2008-04-25 00:16:53 -0400 |
commit | 4f6676274fb6303a8e8100d086ea8c2c00c0d8e3 (patch) | |
tree | c59746300f9a343e13bf102c063c372e34f809d4 /drivers/usb/host | |
parent | c6dbf554bc8a79c9caab3dbf891a33c19068f646 (diff) |
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 <dbrownell@users.sourceforge.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
Diffstat (limited to 'drivers/usb/host')
-rw-r--r-- | drivers/usb/host/ehci-q.c | 34 |
1 files 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) | |||
242 | if (unlikely(urb->unlinked)) { | 242 | if (unlikely(urb->unlinked)) { |
243 | COUNT(ehci->stats.unlink); | 243 | COUNT(ehci->stats.unlink); |
244 | } else { | 244 | } else { |
245 | if (likely(status == -EINPROGRESS)) | 245 | /* report non-error and short read status as zero */ |
246 | if (status == -EINPROGRESS || status == -EREMOTEIO) | ||
246 | status = 0; | 247 | status = 0; |
247 | COUNT(ehci->stats.complete); | 248 | COUNT(ehci->stats.complete); |
248 | } | 249 | } |
@@ -283,7 +284,6 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh) | |||
283 | int last_status = -EINPROGRESS; | 284 | int last_status = -EINPROGRESS; |
284 | int stopped; | 285 | int stopped; |
285 | unsigned count = 0; | 286 | unsigned count = 0; |
286 | int do_status = 0; | ||
287 | u8 state; | 287 | u8 state; |
288 | u32 halt = HALT_BIT(ehci); | 288 | u32 halt = HALT_BIT(ehci); |
289 | 289 | ||
@@ -309,7 +309,6 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh) | |||
309 | struct ehci_qtd *qtd; | 309 | struct ehci_qtd *qtd; |
310 | struct urb *urb; | 310 | struct urb *urb; |
311 | u32 token = 0; | 311 | u32 token = 0; |
312 | int qtd_status; | ||
313 | 312 | ||
314 | qtd = list_entry (entry, struct ehci_qtd, qtd_list); | 313 | qtd = list_entry (entry, struct ehci_qtd, qtd_list); |
315 | urb = qtd->urb; | 314 | urb = qtd->urb; |
@@ -377,13 +376,6 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh) | |||
377 | else if (last_status == -EINPROGRESS && !urb->unlinked) | 376 | else if (last_status == -EINPROGRESS && !urb->unlinked) |
378 | continue; | 377 | continue; |
379 | 378 | ||
380 | /* issue status after short control reads */ | ||
381 | if (unlikely (do_status != 0) | ||
382 | && QTD_PID (token) == 0 /* OUT */) { | ||
383 | do_status = 0; | ||
384 | continue; | ||
385 | } | ||
386 | |||
387 | /* qh unlinked; token in overlay may be most current */ | 379 | /* qh unlinked; token in overlay may be most current */ |
388 | if (state == QH_STATE_IDLE | 380 | if (state == QH_STATE_IDLE |
389 | && cpu_to_hc32(ehci, qtd->qtd_dma) | 381 | && cpu_to_hc32(ehci, qtd->qtd_dma) |
@@ -401,15 +393,21 @@ halt: | |||
401 | } | 393 | } |
402 | } | 394 | } |
403 | 395 | ||
404 | /* remove it from the queue */ | 396 | /* unless we already know the urb's status, collect qtd status |
405 | qtd_status = qtd_copy_status(ehci, urb, qtd->length, token); | 397 | * and update count of bytes transferred. in common short read |
406 | if (unlikely(qtd_status == -EREMOTEIO)) { | 398 | * cases with only one data qtd (including control transfers), |
407 | do_status = (!urb->unlinked && | 399 | * queue processing won't halt. but with two or more qtds (for |
408 | usb_pipecontrol(urb->pipe)); | 400 | * example, with a 32 KB transfer), when the first qtd gets a |
409 | qtd_status = 0; | 401 | * short read the second must be removed by hand. |
402 | */ | ||
403 | if (last_status == -EINPROGRESS) { | ||
404 | last_status = qtd_copy_status(ehci, urb, | ||
405 | qtd->length, token); | ||
406 | if (last_status == -EREMOTEIO | ||
407 | && (qtd->hw_alt_next | ||
408 | & EHCI_LIST_END(ehci))) | ||
409 | last_status = -EINPROGRESS; | ||
410 | } | 410 | } |
411 | if (likely(last_status == -EINPROGRESS)) | ||
412 | last_status = qtd_status; | ||
413 | 411 | ||
414 | /* if we're removing something not at the queue head, | 412 | /* if we're removing something not at the queue head, |
415 | * patch the hardware queue pointer. | 413 | * patch the hardware queue pointer. |