[PATCH] iso pipe support for usbatm

[matthieu castet]

Hi Roman,

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.


>>-	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
> 
> Anyway I think this test should be removed here (for bulk too),
> otherwise how the buffers and urbs are going to be recycled?  The urb
> submission errors are problematic too (I've marked that in my patch),
> but urb return status should only be used to make sure the buffer passed
> on to the processing routine is empty.  For bulk relying on
> urb->actual_length is enough.
I agree that we need to manage the error differently in order to avoid 
deadlock.
> 
> 
>>@@ -1064,9 +1151,18 @@
>> 	instance->usb_dev = dev;
>> 	instance->usb_intf = intf;
>> 	instance->tx_endpoint = usb_sndbulkpipe(dev, driver->out);
>>-	instance->rx_endpoint = usb_rcvbulkpipe(dev, driver->in);
>>+	if (driver->iso_frame_size) {
>>+		nb_frames = rcv_buf_per_urb;
>>+		urb_size = driver->iso_frame_size * rcv_buf_per_urb;
> 
> 
> 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 :
E:  Ad=88(I) Atr=01(Isoc) MxPS= 159 Ivl=1ms
E:  Ad=88(I) Atr=01(Isoc) MxPS= 265 Ivl=1ms
E:  Ad=88(I) Atr=01(Isoc) MxPS= 424 Ivl=1ms
E:  Ad=88(I) Atr=01(Isoc) MxPS= 530 Ivl=1ms
E:  Ad=88(I) Atr=01(Isoc) MxPS= 636 Ivl=1ms
E:  Ad=88(I) Atr=01(Isoc) MxPS= 795 Ivl=1ms
E:  Ad=88(I) Atr=01(Isoc) MxPS= 901 Ivl=1ms
E:  Ad=88(I) Atr=01(Isoc) MxPS=1007 Ivl=1ms


Matthieu