[PATCH] driverfs support for USB - take 2

[PATCH] driverfs support for USB - take 2

[Greg KH]

Hi,

Well after determining that the last version of this patch doesn't show
the USB tree properly, here's another patch against 2.5.3-pre6 that
fixes this issue.

With this patch (the driver core changes were from Pat Mochel, thanks
Pat for putting up with my endless questions) my machine now shows the
following tree:

[greg@soap x]$ tree
.
`-- root
    |-- pci0
    |   |-- 00:00.0
    |   |   |-- power
    |   |   `-- status
    |   |-- 00:01.0
    |   |   |-- 02:00.0
    |   |   |   |-- power
    |   |   |   `-- status
    |   |   |-- power
    |   |   `-- status
    |   |-- 00:1e.0
    |   |   |-- 01:08.0
    |   |   |   |-- power
    |   |   |   `-- status
    |   |   |-- 01:0d.0
    |   |   |   |-- power
    |   |   |   |-- status
    |   |   |   `-- usb_bus
    |   |   |       |-- power
    |   |   |       `-- status
    |   |   |-- 01:0d.1
    |   |   |   |-- power
    |   |   |   |-- status
    |   |   |   `-- usb_bus
    |   |   |       |-- 003
    |   |   |       |   |-- power
    |   |   |       |   `-- status
    |   |   |       |-- power
    |   |   |       `-- status
    |   |   |-- 01:0d.2
    |   |   |   |-- power
    |   |   |   |-- status
    |   |   |   `-- usb_bus
    |   |   |       |-- power
    |   |   |       `-- status
    |   |   |-- power
    |   |   `-- status
    |   |-- 00:1f.0
    |   |   |-- power
    |   |   `-- status
    |   |-- 00:1f.1
    |   |   |-- power
    |   |   `-- status
    |   |-- 00:1f.2
    |   |   |-- power
    |   |   |-- status
    |   |   `-- usb_bus
    |   |       |-- 002
    |   |       |   |-- 003
    |   |       |   |   |-- power
    |   |       |   |   `-- status
    |   |       |   |-- power
    |   |       |   `-- status
    |   |       |-- power
    |   |       `-- status
    |   |-- 00:1f.3
    |   |   |-- power
    |   |   `-- status
    |   |-- 00:1f.5
    |   |   |-- power
    |   |   `-- status
    |   |-- power
    |   `-- status
    |-- power
    `-- status


I have 2 USB controllers in this system, 1 UHCI controller on the
motherboard that shows up under PCI device 00:1f.2. There are two
devices plugged into this bus, a hub, and a trackball plugged into the
hub.  This topology is now shown properly.

The other controller is a USB 2.0 controller that shows up in 3 places
on the pci bus: 01:0d.0, 01:0d.1, and 01:0d.2.  The 01:0d.1 device is
controlled by the ohci-hcd driver, and a USB hub is plugged into it.
The ehci-hcd driver controls the other 2 pci devices on the card.

Yes, I need to have better names for the devices than just "usb_bus",
any suggestions?  These devices nodes are really the USB root hubs in
the USB controller, so they could just have the USB number as the name
like the other USB devices (001), but that's pretty boring :)

greg k-h



diff -Nru a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c	Tue Jan 29 16:02:26 2002
+++ b/drivers/base/core.c	Tue Jan 29 16:02:26 2002
@@ -17,10 +17,7 @@
 # define DBG(x...)
 #endif
 
-static struct iobus device_root = {
-	bus_id: "root",
-	name:	"Logical System Root",
-};
+static struct device * device_root;
 
 int (*platform_notify)(struct device * dev) = NULL;
 int (*platform_notify_remove)(struct device * dev) = NULL;
@@ -28,9 +25,6 @@
 extern int device_make_dir(struct device * dev);
 extern void device_remove_dir(struct device * dev);
 
-extern int iobus_make_dir(struct iobus * iobus);
-extern void iobus_remove_dir(struct iobus * iobus);
-
 static spinlock_t device_lock;
 
 /**
@@ -47,19 +41,25 @@
 
 	if (!dev || !strlen(dev->bus_id))
 		return -EINVAL;
-	BUG_ON(!dev->parent);
 
 	spin_lock(&device_lock);
 	INIT_LIST_HEAD(&dev->node);
+	INIT_LIST_HEAD(&dev->children);
 	spin_lock_init(&dev->lock);
 	atomic_set(&dev->refcount,2);
 
-	get_iobus(dev->parent);
-	list_add_tail(&dev->node,&dev->parent->devices);
+	if (dev != device_root) {
+		struct device * parent;
+		if (!dev->parent)
+			dev->parent = device_root;
+		parent = dev->parent;
+		get_device(parent);
+		list_add_tail(&dev->node,&parent->children);
+	}
 	spin_unlock(&device_lock);
 
-	DBG("DEV: registering device: ID = '%s', name = %s, parent = %s\n",
-	    dev->bus_id, dev->name, parent->bus_id);
+	DBG("DEV: registering device: ID = '%s', name = %s\n",
+	    dev->bus_id, dev->name);
 
 	if ((error = device_make_dir(dev)))
 		goto register_done;
@@ -70,8 +70,8 @@
 
  register_done:
 	put_device(dev);
-	if (error)
-		put_iobus(dev->parent);
+	if (error && dev->parent)
+		put_device(dev->parent);
 	return error;
 }
 
@@ -100,9 +100,6 @@
 	/* remove the driverfs directory */
 	device_remove_dir(dev);
 
-	if (dev->subordinate)
-		iobus_remove_dir(dev->subordinate);
-
 	/* Notify the platform of the removal, in case they
 	 * need to do anything...
 	 */
@@ -116,70 +113,18 @@
 	if (dev->driver && dev->driver->remove)
 		dev->driver->remove(dev,REMOVE_FREE_RESOURCES);
 
-	put_iobus(dev->parent);
-}
-
-int iobus_register(struct iobus *bus)
-{
-	int error;
-
-	if (!bus || !strlen(bus->bus_id))
-		return -EINVAL;
-	
-	spin_lock(&device_lock);
-	atomic_set(&bus->refcount,2);
-	spin_lock_init(&bus->lock);
-	INIT_LIST_HEAD(&bus->node);
-	INIT_LIST_HEAD(&bus->devices);
-	INIT_LIST_HEAD(&bus->children);
-
-	if (bus != &device_root) {
-		if (!bus->parent)
-			bus->parent = &device_root;
-		get_iobus(bus->parent);
-		list_add_tail(&bus->node,&bus->parent->children);
-	}
-	spin_unlock(&device_lock);
-
-	DBG("DEV: registering bus. ID = '%s' name = '%s' parent = %p\n",
-	    bus->bus_id,bus->name,bus->parent);
-
-	error = iobus_make_dir(bus);
-
-	put_iobus(bus);
-	if (error && bus->parent)
-		put_iobus(bus->parent);
-	return error;
-}
-
-/**
- * iobus_unregister - remove bus and children from device tree
- * @bus:	pointer to bus structure
- *
- * Remove device from parent's list of children and decrement
- * reference count on controlling device. That should take care of
- * the rest of the cleanup.
- */
-void put_iobus(struct iobus * iobus)
-{
-	if (!atomic_dec_and_lock(&iobus->refcount,&device_lock))
-		return;
-	list_del_init(&iobus->node);
-	spin_unlock(&device_lock);
-
-	if (!list_empty(&iobus->devices) ||
-	    !list_empty(&iobus->children))
-		BUG();
-
-	put_iobus(iobus->parent);
-	/* unregister itself */
-	put_device(iobus->self);
+	put_device(dev->parent);
 }
 
 static int __init device_init_root(void)
 {
-	/* initialize parent bus lists */
-	return iobus_register(&device_root);
+	device_root = kmalloc(sizeof(*device_root),GFP_KERNEL);
+	if (!device_root)
+		return -ENOMEM;
+	memset(device_root,0,sizeof(*device_root));
+	strcpy(device_root->bus_id,"root");
+	strcpy(device_root->name,"System Root");
+	return device_register(device_root);
 }
 
 int __init device_driver_init(void)
@@ -208,5 +153,5 @@
 }
 
 EXPORT_SYMBOL(device_register);
-EXPORT_SYMBOL(iobus_register);
+EXPORT_SYMBOL(put_device);
 EXPORT_SYMBOL(device_driver_init);
diff -Nru a/drivers/base/fs.c b/drivers/base/fs.c
--- a/drivers/base/fs.c	Tue Jan 29 16:02:26 2002
+++ b/drivers/base/fs.c	Tue Jan 29 16:02:26 2002
@@ -102,28 +102,6 @@
 		}
 	}
 	return 0;
-}
-
-void iobus_remove_dir(struct iobus * iobus)
-{
-	if (iobus)
-		driverfs_remove_dir(&iobus->dir);
-}
-
-int iobus_make_dir(struct iobus * iobus)
-{
-	struct driver_dir_entry * parent = NULL;
-	int error;
-
-	INIT_LIST_HEAD(&iobus->dir.files);
-	iobus->dir.mode = (S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO);
-	iobus->dir.name = iobus->bus_id;
-
-	if (iobus->parent)
-		parent = &iobus->parent->dir;
-
-	error = driverfs_create_dir(&iobus->dir,parent);
-	return error;
 }
 
 EXPORT_SYMBOL(device_create_file);
diff -Nru a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c	Tue Jan 29 16:02:26 2002
+++ b/drivers/pci/pci.c	Tue Jan 29 16:02:26 2002
@@ -1086,13 +1086,7 @@
 	child->parent = parent;
 	child->ops = parent->ops;
 	child->sysdata = parent->sysdata;
-
-	/* init generic fields */
-	child->iobus.self = &dev->dev;
-	child->iobus.parent = &parent->iobus;
-	dev->dev.subordinate = &child->iobus;
-
-	strcpy(child->iobus.name,dev->dev.name);
+	child->dev = &dev->dev;
 
 	/*
 	 * Set up the primary, secondary and subordinate
@@ -1361,16 +1355,11 @@
 	DBG("Scanning bus %02x\n", bus->number);
 	max = bus->secondary;
 
-	/* we should know for sure what the bus number is, so set the bus ID
-	 * for the bus and make sure it's registered in the device tree */
-	sprintf(bus->iobus.bus_id,"pci%d",bus->number);
-	iobus_register(&bus->iobus);
-
 	/* Create a device template */
 	memset(&dev0, 0, sizeof(dev0));
 	dev0.bus = bus;
 	dev0.sysdata = bus->sysdata;
-	dev0.dev.parent = &bus->iobus;
+	dev0.dev.parent = bus->dev;
 	dev0.dev.driver = &pci_device_driver;
 
 	/* Go find them, Rover! */
@@ -1430,9 +1419,11 @@
 		return NULL;
 	list_add_tail(&b->node, &pci_root_buses);
 
-	sprintf(b->iobus.bus_id,"pci%d",bus);
-	strcpy(b->iobus.name,"Host/PCI Bridge");
-	iobus_register(&b->iobus);
+	b->dev = kmalloc(sizeof(*(b->dev)),GFP_KERNEL);
+	memset(b->dev,0,sizeof(*(b->dev)));
+	sprintf(b->dev->bus_id,"pci%d",bus);
+	strcpy(b->dev->name,"Host/PCI Bridge");
+	device_register(b->dev);
 
 	b->number = b->secondary = bus;
 	b->resource[0] = &ioport_resource;
diff -Nru a/drivers/usb/hcd/ehci-hcd.c b/drivers/usb/hcd/ehci-hcd.c
--- a/drivers/usb/hcd/ehci-hcd.c	Tue Jan 29 16:02:26 2002
+++ b/drivers/usb/hcd/ehci-hcd.c	Tue Jan 29 16:02:26 2002
@@ -290,6 +290,12 @@
 		goto done2;
 	}
 
+	/* hook up the root hub to the pci controller in the device tree */
+	udev->dev.parent = &ehci->hcd.pdev->dev;
+	strcpy (udev->dev.name, "usb_name");
+	strcpy (udev->dev.bus_id, "usb_bus");
+	device_register (&udev->dev);
+
 	return 0;
 }
 
diff -Nru a/drivers/usb/hcd/ohci-hcd.c b/drivers/usb/hcd/ohci-hcd.c
--- a/drivers/usb/hcd/ohci-hcd.c	Tue Jan 29 16:02:26 2002
+++ b/drivers/usb/hcd/ohci-hcd.c	Tue Jan 29 16:02:26 2002
@@ -475,7 +475,13 @@
 // FIXME cleanup
 		return -ENODEV;
 	}
-	
+
+	/* hook up the root hub to the pci controller in the device tree */
+	udev->dev.parent = &ohci->hcd.pdev->dev;
+	strcpy (udev->dev.name, "usb_name");
+	strcpy (udev->dev.bus_id, "usb_bus");
+	device_register (&udev->dev);
+
 	return 0;
 }
 
diff -Nru a/drivers/usb/hub.c b/drivers/usb/hub.c
--- a/drivers/usb/hub.c	Tue Jan 29 16:02:27 2002
+++ b/drivers/usb/hub.c	Tue Jan 29 16:02:27 2002
@@ -722,8 +722,16 @@
 			dev->bus->busnum, dev->devpath, dev->devnum);
 
 		/* Run it through the hoops (find a driver, etc) */
-		if (!usb_new_device(dev))
+		if (!usb_new_device(dev)) {
+			/* put the device in the global device tree */
+			dev->dev.parent = &dev->parent->dev;
+			sprintf (&dev->dev.name[0], "USB device %04x:%04x",
+				 dev->descriptor.idVendor,
+				 dev->descriptor.idProduct);
+			sprintf (&dev->dev.bus_id[0], "%03d", dev->devnum);
+			device_register (&dev->dev);
 			goto done;
+		}
 
 		/* Free the configuration if there was an error */
 		usb_free_dev(dev);
diff -Nru a/drivers/usb/uhci.c b/drivers/usb/uhci.c
--- a/drivers/usb/uhci.c	Tue Jan 29 16:02:26 2002
+++ b/drivers/usb/uhci.c	Tue Jan 29 16:02:26 2002
@@ -2848,6 +2848,12 @@
 		goto err_start_root_hub;
 	}
 
+	/* hook the root hub up to the pci controller in the device tree */
+	uhci->rh.dev->dev.parent = &dev->dev;
+	strcpy (uhci->rh.dev->dev.name, "usb_name");
+	strcpy (uhci->rh.dev->dev.bus_id, "usb_bus");
+	device_register (&uhci->rh.dev->dev);
+
 	return 0;
 
 /*
diff -Nru a/drivers/usb/usb-ohci.c b/drivers/usb/usb-ohci.c
--- a/drivers/usb/usb-ohci.c	Tue Jan 29 16:02:26 2002
+++ b/drivers/usb/usb-ohci.c	Tue Jan 29 16:02:26 2002
@@ -2508,6 +2508,13 @@
 #ifdef	DEBUG
 	ohci_dump (ohci, 1);
 #endif
+
+	/* hook the root hub up to the pci controller in the device tree */
+	ohci->bus->root_hub->dev.parent = &dev->dev;
+	strcpy (ohci->bus->root_hub->dev.name, "usb_name");
+	strcpy (ohci->bus->root_hub->dev.bus_id, "usb_bus");
+	device_register (&ohci->bus->root_hub->dev);
+
 	return 0;
 }
 
diff -Nru a/drivers/usb/usb-uhci.c b/drivers/usb/usb-uhci.c
--- a/drivers/usb/usb-uhci.c	Tue Jan 29 16:02:26 2002
+++ b/drivers/usb/usb-uhci.c	Tue Jan 29 16:02:26 2002
@@ -3028,6 +3028,12 @@
 	pci_set_drvdata(dev, s);
 	devs=s;
 
+	/* hook the root hub up to the pci controller in the device tree */
+	s->bus->root_hub->dev.parent = &dev->dev;
+	strcpy (s->bus->root_hub->dev.name, "usb_name");
+	strcpy (s->bus->root_hub->dev.bus_id, "usb_bus");
+	device_register (&s->bus->root_hub->dev);
+
 	return 0;
 }
 
diff -Nru a/drivers/usb/usb.c b/drivers/usb/usb.c
--- a/drivers/usb/usb.c	Tue Jan 29 16:02:27 2002
+++ b/drivers/usb/usb.c	Tue Jan 29 16:02:27 2002
@@ -1925,6 +1951,7 @@
 	if (dev->devnum > 0) {
 		clear_bit(dev->devnum, &dev->bus->devmap.devicemap);
 		usbfs_remove_device(dev);
+		put_device(&dev->dev);
 	}
 
 	/* Free up the device itself */
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h	Tue Jan 29 16:02:26 2002
+++ b/include/linux/device.h	Tue Jan 29 16:02:26 2002
@@ -66,10 +66,8 @@
 
 struct device {
 	struct list_head node;		/* node in sibling list */
-	struct iobus	*parent;	/* parent bus */
-
-	struct iobus	*subordinate;	/* only valid if this device is a
-					   bridge device */
+	struct list_head children;
+	struct device 	*parent;
 
 	char	name[DEVICE_NAME_SIZE];	/* descriptive ascii string */
 	char	bus_id[BUS_ID_SIZE];	/* position on parent bus */
diff -Nru a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h	Tue Jan 29 16:02:26 2002
+++ b/include/linux/pci.h	Tue Jan 29 16:02:26 2002
@@ -432,8 +432,7 @@
 	unsigned char	productver;	/* product version */
 	unsigned char	checksum;	/* if zero - checksum passed */
 	unsigned char	pad1;
-
-	struct iobus	iobus;		/* Generic device interface */
+	struct	device	* dev;
 };
 
 #define pci_bus_b(n) list_entry(n, struct pci_bus, node)
diff -Nru a/include/linux/usb.h b/include/linux/usb.h
--- a/include/linux/usb.h	Tue Jan 29 16:02:26 2002
+++ b/include/linux/usb.h	Tue Jan 29 16:02:26 2002
@@ -1,6 +1,8 @@
 #ifndef __LINUX_USB_H
 #define __LINUX_USB_H
 
+#include <linux/device.h>
+
 /* USB constants */
 
 /*
@@ -1037,6 +1042,8 @@
 
 	struct usb_device *parent;
 	struct usb_bus *bus;		/* Bus we're part of */
+
+	struct device dev;		/* Generic device interface */
 
 	struct usb_device_descriptor descriptor;/* Descriptor */
 	struct usb_config_descriptor *config;	/* All of the configs */

Re: [PATCH] driverfs support for USB - take 2

[Dave Jones]

On Tue, Jan 29, 2002 at 04:24:18PM -0800, Greg KH wrote:
 > Hi,
 > 
 > Well after determining that the last version of this patch doesn't show
 > the USB tree properly, here's another patch against 2.5.3-pre6 that
 > fixes this issue.
 > 
 > With this patch (the driver core changes were from Pat Mochel, thanks
 > Pat for putting up with my endless questions) my machine now shows the
 > following tree:

 Looks good, now when the PM bits can power down from the leaves,
 and turn off USB devices /before/ the USB controller and PCI bridges,
 which sounds much more sensible.

 > Yes, I need to have better names for the devices than just "usb_bus",
 > any suggestions?  These devices nodes are really the USB root hubs in
 > the USB controller, so they could just have the USB number as the name
 > like the other USB devices (001), but that's pretty boring :)

 "usb_root0" .. "usb_rootN" ?

 btw, a script to marry the busid's from driverfs to lspci/lsusb
 output may be useful in the future especially if combined somehow
 with tree(1). Could be very handy when it gets time to debug
 those "My system won't suspend to disk" "What does /driver look like?"
 situations.


-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [PATCH] driverfs support for USB - take 2

[Greg KH]

On Wed, Jan 30, 2002 at 02:03:12AM +0100, Dave Jones wrote:
> 
>  > Yes, I need to have better names for the devices than just "usb_bus",
>  > any suggestions?  These devices nodes are really the USB root hubs in
>  > the USB controller, so they could just have the USB number as the name
>  > like the other USB devices (001), but that's pretty boring :)
> 
>  "usb_root0" .. "usb_rootN" ?

Hm, that's a good idea, it would match the usbfs bus numbers which
should keep people happy.

>  btw, a script to marry the busid's from driverfs to lspci/lsusb
>  output may be useful in the future especially if combined somehow
>  with tree(1). Could be very handy when it gets time to debug
>  those "My system won't suspend to disk" "What does /driver look like?"
>  situations.

Ah, a lsdrivers program is needed! :)

thanks,

greg k-h

Re: [PATCH] driverfs support for USB - take 2

[Patrick Mochel]

On Tue, 29 Jan 2002, Greg KH wrote:

> On Wed, Jan 30, 2002 at 02:03:12AM +0100, Dave Jones wrote:
> > 
> >  > Yes, I need to have better names for the devices than just "usb_bus",
> >  > any suggestions?  These devices nodes are really the USB root hubs in
> >  > the USB controller, so they could just have the USB number as the name
> >  > like the other USB devices (001), but that's pretty boring :)
> > 
> >  "usb_root0" .. "usb_rootN" ?
> 
> Hm, that's a good idea, it would match the usbfs bus numbers which
> should keep people happy.

Would it be usb_rootN or usb_busN?

> >  btw, a script to marry the busid's from driverfs to lspci/lsusb
> >  output may be useful in the future especially if combined somehow
> >  with tree(1). Could be very handy when it gets time to debug
> >  those "My system won't suspend to disk" "What does /driver look like?"
> >  situations.
> 
> Ah, a lsdrivers program is needed! :)

I started writing an 'lsdev' program a while back. If I ever get some free 
time and feel like playing userspace again, I'll finish it along with a 
couple of other utilities. 

One of the things I fantasized about was making different bus 
'personalities' for it. So, you emulate lspci behavior with the same 
executable. And, extend it to other buses, so you could view all the 
devices of a particular bus type (and only those devices). 

This information will be known by devices as they are registered in the 
tree, and will be easy to export to userspace. So, one could do lspcmcia, 
lssbus, and *drum roll* lsisa. 

Now for the best part: instead of having to do lsdev -t pci etc, I was 
just going to hard link ls{$bus type} to lsdev and make it check what 
argv[0] was to decide how to behave. :) 

People hate that idea, and I admit it's twisted. But, there's something 
kinda cute about it. 

	-pat

Re: [linux-usb-devel] [PATCH] driverfs support for USB - take 2

[Stephen J. Gowdy]

Hi Greg,
	Not that it matters for this discussion, but I thought the USB2.0 
cards had two OHCI and one EHCI controllers?

							regards,

							Stephen.

On Tue, 29 Jan 2002, Greg KH wrote:

> Hi,
> 
> Well after determining that the last version of this patch doesn't show
> the USB tree properly, here's another patch against 2.5.3-pre6 that
> fixes this issue.
> 
> With this patch (the driver core changes were from Pat Mochel, thanks
> Pat for putting up with my endless questions) my machine now shows the
> following tree:
> 
> [greg@soap x]$ tree
> .
> `-- root
>     |-- pci0
>     |   |-- 00:00.0
>     |   |   |-- power
>     |   |   `-- status
>     |   |-- 00:01.0
>     |   |   |-- 02:00.0
>     |   |   |   |-- power
>     |   |   |   `-- status
>     |   |   |-- power
>     |   |   `-- status
>     |   |-- 00:1e.0
>     |   |   |-- 01:08.0
>     |   |   |   |-- power
>     |   |   |   `-- status
>     |   |   |-- 01:0d.0
>     |   |   |   |-- power
>     |   |   |   |-- status
>     |   |   |   `-- usb_bus
>     |   |   |       |-- power
>     |   |   |       `-- status
>     |   |   |-- 01:0d.1
>     |   |   |   |-- power
>     |   |   |   |-- status
>     |   |   |   `-- usb_bus
>     |   |   |       |-- 003
>     |   |   |       |   |-- power
>     |   |   |       |   `-- status
>     |   |   |       |-- power
>     |   |   |       `-- status
>     |   |   |-- 01:0d.2
>     |   |   |   |-- power
>     |   |   |   |-- status
>     |   |   |   `-- usb_bus
>     |   |   |       |-- power
>     |   |   |       `-- status
>     |   |   |-- power
>     |   |   `-- status
>     |   |-- 00:1f.0
>     |   |   |-- power
>     |   |   `-- status
>     |   |-- 00:1f.1
>     |   |   |-- power
>     |   |   `-- status
>     |   |-- 00:1f.2
>     |   |   |-- power
>     |   |   |-- status
>     |   |   `-- usb_bus
>     |   |       |-- 002
>     |   |       |   |-- 003
>     |   |       |   |   |-- power
>     |   |       |   |   `-- status
>     |   |       |   |-- power
>     |   |       |   `-- status
>     |   |       |-- power
>     |   |       `-- status
>     |   |-- 00:1f.3
>     |   |   |-- power
>     |   |   `-- status
>     |   |-- 00:1f.5
>     |   |   |-- power
>     |   |   `-- status
>     |   |-- power
>     |   `-- status
>     |-- power
>     `-- status
> 
> 
> I have 2 USB controllers in this system, 1 UHCI controller on the
> motherboard that shows up under PCI device 00:1f.2. There are two
> devices plugged into this bus, a hub, and a trackball plugged into the
> hub.  This topology is now shown properly.
> 
> The other controller is a USB 2.0 controller that shows up in 3 places
> on the pci bus: 01:0d.0, 01:0d.1, and 01:0d.2.  The 01:0d.1 device is
> controlled by the ohci-hcd driver, and a USB hub is plugged into it.
> The ehci-hcd driver controls the other 2 pci devices on the card.
> 
> Yes, I need to have better names for the devices than just "usb_bus",
> any suggestions?  These devices nodes are really the USB root hubs in
> the USB controller, so they could just have the USB number as the name
> like the other USB devices (001), but that's pretty boring :)
> 
> greg k-h
> 
> 
> 
> diff -Nru a/drivers/base/core.c b/drivers/base/core.c
> --- a/drivers/base/core.c	Tue Jan 29 16:02:26 2002
> +++ b/drivers/base/core.c	Tue Jan 29 16:02:26 2002
> @@ -17,10 +17,7 @@
>  # define DBG(x...)
>  #endif
>  
> -static struct iobus device_root = {
> -	bus_id: "root",
> -	name:	"Logical System Root",
> -};
> +static struct device * device_root;
>  
>  int (*platform_notify)(struct device * dev) = NULL;
>  int (*platform_notify_remove)(struct device * dev) = NULL;
> @@ -28,9 +25,6 @@
>  extern int device_make_dir(struct device * dev);
>  extern void device_remove_dir(struct device * dev);
>  
> -extern int iobus_make_dir(struct iobus * iobus);
> -extern void iobus_remove_dir(struct iobus * iobus);
> -
>  static spinlock_t device_lock;
>  
>  /**
> @@ -47,19 +41,25 @@
>  
>  	if (!dev || !strlen(dev->bus_id))
>  		return -EINVAL;
> -	BUG_ON(!dev->parent);
>  
>  	spin_lock(&device_lock);
>  	INIT_LIST_HEAD(&dev->node);
> +	INIT_LIST_HEAD(&dev->children);
>  	spin_lock_init(&dev->lock);
>  	atomic_set(&dev->refcount,2);
>  
> -	get_iobus(dev->parent);
> -	list_add_tail(&dev->node,&dev->parent->devices);
> +	if (dev != device_root) {
> +		struct device * parent;
> +		if (!dev->parent)
> +			dev->parent = device_root;
> +		parent = dev->parent;
> +		get_device(parent);
> +		list_add_tail(&dev->node,&parent->children);
> +	}
>  	spin_unlock(&device_lock);
>  
> -	DBG("DEV: registering device: ID = '%s', name = %s, parent = %s\n",
> -	    dev->bus_id, dev->name, parent->bus_id);
> +	DBG("DEV: registering device: ID = '%s', name = %s\n",
> +	    dev->bus_id, dev->name);
>  
>  	if ((error = device_make_dir(dev)))
>  		goto register_done;
> @@ -70,8 +70,8 @@
>  
>   register_done:
>  	put_device(dev);
> -	if (error)
> -		put_iobus(dev->parent);
> +	if (error && dev->parent)
> +		put_device(dev->parent);
>  	return error;
>  }
>  
> @@ -100,9 +100,6 @@
>  	/* remove the driverfs directory */
>  	device_remove_dir(dev);
>  
> -	if (dev->subordinate)
> -		iobus_remove_dir(dev->subordinate);
> -
>  	/* Notify the platform of the removal, in case they
>  	 * need to do anything...
>  	 */
> @@ -116,70 +113,18 @@
>  	if (dev->driver && dev->driver->remove)
>  		dev->driver->remove(dev,REMOVE_FREE_RESOURCES);
>  
> -	put_iobus(dev->parent);
> -}
> -
> -int iobus_register(struct iobus *bus)
> -{
> -	int error;
> -
> -	if (!bus || !strlen(bus->bus_id))
> -		return -EINVAL;
> -	
> -	spin_lock(&device_lock);
> -	atomic_set(&bus->refcount,2);
> -	spin_lock_init(&bus->lock);
> -	INIT_LIST_HEAD(&bus->node);
> -	INIT_LIST_HEAD(&bus->devices);
> -	INIT_LIST_HEAD(&bus->children);
> -
> -	if (bus != &device_root) {
> -		if (!bus->parent)
> -			bus->parent = &device_root;
> -		get_iobus(bus->parent);
> -		list_add_tail(&bus->node,&bus->parent->children);
> -	}
> -	spin_unlock(&device_lock);
> -
> -	DBG("DEV: registering bus. ID = '%s' name = '%s' parent = %p\n",
> -	    bus->bus_id,bus->name,bus->parent);
> -
> -	error = iobus_make_dir(bus);
> -
> -	put_iobus(bus);
> -	if (error && bus->parent)
> -		put_iobus(bus->parent);
> -	return error;
> -}
> -
> -/**
> - * iobus_unregister - remove bus and children from device tree
> - * @bus:	pointer to bus structure
> - *
> - * Remove device from parent's list of children and decrement
> - * reference count on controlling device. That should take care of
> - * the rest of the cleanup.
> - */
> -void put_iobus(struct iobus * iobus)
> -{
> -	if (!atomic_dec_and_lock(&iobus->refcount,&device_lock))
> -		return;
> -	list_del_init(&iobus->node);
> -	spin_unlock(&device_lock);
> -
> -	if (!list_empty(&iobus->devices) ||
> -	    !list_empty(&iobus->children))
> -		BUG();
> -
> -	put_iobus(iobus->parent);
> -	/* unregister itself */
> -	put_device(iobus->self);
> +	put_device(dev->parent);
>  }
>  
>  static int __init device_init_root(void)
>  {
> -	/* initialize parent bus lists */
> -	return iobus_register(&device_root);
> +	device_root = kmalloc(sizeof(*device_root),GFP_KERNEL);
> +	if (!device_root)
> +		return -ENOMEM;
> +	memset(device_root,0,sizeof(*device_root));
> +	strcpy(device_root->bus_id,"root");
> +	strcpy(device_root->name,"System Root");
> +	return device_register(device_root);
>  }
>  
>  int __init device_driver_init(void)
> @@ -208,5 +153,5 @@
>  }
>  
>  EXPORT_SYMBOL(device_register);
> -EXPORT_SYMBOL(iobus_register);
> +EXPORT_SYMBOL(put_device);
>  EXPORT_SYMBOL(device_driver_init);
> diff -Nru a/drivers/base/fs.c b/drivers/base/fs.c
> --- a/drivers/base/fs.c	Tue Jan 29 16:02:26 2002
> +++ b/drivers/base/fs.c	Tue Jan 29 16:02:26 2002
> @@ -102,28 +102,6 @@
>  		}
>  	}
>  	return 0;
> -}
> -
> -void iobus_remove_dir(struct iobus * iobus)
> -{
> -	if (iobus)
> -		driverfs_remove_dir(&iobus->dir);
> -}
> -
> -int iobus_make_dir(struct iobus * iobus)
> -{
> -	struct driver_dir_entry * parent = NULL;
> -	int error;
> -
> -	INIT_LIST_HEAD(&iobus->dir.files);
> -	iobus->dir.mode = (S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO);
> -	iobus->dir.name = iobus->bus_id;
> -
> -	if (iobus->parent)
> -		parent = &iobus->parent->dir;
> -
> -	error = driverfs_create_dir(&iobus->dir,parent);
> -	return error;
>  }
>  
>  EXPORT_SYMBOL(device_create_file);
> diff -Nru a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c	Tue Jan 29 16:02:26 2002
> +++ b/drivers/pci/pci.c	Tue Jan 29 16:02:26 2002
> @@ -1086,13 +1086,7 @@
>  	child->parent = parent;
>  	child->ops = parent->ops;
>  	child->sysdata = parent->sysdata;
> -
> -	/* init generic fields */
> -	child->iobus.self = &dev->dev;
> -	child->iobus.parent = &parent->iobus;
> -	dev->dev.subordinate = &child->iobus;
> -
> -	strcpy(child->iobus.name,dev->dev.name);
> +	child->dev = &dev->dev;
>  
>  	/*
>  	 * Set up the primary, secondary and subordinate
> @@ -1361,16 +1355,11 @@
>  	DBG("Scanning bus %02x\n", bus->number);
>  	max = bus->secondary;
>  
> -	/* we should know for sure what the bus number is, so set the bus ID
> -	 * for the bus and make sure it's registered in the device tree */
> -	sprintf(bus->iobus.bus_id,"pci%d",bus->number);
> -	iobus_register(&bus->iobus);
> -
>  	/* Create a device template */
>  	memset(&dev0, 0, sizeof(dev0));
>  	dev0.bus = bus;
>  	dev0.sysdata = bus->sysdata;
> -	dev0.dev.parent = &bus->iobus;
> +	dev0.dev.parent = bus->dev;
>  	dev0.dev.driver = &pci_device_driver;
>  
>  	/* Go find them, Rover! */
> @@ -1430,9 +1419,11 @@
>  		return NULL;
>  	list_add_tail(&b->node, &pci_root_buses);
>  
> -	sprintf(b->iobus.bus_id,"pci%d",bus);
> -	strcpy(b->iobus.name,"Host/PCI Bridge");
> -	iobus_register(&b->iobus);
> +	b->dev = kmalloc(sizeof(*(b->dev)),GFP_KERNEL);
> +	memset(b->dev,0,sizeof(*(b->dev)));
> +	sprintf(b->dev->bus_id,"pci%d",bus);
> +	strcpy(b->dev->name,"Host/PCI Bridge");
> +	device_register(b->dev);
>  
>  	b->number = b->secondary = bus;
>  	b->resource[0] = &ioport_resource;
> diff -Nru a/drivers/usb/hcd/ehci-hcd.c b/drivers/usb/hcd/ehci-hcd.c
> --- a/drivers/usb/hcd/ehci-hcd.c	Tue Jan 29 16:02:26 2002
> +++ b/drivers/usb/hcd/ehci-hcd.c	Tue Jan 29 16:02:26 2002
> @@ -290,6 +290,12 @@
>  		goto done2;
>  	}
>  
> +	/* hook up the root hub to the pci controller in the device tree */
> +	udev->dev.parent = &ehci->hcd.pdev->dev;
> +	strcpy (udev->dev.name, "usb_name");
> +	strcpy (udev->dev.bus_id, "usb_bus");
> +	device_register (&udev->dev);
> +
>  	return 0;
>  }
>  
> diff -Nru a/drivers/usb/hcd/ohci-hcd.c b/drivers/usb/hcd/ohci-hcd.c
> --- a/drivers/usb/hcd/ohci-hcd.c	Tue Jan 29 16:02:26 2002
> +++ b/drivers/usb/hcd/ohci-hcd.c	Tue Jan 29 16:02:26 2002
> @@ -475,7 +475,13 @@
>  // FIXME cleanup
>  		return -ENODEV;
>  	}
> -	
> +
> +	/* hook up the root hub to the pci controller in the device tree */
> +	udev->dev.parent = &ohci->hcd.pdev->dev;
> +	strcpy (udev->dev.name, "usb_name");
> +	strcpy (udev->dev.bus_id, "usb_bus");
> +	device_register (&udev->dev);
> +
>  	return 0;
>  }
>  
> diff -Nru a/drivers/usb/hub.c b/drivers/usb/hub.c
> --- a/drivers/usb/hub.c	Tue Jan 29 16:02:27 2002
> +++ b/drivers/usb/hub.c	Tue Jan 29 16:02:27 2002
> @@ -722,8 +722,16 @@
>  			dev->bus->busnum, dev->devpath, dev->devnum);
>  
>  		/* Run it through the hoops (find a driver, etc) */
> -		if (!usb_new_device(dev))
> +		if (!usb_new_device(dev)) {
> +			/* put the device in the global device tree */
> +			dev->dev.parent = &dev->parent->dev;
> +			sprintf (&dev->dev.name[0], "USB device %04x:%04x",
> +				 dev->descriptor.idVendor,
> +				 dev->descriptor.idProduct);
> +			sprintf (&dev->dev.bus_id[0], "%03d", dev->devnum);
> +			device_register (&dev->dev);
>  			goto done;
> +		}
>  
>  		/* Free the configuration if there was an error */
>  		usb_free_dev(dev);
> diff -Nru a/drivers/usb/uhci.c b/drivers/usb/uhci.c
> --- a/drivers/usb/uhci.c	Tue Jan 29 16:02:26 2002
> +++ b/drivers/usb/uhci.c	Tue Jan 29 16:02:26 2002
> @@ -2848,6 +2848,12 @@
>  		goto err_start_root_hub;
>  	}
>  
> +	/* hook the root hub up to the pci controller in the device tree */
> +	uhci->rh.dev->dev.parent = &dev->dev;
> +	strcpy (uhci->rh.dev->dev.name, "usb_name");
> +	strcpy (uhci->rh.dev->dev.bus_id, "usb_bus");
> +	device_register (&uhci->rh.dev->dev);
> +
>  	return 0;
>  
>  /*
> diff -Nru a/drivers/usb/usb-ohci.c b/drivers/usb/usb-ohci.c
> --- a/drivers/usb/usb-ohci.c	Tue Jan 29 16:02:26 2002
> +++ b/drivers/usb/usb-ohci.c	Tue Jan 29 16:02:26 2002
> @@ -2508,6 +2508,13 @@
>  #ifdef	DEBUG
>  	ohci_dump (ohci, 1);
>  #endif
> +
> +	/* hook the root hub up to the pci controller in the device tree */
> +	ohci->bus->root_hub->dev.parent = &dev->dev;
> +	strcpy (ohci->bus->root_hub->dev.name, "usb_name");
> +	strcpy (ohci->bus->root_hub->dev.bus_id, "usb_bus");
> +	device_register (&ohci->bus->root_hub->dev);
> +
>  	return 0;
>  }
>  
> diff -Nru a/drivers/usb/usb-uhci.c b/drivers/usb/usb-uhci.c
> --- a/drivers/usb/usb-uhci.c	Tue Jan 29 16:02:26 2002
> +++ b/drivers/usb/usb-uhci.c	Tue Jan 29 16:02:26 2002
> @@ -3028,6 +3028,12 @@
>  	pci_set_drvdata(dev, s);
>  	devs=s;
>  
> +	/* hook the root hub up to the pci controller in the device tree */
> +	s->bus->root_hub->dev.parent = &dev->dev;
> +	strcpy (s->bus->root_hub->dev.name, "usb_name");
> +	strcpy (s->bus->root_hub->dev.bus_id, "usb_bus");
> +	device_register (&s->bus->root_hub->dev);
> +
>  	return 0;
>  }
>  
> diff -Nru a/drivers/usb/usb.c b/drivers/usb/usb.c
> --- a/drivers/usb/usb.c	Tue Jan 29 16:02:27 2002
> +++ b/drivers/usb/usb.c	Tue Jan 29 16:02:27 2002
> @@ -1925,6 +1951,7 @@
>  	if (dev->devnum > 0) {
>  		clear_bit(dev->devnum, &dev->bus->devmap.devicemap);
>  		usbfs_remove_device(dev);
> +		put_device(&dev->dev);
>  	}
>  
>  	/* Free up the device itself */
> diff -Nru a/include/linux/device.h b/include/linux/device.h
> --- a/include/linux/device.h	Tue Jan 29 16:02:26 2002
> +++ b/include/linux/device.h	Tue Jan 29 16:02:26 2002
> @@ -66,10 +66,8 @@
>  
>  struct device {
>  	struct list_head node;		/* node in sibling list */
> -	struct iobus	*parent;	/* parent bus */
> -
> -	struct iobus	*subordinate;	/* only valid if this device is a
> -					   bridge device */
> +	struct list_head children;
> +	struct device 	*parent;
>  
>  	char	name[DEVICE_NAME_SIZE];	/* descriptive ascii string */
>  	char	bus_id[BUS_ID_SIZE];	/* position on parent bus */
> diff -Nru a/include/linux/pci.h b/include/linux/pci.h
> --- a/include/linux/pci.h	Tue Jan 29 16:02:26 2002
> +++ b/include/linux/pci.h	Tue Jan 29 16:02:26 2002
> @@ -432,8 +432,7 @@
>  	unsigned char	productver;	/* product version */
>  	unsigned char	checksum;	/* if zero - checksum passed */
>  	unsigned char	pad1;
> -
> -	struct iobus	iobus;		/* Generic device interface */
> +	struct	device	* dev;
>  };
>  
>  #define pci_bus_b(n) list_entry(n, struct pci_bus, node)
> diff -Nru a/include/linux/usb.h b/include/linux/usb.h
> --- a/include/linux/usb.h	Tue Jan 29 16:02:26 2002
> +++ b/include/linux/usb.h	Tue Jan 29 16:02:26 2002
> @@ -1,6 +1,8 @@
>  #ifndef __LINUX_USB_H
>  #define __LINUX_USB_H
>  
> +#include <linux/device.h>
> +
>  /* USB constants */
>  
>  /*
> @@ -1037,6 +1042,8 @@
>  
>  	struct usb_device *parent;
>  	struct usb_bus *bus;		/* Bus we're part of */
> +
> +	struct device dev;		/* Generic device interface */
>  
>  	struct usb_device_descriptor descriptor;/* Descriptor */
>  	struct usb_config_descriptor *config;	/* All of the configs */
> 
> _______________________________________________
> linux-usb-devel@lists.sourceforge.net
> To unsubscribe, use the last form field at:
> https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
> 

-- 
 /------------------------------------+-------------------------\
|Stephen J. Gowdy                     | SLAC, MailStop 17,       |
|http://www.slac.stanford.edu/~gowdy/ | 2575 Sand Hill Road,     |
|                                     | Menlo Park CA 94025, USA |
|EMail: gowdy@slac.stanford.edu       | Tel: +1 650 926 3144     |
 \------------------------------------+-------------------------/

Re: [linux-usb-devel] Re: [PATCH] driverfs support for USB - take 2

[David Brownell]

> > >  > Yes, I need to have better names for the devices than just "usb_bus",
> > >  > any suggestions?  These devices nodes are really the USB root hubs in
> > >  > the USB controller, so they could just have the USB number as the name
> > >  > like the other USB devices (001), but that's pretty boring :)

Actually one of my criticisms of Greg's patch is that
it hides the actual device tree.   The root hub is easily
distinguishable, it's the topmost one in the tree!  There
should be no need to name it specially.

I'd really rather move away from the model which
exposes a USB bus as a flat non-hierarchical
setup, and move instead to a model reflects the
actual topology of the USB devices and hubs.


> > >  "usb_root0" .. "usb_rootN" ?
> > 
> > Hm, that's a good idea, it would match the usbfs bus numbers which
> > should keep people happy.
> 
> Would it be usb_rootN or usb_busN?

I'd rather see neither, and have the device names reflect
physical topology ... so they could make sense to users.

For example, if you plug a USB device into a particular
USB socket it would have a particular name, and that
name would show up in diagnostics.  So when something
goes flakey about the device, the diagnostic will be able
to completely identify it.  And likewise, when userspace
tools need to do something, they should be able to use
the same pathname each time, unless the devices got
re-cabled ... re-enumerating shouldn't affect those names.

The notion of a "bus number" is bothersome, since it's
a function only of the order of driver initialization, and that
isn't a "stable" way to identify anything.  Re-ordering
driver initialization shouldn't change device name.

- Dave

Re: [PATCH] driverfs support for USB - take 2

[Greg KH]

On Tue, Jan 29, 2002 at 06:15:13PM -0800, David Brownell wrote:
> > > >  > Yes, I need to have better names for the devices than just "usb_bus",
> > > >  > any suggestions?  These devices nodes are really the USB root hubs in
> > > >  > the USB controller, so they could just have the USB number as the name
> > > >  > like the other USB devices (001), but that's pretty boring :)
> 
> Actually one of my criticisms of Greg's patch is that
> it hides the actual device tree.   The root hub is easily
> distinguishable, it's the topmost one in the tree!  There
> should be no need to name it specially.
> 
> I'd really rather move away from the model which
> exposes a USB bus as a flat non-hierarchical
> setup, and move instead to a model reflects the
> actual topology of the USB devices and hubs.

<good description of what the topology tree should show snipped>

I completly agree.  My main concern with these first patches, was
getting the driverfs - usb subsystem linkage working properly.  I wasn't
trying to work on the names yet (you should have seen some of my testing
patches, the names there were pretty horrible :)

As we wait for the rest of Pat's patches to be merged into the kernel,
we can work on nailing down the proper naming scheme for the USB trees.

However, the root hub name _does_ propose a problem.  I feel we have two
solutions:
	- use the bus number (usb_bus_00x)
	  Pros:
	  	matches the usbfs naming and directory structure
	  Cons:
	  	depends on the initialization order of the busses.

	- use a generic name like I did (usb_bus)
	  Pros:
	  	does not depend on the init order, and relies on the
		location in the entire pci topology tree to show its
		uniqueness.
	  Cons:
	  	boring :)

And also remember, the status file in a device's directory also provides
a _lot_ of information.  We haven't even started to fill up the fields
there...

thanks,

greg k-h

Re: [PATCH] driverfs support for USB - take 2

[Greg KH]

On Tue, Jan 29, 2002 at 05:49:36PM -0800, Stephen J. Gowdy wrote:
> Hi Greg,
> 	Not that it matters for this discussion, but I thought the USB2.0 
> cards had two OHCI and one EHCI controllers?

I think you're right.

greg k-h

Re: [PATCH] driverfs support for USB - take 2

[Patrick Mochel]

On Tue, 29 Jan 2002, Greg KH wrote:

> Hi,
> 
> Well after determining that the last version of this patch doesn't show
> the USB tree properly, here's another patch against 2.5.3-pre6 that
> fixes this issue.
> 
> With this patch (the driver core changes were from Pat Mochel, thanks
> Pat for putting up with my endless questions) my machine now shows the
> following tree:

This is great! Thanks to Greg for testing and providing feedback. 


One question:

>     |   |   |-- 01:0d.1
>     |   |   |   |-- power
>     |   |   |   |-- status
>     |   |   |   `-- usb_bus
>     |   |   |       |-- 003
>     |   |   |       |   |-- power
>     |   |   |       |   `-- status
>     |   |   |       |-- power
>     |   |   |       `-- status

You have a PCI device that is the USB controller. You create a child of 
that represents the USB bus. Then, devices are added as children of that.

Logically, couldn't you skip that extra layer of indirection and make USB 
devices children of the USB controller? Or, do you see benefit in the 
explicit distinction?



> diff -Nru a/include/linux/device.h b/include/linux/device.h
> --- a/include/linux/device.h	Tue Jan 29 16:02:26 2002
> +++ b/include/linux/device.h	Tue Jan 29 16:02:26 2002
> @@ -66,10 +66,8 @@
>  
>  struct device {
>  	struct list_head node;		/* node in sibling list */
> -	struct iobus	*parent;	/* parent bus */
> -
> -	struct iobus	*subordinate;	/* only valid if this device is a
> -					   bridge device */
> +	struct list_head children;
> +	struct device 	*parent;

(To everyone else)

Note this change: This patch removed struct iobus, so that everything 
becomes a device and only a device. A 'bus' is simply a device that has 
children. 

This concept is something that I have argued both for and against since I 
started on this. Initially, the goal was to separate the two because they 
followed such different semantics. 

But, I've found it, IMO, it creates more problems than it solves. There 
was/is a lot of code replication just because you want to do the same 
thing to each object, but have two different pointer types to deal with. 

And, it's quite easy to add extra semantics for devices that have 
children. 

	-pat

Re: [linux-usb-devel] Re: [PATCH] driverfs support for USB - take 2

[David Brownell]

"> " ==  "Greg KH" <greg@kroah.com>

> I completly agree.  

As I see -- some mailer daemons were adding delays and
fomenting confusion!


> However, the root hub name _does_ propose a problem.  I feel we have two
> solutions:
> - use the bus number (usb_bus_00x)
>   Pros:
>   matches the usbfs naming and directory structure
>   Cons:
>   depends on the initialization order of the busses.

At this point, I'd propose that "driverfs" should adopt a policy
that naming dependencies on initialization order are a virus
that should be obliterated to the degree it's possible ... which
in this case is completely!  :)


> - use a generic name like I did (usb_bus)
>   Pros:
>   does not depend on the init order, and relies on the
> location in the entire pci topology tree to show its
> uniqueness.
>   Cons:
>   boring :)

Or a third option is to get rid of the extra level of indirection
in the current driverfs naming policy, as Patrick suggested
in a separate email.  I'll follow up separately.


> And also remember, the status file in a device's directory also provides
> a _lot_ of information.  We haven't even started to fill up the fields
> there...

And there can be a lot more such files.  Though that 4KB limit
may become an issue at some point.

What sort of USB information were you thinking should show
up?  Current configuration and altsetting?  Power consumption
for hubs (not that we budget that yet :)?  There really aren't any
examples of this in the kernel yet.

Also, one could argue that each USB function ("interface")
should be presented as an individual device, just like each PCI
function is handled ... after all, USB drivers bind to interfaces,
not devices, and this is the "driver" FS!  :)

- Dave

Re: [linux-usb-devel] Re: [PATCH] driverfs support for USB - take 2

[David Brownell]

"> " == "Patrick Mochel" <mochel@osdl.org>

> You have a PCI device that is the USB controller. You create a child of 
> that represents the USB bus. Then, devices are added as children of that.
> 
> Logically, couldn't you skip that extra layer of indirection and make USB 
> devices children of the USB controller? Or, do you see benefit in the 
> explicit distinction?

Since I don't see a benefit from that extra indirection, I was going to ask
almost that same question ... :)

But it's broader than that:  Why shouldn't that apply to _every_ kind
of bridge, not just USB controllers ("PCI-to-USB bridges")?

For example, with PCI why should there ever be "pci0" directories,
with children "00:??.?" and "pci1"?


>    ... A 'bus' is simply a device that has children. 
>
> This concept is something that I have argued both for and against since I 
> started on this. Initially, the goal was to separate the two because they 
> followed such different semantics. 
> 
> But, I've found it, IMO, it creates more problems than it solves. ...

If the model that driverfs exposes is that directories denote devices
(of any type including bridge) and (text) files expose device properties,
then my initial suspicion is that the additional level of indirection would
not be necessary.  Maybe some "type" property would be desirable
though, to better separate namespace structure from typing issues.
(It might need to be provided by the device/bridge driver.)

When I've seen corresponding problems in other areas, the extra
indirection has not been necessary.  The simpler naming policies
have been a lot easier to work with.

I suspect it'd be better to simplify the naming policy for bridges and
busses everywhere in driverfs, not just for USB, but I'd like to know
any arguments for keeping that extra level of indirection.

- Dave

Re: [PATCH] driverfs support for USB - take 2

[Pavel Machek]

Hi!

>  static int __init device_init_root(void)
>  {
> -	/* initialize parent bus lists */
> -	return iobus_register(&device_root);
> +	device_root = kmalloc(sizeof(*device_root),GFP_KERNEL);
> +	if (!device_root)
> +		return -ENOMEM;
> +	memset(device_root,0,sizeof(*device_root));
> +	strcpy(device_root->bus_id,"root");
> +	strcpy(device_root->name,"System Root");
> +	return device_register(device_root);
>  }

Why don't you leave device_root allocated statically?

> @@ -1430,9 +1419,11 @@
>  		return NULL;
>  	list_add_tail(&b->node, &pci_root_buses);
>  
> -	sprintf(b->iobus.bus_id,"pci%d",bus);
> -	strcpy(b->iobus.name,"Host/PCI Bridge");
> -	iobus_register(&b->iobus);
> +	b->dev = kmalloc(sizeof(*(b->dev)),GFP_KERNEL);
Uff...				~~~~~~~~~ would not "struct device" (or
what should it be) look better?

> +	memset(b->dev,0,sizeof(*(b->dev)));

								Pavel
-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Re: [PATCH] driverfs support for USB - take 2

[Horst von Brand]

Pavel Machek <pavel@suse.cz> said:

[...]

> > -	iobus_register(&b->iobus);
> > +	b->dev = kmalloc(sizeof(*(b->dev)),GFP_KERNEL);
> Uff...				~~~~~~~~~ would not "struct device" (or
> what should it be) look better?

Yep, but there is _no_ chance to screw up ("or what should it be" ;-) this
way.
-- 
Horst von Brand			     http://counter.li.org # 22616

Re: [linux-usb-devel] Re: [PATCH] driverfs support for USB - take 2

[Greg KH]

On Wed, Jan 30, 2002 at 12:07:26PM -0800, David Brownell wrote:
> > And also remember, the status file in a device's directory also provides
> > a _lot_ of information.  We haven't even started to fill up the fields
> > there...
> 
> And there can be a lot more such files.  Though that 4KB limit
> may become an issue at some point.

I doubt it, we are talking one value per file here.  I can't see any USB
driver wanting to go over 4Kb for 1 value (and if it does, I'll change
it :)

> What sort of USB information were you thinking should show
> up?  Current configuration and altsetting?  Power consumption
> for hubs (not that we budget that yet :)?  There really aren't any
> examples of this in the kernel yet.

Bandwidth is a good one.
Current config and altsetting might be useful.
I haven't really thought about the different files yet.

> Also, one could argue that each USB function ("interface")
> should be presented as an individual device, just like each PCI
> function is handled ... after all, USB drivers bind to interfaces,
> not devices, and this is the "driver" FS!  :)

No, I'll say that we need to stay one physical device per device in the
tree.  If you want to do an interface tree, let's put that in usbfs,
where it belongs :)

thanks,

greg k-h

Re: [linux-usb-devel] Re: [PATCH] driverfs support for USB - take 2

[Greg KH]

On Wed, Jan 30, 2002 at 12:24:13PM -0800, David Brownell wrote:
> "> " == "Patrick Mochel" <mochel@osdl.org>
> 
> > You have a PCI device that is the USB controller. You create a child of 
> > that represents the USB bus. Then, devices are added as children of that.
> > 
> > Logically, couldn't you skip that extra layer of indirection and make USB 
> > devices children of the USB controller? Or, do you see benefit in the 
> > explicit distinction?
> 
> Since I don't see a benefit from that extra indirection, I was going to ask
> almost that same question ... :)

But that device _is_ a USB device, it's a root hub.  It has bandwidth
allocation, strings, a configuration, and other stuff.  It operates a
bit differently from other hubs, but it's pretty close to the real
thing.

> But it's broader than that:  Why shouldn't that apply to _every_ kind
> of bridge, not just USB controllers ("PCI-to-USB bridges")?
> 
> For example, with PCI why should there ever be "pci0" directories,
> with children "00:??.?" and "pci1"?

It's information that is useful to the user.  If presented with a tree
that doesn't have the pci? and usb directories, it just looks like a
random tree of different numbers.  If we did that, we would really need
a lsdevice program just to determine what the different devices are
easily :)

I want to be able to easily see where the pci root buses, usb root
buses, ieee1394 root buses, etc. are in the system, by just looking at
the tree.

I'll play around more with the naming scheme next week and post the
results.

thanks,

greg k-h

Re: [linux-usb-devel] Re: [PATCH] driverfs support for USB - take 2

[David Brownell]

> > And there can be a lot more such files.  Though that 4KB limit
> > may become an issue at some point.
> 
> I doubt it, we are talking one value per file here.  I can't see any USB
> driver wanting to go over 4Kb for 1 value (and if it does, I'll change
> it :)

I could see descriptors getting that large, particularly if
they're turned from binary form into text.  I seem to recall
Patrick was anticipating troubles with that 4K limit, at
some point.


> > Also, one could argue that each USB function ("interface")
> > should be presented as an individual device, just like each PCI
> > function is handled ... after all, USB drivers bind to interfaces,
> > not devices, and this is the "driver" FS!  :)
> 
> No, I'll say that we need to stay one physical device per device in the
> tree. 

But we aren't that way today.  Examples:

    - Take a multifunction PCI card ("physical device") and plug it in.
      Each function shows up as another "logical device" in the tree.
      Each such logical device gets one driver.

    - Take a composite USB device ("physical device", like a keyboard
      with hub) and plug it in.  Each logical device shows up separately.
      Each such logical device gets one driver.

The issue with USB is that it's got a much more complex configuration
model, not all of which is well supported yet in Linux.   There's a type
of device which is handled inconsistently:

    - Take a multiple-interface USB device ("physical device") and
      plug it in.  It's presented as one logical device.  BUT (!!) such
      devices need MULTIPLE drivers!!  (Example:  speaker with
      built in volume control, needs audio and HID drivers.)

I was observing that we have a chance to make things consistent.
In my experience, that's normally a good thing.  Similarly, I think USB
should handle configurations more as first class entities.  Changing
a device's config doesn't trigger driver rebinding, for example; we're
in luck, so far, that most devices don't have many configurations.


>     If you want to do an interface tree, let's put that in usbfs,
> where it belongs :)

Ah, but changing usbfs is impractical at this point since lots of
userspace programs rely on it not changing.  Which is why I
was pointing this out in the context of driverfs, which can still
be improved in such ways ... "usbdevfs" was always advertised
as "preliminary", anyway! :)

- Dave

Re: [linux-usb-devel] Re: [PATCH] driverfs support for USB - take 2

[David Brownell]

> > > You have a PCI device that is the USB controller. You create a child of 
> > > that represents the USB bus. Then, devices are added as children of that.
> > > 
> > > Logically, couldn't you skip that extra layer of indirection and make USB 
> > > devices children of the USB controller? Or, do you see benefit in the 
> > > explicit distinction?
> > 
> > Since I don't see a benefit from that extra indirection, I was going to ask
> > almost that same question ... :)
> 
> But that device _is_ a USB device, it's a root hub.  It has bandwidth
> allocation, strings, a configuration, and other stuff.  It operates a
> bit differently from other hubs, but it's pretty close to the real
> thing.

Sure it's a hub (with the firmware provided by Linux :) but it's also a
PCI device.  The extra indirection is the one which makes the root
hub be a different device than the PCI device.  It'll have some
attributes that relate to its PCI device role, others that relate to
the more specialized "USB root hub" role.


> > But it's broader than that:  Why shouldn't that apply to _every_ kind
> > of bridge, not just USB controllers ("PCI-to-USB bridges")?
> > 
> > For example, with PCI why should there ever be "pci0" directories,
> > with children "00:??.?" and "pci1"?
> 
> It's information that is useful to the user.

Not useful to this user ... and I'm unclear why it'd be useful to
any others.  Initialization order can change, and embedding
it in device naming is basically just trouble.


>      If presented with a tree
> that doesn't have the pci? and usb directories, it just looks like a
> random tree of different numbers.

Just like most directories.  That's why I commented that it might
be desirable to have any typing information just show up as one
of the directory attributes.

- Dave

Re: [PATCH] driverfs support for USB - take 2

[Greg KH]

On Sat, Feb 02, 2002 at 11:13:26AM -0800, David Brownell wrote:
> > No, I'll say that we need to stay one physical device per device in the
> > tree. 
> 
> But we aren't that way today.  Examples:

<snip>

Ok, you're right.  We want to tell the drivers to shut down (remember,
the original goal of driverfs was for power management), so all drivers
that attach to a device need to be shown.

I'll play with the code some more and make this kind of change.

> >     If you want to do an interface tree, let's put that in usbfs,
> > where it belongs :)
> 
> Ah, but changing usbfs is impractical at this point since lots of
> userspace programs rely on it not changing.  Which is why I
> was pointing this out in the context of driverfs, which can still
> be improved in such ways ... "usbdevfs" was always advertised
> as "preliminary", anyway! :)

Heh, I took the "preliminary" tag off of it a short while ago, as so
many different userspace programs were using it.  Maybe usbfs2? :)

Seriously, I've had some ideas of a different way to implement the
functionality of usbfs, possibly without all of the ioctl calls, but I
have not had the time to experiment with it...

thanks,

greg k-h