On Fri, 25 Jun 2004, David Brownell wrote:
> > 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 ...
Well yes... Since they get corrected, they don't really count as errors.
Or to put it another way, the original code handled uncorrectible errors
during submission, whereas the patched code handles all uncorrectible
errors.
> > 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.
Where does it affect cleanup? I looked for some such possibility and
didn't see any.
> > @@ -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.
I don't see how. The cleanup code ignores io->count. In fact, io->count
isn't used again until after the submissions start.
> The previous version had an invariant that io->count listed the
> number of URB pointers that were actually valid.
The new invariant is that io->count is the number of URBs still expected
to complete, even if they haven't been submitted yet.
> 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 ...
Of course it won't work. The part below needs to changed as follows:
> > @@ -495,6 +487,9 @@
> > if (retval && io->status == -ECONNRESET)
Change that to:
if (retval && (io->status == 0 || io->status == -ECONNRESET))
> > io->status = retval;
> > }
> > + io->count -= entries - i;
> > + if (io->count == 0)
> > + complete (&io->complete);
> > spin_unlock_irq (&io->lock);
> >
This code has the potential for calling complete() twice if all the URBs
complete before the loop ends, but that won't hurt anything.
Revised patch as316c below.
Alan Stern
Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
===== drivers/usb/core/message.c 1.95 vs edited =====
--- 1.95/drivers/usb/core/message.c Thu Jun 24 16:43:27 2004
+++ edited/drivers/usb/core/message.c Fri Jun 25 14:30:15 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]);
@@ -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;
@@ -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);
@@ -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",
@@ -492,9 +484,12 @@
usb_sg_cancel (io);
}
spin_lock_irq (&io->lock);
- if (retval && io->status == -ECONNRESET)
+ if (retval && (io->status == 0 || 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
