On Sat, Jul 20, 2013 at 11:57 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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.
OK, if you are sure just snd-usb-audio and very less drivers need the change,
using the API only is correct way.
>
> 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.
Yes.
>
>> > 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?
So do you only want to set streaming on and not streaming off?
If so, that looks a bit weird, could you describe its usage?
>
>> 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
IMO, it is very simple, and only one flag inside urb is required for meeting
the demand.
For the non-robust part(new drivers don't resubmit URBs in complete()), we
can let the new introduced API cover it.
Looks it should cover all old drivers automatically.
> (with extra overhead for every URB!) to fix a problem that ought to be
In a normal system, the URB count won't be very much(<< 1000), and if
you mean the overhead is from memory, I guess it won't cause real memory
increase per URB for slab's sake. If you mean performance loss with
set/get the single lockless/common variable, maybe we should focus on
the big HCD lock, which might be the biggest bottle of USB protocol, :-)
> fixed by changing drivers.
If only very less drivers need the change, I agree with you.
Thanks,
--
Ming Lei
--
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
