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

Roman Kagan rkagan at mail.ru
Tue May 3 13:45:03 EDT 2005


On Tue, May 03, 2005 at 09:48:06AM +0200, Duncan Sands wrote:
> > > > No, I thought about using static endpoint number from usbatm_driver.in
> > > > or .out and poking at the endpoint descriptor after .bind() (that can't
> > > > be done before .bind() because it may need to choose the proper
> > > > altsetting).
> > > You can do it in usb_probe so before the .bind(). That how it was done in my iso
> > > pipe patch.
> > 
> > Well, yes, but I thought that we considered driver's probe as workaround
> > for the usb core API which didn't allow for easy creation of extra
> > intermediate layers, which usbatm core was: if we could figure out the
> > subdriver in usbatm_usb_probe() without much hassle we wouldn't need
> > subdriver's usb_driver.probe() methods at all, only
> > usbatm_driver.bind().  So, generally speaking, the driver's probe should
> > only be a one-liner calling usbatm_usb_probe().
> 
> 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 :)

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?

Cheers,
  Roman.



More information about the Usbatm mailing list