[PATCH] iso pipe support for usbatm

[Roman Kagan]

On Sat, Mar 19, 2005 at 03:37:40PM +0100, matthieu castet wrote:
> Roman Kagan wrote:
> >On Sat, Mar 19, 2005 at 12:38:19AM +0100, matthieu castet wrote:
> >
> >>@@ -472,17 +510,44 @@
> >>	instance = rcv->instance;
> >>	buf = rcv->buffer;
> >>
> >>-	buf->filled_cells = urb->actual_length / (ATM_CELL_SIZE + 
> >>instance->rx_padding);
> >>+	if ( urb->actual_length <= 0 && !urb->status) {
> >>+		int err;
> >>+		if ((err = usb_submit_urb(rcv->urb, GFP_ATOMIC)) < 0) {
> >
> >
> >Why do you resubmit the failed urb here?  It used to go in the normal
> >codepath and get resubmitted in receive_tasklet.
> >
> the buffer is empty and the urb didn't failed, so we could reuse the 
> same urb with the same buffer : it avoids to go through receive_tasklet 
> and locking for releasing/taking buffer.
> This is needed for performance issue with iso pipe because the urb are 
> returned every 1ms (or something like that), even if there is nothing 
> received in order to keep the allocated bandwidth.

So for bulk this should never happen?

Anyway it may make more sense to use more urbs in this case, so that
some urbs are always submitted.  It's for that reason that I've
increased the default number of urbs in my patch.

> >>-	if (likely(!urb->status))
> >>+	if (likely(!urb->status) ||
> >>+			/* via chipset produce lot's of crc error...*/
> >>+			(usb_pipeisoc(instance->rx_endpoint) && urb->status 
> >>== -EILSEQ))
> >>		tasklet_schedule(&instance->receive_tasklet);
> >
> >
> >Sounds like iso occasionally misses data too?  BTW the condition you add
> >here is ignored because it's masked by the other one.
> I don't think so : if urb->status == -EILSEQ and iso pipe, we have "if 
> (0 || 1 && 1)" which is true

Ups, you're certainly right.

BTW do you get -EILSEQ often?  Documentation/usb/error-codes.txt reads:

(*) Error codes like -EPROTO, -EILSEQ and -EOVERFLOW normally indicate
hardware problems such as bad devices (including firmware) or cables.

> >What is the typical ->iso_frame_size you're using? 
> This is given by the modem, I use the maximum : 1007, but others are 
> also available :

So this means the buffer size per urb is 1007 * 16 / 53 = 304 ATM cells,
which is almost 5 times the default 64 for bulk.  Maybe simply
increasing the buffer size in bulk mode will solve the problem of
dropped cells?

Same for the number of urbs: the original default 2 is probably way too
few.  drivers/usb/core/urb.c explicitly suggests submitting urbs "before
previous ones complete, to minimize the impact of interrupt latencies
and system overhead on data throughput.  With that queuing policy, an
endpoint's queue would never be empty. [...] Such queuing also maximizes
bandwidth utilization by letting USB controllers start work on later
requests before driver software has finished the completion processing
for earlier (successful) requests."

Roman.