speedtch usbatm.c,1.20,1.21 usbatm.h,1.13,1.14

[Roman Kagan]

On Tue, May 03, 2005 at 08:51:15PM +0200, Duncan Sands wrote:
> > > I have a general solution for this kind of problem: suppose a minidriver
> > > wants to modify in bind some of things it specified in its
> > > struct usbatm_driver.  Then it should dynamically allocate a
> > > struct usbatm_driver in its probe method (maybe copying into it a static
> > > struct usbatm_driver that it has prepared in advance), and pass that to
> > > usbatm_usb_probe.  In the bind call it modifies that struct via
> > > usbatm->driver, eg: changing the rx/tx endpoints.  It frees the struct
> > > in unbind.  This will work as long as usbatm reads the driver data after
> > > the call to bind, not before (except for the bind method of course).  The
> > > reason for allocating the struct rather than modifying the static struct
> > > is that that avoids problems if there are several devices for the same
> > > minidriver, all trying to modify the same bit of memory...
> > 
> > This seems a bit of an overdesign to me...  IMHO allowing the minidriver
> > to directly modify channel's pipe (as opposed to endpoint) is both
> > simpler to use and requires less code.  Violating encapsulation is not a
> > deadly sin, we aren't in C++ in the end :)
> 
> I don't see how it can be overdesign since I'm just pointing out that we
> can already accommodate this kind of thing and more by doing... nothing
> at all!  Since there are currently no drivers which need this, doing
> nothing seems like a good strategy to me.

But that won't work as soon as you want to use different transfer types:
you'll have to add code to detect which endpoint type it is, and set
channel.endpoint to the appropriate pipe (and handle wrong endpoint
number and type).

Actually, the do-nothing strategy was implemented in my original code:
it initialized the .endpoint in each channel to a bulk pipe on the
endpoint number from struct usbatm_driver, and assumed that the
minidriver can override it with a different pipe in its .bind(); still,
you could trust the minidriver to do it right, without error checking.
Thus leaving this thing as it was will make it work unmodified for bulk
transfers, and, in order to use isoc in transfer, the subdriver would
only need _one_ extra line of code in its .bind(),

usbatm->rx_channel.endpoint = usb_rcvisocpipe(usbatm->usb_dev, isoc_ep_num);

> > BTW speaking of using private fields: from include/linux/usb.h:
> > 
> > /**
> >  * struct urb - USB Request Block
> >  * @urb_list: For use by current owner of the URB.
> >  ...
> >  */
> > struct urb
> > {
> > 	/* private, usb core and host controller only fields in the urb */
> > 	...
> > 	struct list_head urb_list;	/* list pointer to all active urbs */
> > 	...
> > };
> > 
> > If the USB layer can guarantee that urb.urb_list isn't used by the USB
> > core in the completion routine (and it certainly isn't used after the
> > completion routine and until submission), we could use it and get rid of
> > struct usbatm_transceiver, whose only purpose is to wrap struct urb with
> > struct list_head.  Anybody knows if it can be done?
> 
> Maybe it can be done, but I don't like it - what if the USB core starts
> using it completion handlers from time to time?  It's not like anybody
> is going to tell us.

Well, we can humbly ask if urb.urb_list can be declared "public", and,
once it is, then the core developers will hopefully stick to not using
it outside the allowed scope.  Frankly, I'm not even sure that that
assumption is true in the current tree, but I see no problem asking.
Since multiple urbs queued on an endpoint is the recommended practice
now, I guess many other drivers would profit from the general-purpose
list head present in struct urb.

> Another possibility is to define a "fat urb type":
> a struct containing an urb followed by a list head.

But that's exactly what our struct usbatm_transceiver is.

Cheers,
  Roman.