pci_domain_nr vs. /sys/devices

The new pci_domain_nr() is good for adding the PCI domain number to the /sys/devices/pciN/* names, but I think that's not the proper representation. It should really be

/sys/devices/pci-domainN/pciN/*

So we can pave the way for when we'll stop play bus number tricks and actually have overlapping PCI bus numbers between domains. (I don't plan to do that immediately because that would break userland & /proc/bus/pci backward compatiblity)

What do you think ?

Ben.

I don't think sysfs works like that (please correct me if I've misunderstood, mochel..)

Look in /sys/bus/pci/devices/ There you have all the PCI devices lumped together in one place, and we obviously need the domain number in the name. I don't know where the 0 on the end of /sys/devices/pci0/ comes from, but if we could, I wouldn't say no to:

/sys/devices/pciDDDD/DDDD:BB:SS.F
or
/sys/devices/pciDDDD:BB/DDDD:BB:SS.F
(Domain,Bus,Slot,Func)

I don't think the extra level of hierarchy in your suggestion is necessary or particularly desirable.

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Looks like nobody really understands it then ;)

Nah, you are mixing up /sys/bus/* which is a flat list of busses in the machine, with /sys/devices/* which is the hierarchical device tree.

It's probably not, then it's a matter of properly renaming the pciN entries in /sys/devices to be /sys/devices/pciDD:NN where DD is the domain number and NN is the first bus on this domain, or just pciDD (though I like having the bus number there as well)

Ben.

Well, it keeps changing ;-)

I'm not mixing them up, I'm just saying that the domain number has to be part of the leafname.

Yep. Can you find where this happens, because it's not in pci-sysfs.c where one might logically expect it to be ...

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

See pci_alloc_primary_bus_parented() in drivers/pci/probe.c. The '0' is the bus number passed in. So, the names include the pci bus numbers of the root buses.

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

This is the right solution imho, yes. Adding more indirection with pci-domain isn't useful.

Now, we should also fix pci_setup_device to make this naming generic to the entire kernel don't you think ? This won't affect /proc/bus/pci as it doesn't use the slot_name field in pci_dev, but at least it will make naming consistent.

(That also mean increasing slot_name size in pci.h)

Ben.

Agreed. I did that in my patch since its important to be able to uniquely identify a device:

PCI: Enabling device: (0000:21:01.0), cmd 143
PCI: Enabling bus mastering for device 0000:21:01.0
e100: selftest OK.
...

We can do that, in two different ways. As PCI domains are discovered, devices or kobjects can be registered that represent their existence. Depending on what their parent is, they can be added anywhere into the hierarchy.

It might be good to do this anyway, since it will give us the ability to keep a list of all PCI domains in the system, assuming that's desired. :)

We could do that (as well as what is described above). However, it's important to note that this exposes a limitation of the driver model. We have a flat name space in /sys/bus/<bus>/devices/, which is very handy, and nearly free, since each bus maintains a list of all devices anyway. However, since it's a flat listing, the names must be unique across all instances of that bus.

Fine, but the names are currently taken directly from dev->bus_id, meaning that the local directories for the devices must also be unique across the entire bus namespace. This is certainly not true, and something that Linus has complained about before. The names in the local directories only need to be unique in the local namespace.

Unfortunately, the symlinks are created in the core, and the core doesn't know about bus/device organization. A possilbe solution would be to represent bus instances in the core, and have the symlink names created from a concatenation of the bus name (e.g. DD:BB) and the local device ID (SS.F). This would reduce pressure on bus_id size, and make things easier to read..

-pat

I like it. I think we do need the bus number in the top level since we could have multiple host bridges on the same domain. I put a quick patch together, it lays things out as such:

/sys/devices/pci0002:00/0002:00:02.2

It also adds the domain to /proc/pci (are there userspace tools that parse this directly?):

Domain 2, Bus 0, device 2, function 2:

And only creates /proc/bus/pci entries for the first domain. I was going to extend it one level to encode the domain but I now think we should just move that functionality into sysfs and be done with it. Willy, you had a patch that exposed BARS etc in sysfs didnt you? X and lspci etc will need updating to match, but they are currently broken.

I chose to add the domain into dev->slot_name since its needed for matching kernel messages to drivers. Im wondering if we should make this conditional on pci domain support since it does add some noise for those who couldnt care less about domains.

Finally there was some shuffling required to make pci_bus_exists work (passing in a pci_bus *, ->sysdata and ->number must be initialised before calling it). There are some uses of pci_bus_exists in x86 that will need updating.

Thoungts?

Anton

===== drivers/pci/probe.c 1.43 vs edited =====

@@ -400,8 +400,8 @@
 {
 	u32 class;
 
-	sprintf(dev->slot_name, "%02x:%02x.%d", dev->bus->number,
-		PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
+	sprintf(dev->slot_name, "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus),
+		dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
 	sprintf(dev->dev.name, "PCI device %04x:%04x",
 		dev->vendor, dev->device);
 

@@ -529,8 +529,7 @@
 	pci_name_device(dev);
 
 	/* now put in global tree */
-	sprintf(dev->dev.bus_id, "%04x:%s", pci_domain_nr(bus),
-			dev->slot_name);
+	strcpy(dev->dev.bus_id,dev->slot_name);
 	dev->dev.dma_mask = &dev->dma_mask;
 
 	return dev;

@@ -632,56 +636,60 @@
 	return max;
 }
 
-int __devinit pci_bus_exists(const struct list_head *list, int nr)
+int __devinit pci_bus_exists(const struct list_head *list, struct pci_bus *bus)
 {
-	const struct pci_bus *b;
+	struct pci_bus *b;
 
 	list_for_each_entry(b, list, node) {
-		if (b->number == nr || pci_bus_exists(&b->children, nr))
+		if (((b->number == bus->number) &&
+				pci_domain_nr(b) == pci_domain_nr(bus)) || 
+			pci_bus_exists(&b->children, bus))
 			return 1;
 	}
 	return 0;
 }
 
-static struct pci_bus * __devinit pci_alloc_primary_bus_parented(struct device *parent, int bus)
+static struct pci_bus * __devinit pci_alloc_primary_bus_parented(struct device *parent, int bus, void *sysdata)
 {
 	struct pci_bus *b;
 
-	if (pci_bus_exists(&pci_root_buses, bus)) {
+	b = pci_alloc_bus();
+	if (!b)
+		return NULL;
+
+	b->sysdata = sysdata;
+	b->number = b->secondary = bus;
+	b->resource[0] = &ioport_resource;
+	b->resource[1] = &iomem_resource;
+
+	if (pci_bus_exists(&pci_root_buses, b)) {
 		/* If we already got to this bus through a different bridge, ignore it */
 		DBG("PCI: Bus %02x already known\n", bus);
+		kfree(b);
 		return NULL;
 	}
 
-	b = pci_alloc_bus();
-	if (!b)
-		return NULL;
-	
 	b->dev = kmalloc(sizeof(*(b->dev)),GFP_KERNEL);
 	if (!b->dev){
 		kfree(b);
 		return NULL;
 	}
-	
+
 	list_add_tail(&b->node, &pci_root_buses);
 
 	memset(b->dev,0,sizeof(*(b->dev)));
-	sprintf(b->dev->bus_id,"pci%d",bus);
+	sprintf(b->dev->bus_id, "pci%04x:%02x", pci_domain_nr(b), bus);
 	strcpy(b->dev->name,"Host/PCI Bridge");
 	b->dev->parent = parent;
 	device_register(b->dev);
 
-	b->number = b->secondary = bus;
-	b->resource[0] = &ioport_resource;
-	b->resource[1] = &iomem_resource;
 	return b;
 }
 
 struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent, int bus, struct pci_ops *ops, void *sysdata)
 {
-	struct pci_bus *b = pci_alloc_primary_bus_parented(parent, bus);
+	struct pci_bus *b = pci_alloc_primary_bus_parented(parent, bus, sysdata);
 	if (b) {
-		b->sysdata = sysdata;
 		b->ops = ops;
 		b->subordinate = pci_scan_child_bus(b);
 		pci_bus_add_devices(b);

===== drivers/pci/proc.c 1.29 vs edited =====

@@ -382,6 +382,10 @@
 	if (!proc_initialized)
 		return -EACCES;
 
+	/* Backwards compatibility for domain 0 only */
+	if (pci_domain_nr(dev->bus) != 0)
+		return 0;
+
 	if (!(de = bus->procdir)) {
 		sprintf(name, "%02x", bus->number);
 		de = bus->procdir = proc_mkdir(name, proc_bus_pci_dir);

@@ -470,8 +474,9 @@
 	pci_read_config_byte (dev, PCI_LATENCY_TIMER, &latency);
 	pci_read_config_byte (dev, PCI_MIN_GNT, &min_gnt);
 	pci_read_config_byte (dev, PCI_MAX_LAT, &max_lat);
-	seq_printf(m, "  Bus %2d, device %3d, function %2d:\n",
-	       dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
+	seq_printf(m, "  Domain %2d, Bus %2d, device %3d, function %2d:\n",
+	       pci_domain_nr(dev->bus), dev->bus->number, PCI_SLOT(dev->devfn),
+	       PCI_FUNC(dev->devfn));
 	class = pci_class_name(class_rev >> 16);
 	if (class)
 		seq_printf(m, "    %s", class);

===== include/linux/pci.h 1.90 vs edited =====

@@ -414,7 +414,7 @@
 	struct resource dma_resource[DEVICE_COUNT_DMA];
 	struct resource irq_resource[DEVICE_COUNT_IRQ];
 
-	char		slot_name[8];	/* slot name */
+	char		slot_name[13];	/* slot name */
 
 	/* These fields are used by common fixups */
 	unsigned int	transparent:1;	/* Transparent PCI bridge */

@@ -536,7 +536,7 @@
 
 /* Generic PCI functions used internally */
 
-int pci_bus_exists(const struct list_head *list, int nr);
+int pci_bus_exists(const struct list_head *list, struct pci_bus *bus);
 struct pci_bus *pci_scan_bus_parented(struct device *parent, int bus, struct pci_ops *ops, void *sysdata);
 static inline struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata)
 {

Err, this definitely breaks X on alpha. On small and mid-range machines we always have pci_domain_nr(bus) == bus->number. Practically, it's only Marvel where we could overflow an 8-bit bus number.

Looks good to me, except this (see above):

	/* Backwards compatibility for first N PCI domains. */
	if (pci_domain_nr(dev->bus) > PCI_PROC_MAX_DOMAIN)
		return 0;

Hi,

OK.

A runtime test would be useful, at least for ppc64. That would allow our older machines to work (multiple host bridges without overlapping buses). What if we had pci_proc_max_domain and arch code could change its value during pcibios_init?

Anton

Maybe. Or pci_proc_max_domain(), which can be a macro or a real function, depending on arch.

Ivan.

I don't think this is going to work out well; it's too complicated.

How about this (PPC & Sparc64 will have to decide what they want to do for this case):

Index: drivers/pci/proc.c
===================================================================
RCS file: /var/cvs/linux-2.5/drivers/pci/proc.c,v
retrieving revision 1.9

diff -u -p -r1.9 proc.c
@@ -383,7 +383,8 @@ int pci_proc_attach_device(struct pci_de
 		return -EACCES;
 
 	if (!(de = bus->procdir)) {
-		sprintf(name, "%02x", bus->number);
+		if (!pci_name_bus(name, bus))
+			return -EEXIST;
 		de = bus->procdir = proc_mkdir(name, proc_bus_pci_dir);
 		if (!de)
 			return -ENOMEM;

Index: include/asm-alpha/pci.h
===================================================================
RCS file: /var/cvs/linux-2.5/include/asm-alpha/pci.h,v retrieving revision 1.10

diff -u -p -r1.10 pci.h
@@ -194,6 +194,13 @@ pcibios_resource_to_bus(struct pci_dev *
 
 #define pci_domain_nr(bus) ((struct pci_controller *)(bus)->sysdata)->index
 
+/* We never have overlapping bus numbers on Alpha */
+static inline int pci_name_bus(char *name, struct pci_bus *bus)
+{
+	sprintf(name, "%02x", bus->number);
+	return 0;
+}
+
 #endif /* __KERNEL__ */
 
 /* Values for the `which' argument to sys_pciconfig_iobase.  */

Index: include/asm-ia64/pci.h
===================================================================
RCS file: /var/cvs/linux-2.5/include/asm-ia64/pci.h,v retrieving revision 1.8

diff -u -p -r1.8 pci.h
@@ -93,6 +93,16 @@ struct pci_controller {
 
 #define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
 #define pci_domain_nr(busdev)    (PCI_CONTROLLER(busdev)->segment)
+
+static inline int pci_name_bus(char *name, struct pci_bus *bus)
+{
+	if (pci_domain_nr(bus) == 0) {
+		sprintf(name, "%02x", bus->number);
+	} else {
+		sprintf(name, "%04x:%02x", pci_domain_nr(bus), bus->number);
+	}
+	return 0;
+}
 
 /* generic pci stuff */
 #include <asm-generic/pci.h>

Index: include/linux/pci.h
===================================================================
RCS file: /var/cvs/linux-2.5/include/linux/pci.h,v retrieving revision 1.17

diff -u -p -r1.17 pci.h
@@ -808,6 +808,11 @@ extern int pci_pci_problems;
 
 #ifndef CONFIG_PCI_DOMAINS
 static inline int pci_domain_nr(struct pci_bus *bus) { return 0; }
+static inline int pci_name_bus(char *name, struct pci_bus *bus)
+{
+	sprintf(name, "%02x", bus->number);
+	return 0;
+}
 #endif
 
 #endif /* __KERNEL__ */

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

I'm fine with it.

"Never" is not quite correct - "in general we don't have" would be better. Full-sized Marvel can have up to 512 root buses.

Ivan.

So what do you want to do about that case? If it's going to turn out to be the same as other architectures, maybe we can do it differently...

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Probably something like this:

static inline int pci_name_bus(char *name, struct pci_bus *bus)

{
	if (pci_domain_nr(bus) < 256) {
		sprintf(name, "%02x", bus->number);
	} else {
		sprintf(name, "%04x:%02x", pci_domain_nr(bus), bus->number);
	}
	return 0;
}

Ivan.

Yep, I have a patch to do the same thing. Sorry, didn't realise you were working on this too; I should've cc'd you.

Probably ;-(

Yes. Some people felt my patch didn't go far enough (they wanted _everything_ as its own little file, and damn the dentry/inode consumption!), but it's resurrectable.

My personal feeling is that we should leave the resources file alone (except for fixing its formatting; patch already with greg), expose the config file and expose the contents of the resources.

I think we probably shouldn't since that requires additional testing & fixing of stuff for those of us with multiple-domain boxes.

I actually eliminated pci_bus_exists() completely in my tree. pci_find_bus() does the job just as well.

We're definitely moving in the same direction ;-)

Here's one place we differ...

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

It's also exposed to userspace in some ways I don't think I like. Here's some of the fun ones:

./sound/oss/emu10k1/main.c: sprintf(s, "driver/emu10k1/%s", card->pci_dev->slot_name);
./drivers/scsi/scsi_ioctl.c: * pci_dev::slot_name (8 characters) for the PCI device (if any). (oops, that one already changed to use the device->bus_id, so that broke ...)

And some potential buffer overruns:
./drivers/input/gameport/cs461x.c: sprintf(phys, "pci%s/gameport0", pdev->slot_name);
./drivers/net/3c59x.c: strcpy(info.bus_info, VORTEX_PCI(vp)->slot_name);

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

that one is for ethtool ioctl.
Is 'ethtool -i' obsolete in 2.6?
It seems sysfs has every info already.

olaf@smirnow:~/linux-2.5.72> l /sys/class/net/eth0/ total 0

drwxr-xr-x    3 root     root            0 Jun 17 13:14 ./
drwxr-xr-x    4 root     root            0 Jun 17 13:14 ../
-r--r--r--    1 root     root         4096 Jun 17 13:14 addr_len
-r--r--r--    1 root     root         4096 Jun 17 13:14 address
-r--r--r--    1 root     root         4096 Jun 17 13:14 broadcast
lrwxrwxrwx    1 root     root           34 Jun 17 13:14 device -> ../../../devices/pci0/0000:00:0a.0/
lrwxrwxrwx    1 root     root           30 Jun 17 13:14 driver -> ../../../bus/pci/drivers/3c59x/
-r--r--r--    1 root     root         4096 Jun 17 13:14 features
-rw-r--r--    1 root     root         4096 Jun 17 13:14 flags
-r--r--r--    1 root     root         4096 Jun 17 13:14 ifindex
-r--r--r--    1 root     root         4096 Jun 17 13:14 iflink
-rw-r--r--    1 root     root         4096 Jun 17 13:14 mtu
drwxr-xr-x    2 root     root            0 Jun 17 13:14 statistics/
-rw-r--r--    1 root     root         4096 Jun 17 13:14 tx_queue_len
-r--r--r--    1 root     root         4096 Jun 17 13:14 type

driver, businfo.
drivers/<foo> does not provide a version string.
Should there be an entry for that?

USB is for mice, FireWire is for men!

So, "pci-domainN" would be a device itself, separate from the "pciN" device. What physical hardware do these represent, or are they just eye candy?

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

As davem suggested, /proc/bus/pci should present domain 0 in the old format even with pci domains enabled. If your graphics card is on domain 0 then X continues to work :)

Anton

Hrm... On most pmacs, it is, since domain 0 is the AGP port. Though people with an additional PCI video card will not be happy. But X will be fixed, so....

Ben.