Physical PCI slot objects

drivers/pci/hotplug/fakephp.c static int add_slot(struct pci_dev *dev) · line 111

Quoted 2 lines · Alex Chiang

drivers/pci/hotplug/fakephp.c +2 -0

+	slot->name = kmalloc(8, GFP_KERNEL);
+	sprintf(slot->name, "fake%d", count++);

Please use snprintf to avoid buffer overruns!

Register one slot per slot, rather than one slot per function. Change the name of the slot to fake%d instead of the pci address.

Signed-off-by Alex Chiang <[email protected]>

Signed-off-by Matthew Wilcox <[email protected]>

---
 drivers/pci/hotplug/fakephp.c |   75

+++++++++++++++-------------------------- 1 files changed, 27 insertions(+), 48 deletions(-)

drivers/pci/hotplug/fakephp.c +1 -0

index 027f686..828335e 100644
@@ -93,6 +93,7 @@ static int add_slot(struct pci_dev *dev)
 	struct dummy_slot *dslot;
 	struct hotplug_slot *slot;
 	int retval = -ENOMEM;
+	static int count = 1;
 
 	slot = kzalloc(sizeof(struct hotplug_slot), GFP_KERNEL);
 	if (!slot)

Quoted 19 lines · Alex Chiang

drivers/pci/hotplug/fakephp.c +3 -1

@@ -93,6 +93,7 @@ static int add_slot(struct pci_dev *dev)
 	struct dummy_slot *dslot;
 	struct hotplug_slot *slot;
 	int retval = -ENOMEM;
+	static int count = 1;
 
 	slot = kzalloc(sizeof(struct hotplug_slot), GFP_KERNEL);
 	if (!slot)

@@ -106,7 +107,8 @@ static int add_slot(struct pci_dev *dev)
 	slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
 	slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
 
-	slot->name = &dev->dev.bus_id[0];
+	slot->name = kmalloc(8, GFP_KERNEL);
+	sprintf(slot->name, "fake%d", count++);
 	dbg("slot->name = %s\n", slot->name);
 

drivers/pci/hotplug/fakephp.c static int add_slot(struct pci_dev *dev) · line 114

Quoted 1 line

drivers/pci/hotplug/fakephp.c +0 -0

 	dslot = kmalloc(sizeof(struct dummy_slot), GFP_KERNEL);

This is ugly. Please do it the way we already do e.g. for
acpiphp: add a char[8] to "struct dummy_slot" and just reference that here.

Hi Eike,

* Rolf Eike Beer <[email protected]>:

Quoted 20 lines

Quoted 19 lines · Alex Chiang

drivers/pci/hotplug/fakephp.c +3 -1

@@ -93,6 +93,7 @@ static int add_slot(struct pci_dev *dev)
 	struct dummy_slot *dslot;
 	struct hotplug_slot *slot;
 	int retval = -ENOMEM;
+	static int count = 1;
 
 	slot = kzalloc(sizeof(struct hotplug_slot), GFP_KERNEL);
 	if (!slot)

@@ -106,7 +107,8 @@ static int add_slot(struct pci_dev *dev)
 	slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
 	slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
 
-	slot->name = &dev->dev.bus_id[0];
+	slot->name = kmalloc(8, GFP_KERNEL);
+	sprintf(slot->name, "fake%d", count++);
 	dbg("slot->name = %s\n", slot->name);
 

Quoted 5 lines

drivers/pci/hotplug/fakephp.c static int add_slot(struct pci_dev *dev) · line 114

Quoted 1 line

drivers/pci/hotplug/fakephp.c +0 -0

 	dslot = kmalloc(sizeof(struct dummy_slot), GFP_KERNEL);

This is ugly. Please do it the way we already do e.g. for
acpiphp: add a char[8] to "struct dummy_slot" and just reference that here.

I took at look at the code in acpiphp you're talking about, and I'm not sure why you think the above is "ugly". We're talking about a runtime vs compile time storage for the name, and doing a kmalloc/sprintf is a pretty standard idiom.

BTW, I did incorporate both Linas' and Willy's comments, by changing the kmalloc size to an explicit 32, and using snprintf instead.

In any case, for your particular comment, I think I'm going to leave it alone for now, and let Greg weigh in with the final recommendation.

Am Mittwoch, 14. November 2007 schrieb Alex Chiang:

Quoted 11 lines

* Rolf Eike Beer <[email protected]>:

Quoted 4 lines

This is ugly. Please do it the way we already do e.g. for
acpiphp: add a char[8] to "struct dummy_slot" and just reference that here.

I took at look at the code in acpiphp you're talking about, and I'm not sure why you think the above is "ugly". We're talking about a runtime vs compile time storage for the name, and doing a kmalloc/sprintf is a pretty standard idiom.

Quoted 7 lines

BTW, I did incorporate both Linas' and Willy's comments, by changing the kmalloc size to an explicit 32, and using snprintf instead.

In any case, for your particular comment, I think I'm going to leave it alone for now, and let Greg weigh in with the final recommendation.

Because we have another allocation of very small size for every slot here.

struct dummy_slot has a size of 4 pointers, that's 16 byte for 32 bit architectures. Putting 8 byte of additional storage here would save a complete allocation on 32 bit platforms. For 64 bit platforms the memory usage is the same but we do less allocations (i.e. less points to fail) and less memory fragmentation.

Btw: your code lacks a check if kmalloc() returns NULL.

include/linux/pci.h struct pci_cap_saved_state { · line 136

Quoted 5 lines · Alex Chiang

include/linux/pci.h +5 -0

+/* pci_slot represents a physical slot */
+struct pci_slot {
+	struct pci_bus *bus;		/* The bus this slot is on */
+	struct pci_slot *next;		/* Next slot on this bus */
+	struct hotplug_slot *hotplug;	/* Hotplug info (migrate over time) */

How much migration do you expect? Some of it? All of it? If the answer is "all of it", wouldn't it just be easier to rename struct hotplug_slot, and go from there?

Hello,

[this patch series touches a few subsystems; hopefully I got all the right maintainers]

Recently, Matthew Wilcox sent out the following mail about PCI slots:

http://marc.info/?l=linux-pci&m=119432330418980&w=2

The following patch series is a rough first cut at implementing the ideas he outlined, namely, that PCI slots are physical objects that we care about, independent of their hotplug capabilities.

I'm still not sold on this idea at all. I'm really betting that there is a lot of incorrect acpi slot information floating around in machines and odd things will show up in these slot entries.

I say this because for a long time there was no "standard" acpi entries for hotplug slots and different companies did different things. Hence the "odd" IBM acpi hotplug implementation as one example. If this is going to go anywhere, you need to get IBM to agree that it works properly with all their machines...

Also, some companies already provide userspace tools to get all of this information about the different slots in a system and what is where, from userspace, no kernel changes are needed. So, why add all this extra complexity to the kernel if it is not needed?

Quoted 3 lines · Greg KH

I'm still not sold on this idea at all. I'm really betting that there is a lot of incorrect acpi slot information floating around in machines and odd things will show up in these slot entries.

Is that the end of the world? Instead of having no information, we'll end up with odd information. If people complain, we can always blacklist (indeed, won't the ACPI rolling blacklist catch the majority of these machines?)

Quoted 5 lines

I say this because for a long time there was no "standard" acpi entries for hotplug slots and different companies did different things. Hence the "odd" IBM acpi hotplug implementation as one example. If this is going to go anywhere, you need to get IBM to agree that it works properly with all their machines...

Not in terms of slot names. There were various things that ACPI didn't specify, like attention and latches, but the description of _SUN hasn't changed.

Quoted 4 lines

Also, some companies already provide userspace tools to get all of this information about the different slots in a system and what is where, from userspace, no kernel changes are needed. So, why add all this extra complexity to the kernel if it is not needed?

Do you have any examples of this?

We should, IMO, be improving the way we tell users which device has a problem. 'tulip7', 'eth4', 'pci 0000:04:1e.3' ... all the user wants to know is which damn card it is so they can replace it. And slot name tells them that.

Ok, again, I want to see the IBM people sign off on this, after testing on all of their machines, before I'll consider this, as I know the IBM acpi tables are "odd".

Also, how about Dell machines? I know they are probably not expecting this information to show up and who knows if the numbering of their slots match up with their physical diagrams (I say this based on all of the eth0/eth1 "issues" with Dell machines over the years...)

IBM sells a program that does this for server rooms. It's probably part of some Tivoli package somewhere, sorry I don't remember the name. I did see it working many years ago and it required no kernel changes at all to work properly.

Quoted 3 lines · Greg KH

Ok, again, I want to see the IBM people sign off on this, after testing on all of their machines, before I'll consider this, as I know the IBM acpi tables are "odd".

That seems a little higher standard than patches are normally held to. How about the patches get sent to the appropriate people at IBM (who are they?) and if we haven't heard a NAK from them after a month, then they get applied?

Quoted 4 lines

Also, how about Dell machines? I know they are probably not expecting this information to show up and who knows if the numbering of their slots match up with their physical diagrams (I say this based on all of the eth0/eth1 "issues" with Dell machines over the years...)

I'm pretty sure that Windows uses the _SUN information, so I think this is going to be well-tested.

Quoted 4 lines

IBM sells a program that does this for server rooms. It's probably part of some Tivoli package somewhere, sorry I don't remember the name. I did see it working many years ago and it required no kernel changes at all to work properly.

Well, yes, it doesn't take kernel effort if you're willing to build proprietary knowledge into a userspace program. This seems similar to the argument that filesystems don't need to be in the kernel since they can be implemented in user space. They work better in the kernel, just like this does.

Quoted 3 lines · Greg KH

Ok, again, I want to see the IBM people sign off on this, after testing on all of their machines, before I'll consider this, as I know the IBM acpi tables are "odd".

That seems a little higher standard than patches are normally held to. How about the patches get sent to the appropriate people at IBM (who are they?)

and if we haven't heard a NAK from them after a month, then they get applied?

Quoted 5 lines · Greg KH

Quoted 3 lines

Ok, again, I want to see the IBM people sign off on this, after testing on all of their machines, before I'll consider this, as I know the IBM acpi tables are "odd".

Quoted 3 lines

That seems a little higher standard than patches are normally held to. How about the patches get sent to the appropriate people at IBM (who are they?)

I be one of them. :) I have been involved in many (but not all) of IBM's x86 based (IBM System x) servers with hotplug capable PCI slots. I have mostly worked on 'acpiphp' associated issues.

Have you possibly considered a kernel option as a kinder and gentler way of introducing the changes?

====
IBM x3850
Slots 1-2: PCI-X under PCI root bridges
Slots 3-6: PCIe under transparent P2P bridges
Slot 1: PCI-X - populated
Slot 2: PCI-X - !populated
Slot 3: PCIe - populated
Slot 4: PCIe - !populated
Slot 5: PCIe - !populated
Slot 6: PCIe - populated

result is with 2.6.24-rc2 plus all 4 proposed patches

problem: acpiphp failed to register empty PCIe slots 4 and 5

acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:0f:00.0

acpiphp_glue: found ACPI PCI Hotplug slot 4 at PCI 0000:10:00
acpiphp: pci_hp_register failed with error -17
acpiphp_glue: acpiphp_register_hotplug_slot failed(err code = 0xffffffef)

acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:14:00.0
acpiphp_glue: found ACPI PCI Hotplug slot 5 at PCI 0000:15:00
acpiphp: pci_hp_register failed with error -17
acpiphp_glue: acpiphp_register_hotplug_slot failed(err code = 0xffffffef)

# find /sys/bus/pci/slots
/sys/bus/pci/slots

/sys/bus/pci/slots/4
/sys/bus/pci/slots/4/address
/sys/bus/pci/slots/5
/sys/bus/pci/slots/5/address

Hi Gary,

* Gary Hade <[email protected]>:

Quoted 6 lines · Matthew Wilcox

Quoted 5 lines · Greg KH

Quoted 3 lines

Ok, again, I want to see the IBM people sign off on this, after testing on all of their machines, before I'll consider this, as I know the IBM acpi tables are "odd".

Quoted 7 lines

Quoted 3 lines

That seems a little higher standard than patches are normally held to. How about the patches get sent to the appropriate people at IBM (who are they?)

I be one of them. :) I have been involved in many (but not all) of IBM's x86 based (IBM System x) servers with hotplug capable PCI slots. I have mostly worked on 'acpiphp' associated issues.

Thanks for testing the series. It's much appreciated.

Quoted 2 lines

Have you possibly considered a kernel option as a kinder and gentler way of introducing the changes?

That is a good idea. I will work on that.

Quoted 12 lines

====
IBM x3850
Slots 1-2: PCI-X under PCI root bridges
Slots 3-6: PCIe under transparent P2P bridges
Slot 1: PCI-X - populated
Slot 2: PCI-X - !populated
Slot 3: PCIe - populated
Slot 4: PCIe - !populated
Slot 5: PCIe - !populated
Slot 6: PCIe - populated

Quoted 1 line

result is with 2.6.24-rc2 plus all 4 proposed patches

Silly question, but I have to ask. :)

I sent out 5 patches -- is this simply a typo on your part, or did you only apply 4/5 patches?

Quoted 2 lines

problem: acpiphp failed to register empty PCIe slots 4 and 5

Ok, so acpiphp wasn't going to register those slots anyway, since they are empty.

It would have bailed out after not seeing _ADR or
_EJ0 on those slots.

The acpi-pci-slot driver created those slots anyway, which is one of the points of the patch -- to create sysfs entries even for empty slots.

Quoted 1 line

acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:0f:00.0

This is the real address of slot 4.

Quoted 4 lines

acpiphp_glue: found ACPI PCI Hotplug slot 4 at PCI 0000:10:00
acpiphp: pci_hp_register failed with error -17
acpiphp_glue: acpiphp_register_hotplug_slot failed(err code = 0xffffffef)

[repeated 7x]

We saw this message 8x, once for each SxFy object under your p2p bridge. I actually somewhat did expect to see this error message (hence the RFC part of my patch ;)

I currently don't have a good way to determine if we've already seen an empty slot under a p2p bridge, so we try to register every SxFy object. Of course, a /sys/bus/pci/slots/4/ entry already exists, so that's why we're getting -17 (-EEXIST).

Quoted 5 lines

acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:14:00.0
acpiphp_glue: found ACPI PCI Hotplug slot 5 at PCI 0000:15:00
acpiphp: pci_hp_register failed with error -17
acpiphp_glue: acpiphp_register_hotplug_slot failed(err code = 0xffffffef)

Same explanation as above.

Quoted 2 lines

# find /sys/bus/pci/slots
/sys/bus/pci/slots

[snip]

Quoted 4 lines

/sys/bus/pci/slots/4
/sys/bus/pci/slots/4/address
/sys/bus/pci/slots/5
/sys/bus/pci/slots/5/address

Arguably, the right thing happened here. We got entries for empty slots, and we know their addresses.

If anyone can clue me in on a better way to implement patch 4/5 in my series so that we're not seeing those multiple attempts to register slots under p2p bridges, I'd love to hear your ideas.

No, acpiphp should (and did before your changes) register all hotplug capable slots. All 6 slots (2 PCI-X, 4 PCIe) in that system are hotplug capable. Emptyness shouldn't matter. If the empty slots are not registered it is not be possible to successfully hotplug cards to them.

Quoted 4 lines

Quoted 2 lines

Have you possibly considered a kernel option as a kinder and gentler way of introducing the changes?

That is a good idea. I will work on that.

Thanks. This will allow everyone to focus on the systems where the changes are most beneficial and not waste a bunch of time trying to test everywhere.

Ok, so all that said, after re-implementing my ACPI-PCI slot driver, we get all the correct answers, but with the additional appearance of slots 1 and 2 (which aren't hotpluggable):

So if you could try out v2 of my patch series when you get access to your machine back, I'd much appreciate it.

I feel a lot better about it compared to v1.

Here is some dmesg and lspci -vt output in case you want to double-check my work:

I am not fundamentally opposed to this new capability but share the same concerns that Greg and others have expressed. So far, I have only tried the changes on one single node system (IBM x3850) but the below NAK-worthy result supports the idea that the changes need to be well and widely tested.

Hi Gary,

* Gary Hade <[email protected]>:

Quoted 1 line

Quoted 5 lines

I am not fundamentally opposed to this new capability but share the same concerns that Greg and others have expressed. So far, I have only tried the changes on one single node system (IBM x3850) but the below NAK-worthy result supports the idea that the changes need to be well and widely tested.

Can you send me the lspci -vv and lspci -vt output from this machine?

Quoted 10 lines

Hi Gary,

* Gary Hade <[email protected]>:

Quoted 6 lines

I am not fundamentally opposed to this new capability but share the same concerns that Greg and others have expressed. So far, I have only tried the changes on one single node system (IBM x3850) but the below NAK-worthy result supports the idea that the changes need to be well and widely tested.

Quoted 2 lines

Can you send me the lspci -vv and lspci -vt output from this machine?

Alex, I added some slot number hints to the below lspci -vt output.

I am still looking at your other message and will have some hopefully helpful comments soon. I did apply all 5 patches, 4 was a typo.

Ok, again, I want to see the IBM people sign off on this, after testing on all of their machines, before I'll consider this, as I know the IBM acpi tables are "odd".

Also, how about Dell machines? I know they are probably not expecting this information to show up and who knows if the numbering of their slots match up with their physical diagrams

IBM sells a program that does this for server rooms. It's probably part of some Tivoli package somewhere, sorry I don't remember the name. I did see it working many years ago and it required no kernel changes at all to work properly.

* Greg KH <[email protected]>:

Quoted 4 lines

Ok, again, I want to see the IBM people sign off on this, after testing on all of their machines, before I'll consider this, as I know the IBM acpi tables are "odd".

Who would be a good contact at IBM to get some eyes / machine time on this?

Quoted 3 lines

Also, how about Dell machines? I know they are probably not expecting this information to show up and who knows if the numbering of their slots match up with their physical diagrams

Who would be a good contact at Dell for the same?

I don't have as much experience with oddball firmware from various vendors as others on this list, but given the rather stable definition of _SUN in the ACPI spec, I'd be surprised to see vendors abusing that method. [I fully accept the possibility that I'm just naive ;)]

More likely, a vendor will do what the HP Proliant folks did, that being simply omitting a _SUN method altogether.

One more thought on that -- at *worst* my patch series will do no worse than the status quo of what the acpiphp module is doing today. That module walks through the namespace looking for _SUN methods, and when it finds them, it creates an entry in exactly the same spot (/sys/bus/pci/slots/N) that my patch series does.

What this series adds beyond acpiphp is adding entries for slots that aren't hotpluggable.

Quoted 4 lines

IBM sells a program that does this for server rooms. It's probably part of some Tivoli package somewhere, sorry I don't remember the name. I did see it working many years ago and it required no kernel changes at all to work properly.

Like I said in an earlier email, HP ia64 systems will require a kernel change to get this information. Whether it comes via a generic ACPI access layer like dev_acpi, or something like this patch series, the kernel will still get touched.

Quoted 5 lines · Alex Chiang

* Greg KH <[email protected]>:

Quoted 4 lines

IBM sells a program that does this for server rooms. It's probably part of some Tivoli package somewhere, sorry I don't remember the name. I did see it working many years ago and it required no kernel changes at all to work properly.

Quoted 4 lines

Like I said in an earlier email, HP ia64 systems will require a kernel change to get this information. Whether it comes via a generic ACPI access layer like dev_acpi, or something like this patch series, the kernel will still get touched.

And like I said, I'm pretty sure you don't need to touch the kernel today as there are people doing this just fine from userspace without any kernel changes needed :)

Quoted 6 lines · Greg KH

Quoted 5 lines · Alex Chiang

* Greg KH <[email protected]>:

Quoted 4 lines

IBM sells a program that does this for server rooms. It's probably part of some Tivoli package somewhere, sorry I don't remember the name. I did see it working many years ago and it required no kernel changes at all to work properly.

Do you know how this Tivoli package works or where it gets the information?

Quoted 8 lines

Quoted 4 lines

Like I said in an earlier email, HP ia64 systems will require a kernel change to get this information. Whether it comes via a generic ACPI access layer like dev_acpi, or something like this patch series, the kernel will still get touched.

And like I said, I'm pretty sure you don't need to touch the kernel today as there are people doing this just fine from userspace without any kernel changes needed :)

I think you are assuming userspace can just dump the raw ACPI tables and extract this information from them. But I don't think that's possible because _SUN is a method that can contain arbitrary AML. That AML has to be *executed*, and you can't do that safely in userspace.

Quoted 9 lines · Matthew Wilcox

Quoted 3 lines · Greg KH

I'm still not sold on this idea at all. I'm really betting that there is a lot of incorrect acpi slot information floating around in machines and odd things will show up in these slot entries.

Is that the end of the world? Instead of having no information, we'll end up with odd information. If people complain, we can always blacklist (indeed, won't the ACPI rolling blacklist catch the majority of these machines?)

I don't think the rolling blacklist will catch this, as the rest of the ACPI code is "correct".

And yes, incorrect information exported by the kernel is not good at all, especially if a user expects it to work properly :)

Quoted 9 lines

Quoted 5 lines

I say this because for a long time there was no "standard" acpi entries for hotplug slots and different companies did different things. Hence the "odd" IBM acpi hotplug implementation as one example. If this is going to go anywhere, you need to get IBM to agree that it works properly with all their machines...

Not in terms of slot names. There were various things that ACPI didn't specify, like attention and latches, but the description of _SUN hasn't changed.

Ok, again, I want to see the IBM people sign off on this, after testing on all of their machines, before I'll consider this, as I know the IBM acpi tables are "odd".

Also, how about Dell machines? I know they are probably not expecting this information to show up and who knows if the numbering of their slots match up with their physical diagrams (I say this based on all of the eth0/eth1 "issues" with Dell machines over the years...)

Quoted 4 lines

We should, IMO, be improving the way we tell users which device has a problem. 'tulip7', 'eth4', 'pci 0000:04:1e.3' ... all the user wants to know is which damn card it is so they can replace it. And slot name tells them that.

Not if the slot information is incorrect :)

That's my main concern.

The only reported _SUN problems on Dell systems were on the PE6800 and PE6850 systems, which we've fixed with an updated BIOS several months ago. IIRC the values weren't always unique which kind of defeated the purpose.

* Matt Domsch <[email protected]>:

Quoted 5 lines

The only reported _SUN problems on Dell systems were on the PE6800 and PE6850 systems, which we've fixed with an updated BIOS several months ago. IIRC the values weren't always unique which kind of defeated the purpose.

FWIW, the ACPI 2.0 spec did not require uniqueness for _SUN. (although there is a strange table that refers to _SUN as the slot-unique ID (table 6-1 in spec v2.0b), the actual definition of _SUN does not mention uniqueness).

* Matt Domsch <[email protected]>:

Quoted 5 lines

The only reported _SUN problems on Dell systems were on the PE6800 and PE6850 systems, which we've fixed with an updated BIOS several months ago. IIRC the values weren't always unique which kind of defeated the purpose.

FWIW, the ACPI 2.0 spec did not require uniqueness for _SUN. (although there is a strange table that refers to _SUN as the slot-unique ID (table 6-1 in spec v2.0b), the actual definition of _SUN does not mention uniqueness).

Quoted 5 lines

FWIW, the ACPI 2.0 spec did not require uniqueness for _SUN. (although there is a strange table that refers to _SUN as the slot-unique ID (table 6-1 in spec v2.0b), the actual definition of _SUN does not mention uniqueness).

Does your code handle if these are not unique?

Quoted 27 lines · Greg KH

Quoted 9 lines · Matthew Wilcox

Quoted 3 lines · Greg KH

I'm still not sold on this idea at all. I'm really betting that there is a lot of incorrect acpi slot information floating around in machines and odd things will show up in these slot entries.

Is that the end of the world? Instead of having no information, we'll end up with odd information. If people complain, we can always blacklist (indeed, won't the ACPI rolling blacklist catch the majority of these machines?)

I don't think the rolling blacklist will catch this, as the rest of the ACPI code is "correct".

And yes, incorrect information exported by the kernel is not good at all, especially if a user expects it to work properly :)

Quoted 9 lines

Quoted 5 lines

I say this because for a long time there was no "standard" acpi entries for hotplug slots and different companies did different things. Hence the "odd" IBM acpi hotplug implementation as one example. If this is going to go anywhere, you need to get IBM to agree that it works properly with all their machines...

Not in terms of slot names. There were various things that ACPI didn't specify, like attention and latches, but the description of _SUN hasn't changed.

Quoted 8 lines

Ok, again, I want to see the IBM people sign off on this, after testing on all of their machines, before I'll consider this, as I know the IBM acpi tables are "odd".

Also, how about Dell machines? I know they are probably not expecting this information to show up and who knows if the numbering of their slots match up with their physical diagrams (I say this based on all of the eth0/eth1 "issues" with Dell machines over the years...)

The only reported _SUN problems on Dell systems were on the PE6800 and PE6850 systems, which we've fixed with an updated BIOS several months ago. IIRC the values weren't always unique which kind of defeated the purpose.

The eth0/eth1 "issues" are completely separate, with no relation to ACPI. That's purely a PCI tree depth-first discovery
vs. breadth-first discovery (where it indeed seems everyone except Linux 2.6 uses and expects breadth-first). We looked at extending ACPI to export device naming preference, but instead extended SMBIOS tables because we could get that change in far easier.

Quoted 10 lines

Recently, Matthew Wilcox sent out the following mail about PCI slots:

http://marc.info/?l=linux-pci&m=119432330418980&w=2

The following patch series is a rough first cut at implementing the ideas he outlined, namely, that PCI slots are physical objects that we care about, independent of their hotplug capabilities.

Also, some companies already provide userspace tools to get all of this information about the different slots in a system and what is where, from userspace, no kernel changes are needed. So, why add all this extra complexity to the kernel if it is not needed?

* Greg KH <[email protected]>:

Quoted 12 lines · Alex Chiang

Quoted 10 lines

Recently, Matthew Wilcox sent out the following mail about PCI slots:

http://marc.info/?l=linux-pci&m=119432330418980&w=2

The following patch series is a rough first cut at implementing the ideas he outlined, namely, that PCI slots are physical objects that we care about, independent of their hotplug capabilities.

Quoted 5 lines

Also, some companies already provide userspace tools to get all of this information about the different slots in a system and what is where, from userspace, no kernel changes are needed. So, why add all this extra complexity to the kernel if it is not needed?

On HP ia64 systems, that information is locked away in ACPI, and there's no easy way to get at it. Alex Williamson tried providing a generic dev_acpi driver, so that userspace could do whatever they wanted to with the information:

http://lkml.org/lkml/2004/8/3/106

And that effort kinda died. I'm of mixed feelings on that driver, since it would be really nice to get unfettered access to the ACPI namespace, but it's pretty dangerous, since any interesting thing you might want to do is actually a method (aka, it calls into firmware) and who knows what side effects there might be.

So from my point of view, if ia64 customers want to know about the slots they have in their systems, we're gonna have to do something kernel-intrusive anyhow.

Doesn't /sys/firmware/acpi give you raw access to the correct tables already?

And isn't there some other tool that dumps the raw ACPI tables? I thought the acpi developers used it all the time when debugging things with users.

Quoted 5 lines · Greg KH

Doesn't /sys/firmware/acpi give you raw access to the correct tables already?
And isn't there some other tool that dumps the raw ACPI tables? I thought the acpi developers used it all the time when debugging things with users.

I'm neither an acpi developer (well I don't think that I am :) nor an end-user, but here are the two things for which I was going to use the information being presented by Alex's patch:

1) a not-yet, but on track to be released tool to be used by end-users to diagnose I/O bottlenecks - the information in /sys/bus/pci/slot/<foo>/address would be used to associated interfaces and/or pci busses etc with something the end user would grok - the number next to the slot.

2) I was also going to get the folks doing installers to make use of the "end-user" slot ID. Even without going to the extreme of the aforementioned 192 slot system, an 8 slot system with a bunch of dual-port NICs in it (for example) is going to present this huge list of otherwise identical entries. Even if the installers show the MAC for a NIC (or I guess a WWN for an HBA or whatnot) that still doesn't tell one without prior knowledge of what MACs were installed in which slot, which slot is associated with a given ethN. Having the end-user slot ID visible is then going to be a great help to that poor admin who is doing the install.

Why not just use the code in the linux firmware kit that does this already today from userspace (thanks to Kristen for pointing this out to me on irc.)?

Quoted 3 lines · Greg KH

Why not just use the code in the linux firmware kit that does this already today from userspace (thanks to Kristen for pointing this out to me on irc.)?

So then we have something that works on ACPI-based machines. Who will add support for all the other kinds of firmware and non-firmware based slots?

Quoted 8 lines · Matthew Wilcox

Quoted 3 lines · Greg KH

Why not just use the code in the linux firmware kit that does this already today from userspace (thanks to Kristen for pointing this out to me on irc.)?

So then we have something that works on ACPI-based machines. Who will add support for all the other kinds of firmware and non-firmware based slots?

Well, it seems that the powerpc people do not want this at all, as they just have "virtual" slots that they don't want people trying to root around and find the real thing.

What other types of machines can export this kind of information? How will you discover non-firmware based slots?

Quoted 3 lines · Greg KH

Why not just use the code in the linux firmware kit that does this already today from userspace (thanks to Kristen for pointing this out to me on irc.)?

So then we have something that works on ACPI-based machines. Who will add support for all the other kinds of firmware and non-firmware based slots?

As far as being able to retrieve the slot number (which it seemed from the HP manageablity application perspective is the goal here), that information is available from userspace as well for at least standard PCI and pcie based systems for occupied slots. For standard pci, you have to make something up anyway - for shpchp we just use an incremental number and combine it with the bus number to represent the slot. For pcie, you can get this info from the slot capabilities register.

Quoted 6 lines

As far as being able to retrieve the slot number (which it seemed from
the HP manageablity application perspective is the goal here), that
information is available from userspace as well for at least standard PCI
and pcie based systems for occupied slots. For standard pci, you have
to make something up anyway - for shpchp we just use an incremental number and combine it with the bus number to represent the slot. For pcie, you can get this info from the slot capabilities register.

Ummm ... that's not what the /spec/ says. I've never worked on any shpc machines, but the shpc driver reads the slot values from the SLOT_CONFIG register, just like the spec says to.

Matthew Wilcox ????????:

Quoted 9 lines · Kristen Carlson Accardi

Quoted 7 lines

As far as being able to retrieve the slot number (which it seemed from the HP manageablity application perspective is the goal here), that information is available from userspace as well for at least standard PCI and pcie based systems for occupied slots. For standard pci, you have to make something up anyway - for shpchp we just use an incremental number and combine it with the bus number to represent the slot. For pcie, you can get this info from the slot capabilities register.

Quoted 4 lines

Ummm ... that's not what the /spec/ says. I've never worked on any shpc machines, but the shpc driver reads the slot values from the SLOT_CONFIG register, just like the spec says to.

The slot number for shpc slot is like 'YYYY_XXXX'.

YYYY is the bus number, though I don't know the specific reason why it was added.

XXXX is slot number decided according to the shpc specification, as you said.

Thanks,
Kenji Kaneshige

Quoted 32 lines · Alex Chiang

* Greg KH <[email protected]>:

Quoted 12 lines · Alex Chiang

Quoted 10 lines

Recently, Matthew Wilcox sent out the following mail about PCI slots:

http://marc.info/?l=linux-pci&m=119432330418980&w=2

The following patch series is a rough first cut at implementing the ideas he outlined, namely, that PCI slots are physical objects that we care about, independent of their hotplug capabilities.

Quoted 5 lines

Also, some companies already provide userspace tools to get all of this information about the different slots in a system and what is where, from userspace, no kernel changes are needed. So, why add all this extra complexity to the kernel if it is not needed?

On HP ia64 systems, that information is locked away in ACPI, and there's no easy way to get at it. Alex Williamson tried providing a generic dev_acpi driver, so that userspace could do whatever they wanted to with the information:

http://lkml.org/lkml/2004/8/3/106

And that effort kinda died. I'm of mixed feelings on that driver, since it would be really nice to get unfettered access to the ACPI namespace, but it's pretty dangerous, since any interesting thing you might want to do is actually a method (aka, it calls into firmware) and who knows what side effects there might be.

Quoted 3 lines

So from my point of view, if ia64 customers want to know about the slots they have in their systems, we're gonna have to do something kernel-intrusive anyhow.

Doesn't /sys/firmware/acpi give you raw access to the correct tables already?

And isn't there some other tool that dumps the raw ACPI tables? I thought the acpi developers used it all the time when debugging things with users.

thanks,

greg k-h

Quoted 3 lines · On Tue, 13 Nov 2007 12:26:32 -0800 Greg KH <[email protected]> wrote:

And isn't there some other tool that dumps the raw ACPI tables? I thought the acpi developers used it all the time when debugging things with users.

There are - people should take a look at the Intel Linux Firmware Kit for an example of how to parse ACPI tables in userspace - the code is also GPL'd, so you are free to use it in another GPL application.

http://www.linuxfirmwarekit.org/download.php#source

In many DSDTs I've seen, _SUN is hardcoded anyway and can be found by reading the DSDT from userspace. This is what the firmwarekit does to check for duplicate _SUN in one of it's tests.

Quoted 9 lines · Kristen Carlson Accardi

Quoted 3 lines · On Tue, 13 Nov 2007 12:26:32 -0800 Greg KH <[email protected]> wrote:

And isn't there some other tool that dumps the raw ACPI tables? I thought the acpi developers used it all the time when debugging things with users.

There are - people should take a look at the Intel Linux Firmware Kit for an example of how to parse ACPI tables in userspace - the code is also GPL'd, so you are free to use it in another GPL application.

Quoted 5 lines

http://www.linuxfirmwarekit.org/download.php#source

In many DSDTs I've seen, _SUN is hardcoded anyway and can be found by reading the DSDT from userspace. This is what the firmwarekit does to check for duplicate _SUN in one of it's tests.

I see three relevant things in the firmware kit:

1) ExecuteAmlMethod() in amlpoke/amlpoke.c. This uses /proc/acpi/hotkey/event_config to cause the kernel to execute an AML method. This looks similar to what dev_acpi does and is unsafe for the same reasons (the method may have side effects that interfere with kernel drivers). The kernel support for this was removed in 2.6.21:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=5ee6edbcde4d3b14e4e03d4b331df1099a34aa8d

2) execute_aml_method() in acpitable.c. Similar to above.

3) parse_SUN_name() in SUN/SUN.c. This uses acpidump, acpixtract, and iasl -d to disassemble the DSDT and SSDTs, then looks for things like "Name (_SUN, 0x0000012C)". That works well if _SUN merely returns a constant, and many DSDTs do that.

But _SUN can be implemented as a control method, and then we have a problem because we can't determine the _SUN value by inspecting the DSDT. We have to evaluate the method, and that may require operation regions, semaphores, etc., so it can only be done in the kernel.

So I agree that the firmware kit has a clever hack that works on much existing x86 firmware, and it sounds like Tivoli might even rely on it. But I don't feel good about it, and it could easily break when some BIOS writer needs to make _SUN slightly more complicated.

Quoted 6 lines

So I agree that the firmware kit has a clever hack that works on much existing x86 firmware, and it sounds like Tivoli might even rely on it. But I don't feel good about it, and it could easily break when some BIOS writer needs to make _SUN slightly more complicated.

Do you know of such BIOSes out there that do this? Will the above scheme not work for the ia64 boxes that you know of that are out in the world today?

Quoted 6 lines

So I agree that the firmware kit has a clever hack that works on much existing x86 firmware, and it sounds like Tivoli might even rely on it. But I don't feel good about it, and it could easily break when some BIOS writer needs to make _SUN slightly more complicated.

Do you know of such BIOSes out there that do this? Will the above scheme not work for the ia64 boxes that you know of that are out in the world today?

Quoted 6 lines

So I agree that the firmware kit has a clever hack that works on much existing x86 firmware, and it sounds like Tivoli might even rely on it. But I don't feel good about it, and it could easily break when some BIOS writer needs to make _SUN slightly more complicated.

Do you know of such BIOSes out there that do this? Will the above scheme not work for the ia64 boxes that you know of that are out in the world today?

* Greg KH <[email protected]>:

Quoted 8 lines · Bjorn Helgaas

Quoted 6 lines

So I agree that the firmware kit has a clever hack that works on much existing x86 firmware, and it sounds like Tivoli might even rely on it. But I don't feel good about it, and it could easily break when some BIOS writer needs to make _SUN slightly more complicated.

Quoted 3 lines

Do you know of such BIOSes out there that do this? Will the above scheme not work for the ia64 boxes that you know of that are out in the world today?

One last mail on this subject -- Bjorn has pointed out to me that the Dell pe6800 and rez1850 both implement _SUN as control methods today.

Does Tivoli run on those machines? If so, how is it getting slot information?

Quoted 13 lines

* Greg KH <[email protected]>:

Quoted 11 lines · Bjorn Helgaas

Quoted 6 lines

So I agree that the firmware kit has a clever hack that works on much existing x86 firmware, and it sounds like Tivoli might even rely on it. But I don't feel good about it, and it could easily break when some BIOS writer needs to make _SUN slightly more complicated.

Do you know of such BIOSes out there that do this? Will the above scheme not work for the ia64 boxes that you know of that are out in the world today?

Quoted 3 lines

One last mail on this subject -- Bjorn has pointed out to me that the Dell pe6800 and rez1850 both implement _SUN as control methods today.

Does the firmware kit break on them?

Doesn't /sys/firmware/acpi give you raw access to the correct tables already?

And isn't there some other tool that dumps the raw ACPI tables? I thought the acpi developers used it all the time when debugging things with users.

Quoted 3 lines · Greg KH

Doesn't /sys/firmware/acpi give you raw access to the correct tables already?

Quoted 3 lines

And isn't there some other tool that dumps the raw ACPI tables? I thought the acpi developers used it all the time when debugging things with users.

Dumping raw ACPI tables isn't adequate - _SUN might be a complex ACPI method with multiple reads and writes to raw hardware, and we really don't want to do that in userspace. The only way to do this reliably is in the kernel.

Quoted 4 lines · Matthew Garrett

Dumping raw ACPI tables isn't adequate - _SUN might be a complex ACPI method with multiple reads and writes to raw hardware, and we really don't want to do that in userspace. The only way to do this reliably is in the kernel.

But it really isn't, as the firmware kit has proven...

Also, some companies already provide userspace tools to get all of this information about the different slots in a system and what is where, from userspace, no kernel changes are needed. So, why add all this extra complexity to the kernel if it is not needed?

Quoted 6 lines

Also, some companies already provide userspace tools to get all of this information about the different slots in a system and what is where, from userspace, no kernel changes are needed. So, why add all this extra complexity to the kernel if it is not needed?

Second that motion.... I don't get it. What are the goals of this patch, really? Just to get a "slot geographical location" from the kernel?

I'm balancing the intellectual appeal of having a kernel struct for representing physical objects, against the headache of reading (debugging, modifying) code that has yet another struct doing yet another thing. So far, the dread of future headaches is winning.

On pseries systems, I deal with something called the "partitionable endpoint", which I think probably usually corresponds to physical slots, but I don't really know.

So, naively, the physical slot concept doesn't really map to what I have to work with; it just adds one more appendix to it all, one more thing to get confused about.

To be clear: above remarks are for the PowerPC boxes. I have no clue about how things work on the IBM Intel-based boxes. And Greg's original "get IBM to agree" remark is about the Intel-based boxes.

Quoted 5 lines

On pseries systems, I deal with something called the "partitionable endpoint", which I think probably usually corresponds to physical slots, but I don't really know.

Quoted 3 lines

So, naively, the physical slot concept doesn't really map to what I have to work with; it just adds one more appendix to it all, one more thing to get confused about.

Sorry, I'm a bit ignorant about pseries -- what kind of name does your PCI hotplug driver give to those slots? What shows up in /sys/bus/pci/slots/?

/sys/bus/pci/slots
/sys/bus/pci/slots/control
/sys/bus/pci/slots/control/remove_slot
/sys/bus/pci/slots/control/add_slot
/sys/bus/pci/slots/0001:00:02.0
/sys/bus/pci/slots/0001:00:02.0/phy_location

~ # cat /sys/bus/pci/slots/0000:00:02.2/phy_location U787A.001.DNZ00Z5-P1-C2

Also, some companies already provide userspace tools to get all of this information about the different slots in a system and what is where, from userspace, no kernel changes are needed. So, why add all this extra complexity to the kernel if it is not needed?

It's not only complexity. Each new sysfs entry costs memory. Memory is not free. There should be always a good reason for those.

Quoted 2 lines · Andi Kleen

It's not only complexity. Each new sysfs entry costs memory. Memory is not free. There should be always a good reason for those.

It's not a lot of memory; it's one directory and a couple of files for each PCI slot in the system. Even huge systems have maybe 200 slots. In order for this to take up as much as one page of ram on a typical PC with six slots, this would have to consume 680 bytes per directory. I don't think sysfs is that inefficient (and if it is, maybe this feature is not where the problem is, given the 'power' directory per device, the 19 files per scsi device, the huge numbers of symlinks, etc).

It becomes much more when someone does a find /sys. dentries are expensive. They eventually can get pruned again, but it's still costly to do that.

Quoted 3 lines · Andi Kleen

It becomes much more when someone does a find /sys. dentries are expensive. They eventually can get pruned again, but it's still costly to do that.

Again, if this is a big concern for you, there are better places to look at for savings.

Whoever is proposing a feature has the burden to justify that its usefulness is larger than the overhead/cost it adds.

Doesn't seem to be the case with this one so far.

And in general ignoring overhead in new features is a pretty sad approach. Big bloat does come in small steps with each new feature.

Quoted 5 lines · Andi Kleen

Quoted 3 lines

It becomes much more when someone does a find /sys. dentries are expensive. They eventually can get pruned again, but it's still costly to do that.

Quoted 2 lines

Again, if this is a big concern for you, there are better places to look at for savings.

Whoever is proposing a feature has the burden to justify that its usefulness is larger than the overhead/cost it adds.

Doesn't seem to be the case with this one so far.

And in general ignoring overhead in new features is a pretty sad approach. Big bloat does come in small steps with each new feature.

Hello,

[this patch series touches a few subsystems; hopefully I got all the right maintainers]

Recently, Matthew Wilcox sent out the following mail about PCI slots:

http://marc.info/?l=linux-pci&m=119432330418980&w=2

The following patch series is a rough first cut at implementing the ideas he outlined, namely, that PCI slots are physical objects that we care about, independent of their hotplug capabilities.

We introduce a struct pci_slot as a first-class citizen, and turn hotplug_slot into a subsidiary structure. A brief glimpse at the potential for cleanup is given, as we modify the existing hotplug drivers and remove the multiple get_address() methods. Certainly more cleanup can be done with this new structure.

Additionally, we introduce an ACPI PCI slot driver, which will detect all physical PCI slots in the system (as described by ACPI). De-coupling the notion of a physical slot from its hotplug capability allows users to understand the physical geography of their machines, whether their slots are hotpluggable or not.

This patch series builds heavily on Willy's prior work (with a bit of the typical refresh needed when aiming at the moving target of the kernel). The ACPI PCI slot driver is new.

I have tested this series on both ia64 (hp rx6600) and x86 (hp dl380g). On ia64, system firmware provides _SUN methods for the PCI slots and we get entries in /sys/bus/pci/slots/. On my x86 machine, firmware didn't seem to provide a _SUN, so we don't get any entries in /sys/bus/pci/slots/, but nothing really bad happens either. ;)

Comments/feedback are appreciated.

Thanks.

/ac