On Sat, 20 Jul 2013, Ming Lei wrote:
> > Trying to check the context of an URB submission strikes me as not
> > robust at all. Who knows what tricks drivers will get up to in the
> > future?
>
> Firstly, it shouldn't be fragile as you image, with the below simple script, we
> can find almost all isoc completion handler(276, but most of them are
> false positive).
>
> #/bin/sh
> USB="drivers/usb"
> dirs=`ls -d drivers/* | grep -v $USB`
> dirs="$dirs $USB/storage $USB/serial $USB/image $USB/class $USB/atm
> $USB/misc sound/usb"
>
> export dir
> for dir in ${dirs}; do
> ss=$(git grep -l -w -E "iso_frame_desc" $dir)
> if [ -n "$ss" ]; then
> git grep -n "struct\surb" $dir | grep -E "\(|\)" | grep -v -E ";|,"
> fi
> done
>
> Secondly, from the above script, only very few drivers resubmits isoc URB
> in tasklet or other context, and most of them do it in complete()
> handler directly.
This is irrelevant. We have to handle drivers that haven't been
written yet, as well as drivers that exist now.
> Finally, suppose drivers do some trick to resubmit URBs in tasklet, workqueue
> or other context, we even can ignore them since they worked well for long time
> in case of underrun with empty queue, right?
No. This is an example of trying to be too clever. Keep it simple.
> > Adding a new API seems like the cleanest solution. (Although I still
> > think that usb_reset_endpoint() would be appropriate.)
>
> After adding a new API, we still need to change every ISOC driver, and it
> won't be simpler than only checking the completion handler, will it?
No, we don't have to change every driver using isochronous URBs. Many
of them set URB_ISO_ASAP on every URB -- in fact, AFAIK snd-usb-audio
is the only one that has been fixed not to do this.
We also don't have to change the isochronous code in every HCD. Many
of them treat every URB as though URB_ISO_ASAP was set. Only a few
make an attempt to implement proper handling of isochronous underruns
-- and those have to be changed anyway.
> > But never mind. For a new API, how about
> >
> > void usb_new_iso_stream(struct usb_device *, struct usb_host_endpoint *);
> >
> > Does that look okay?
>
> Looks you missed one parameter of 'int on'.
I don't understand. Why is another parameter needed?
> But we need to change every ISOC driver to use the API, that is why I
> suggest to add urb->completing to minimize changes on drivers.
>
> So how about only using the API on drivers which don't resubmit isoc URBs
> in its completion handler()? And I think we can document this usage.
I do not like the idea of adding a complex and non-robust mechanism
(with extra overhead for every URB!) to fix a problem that ought to be
fixed by changing drivers.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
