On Sun, 10 Jul 2005, Pete Zaitcev wrote:
> Hi, All:
>
> I've refactored the friends of usb_sg_request to permit it being used
> by ub. The result is attached FYI. It's probably buggy. I cannot convince
> myself that all this is correct.
>
> In case anyone is curious, fields in usb_sg_request were renamed because
> their functions morphed in a way which made some of uses incorrect for
> each of them. The renaming ensures that programmer examines every use.
In general this looks pretty good. I didn't read through your changes to
ub, just to the usb_sg library. Here's what I noticed:
In sg_setup the fourth argument is always io->nmaps. So you don't need to
pass it as a separate argument.
Without checking to see if this is really feasible, it certainly seems
like the two places where you do the dma and usb_buffer_sg_map stuff can
be combined into one, in sg_setup. Of course then you would have to pass
nents as the fourth argument, negating my previous point.
In sg_setup, the "if (length)" test isn't needed; the code it protects
can execute unconditionally. Also, the "io->urbs [--i]" near the end
looks strange. I would prefer to see "[i-1]", since the value of i isn't
needed anywhere else. Finally, although this wasn't present in the
original code, it might be a good idea to check that length == 0 at the
end of the loop. Otherwise the scatter-gather list is too short.
In usb_sg_setup: Rather than return -EFAULT, why not allow 0-length
transfers?
In usb_sg_submit: The logic at the end is wrong. This is very difficult
to get right. There are two main issues to worry about:
If any URBs were submitted successfully then you _have_ to
make a callback. Otherwise the user won't know when the
memory buffers are available again.
At the end of usb_sg_submit, a callback may or may not already
have occurred. If the
io->count -= start_count - i;
line changes io->count from non-zero to 0, then usb_sg_submit
must be responsible for making the callback.
At the end of the loop, i should be the number of successfully submitted
URBs. Its value will be wrong unless you add
if (retval)
break;
to the bottom part of the loop. Otherwise the routine would think that
the last, failed submission is outstanding. The code following the loop
should look something like this:
io->count -= start_count - i;
retval = 0;
if (i == 0)
retval = io->status;
else if (i == start_count || io->count > 0)
i = 0;
spin_unlock_irqrestore(&io->lock, flags);
if (i)
(*io->complete) (io->context);
return retval;
HTH,
Alan Stern
-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening
July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual
core and dual graphics technology at this free one hour event hosted by HP,
AMD, and NVIDIA. To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
