Sorry, that patch had a mistake. It didn't take into account the possibility that the first URB might complete before the second could be submitted, causing io->count to reach 0 prematurely.
Yeah, such "fault can happen at any time" logic is easy to get wrong. And that particular logic should probably only drop the spinlock in the switch() branches after other cleanup/setup.
This patch (as316b) should do a better job. It retains the strategy of adjusting io->count when something strange happens -- but now it does so whenever _anything_ strange happens, rather than only when a submission fails.
Erm, it should adjust that when there's an uncorrectible error. You left the code that retries on -EAGAIN, -ENOMEM, -ENXIO ...
Alan Stern
P.S.: Is there some reason for that strange assignment to io->count inside the "for" statement? I didn't see any, so I moved the assignment earlier.
See below ... it affects cleanup.
Also that test for "if (io->status == -EINPROGRESS)" in usb_sg_wait() doesn't look like it could ever succeed, so I removed it.
Right ... initialized to zero, it should check for that value. The idea is to make the status of the sg_wait() call be that fault code, rather than the unlink/cleanup fault code.
Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
===== drivers/usb/core/message.c 1.92 vs edited ===== --- 1.92/drivers/usb/core/message.c Fri Jun 11 07:49:33 2004 +++ edited/drivers/usb/core/message.c Wed Jun 16 09:56:54 2004 @@ -248,7 +248,7 @@ * unlink pending urbs so they won't rx/tx bad data. */ for (i = 0, found = 0; i < io->entries; i++) { - if (!io->urbs [i]) + if (!io->urbs [i] || !io->urbs [i]->dev) continue; if (found) { status = usb_unlink_urb (io->urbs [i]);
Clearly right ...
@@ -337,7 +337,7 @@
if (io->entries <= 0)
return io->entries;
- io->count = 0;
+ io->count = io->entries;
io->urbs = kmalloc (io->entries * sizeof *io->urbs, mem_flags);
if (!io->urbs)
goto nomem;
This can make the cleanup code see garbage pointers in io->urbs[] in cases when some later setup fails, since it's never zeroed. The previous version had an invariant that io->count listed the number of URB pointers that were actually valid.
@@ -347,7 +347,7 @@
if (usb_pipein (pipe))
urb_flags |= URB_SHORT_NOT_OK;
- for (i = 0; i < io->entries; i++, io->count = i) {
+ for (i = 0; i < io->entries; i++) {
unsigned len;
io->urbs [i] = usb_alloc_urb (0, mem_flags);
And next the sg_wait() code, in the submit/wait loop then the wait-for-completion code:
@@ -477,14 +477,6 @@
/* fail any uncompleted urbs */
default:
- spin_lock_irq (&io->lock);
- io->count -= entries - i;
- if (io->status == -EINPROGRESS)
- io->status = retval;
- if (io->count == 0)
- complete (&io->complete);
- spin_unlock_irq (&io->lock);
-
io->urbs [i]->dev = 0;
io->urbs [i]->status = retval;
dev_dbg (&io->dev->dev, "%s, submit --> %d\n",
I'm not sure I see how that'd work. Since io->status isn't changing, the submit loop won't stop unless some URB manages to complete (with nonzero status) early ...
@@ -495,6 +487,9 @@
if (retval && io->status == -ECONNRESET)
io->status = retval;
}
+ io->count -= entries - i;
+ if (io->count == 0)
+ complete (&io->complete);
spin_unlock_irq (&io->lock);
/* OK, yes, this could be packaged as non-blocking.
-------------------------------------------------------
This SF.Net email sponsored by Black Hat Briefings & Training.
Attend Black Hat Briefings & Training, Las Vegas July 24-29 - digital self defense, top technical experts, no vendor pitches, unmatched networking opportunities. Visit www.blackhat.com
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
