Hi all,
Here's a patch against 2.5.30 that is the start of converting the USB
code over to using the new core "struct device_driver" logic and
functionality. Right now only the HUB and HID drivers are converted,
and so they are the only ones that will work properly (the others will
compile, but no devices are ever bound to them.)
The code is still quite rough in places, but the baic functions seem to
work well (driver callbacks, etc.) There is some odd USB specific
tweaks that were needed to be done in order to get this working
properly.
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.
Also, the driverfs representation of the USB devices has changed,
possibly for the worse. Just try the patch to see what I mean :)
There is a known bug that happens in put_device() when a USB device is
removed from the system, but the proper person already knows about it.
Comments?
If I don't hear any objections, I'll work on converting all of the USB
drivers over to this model (the probe and disconnect function parameters
have changed) and send the patch to Linus.
Many thanks to Pat Mochel for the original version of this patch (way
back against 2.5.15) and for putting up with all of the USB device /
interface madness.
thanks,
greg k-h
diff -Nru a/drivers/usb/core/config.c b/drivers/usb/core/config.c
--- a/drivers/usb/core/config.c Fri Aug 9 16:59:55 2002
+++ b/drivers/usb/core/config.c Fri Aug 9 16:59:55 2002
@@ -89,7 +89,7 @@
return parsed;
}
-static int usb_parse_interface(struct usb_interface *interface, unsigned char *buffer, int size)
+static int usb_parse_interface(struct usb_interface *interface, int ifnum, unsigned char *buffer, int size)
{
int i, len, numskipped, retval, parsed = 0;
struct usb_descriptor_header *header;
@@ -99,6 +99,7 @@
interface->act_altsetting = 0;
interface->num_altsetting = 0;
interface->max_altsetting = USB_ALTSETTINGALLOC;
+ interface->ifnum = ifnum;
interface->altsetting = kmalloc(sizeof(struct usb_interface_descriptor) * interface->max_altsetting, GFP_KERNEL);
@@ -323,7 +324,7 @@
}
}
- retval = usb_parse_interface(config->interface + i, buffer, size);
+ retval = usb_parse_interface(config->interface + i, i, buffer, size);
if (retval < 0)
return retval;
diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c Fri Aug 9 16:59:55 2002
+++ b/drivers/usb/core/hcd.c Fri Aug 9 16:59:55 2002
@@ -723,13 +723,15 @@
{
int retval;
- usb_dev->dev.parent = parent_dev;
- strcpy (&usb_dev->dev.name[0], "usb_name");
- strcpy (&usb_dev->dev.bus_id[0], "usb_bus");
- retval = usb_new_device (usb_dev);
+// usb_dev->dev.parent = parent_dev;
+// strcpy (&usb_dev->dev.name[0], "usb_name");
+// strcpy (&usb_dev->dev.bus_id[0], "usb_bus");
+ retval = usb_new_device (usb_dev, parent_dev);
if (retval)
- put_device (&usb_dev->dev);
+ dbg("%s - retval = %d", __FUNCTION__, retval);
+// put_device (&usb_dev->dev);
return retval;
+// return usb_new_device (usb_dev, parent_dev);
}
EXPORT_SYMBOL (usb_register_root_hub);
diff -Nru a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h
--- a/drivers/usb/core/hcd.h Fri Aug 9 16:59:55 2002
+++ b/drivers/usb/core/hcd.h Fri Aug 9 16:59:55 2002
@@ -230,7 +230,7 @@
/* -------------------------------------------------------------------------- */
/* Enumeration is only for the hub driver, or HCD virtual root hubs */
-extern int usb_new_device(struct usb_device *dev);
+extern int usb_new_device(struct usb_device *dev, struct device *parent);
extern void usb_connect(struct usb_device *dev);
extern void usb_disconnect(struct usb_device **);
diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
--- a/drivers/usb/core/hub.c Fri Aug 9 16:59:55 2002
+++ b/drivers/usb/core/hub.c Fri Aug 9 16:59:55 2002
@@ -183,13 +183,14 @@
/* drop lock so HCD can concurrently report other TT errors */
spin_unlock_irqrestore (&hub->tt.lock, flags);
- status = hub_clear_tt_buffer (hub->dev,
+ status = hub_clear_tt_buffer (hub->intf->usb_device,
clear->devinfo, clear->tt);
spin_lock_irqsave (&hub->tt.lock, flags);
if (status)
err ("usb-%s-%s clear tt %d (%04x) error %d",
- hub->dev->bus->bus_name, hub->dev->devpath,
+ hub->intf->usb_device->bus->bus_name,
+ hub->intf->usb_device->devpath,
clear->tt, clear->devinfo, status);
kfree (clear);
}
@@ -250,7 +251,7 @@
/* Enable power to the ports */
dbg("enabling power on all ports");
for (i = 0; i < hub->descriptor->bNbrPorts; i++)
- usb_set_port_feature(hub->dev, i + 1, USB_PORT_FEAT_POWER);
+ usb_set_port_feature(hub->intf->usb_device, i + 1, USB_PORT_FEAT_POWER);
/* Wait for power to be enabled */
wait_ms(hub->descriptor->bPwrOn2PwrGood * 2);
@@ -259,7 +260,7 @@
static int usb_hub_configure(struct usb_hub *hub,
struct usb_endpoint_descriptor *endpoint)
{
- struct usb_device *dev = hub->dev;
+ struct usb_device *dev = hub->intf->usb_device;
struct usb_hub_status hubstatus;
char portstr[USB_MAXCHILDREN + 1];
unsigned int pipe;
@@ -426,39 +427,78 @@
return 0;
}
-static void *hub_probe(struct usb_device *dev, unsigned int i,
- const struct usb_device_id *id)
+static void hub_disconnect(struct usb_interface *intf)
{
- struct usb_interface_descriptor *interface;
+ struct usb_hub *hub = (struct usb_hub *)intf->dev.driver_data;
+ unsigned long flags;
+
+ spin_lock_irqsave(&hub_event_lock, flags);
+
+ /* Delete it and then reset it */
+ list_del(&hub->event_list);
+ INIT_LIST_HEAD(&hub->event_list);
+ list_del(&hub->hub_list);
+ INIT_LIST_HEAD(&hub->hub_list);
+
+ spin_unlock_irqrestore(&hub_event_lock, flags);
+
+ down(&hub->khubd_sem); /* Wait for khubd to leave this hub alone. */
+ up(&hub->khubd_sem);
+
+ /* assuming we used keventd, it must quiesce too */
+ if (hub->tt.hub)
+ flush_scheduled_tasks ();
+
+ if (hub->urb) {
+ usb_unlink_urb(hub->urb);
+ usb_free_urb(hub->urb);
+ hub->urb = NULL;
+ }
+
+ if (hub->descriptor) {
+ kfree(hub->descriptor);
+ hub->descriptor = NULL;
+ }
+
+ /* Free the memory */
+ kfree(hub);
+ intf->dev.driver_data = NULL;
+}
+
+static int hub_probe(struct usb_interface *intf)
+{
+ struct usb_interface_descriptor *desc;
struct usb_endpoint_descriptor *endpoint;
+ struct usb_device *dev;
struct usb_hub *hub;
unsigned long flags;
- interface = &dev->actconfig->interface[i].altsetting[0];
+ desc = intf->altsetting + intf->act_altsetting;
+ dev = intf->usb_device;
/* Some hubs have a subclass of 1, which AFAICT according to the */
/* specs is not defined, but it works */
- if ((interface->bInterfaceSubClass != 0) &&
- (interface->bInterfaceSubClass != 1)) {
+ if ((desc->bInterfaceSubClass != 0) &&
+ (desc->bInterfaceSubClass != 1)) {
err("invalid subclass (%d) for USB hub device #%d",
- interface->bInterfaceSubClass, dev->devnum);
- return NULL;
+ desc->bInterfaceSubClass, dev->devnum);
+ return -EIO;
}
/* Multiple endpoints? What kind of mutant ninja-hub is this? */
- if (interface->bNumEndpoints != 1) {
+ if (desc->bNumEndpoints != 1) {
err("invalid bNumEndpoints (%d) for USB hub device #%d",
- interface->bNumEndpoints, dev->devnum);
- return NULL;
+ desc->bNumEndpoints, dev->devnum);
+ return -EIO;
}
- endpoint = &interface->endpoint[0];
+ endpoint = &desc->endpoint[0];
/* Output endpoint? Curiousier and curiousier.. */
if (!(endpoint->bEndpointAddress & USB_DIR_IN)) {
err("Device #%d is hub class, but has output endpoint?",
dev->devnum);
- return NULL;
+ return -EIO;
}
/* If it's not an interrupt endpoint, we'd better punt! */
@@ -466,7 +506,7 @@
!= USB_ENDPOINT_XFER_INT) {
err("Device #%d is hub class, but endpoint is not interrupt?",
dev->devnum);
- return NULL;
+ return -EIO;
}
/* We found a hub */
@@ -475,13 +515,13 @@
hub = kmalloc(sizeof(*hub), GFP_KERNEL);
if (!hub) {
err("couldn't kmalloc hub struct");
- return NULL;
+ return -ENOMEM;
}
memset(hub, 0, sizeof(*hub));
INIT_LIST_HEAD(&hub->event_list);
- hub->dev = dev;
+ hub->intf = intf;
init_MUTEX(&hub->khubd_sem);
/* Record the new hub's existence */
@@ -490,14 +530,19 @@
list_add(&hub->hub_list, &hub_list);
spin_unlock_irqrestore(&hub_event_lock, flags);
+ intf->dev.driver_data = hub;
+
if (usb_hub_configure(hub, endpoint) >= 0) {
- strcpy (dev->actconfig->interface[i].dev.name,
- "Hub/Port Status Changes");
- return hub;
+// strcpy (dev->actconfig->interface[i].dev.name,
+// "Hub/Port Status Changes");
+ return 0;
}
err("hub configuration failed for device at %s", dev->devpath);
+ hub_disconnect (intf);
+ return -ENODEV;
+#if 0
/* free hub, but first clean up its list. */
spin_lock_irqsave(&hub_event_lock, flags);
@@ -512,43 +557,7 @@
kfree(hub);
return NULL;
-}
-
-static void hub_disconnect(struct usb_device *dev, void *ptr)
-{
- struct usb_hub *hub = (struct usb_hub *)ptr;
- unsigned long flags;
-
- spin_lock_irqsave(&hub_event_lock, flags);
-
- /* Delete it and then reset it */
- list_del(&hub->event_list);
- INIT_LIST_HEAD(&hub->event_list);
- list_del(&hub->hub_list);
- INIT_LIST_HEAD(&hub->hub_list);
-
- spin_unlock_irqrestore(&hub_event_lock, flags);
-
- down(&hub->khubd_sem); /* Wait for khubd to leave this hub alone. */
- up(&hub->khubd_sem);
-
- /* assuming we used keventd, it must quiesce too */
- if (hub->tt.hub)
- flush_scheduled_tasks ();
-
- if (hub->urb) {
- usb_unlink_urb(hub->urb);
- usb_free_urb(hub->urb);
- hub->urb = NULL;
- }
-
- if (hub->descriptor) {
- kfree(hub->descriptor);
- hub->descriptor = NULL;
- }
-
- /* Free the memory */
- kfree(hub);
+#endif
}
static int hub_ioctl(struct usb_device *hub, unsigned int code, void *user_data)
@@ -585,7 +594,7 @@
static int usb_hub_reset(struct usb_hub *hub)
{
- struct usb_device *dev = hub->dev;
+ struct usb_device *dev = hub->intf->usb_device;
int i;
/* Disconnect any attached devices */
@@ -797,7 +806,7 @@
static void usb_hub_port_connect_change(struct usb_hub *hubstate, int port,
u16 portstatus, u16 portchange)
{
- struct usb_device *hub = hubstate->dev;
+ struct usb_device *hub = hubstate->intf->usb_device;
struct usb_device *dev;
unsigned int delay = HUB_SHORT_RESET_TIME;
int i;
@@ -896,7 +905,7 @@
sprintf (&dev->dev.bus_id[0], "%d", port + 1);
/* Run it through the hoops (find a driver, etc) */
- if (!usb_new_device(dev))
+ if (!usb_new_device(dev, &hubstate->intf->dev))
goto done;
/* Free the configuration if there was an error */
@@ -941,7 +950,7 @@
tmp = hub_event_list.next;
hub = list_entry(tmp, struct usb_hub, event_list);
- dev = hub->dev;
+ dev = hub->intf->usb_device;
list_del(tmp);
INIT_LIST_HEAD(tmp);
@@ -1081,9 +1090,9 @@
static struct usb_driver hub_driver = {
.name = "hub",
- .probe = hub_probe,
+ .new_probe = hub_probe,
.ioctl = hub_ioctl,
- .disconnect = hub_disconnect,
+ .new_disco = hub_disconnect,
.id_table = hub_id_table,
};
diff -Nru a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
--- a/drivers/usb/core/hub.h Fri Aug 9 16:59:55 2002
+++ b/drivers/usb/core/hub.h Fri Aug 9 16:59:55 2002
@@ -166,7 +166,8 @@
extern void usb_hub_tt_clear_buffer (struct usb_device *dev, int pipe);
struct usb_hub {
- struct usb_device *dev; /* the "real" device */
+// struct usb_device *dev; /* the "real" device */
+ struct usb_interface *intf; /* the "real" device */
struct urb *urb; /* for interrupt polling pipe */
/* buffer for urb ... 1 bit each for hub and children, rounded up */
diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
--- a/drivers/usb/core/usb.c Fri Aug 9 16:59:55 2002
+++ b/drivers/usb/core/usb.c Fri Aug 9 16:59:55 2002
@@ -61,6 +61,50 @@
LIST_HEAD(usb_driver_list);
+static int generic_probe (struct device *dev)
+{
+ return 0;
+}
+static int generic_remove (struct device *dev)
+{
+ return 0;
+}
+static void generic_release (struct device_driver * drv)
+{
+}
+
+static struct device_driver usb_generic_driver = {
+ .name = "generic usb driver",
+ .probe = generic_probe,
+ .remove = generic_remove,
+ .release = generic_release,
+};
+
+
+static int usb_device_probe(struct device * dev)
+{
+ struct usb_interface * intf = list_entry(dev,struct usb_interface,dev);
+ struct usb_driver * drv = list_entry(dev->driver,struct usb_driver,driver);
+ int error = -ENODEV;
+
+ dbg("%s", __FUNCTION__);
+
+ if (drv->new_probe)
+ error = drv->new_probe(intf);
+ if (!error)
+ intf->driver = drv;
+ return error;
+}
+
+static int usb_device_remove(struct device * dev)
+{
+ struct usb_interface * intf = list_entry(dev,struct usb_interface,dev);
+
+ if (intf->driver && intf->driver->new_disco)
+ intf->driver->new_disco(intf);
+ return 0;
+}
+
/**
* usb_register - register a USB driver
* @new_driver: USB operations for the driver
@@ -78,16 +122,26 @@
{
int retval = 0;
- info("registered new driver %s", new_driver->name);
+ new_driver->driver.name = (char *)new_driver->name;
+ new_driver->driver.bus = &usb_bus_type;
+ new_driver->driver.probe = usb_device_probe;
+ new_driver->driver.remove = usb_device_remove;
init_MUTEX(&new_driver->serialize);
- /* Add it to the list of known drivers */
- list_add_tail(&new_driver->driver_list, &usb_driver_list);
-
- usb_scan_devices();
-
- usbfs_update_special();
+// /* Add it to the list of known drivers */
+// list_add_tail(&new_driver->driver_list, &usb_driver_list);
+//
+// usb_scan_devices();
+ retval = driver_register(&new_driver->driver);
+
+ if (!retval) {
+ info("registered new driver %s", new_driver->name);
+ usbfs_update_special();
+ } else {
+ err("problem %d when registering driver %s",
+ retval, new_driver->name);
+ }
return retval;
}
@@ -604,6 +658,75 @@
return NULL;
}
+static int usb_device_bind (struct device *dev, struct device_driver *drv)
+{
+ struct usb_interface *intf = list_entry(dev, struct usb_interface, dev);
+ struct usb_interface_descriptor *desc;
+ struct usb_device *usb_dev;
+ struct usb_driver *usb_drv;
+ const struct usb_device_id *id;
+
+ desc = &intf->altsetting[intf->act_altsetting];
+ usb_dev = intf->usb_device;
+ usb_drv = list_entry(drv, struct usb_driver, driver);
+ id = usb_drv->id_table;
+
+ /* It is important to check that id->driver_info is nonzero,
+ since an entry that is all zeroes except for a nonzero
+ id->driver_info is the way to create an entry that
+ indicates that the driver want to examine every
+ device and interface. */
+ for (; (id) && (id->idVendor || id->bDeviceClass || id->bInterfaceClass ||
+ id->driver_info); id++) {
+
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_VENDOR) &&
+ id->idVendor != usb_dev->descriptor.idVendor)
+ continue;
+
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_PRODUCT) &&
+ id->idProduct != usb_dev->descriptor.idProduct)
+ continue;
+
+ /* No need to test id->bcdDevice_lo != 0, since 0 is never
+ greater than any unsigned number. */
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_LO) &&
+ (id->bcdDevice_lo > usb_dev->descriptor.bcdDevice))
+ continue;
+
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_HI) &&
+ (id->bcdDevice_hi < usb_dev->descriptor.bcdDevice))
+ continue;
+
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_CLASS) &&
+ (id->bDeviceClass != usb_dev->descriptor.bDeviceClass))
+ continue;
+
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_SUBCLASS) &&
+ (id->bDeviceSubClass!= usb_dev->descriptor.bDeviceSubClass))
+ continue;
+
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_PROTOCOL) &&
+ (id->bDeviceProtocol != usb_dev->descriptor.bDeviceProtocol))
+ continue;
+
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_INT_CLASS) &&
+ (id->bInterfaceClass != desc->bInterfaceClass))
+ continue;
+
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_INT_SUBCLASS) &&
+ (id->bInterfaceSubClass != desc->bInterfaceSubClass))
+ continue;
+
+ if ((id->match_flags & USB_DEVICE_ID_MATCH_INT_PROTOCOL) &&
+ (id->bInterfaceProtocol != desc->bInterfaceProtocol))
+ continue;
+
+ return 1;
+ }
+
+ return 0;
+}
+
/*
* This entrypoint gets called for each new device.
*
@@ -1346,11 +1469,12 @@
*/
#define NEW_DEVICE_RETRYS 2
#define SET_ADDRESS_RETRYS 2
-int usb_new_device(struct usb_device *dev)
+int usb_new_device(struct usb_device *dev, struct device *parent)
{
int err = 0;
int i;
int j;
+ int ifnum;
/* USB v1.1 5.5.3 */
/* We read the first 8 bytes from the device descriptor to get to */
@@ -1437,6 +1561,12 @@
#endif
/* register this device in the driverfs tree */
+ usb_generic_driver.bus = &usb_bus_type;
+ dev->dev.parent = parent;
+ dev->dev.driver = &usb_generic_driver;
+ dev->dev.bus = &usb_bus_type;
+ sprintf (&dev->dev.bus_id[0], "%s-%s",
+ dev->bus->bus_name, dev->devpath);
err = device_register (&dev->dev);
if (err)
return err;
@@ -1450,10 +1580,39 @@
/* now that the basic setup is over, add a /proc/bus/usb entry */
usbfs_add_device(dev);
-
/* find drivers willing to handle this device */
- usb_find_drivers(dev);
+// usb_find_drivers(dev);
+ for (ifnum = 0; ifnum < dev->actconfig->bNumInterfaces; ifnum++) {
+ struct usb_interface *interface = &dev->actconfig->interface[ifnum];
+ struct usb_interface_descriptor *desc = interface->altsetting;
+ /* register this interface with driverfs */
+ interface->usb_device = dev;
+ interface->dev.parent = &dev->dev;
+ interface->dev.bus = &usb_bus_type;
+ sprintf (&interface->dev.bus_id[0], "%s-%s:%d",
+ dev->bus->bus_name, dev->devpath,
+ interface->altsetting->bInterfaceNumber);
+ if (!desc->iInterface
+ || usb_string (dev, desc->iInterface,
+ interface->dev.name,
+ sizeof interface->dev.name) <= 0) {
+ /* typically devices won't bother with interface
+ * descriptions; this is the normal case. an
+ * interface's driver might describe it better.
+ * (also: iInterface is per-altsetting ...)
+ */
+ sprintf (&interface->dev.name[0],
+ "usb-%s-%s interface %d",
+ dev->bus->bus_name, dev->devpath,
+ interface->altsetting->bInterfaceNumber);
+ }
+ dbg("%s - registering %s", __FUNCTION__, interface->dev.bus_id);
+ device_register (&interface->dev);
+ device_create_file (&interface->dev, &dev_attr_altsetting);
+ }
+
+
/* userspace may load modules and/or configure further */
call_policy ("add", dev);
@@ -1620,7 +1779,8 @@
#endif
struct bus_type usb_bus_type = {
- .name = "usb",
+ .name = "usb",
+ .match = usb_device_bind,
};
/*
diff -Nru a/drivers/usb/input/hid-core.c b/drivers/usb/input/hid-core.c
--- a/drivers/usb/input/hid-core.c Fri Aug 9 16:59:55 2002
+++ b/drivers/usb/input/hid-core.c Fri Aug 9 16:59:55 2002
@@ -1338,9 +1338,10 @@
{ 0, 0 }
};
-static struct hid_device *usb_hid_configure(struct usb_device *dev, int ifnum)
+static struct hid_device *usb_hid_configure(struct usb_interface *intf)
{
- struct usb_interface_descriptor *interface = dev->actconfig->interface[ifnum].altsetting + 0;
+ struct usb_interface_descriptor *interface = intf->altsetting + intf->act_altsetting;
+ struct usb_device *dev = intf->usb_device;
struct hid_descriptor *hdesc;
struct hid_device *hid;
unsigned quirks = 0, rsize = 0;
@@ -1450,7 +1451,7 @@
snprintf(hid->name, 128, "%04x:%04x", dev->descriptor.idVendor, dev->descriptor.idProduct);
usb_make_path(dev, buf, 64);
- snprintf(hid->phys, 64, "%s/input%d", buf, ifnum);
+ snprintf(hid->phys, 64, "%s/input%d", buf, intf->ifnum);
if (usb_string(dev, dev->descriptor.iSerialNumber, hid->uniq, 64) <= 0)
hid->uniq[0] = 0;
@@ -1472,9 +1473,9 @@
return NULL;
}
-static void hid_disconnect(struct usb_device *dev, void *ptr)
+static void hid_disconnect(struct usb_interface *intf)
{
- struct hid_device *hid = ptr;
+ struct hid_device *hid = intf->dev.driver_data;
usb_unlink_urb(hid->urbin);
usb_unlink_urb(hid->urbout);
@@ -1491,20 +1492,20 @@
usb_free_urb(hid->urbout);
hid_free_device(hid);
+ intf->dev.driver_data = NULL;
}
-static void* hid_probe(struct usb_device *dev, unsigned int ifnum,
- const struct usb_device_id *id)
+static int hid_probe (struct usb_interface *intf)
{
struct hid_device *hid;
char path[64];
int i;
char *c;
- dbg("HID probe called for ifnum %d", ifnum);
+ dbg("HID probe called for ifnum %d", intf->ifnum);
- if (!(hid = usb_hid_configure(dev, ifnum)))
- return NULL;
+ if (!(hid = usb_hid_configure(intf)))
+ return -EIO;
hid_init_reports(hid);
hid_dump_device(hid);
@@ -1516,9 +1517,11 @@
if (!hiddev_connect(hid))
hid->claimed |= HID_CLAIMED_HIDDEV;
+ intf->dev.driver_data = hid;
+
if (!hid->claimed) {
- hid_disconnect(dev, hid);
- return NULL;
+ hid_disconnect(intf);
+ return -EIO;
}
printk(KERN_INFO);
@@ -1540,12 +1543,12 @@
}
}
- usb_make_path(dev, path, 63);
+ usb_make_path(intf->usb_device, path, 63);
printk(": USB HID v%x.%02x %s [%s] on %s\n",
hid->version >> 8, hid->version & 0xff, c, hid->name, path);
- return hid;
+ return 0;
}
static struct usb_device_id hid_usb_ids [] = {
@@ -1558,8 +1561,8 @@
static struct usb_driver hid_driver = {
.name = "hid",
- .probe = hid_probe,
- .disconnect = hid_disconnect,
+ .new_probe = hid_probe,
+ .new_disco = hid_disconnect,
.id_table = hid_usb_ids,
};
diff -Nru a/include/linux/usb.h b/include/linux/usb.h
--- a/include/linux/usb.h Fri Aug 9 16:59:55 2002
+++ b/include/linux/usb.h Fri Aug 9 16:59:55 2002
@@ -252,8 +252,10 @@
int act_altsetting; /* active alternate setting */
int num_altsetting; /* number of alternate settings */
int max_altsetting; /* total memory allocated */
-
+ int ifnum; /* number of this interface */
+
struct usb_driver *driver; /* driver */
+ struct usb_device *usb_device; /* device this interface is on */
struct device dev; /* interface specific device info */
void *private_data;
};
@@ -683,6 +685,12 @@
struct usb_driver {
struct module *owner;
const char *name;
+
+ int (*new_probe) (struct usb_interface *intf);
+ int (*init) (struct usb_interface *intf);
+ void (*new_disco) (struct usb_interface *intf);
+
+ struct device_driver driver;
void *(*probe)(
struct usb_device *dev, /* the device */
Am Samstag, 10. August 2002 02:10 schrieb Greg KH:
> 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.
Regards
Oliver
On Sat, Aug 10, 2002 at 11:57:28AM +0200, Oliver Neukum wrote:
> Am Samstag, 10. August 2002 02:10 schrieb Greg KH:
> > 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.
Hm, I don't really understand your question, but I'll try to explain
how I did this.
Both struct usb_device and struct usb_interface now have a struct
device. This is because _both_ things need to show up in the driverfs
tree. I tried only making interfaces be devices (which is more of what
the USB core code things of as devices), but the tree just didn't make
much sense. Also, a USB device contains such things as the control
pipe, and the usb descriptors and strings. So it is useful to show the
device, as that's where you get the device name and serial number from :)
Does that help?
Try looking at the usb driverfs implementation as it is in 2.5.30, both
the interfaces and the main device show up in the tree, I didn't change
this. All this patch does is start to use "struct device_driver", and
bind the drivers to the interfaces, _which_ is what the USB core has
always done (no change in functionality here.) What this allows us to
do is remove (eventually) the USB list of devices, and remove some other
device list logic, as well as clean up the probe/disconnect interface.
thanks,
greg k-h
>>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.
>>>struct usb_driver {
>>> struct module *owner;
>>> const char *name;
>>>+
>>>+ int (*new_probe) (struct usb_interface *intf);
>>>+ int (*init) (struct usb_interface *intf);
>>>+ void (*new_disco) (struct usb_interface *intf);
>>>+
>>>+ struct device_driver driver;
>>>
>>> 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.
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"? :)
>>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?
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.
>>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.
I though that was what was going on!
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 ... :)
- Dave
> thanks for the comments,
>
> greg k-h
>
On Mon, Aug 12, 2002 at 11:30:38AM -0700, David Brownell wrote:
> >>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.
Yeah, it is a bit ugly, and the whole symlink thing is going to be
revisited. So for now I'm ignoring them, and hopefully Pat will decide
on something later on :)
> 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).
Ugh, forgot about ->driver_info. Ok, I'll go work on that, pci gets
around it by duplicating the "old" probe() interface. I don't want to
throw away that info, as it is useful for some drivers.
> 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"? :)
"new_disco" will be dead, once all the drivers are converted :)
> >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.
Agreed, I'll look into fixing up the above functions, unless someone
wants to send me a patch doing it first :)
> 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 :)
thanks,
greg k-h
>> 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.
>
>
> Agreed, I'll look into fixing up the above functions, unless someone
> wants to send me a patch doing it first :)
Maybe when Linus' release is a bit closer to the USB tree I could help
with that. Right now it's sufficiently divergent to make usb patches
against usbcore be rather helpful in general (though maybe not in that
particular case). I'll certainly be glad to take a whack at that.
>>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 :)
I think it's just been that way for ages, no changes. It's actually
usb_disconnect(), not the hub driver, that goes top down, disconnecting
children only _after_ the parent is disconnected. (Might be nice to
flag all devices as "dead" before disconnecting in such cases, but we
don't have a way to flag devices as "dead" AFAIK. That'd make it easy
to fail new urb submissions for now-gone devices.)
If the current changes don't automagically make disconnection work
according to the device tree, something needs to change to make that
work correctly. But I'm not sure what it'd be; seems like the converse
of the probe() changes you started.
- Dave