pci_domain_nr vs. /sys/devices

pci_domain_nr vs. /sys/devices

[Benjamin Herrenschmidt]

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.

Re: pci_domain_nr vs. /sys/devices

[Matthew Wilcox]

On Wed, Jun 11, 2003 at 04:30:42PM +0200, Benjamin Herrenschmidt wrote:
> 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 ?

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

Re: pci_domain_nr vs. /sys/devices

[Benjamin Herrenschmidt]

On Wed, 2003-06-11 at 16:48, Matthew Wilcox wrote:

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

Looks like nobody really understands it then ;)

> 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:

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.

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

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.

Re: pci_domain_nr vs. /sys/devices

[Matthew Wilcox]

On Wed, Jun 11, 2003 at 05:06:20PM +0200, Benjamin Herrenschmidt wrote:
> Looks like nobody really understands it then ;)

Well, it keeps changing ;-)

> > 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:
> 
> 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.

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

> 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)

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

Re: pci_domain_nr vs. /sys/devices

[Russell King]

On Wed, Jun 11, 2003 at 03:48:01PM +0100, Matthew Wilcox wrote:
> 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:

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: pci_domain_nr vs. /sys/devices

[Russell King]

On Wed, Jun 11, 2003 at 04:30:42PM +0200, Benjamin Herrenschmidt wrote:
> 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, "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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: pci_domain_nr vs. /sys/devices

[Benjamin Herrenschmidt]

On Wed, 2003-06-11 at 17:40, Russell King wrote:

> 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.

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.

Re: pci_domain_nr vs. /sys/devices

[Patrick Mochel]

> >   /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 ?
> 
> I don't think sysfs works like that (please correct me if I've
> misunderstood, mochel..)

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. :)

> 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)


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

Re: pci_domain_nr vs. /sys/devices

[Anton Blanchard]

 
> 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)

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

Re: pci_domain_nr vs. /sys/devices

[Benjamin Herrenschmidt]

On Thu, 2003-06-12 at 02:37, Anton Blanchard wrote:
>  > 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)
> 
> 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 :)

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.

Re: pci_domain_nr vs. /sys/devices

[Anton Blanchard]

 
> 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 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 =====
--- 1.43/drivers/pci/probe.c	Mon Jun  9 02:36:59 2003
+++ edited/drivers/pci/probe.c	Tue Jun 17 10:29:08 2003
@@ -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 =====
--- 1.29/drivers/pci/proc.c	Wed Jun 11 02:33:14 2003
+++ edited/drivers/pci/proc.c	Tue Jun 17 09:32:20 2003
@@ -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 =====
--- 1.90/include/linux/pci.h	Wed Jun 11 16:49:42 2003
+++ edited/include/linux/pci.h	Tue Jun 17 10:27:32 2003
@@ -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)
 {

Re: pci_domain_nr vs. /sys/devices

[Anton Blanchard]

 
> 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)

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.
...

Re: pci_domain_nr vs. /sys/devices

[Ivan Kokshaysky]

On Tue, Jun 17, 2003 at 02:49:48PM +1000, Anton Blanchard wrote:
> And only creates /proc/bus/pci entries for the first domain.

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.

> 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?

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

> +	/* Backwards compatibility for domain 0 only */
> +	if (pci_domain_nr(dev->bus) != 0)
> +		return 0;

How about this instead?

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

PCI_PROC_MAX_DOMAIN could be defined in asm/pci.h (255 on alpha), default 0.

Ivan.

Re: pci_domain_nr vs. /sys/devices

[Anton Blanchard]

Hi,
 
> 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.

OK.
 
> How about this instead?
> 
> 	/* Backwards compatibility for first N PCI domains. */
> 	if (pci_domain_nr(dev->bus) > PCI_PROC_MAX_DOMAIN)
> 		return 0;
> 
> PCI_PROC_MAX_DOMAIN could be defined in asm/pci.h (255 on alpha), default 0.

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

Re: pci_domain_nr vs. /sys/devices

[Ivan Kokshaysky]

On Tue, Jun 17, 2003 at 10:49:50PM +1000, Anton Blanchard wrote:
> 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?

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

Ivan.

Re: pci_domain_nr vs. /sys/devices

[Matthew Wilcox]

On Tue, Jun 17, 2003 at 02:49:48PM +1000, Anton Blanchard wrote:
> 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

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.

> It also adds the domain to /proc/pci (are there userspace tools that
> parse this directly?):
> 
>   Domain  2, Bus  0, device   2, function  2:

Probably ;-(

> 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.

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 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.

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

> 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.

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

> Thoungts?

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

Here's one place we differ...

> ===== drivers/pci/proc.c 1.29 vs edited =====
> --- 1.29/drivers/pci/proc.c	Wed Jun 11 02:33:14 2003
> +++ edited/drivers/pci/proc.c	Tue Jun 17 09:32:20 2003
> @@ -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);

I create them anyway, but put the domain on the front.  I bet that'll
break Alpha too (I've read further in the thread but need to reply from
here ..)

> @@ -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);

I'd prefer not to touch it.

> ===== include/linux/pci.h 1.90 vs edited =====
> --- 1.90/include/linux/pci.h	Wed Jun 11 16:49:42 2003
> +++ edited/include/linux/pci.h	Tue Jun 17 10:27:32 2003
> @@ -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 */

Let's make slot_name a pointer to the struct device busid.

-- 
"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

Re: pci_domain_nr vs. /sys/devices

[Matthew Wilcox]

On Tue, Jun 17, 2003 at 02:49:48PM +1000, Anton Blanchard wrote:
> 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.

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

Re: pci_domain_nr vs. /sys/devices

[Olaf Hering]

 On Tue, Jun 17, Matthew Wilcox wrote:

> ./drivers/net/3c59x.c:                  strcpy(info.bus_info, VORTEX_PCI(vp)->slot_name);

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!

Re: pci_domain_nr vs. /sys/devices

[Matthew Wilcox]

On Tue, Jun 17, 2003 at 05:11:00PM +0400, Ivan Kokshaysky wrote:
> On Tue, Jun 17, 2003 at 10:49:50PM +1000, Anton Blanchard wrote:
> > 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?
> 
> Maybe. Or pci_proc_max_domain(), which can be a macro or a real function,
> depending on arch.

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
--- drivers/pci/proc.c	14 Jun 2003 22:15:29 -0000	1.9
+++ drivers/pci/proc.c	17 Jun 2003 19:36:50 -0000
@@ -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
--- include/asm-alpha/pci.h	14 Jun 2003 22:15:52 -0000	1.10
+++ include/asm-alpha/pci.h	17 Jun 2003 19:37:28 -0000
@@ -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
--- include/asm-ia64/pci.h	14 Jun 2003 22:15:56 -0000	1.8
+++ include/asm-ia64/pci.h	17 Jun 2003 19:37:45 -0000
@@ -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
--- include/linux/pci.h	14 Jun 2003 22:16:01 -0000	1.17
+++ include/linux/pci.h	17 Jun 2003 19:37:56 -0000
@@ -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

Re: pci_domain_nr vs. /sys/devices

[Ivan Kokshaysky]

On Tue, Jun 17, 2003 at 08:42:27PM +0100, Matthew Wilcox wrote:
> How about this (PPC & Sparc64 will have to decide what they want to do
> for this case):

I'm fine with it.

> +/* We never have overlapping bus numbers on Alpha */

"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.

Re: pci_domain_nr vs. /sys/devices

[Matthew Wilcox]

On Wed, Jun 18, 2003 at 01:30:58AM +0400, Ivan Kokshaysky wrote:
> On Tue, Jun 17, 2003 at 08:42:27PM +0100, Matthew Wilcox wrote:
> > How about this (PPC & Sparc64 will have to decide what they want to do
> > for this case):
> 
> I'm fine with it.
> 
> > +/* We never have overlapping bus numbers on Alpha */
> 
> "Never" is not quite correct - "in general we don't have"
> would be better. Full-sized Marvel can have up to 512 root buses.

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

Re: pci_domain_nr vs. /sys/devices

[Ivan Kokshaysky]

On Wed, Jun 18, 2003 at 02:02:34PM +0100, Matthew Wilcox wrote:
> > "Never" is not quite correct - "in general we don't have"
> > would be better. Full-sized Marvel can have up to 512 root buses.
> 
> So what do you want to do about that case?

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.