On Fri, Jul 19, 2013 at 10:53 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 19 Jul 2013, Ming Lei wrote:
>
>> Even we can introduce one flag of 'completing' in 'struct urb' to check if
>> the USB's submit is called inside its own complete(), by which we can
>> check if resubmission is in underrun easily.
>>
>> Consider that isoc URB's resubmission in completion handler should
>> cover 99% cases, I think this approach is doable.
>>
>> For other resubmission from tasklet or workque cases, either let them alone
>> or move the resubmission inside completion() handler, or introduce simple
>> helper to mark start of frame only for them. Anyway, there are very few isoc
>> resubmissions from non-completion handler (only two drivers found in my
>> urb complet() cleanup work), so it shouldn't be a big deal.
>>
>> What do you think about this approach?
>
> Why go to all this trouble? We already have the URB_ISO_ASAP flag.
>
> 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.
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?
>
> 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?
>
>> Actually 'start of stream' should be done inside usbcore because it
>> is HCD-independent function, so it is better to provide the information
>> from usbcore and let HCD use it if HCD needs the flag.
>
> I'm not quite sure what you mean. Note that usb_reset_endpoint() _is_
> a function in usbcore.
>
> 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'.
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.
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
