USB driver conversion to use "struct device_driver"

Hi all,

The USB subsystem only binds drivers to USB "interfaces". A USB device may have many "interfaces", so a single device may have many drivers attached to it, handling different portions of it (think of a USB speaker, which has a audio driver for the audio stream, and a HID driver for the speaker buttons.) Because of this I had to create a "empty" device driver that I attach to the USB device structure. This ensures it shows up properly in the driverfs tree, and that no USB drivers try to bind to it.

Am Samstag, 10. August 2002 02:10 schrieb Greg KH:

Quoted 10 lines

Hi all,

The USB subsystem only binds drivers to USB "interfaces". A USB device may have many "interfaces", so a single device may have many drivers attached to it, handling different portions of it (think of a USB speaker, which has a audio driver for the audio stream, and a HID driver for the speaker buttons.) Because of this I had to create a "empty" device driver that I attach to the USB device structure. This ensures it shows up properly in the driverfs tree, and that no USB drivers try to bind to it.

Hi,

the probe/disconnect changes are an improvement. But what do we call a device? IMHO the device in terms of driverfs is the interface, thus the usb_device should be seen as a bus, which interfaces are attached to.

Also, the driverfs representation of the USB devices has changed, possibly for the worse. Just try the patch to see what I mean :)

+static int usb_device_bind (struct device *dev, struct device_driver *drv)

Why are devices children of the hub _interface_ now? Today they're children of the hub device. The hub spec requires only one interface for all hub devices, so it's safe the way it is today. With devices as children of hubs devices, things look more familiar ... hubs act like pci bridges (which they logically resemble).

Minor rough spot, please rename to usb_device_match() since it's the bus_type->match() callback. Might even be nice to split out some of the relevant code into "usb-device.c", mirroring what the "pci-device.c" code does but with bus-specific differences. There's too much random code in "usb.c" still for my taste! And all code at this level is usbcore-internal anyway, shouldn't be mixed with public driver APIs. :)

Quoted 10 lines

struct usb_driver {
	struct module *owner;
	const char *name;

include/linux/usb.h +6 -0

+
+	int	(*new_probe)	(struct usb_interface *intf);
+	int	(*init)		(struct usb_interface *intf);
+	void	(*new_disco)	(struct usb_interface *intf);
+
+	struct device_driver driver;

Quoted 2 lines

	void *(*probe)(
	    struct usb_device *dev,		/* the device */

I don't see why this isn't done as a transparent wrapper around the existing probe()/disconnect() routines. PCI did this without any such major changes to the driver API, and I thought being able to do that was a (good!) device model design goal.

If we want to change the API to refocus on interfaces, rather than device plus whichever kind of identifying number is used in the relevant context, I'd rather start by getting rid of the APIs that have been problematic (device + number). That'd be nice for configurations and endpoints too, but I think most of those calls involve interfaces. Those changes would be a lot less invasive so far as device drivers are concerned.

Also, the generic driver model calls are probe/disconnect, suspend/resume, and release. Today, USB has probe/disconnect but not the others. I'd expect to see suspend/resume in there before a new init callback ... but I see the init routine is unused, maybe that just snuck in by accident.

Quoted 1 line

Why are devices children of the hub _interface_ now?

Good point, I noticed that this had changed, but haven't fixed it, I'll make sure it goes back to the way it was before.

Quoted 10 lines

Quoted 10 lines

struct usb_driver {
	struct module *owner;
	const char *name;

include/linux/usb.h +6 -0

+
+	int	(*new_probe)	(struct usb_interface *intf);
+	int	(*init)		(struct usb_interface *intf);
+	void	(*new_disco)	(struct usb_interface *intf);
+
+	struct device_driver driver;

Quoted 7 lines

Quoted 2 lines

	void *(*probe)(
	    struct usb_device *dev,		/* the device */

I don't see why this isn't done as a transparent wrapper around the existing probe()/disconnect() routines. PCI did this without any such major changes to the driver API, and I thought being able to do that was a (good!) device model design goal.

I think making the probe() and disconnect() functions match the driver model makes more sense now. We have an easier time of fixing all of the USB drivers at once, compared to the PCI driver interface, which has _lots_ more drivers to convert.

Quoted 7 lines

If we want to change the API to refocus on interfaces, rather than device plus whichever kind of identifying number is used in the relevant context, I'd rather start by getting rid of the APIs that have been problematic (device + number). That'd be nice for configurations and endpoints too, but I think most of those calls involve interfaces. Those changes would be a lot less invasive so far as device drivers are concerned.

What APIs would those be?

Quoted 5 lines

Also, the generic driver model calls are probe/disconnect, suspend/resume, and release. Today, USB has probe/disconnect but not the others. I'd expect to see suspend/resume in there before a new init callback ... but I see the init routine is unused, maybe that just snuck in by accident.

Yes, the other callbacks will get added, and init() was added as part of the old original patch, I'll remove it.

thanks for the comments,

greg k-h

Quoted 4 lines

Quoted 1 line

Why are devices children of the hub _interface_ now?

Good point, I noticed that this had changed, but haven't fixed it, I'll make sure it goes back to the way it was before.

Thanks ... those paths were getting really atrocious!

The other thing I noticed about the tree structure was that the /drivers/bus/usb/devices directory has symlinks to both interfaces and devices. That's good for completeness, but the device links aren't very useful (and they really clutter up that directory, IMO, with two different kinds of object). Do they need to show up there?

We know "<interface>/.." always gives the USB device, so we don't really need such additional entries.

Even so. This particular change also happens to make the usb_device_id->driver_data useless ... if the driver API changes, I'd need to see the MODULE_DEVICE_TABLE entry, or at least its driver_data, get passed too. That's a key level of bus-specific glue that the driver model doesn't explicitly acknowledge (seemingly expects to be handled transparently by the bus glue).

I guess I shouldn't be too sensitive about incompatible changes to the USB driver API, but in this case it bothers me a bit even though I do like the idea of making drivers think "correctly" (interface drivers, not device drivers).

Oh, and "new_disco" ... I thought "Disco is Dead"? :)

Quoted 1 line

What APIs would those be?

From a quick scan of <linux/usb.h>, I see these calls ... all of which would be less error prone if they just passed the relevant descriptor. (Errors happen when folk assume both two kinds of integer ID are the same... and it's also just confusing.) The interface->dev backlink you added is useful, and help get rid of the need for many of these:

  usb_set_configuration(struct usb_device *dev, int);
  usb_set_interface(struct usb_device *dev, int, int);

  usb_find_interface_driver_for_ifnum(struct usb_device *dev, int);

  usb_bind_driver(struct usb_driver *,struct usb_device *, int);
	// usb_unbind_driver takes interface handle already!!

usb_ifnum_to_ifpos(struct usb_device *dev, int);

  usb_ifnum_to_if(struct usb_device *dev, int);
	// basically, abolish the need for these calls

  usb_epnum_to_ep_desc(struct usb_device *dev, int);

Plus I'm strongly tempted to group all the "dev + pipe" calls the same way ... better to just pass the endpoint descriptor than to encode descriptor values as bitfields and then later waste time (repeateadly) tearing apart those bitfields.

Related issue to the suspend/resume code ... I recently noticed (again) that the hub code was disconnecting top-down rather than bottom up. That should eventually get delegated to the driver framework ... any idea whether there was a reason not to do that bottom-up in the first place? At what point does the driver model kick in to handle that part of what the hub driver now does? If the patch you sent around did that, my quick look missed it ... :)

Quoted 14 lines

usb_set_configuration(struct usb_device *dev, int); usb_set_interface(struct usb_device *dev, int, int);

usb_find_interface_driver_for_ifnum(struct usb_device *dev, int);

 usb_bind_driver(struct usb_driver *,struct usb_device *, int);
	// usb_unbind_driver takes interface handle already!!

usb_ifnum_to_ifpos(struct usb_device *dev, int);

 usb_ifnum_to_if(struct usb_device *dev, int);
	// basically, abolish the need for these calls

usb_epnum_to_ep_desc(struct usb_device *dev, int);

Quoted 4 lines

Plus I'm strongly tempted to group all the "dev + pipe" calls the same way ... better to just pass the endpoint descriptor than to encode descriptor values as bitfields and then later waste time (repeateadly) tearing apart those bitfields.

Agreed, I'll look into fixing up the above functions, unless someone wants to send me a patch doing it first :)

Quoted 7 lines

Related issue to the suspend/resume code ... I recently noticed (again) that the hub code was disconnecting top-down rather than bottom up. That should eventually get delegated to the driver framework ... any idea whether there was a reason not to do that bottom-up in the first place? At what point does the driver model kick in to handle that part of what the hub driver now does? If the patch you sent around did that, my quick look missed it ... :)

Hm, I didn't realize it was going top-down at all. What changes caused that? And no, I don't think I changed it, as I didn't realize it had been changed in the first place :)