[RFC] [PATCH] usbatm.[ch]: multiple changes

[Roman Kagan]

On Wed, Mar 30, 2005 at 07:26:37PM +0200, matthieu castet wrote:
> Roman Kagan wrote:
> >On Tue, Mar 29, 2005 at 04:04:45PM +0400, Roman Kagan wrote:
> >for (i = 0; i < num_rcv_urbs + num_snd_urbs; i++) {
> > 		struct usbatm_transceiver *trx = instance->transceivers + i;
> > 		u8 *buffer;
> >+		unsigned int iso_packets = 0, iso_size = 0;
> > 		trx->channel = i < num_rcv_urbs ? &instance->rx_channel : 
> > 		&instance->tx_channel;
> > 
> > 		buffer = kmalloc(trx->channel->buf_size, GFP_KERNEL);
> >@@ -1112,7 +1124,15 @@
> > 		}
> > 		memset(buffer, 0, trx->channel->buf_size);
> > 
> >-		trx->urb = usb_alloc_urb(0, GFP_KERNEL);
> >+		if (usb_pipeisoc(trx->urb->pipe)) {
>                                   ******
> using trx->urb before it is allocated won't work...

Damn, I copied it verbatim from usbatm_rx_process()...  Sorry, it should
have been trx->channel->endpoint.  Thanks for spotting this!

> >+			/* don't expect iso out endpoints */
> But you could add interrupt out endpoint ;)

Interrupt transfers seem to be even less adequate for ATM :)...  The
comment actually refers to the next line

> >+			iso_size = usb_maxpacket(instance->usb_dev, 
> >trx->channel->endpoint, 0);

where usb_maxpacket() expects the boolean indicating the pipe direction
as its third argument, and I always give it 0 (== in) for simplicity.
It'll issue a warning if the actual pipe direction is the opposite.

> >+			iso_size -= iso_size % trx->channel->stride;	/* 
> >alignment */
> >+			BUG_ON(!iso_size);
> >+			iso_packets = (trx->channel->buf_size - 1) / 
> >iso_size + 1;
> using default buf_size is quite low : for iso_size 1000, we have only 3 
> or 4 iso_packets per urb instead of 16.

Is 16 the recommended number?  Anyway, feel free to suggest better
rcv_buf_size default, and for the time being you can set it to a bigger
value using the module parameter.  Also you can cheat and substitute
different ->rx_channel.buf_size in your ->bind(), but don't forget to make sure it's a
multiple of rx_channel.stride.

> >+		}
> >+
> >+		trx->urb = usb_alloc_urb(iso_packets, GFP_KERNEL);
>                  **********************

Sorry, what is wrong here?

> > 		if (!trx->urb) {
> > 			dev_dbg(&intf->dev, "no memory for urb %d!\n", i);
> > 			goto fail_unbind;
> >@@ -1120,6 +1140,17 @@
> > 
> > 		usb_fill_bulk_urb(trx->urb, instance->usb_dev, 
> > 		trx->channel->endpoint,
> > 				  buffer, trx->channel->buf_size, 
> > 				  usbatm_complete, trx);
> does it work for iso packets ?

usb_fill_bulk_urb() initializes the stuff common for all four urb types
(I'm curious why usb_fill_{control,int}_urb() in include/linux/usb.h
aren't implemented like that, i.e. by calling usb_fill_bulk_urb() first
and then initializing the rest few fields).

> I am not sure of that but in lot's of driver, the status and 
> actual_length are cleared for each paquets before resubmitting.

->status and ->actual_length are return-only, and are not required to be
initialized by the caller.  In fact, usb_submit_urb() initializes them
anyway (both for urb and for each iso_frame_desc if applicable).

Cheers,
  Roman.