login
Search
Hi Gary, John, Kenji-san, et. al, Well, first Gary was on holiday for a month, and then I was on holiday for a month, but I'm back now, and have refreshed this patch series against 2.6.25. The major thing that happened was all the kobject changes (learned my lesson about taking long holidays when holding onto a largish chunk of code that hasn't been accepted yet ;), and so the only real change is in patch 3/4. The kobject changes were nice, btw. In the prior versions of this series, I could never figure out why my kobjects weren't getting released when their refcounts went to 1, and had some hacky code in there to manually release them. (I'm sure I was doing something wrong, but I couldn't figure out what.) I was able to remove that hack in this series because the kobjects are working the way they're supposed to. I did turn on kobject debugging, and all seems well except for one little thing. I based my module (pci_slot) on acpiphp, and the kobject system complains: kobject: 'acpiphp' (a00000020476aed0): does not have a release() function, it is broken and must be fixed. kobject: 'pci_slot' (a000000204791e50): does not have a release() function, it is broken and must be fixed. Not quite sure what to do about these yet, but since no one has fixed acpiphp yet, I'm thinking that I can't be *too* wrong. :) I'm *hoping* that my misunderstanding of kobjects last time around is what caused the weirdness on Gary's machine, and that we won't see any more problems. I've tested this series on my hp rx6600 with both acpiphp and pciehp drivers, and as before, any and all combinations of those modules can be modprobe'd/rmmod'ed in any order, etc. I'd love to see some more testing of this, and then (hopefully!) upstream acceptance. Thanks! /ac --
Rename the slot to be the contents of the 'path' sysfs attribute, and
delete the attribute. The mapping from pci address to slot name is
supposed to be done through the 'address' file, which will be provided
automatically later in this series of patches.
Signed-off-by: Alex Chiang <achiang@hp.com>
Signed-off-by: Matthew Wilcox <matthew@wil.cx>
---
drivers/pci/hotplug/sgi_hotplug.c | 32 +-------------------------------
1 files changed, 1 insertions(+), 31 deletions(-)
diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
index ef07c36..693519e 100644
--- a/drivers/pci/hotplug/sgi_hotplug.c
+++ b/drivers/pci/hotplug/sgi_hotplug.c
@@ -91,21 +91,6 @@ static struct hotplug_slot_ops sn_hotplug_slot_ops = {
static DEFINE_MUTEX(sn_hotplug_mutex);
-static ssize_t path_show (struct hotplug_slot *bss_hotplug_slot,
- char *buf)
-{
- int retval = -ENOENT;
- struct slot *slot = bss_hotplug_slot->private;
-
- if (!slot)
- return retval;
-
- retval = sprintf (buf, "%s\n", slot->physical_path);
- return retval;
-}
-
-static struct hotplug_slot_attribute sn_slot_path_attr = __ATTR_RO(path);
-
static int sn_pci_slot_valid(struct pci_bus *pci_bus, int device)
{
struct pcibus_info *pcibus_info;
@@ -173,18 +158,10 @@ static int sn_hp_slot_private_alloc(struct hotplug_slot *bss_hotplug_slot,
return -ENOMEM;
bss_hotplug_slot->private = slot;
- bss_hotplug_slot->name = kmalloc(SN_SLOT_NAME_SIZE, GFP_KERNEL);
- if (!bss_hotplug_slot->name) {
- kfree(bss_hotplug_slot->private);
- return -ENOMEM;
- }
+ bss_hotplug_slot->name = slot->physical_path;
slot->device_num = device;
slot->pci_bus = pci_bus;
- sprintf(bss_hotplug_slot->name, "%04x:%02x:%02x",
- pci_domain_nr(pci_bus),
- ((u16)pcibus_info->pbi_buscommon.bs_persist_busnum),
- device + 1);
sn_generate_path(pci_bus, slot->physical_path);
@@ -203,8 +180,6 @@ static struct hotplug_slot * sn_hp_destroy(void)
bss_hotplug_slot = ...[Adding Prarit to cc since I think this is his code.] --
Ugh ... it has been a while since I've looked at or owned this code ;) -- but I do seem to recall that on SGI boxes the slot's name was different from the physical path of the device (ie, what was stamped on the back of the PCI hotplug chassis was different from Linux' PCI name) -- OTOH, I haven't recently looked at what slot->physical_path has been initialized to, so they might now be one-and-the-same .... P. --
Nope, you are right -- slot->name is quite different from slot->physical_path. I'm going to remove this patch from my series since it will definitely break SGI. Jesse, Prarit, thanks for the good catch. /ac --
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 <achiang@hp.com>
Signed-off-by: Matthew Wilcox <matthew@wil.cx>
---
drivers/pci/hotplug/fakephp.c | 85 ++++++++++++++--------------------------
1 files changed, 30 insertions(+), 55 deletions(-)
diff --git a/drivers/pci/hotplug/fakephp.c b/drivers/pci/hotplug/fakephp.c
index 94b6401..6c14b4d 100644
--- a/drivers/pci/hotplug/fakephp.c
+++ b/drivers/pci/hotplug/fakephp.c
@@ -66,6 +66,7 @@ struct dummy_slot {
struct pci_dev *dev;
struct work_struct remove_work;
unsigned long removed;
+ char name[8];
};
static int debug;
@@ -100,6 +101,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)
@@ -113,13 +115,14 @@ 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];
- dbg("slot->name = %s\n", slot->name);
-
dslot = kzalloc(sizeof(struct dummy_slot), GFP_KERNEL);
if (!dslot)
goto error_info;
+ slot->name = dslot->name;
+ snprintf(slot->name, sizeof(dslot->name), "fake%d", count++);
+ dbg("slot->name = %s\n", slot->name);
+
slot->ops = &dummy_hotplug_slot_ops;
slot->release = &dummy_release;
slot->private = dslot;
@@ -148,17 +151,17 @@ error:
static int __init pci_scan_buses(void)
{
struct pci_dev *dev = NULL;
- int retval = 0;
+ int lastslot = 0;
while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
- retval = add_slot(dev);
- if (retval) {
- pci_dev_put(dev);
- break;
- }
+ if (PCI_FUNC(dev->devfn) > 0 &&
+ lastslot == PCI_SLOT(dev->devfn))
+ continue;
+ lastslot = PCI_SLOT(dev->devfn);
+ add_slot(dev);
}
- return retval;
+ return 0;
...- Make pci_slot the primary sysfs entity. hotplug_slot becomes a
subsidiary structure.
o pci_create_slot() creates and registers a slot with the PCI core
o pci_slot_add_hotplug() gives it hotplug capability
- Change the prototype of pci_hp_register() to take the bus and
slot number (on parent bus) as parameters.
- Remove all the ->get_address methods since this functionality is
now handled by pci_slot directly.
v6 -> v7:
Refresh to new kobject model.
v5 -> v6:
Add debugging information.
v4 -> v5:
Add refcounting for pci_slot objects.
Return -EBUSY if an hp driver attempts to register a slot
that is already registered to another driver. Do not consider
that to be an error condition in acpiphp and pciehp.
v3 -> v4:
Fixed bug with pciehp and rpaphp registering slots
v2 -> v3:
Separated slot creation and slot hotplug ability into two
interfaces. Fixed bugs in pci_destroy_slot(), and now
properly calling from pci_hp_deregister.
v1 -> v2:
No change
Signed-off-by: Alex Chiang <achiang@hp.com>
Signed-off-by: Matthew Wilcox <matthew@wil.cx>
---
drivers/pci/Makefile | 2 +-
drivers/pci/hotplug/acpiphp.h | 1 -
drivers/pci/hotplug/acpiphp_core.c | 25 +---
drivers/pci/hotplug/acpiphp_glue.c | 23 +--
drivers/pci/hotplug/acpiphp_ibm.c | 5 +-
drivers/pci/hotplug/cpci_hotplug_core.c | 2 +-
drivers/pci/hotplug/cpqphp_core.c | 4 +-
drivers/pci/hotplug/fakephp.c | 2 +-
drivers/pci/hotplug/ibmphp_ebda.c | 3 +-
drivers/pci/hotplug/pci_hotplug_core.c | 244 +++++++++++--------------------
drivers/pci/hotplug/pciehp_core.c | 31 ++---
drivers/pci/hotplug/rpadlpar_sysfs.c | 3 +-
drivers/pci/hotplug/rpaphp_slot.c | 3 +-
drivers/pci/hotplug/sgi_hotplug.c | 2 +-
drivers/pci/hotplug/shpchp_core.c | 17 +--
drivers/pci/pci.h | 13 ++
drivers/pci/slot.c ...This describes what you did, but not why you are doing this, making it a pretty bad changelog comment. Can you refresh my memory as to the "why" for all of this and how you are handling machines that do not export this information at all? oh, and don't put "extern" in a .c file, and call kobject_uevent in the same function that you added the kobject in, unless there is a very good reason to do so, otherwise you just missed all of those events... thanks, greg k-h --
Hi Greg,
How about this:
Currently, /sys/bus/pci/slots/ only exposes hotplug attributes
when a hotplug driver is loaded, but PCI slots have attributes
such as address, speed, width, etc. that are not related to
hotplug at all.
Introduce pci_slot as the primary data structure and kobject
model. Hotplug attributes described in hotplug_slot become a
secondary structure associated with the pci_slot.
This patch only creates the infrastructure that allows the
separation of PCI slot attributes and hotplug attributes.
In this patch, the PCI hotplug core remains the only user of this
infrastructure, and thus, /sys/bus/pci/slots/ will still only
become populated when a hotplug driver is loaded.
A later patch in this series will add a second user of this new
infrastructure and demonstrate splitting the task of exposing
pci_slot attributes from hotplug_slot attributes.
- Make pci_slot the primary sysfs entity. hotplug_slot becomes a
subsidiary structure.
o pci_create_slot() creates and registers a slot with the PCI core
o pci_slot_add_hotplug() gives it hotplug capability
- Change the prototype of pci_hp_register() to take the bus and
slot number (on parent bus) as parameters.
- Remove all the ->get_address methods since this functionality is
With this patch, there is no change from existing behavior that
users see; only infrastructure is changed. The existing hotplug
drivers will continue to expose whatever they were exposing
I *think* I might actually have a "good" reason, but welcome your
feedback.
In this patch, pci_create_slot() is responsible for
kobject_init_and_add, and it adds the 'address' attribute in
sysfs.
The caller of pci_create_slot() is pci_hp_register, and it is
calling kobject_uevent after pci_create_slot, because it still
has to expose the hotplug attributes in sysfs, which can only
happen *after* the pci_slot is created.
I don't think I want to emit the uevent until those hotplug
attributes are exposed, ...Ok, that's a bit better, please include it in the changelog information The issue is for non-hotpluggable slots, right? That's a big behavior That is correct, but the location seemed a bit "odd", next time around No, that's a mess, don't do that :) thanks, greg k-h --
Currently, /sys/bus/pci/slots/ only exposes hotplug attributes
when a hotplug driver is loaded, but PCI slots have attributes
such as address, speed, width, etc. that are not related to
hotplug at all.
Introduce pci_slot as the primary data structure and kobject
model. Hotplug attributes described in hotplug_slot become a
secondary structure associated with the pci_slot.
This patch only creates the infrastructure that allows the
separation of PCI slot attributes and hotplug attributes.
In this patch, the PCI hotplug core remains the only user of this
infrastructure, and thus, /sys/bus/pci/slots/ will still only
become populated when a hotplug driver is loaded.
A later patch in this series will add a second user of this new
infrastructure and demonstrate splitting the task of exposing
pci_slot attributes from hotplug_slot attributes.
- Make pci_slot the primary sysfs entity. hotplug_slot becomes a
subsidiary structure.
o pci_create_slot() creates and registers a slot with the PCI core
o pci_slot_add_hotplug() gives it hotplug capability
- Change the prototype of pci_hp_register() to take the bus and
slot number (on parent bus) as parameters.
- Remove all the ->get_address methods since this functionality is
now handled by pci_slot directly.
v7 -> v8:
Incorporate v7 code review comments (no externs in C files)
v6 -> v7:
Refresh to new kobject model.
v5 -> v6:
Add debugging information.
v4 -> v5:
Add refcounting for pci_slot objects.
Return -EBUSY if an hp driver attempts to register a slot
that is already registered to another driver. Do not consider
that to be an error condition in acpiphp and pciehp.
v3 -> v4:
Fixed bug with pciehp and rpaphp registering slots
v2 -> v3:
Separated slot creation and slot hotplug ability into two
interfaces. Fixed bugs in pci_destroy_slot(), and now
properly calling from pci_hp_deregister.
v1 -> v2:
No change
Signed-off-by: Alex Chiang <achiang@hp.com>
Signed-off-by: ...Detect all physical PCI slots as described by ACPI, and create entries in /sys/bus/pci/slots/. Not all physical slots are hotpluggable, and the acpiphp module does not detect them. Now we know the physical PCI geography of our system, without caring about hotplug. v5 -> v6: Add debugging information. v4 -> v5: Convert to a tristate module. Remove #ifdef CONFIG_ACPI_PCI_SLOT, as struct pci_slot objects are properly refcounted, and multiple calls to pci_create/destroy_slot work just fine. Remove prior -EBUSY changes, as they have been folded into the Introduce pci_slot patch. v3 -> v4: Always attempt to call pci_create_slot from pcihp_core to cover cases of a) user did not say Y to Kconfig option ACPI_PCI_SLOT b) native PCIe hotplug driver registering on a non-ACPI system. Return -EBUSY if an hp driver attempts to register a slot that is already registered to another driver. Do not consider that to be an error condition in acpiphp and pciehp. v2 -> v3: Add Kconfig option to driver, allowing users to [de]config this driver. If configured, take slightly different code paths in pci_hp_register and pci_hp_deregister. v1 -> v2: Now recursively discovering p2p bridges and slots underneath them. Hopefully, this will prevent us from trying to register the same slot multiple times. Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/acpi/Kconfig | 9 + drivers/acpi/Makefile | 1 + drivers/acpi/pci_slot.c | 322 ++++++++++++++++++++++++++++++++ drivers/pci/hotplug/pci_hotplug_core.c | 11 +- drivers/pci/hotplug/pciehp_core.c | 2 +- drivers/pci/hotplug/sgi_hotplug.c | 2 +- 6 files changed, 344 insertions(+), 3 deletions(-) create mode 100644 drivers/acpi/pci_slot.c diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index f688c21..24013a7 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -347,6 +347,15 @@ config ACPI_EC the ...
What is the guarantee that the names of these slots are correct and do not happen to be the same as the hotpluggable ones? Why show this information on machines that can not do anything with these slots at all? Will that not just confuse people? thanks, greg k-h --
That would be a bug -- and yes, bugs happen, and we have to deal with Only for people who think that /sys/bus/pci/slots/ is for hotpluggable slots only. There is plenty of useful information available for slots that aren't hotpluggable (eg bus address, speed, width, error status). -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." --
My main concern is that BIOS vendors will not fix these bugs, as no other OS cares/does this kind of thing today. The ammount of bad information out there might be quite large, and I think this was Can the userspace tools that are using the existing directories thinking that only hotplug slots are there, handle "non-hotplug" slots showing up in this location? thanks, greg k-h --
Yeah, but there's a flip side to this too: if no one uses the data, no one will complain when it's wrong. If Linux starts making it easy to see this stuff, there's a chance system vendors will start taking an extra 5 min. before shipment to make sure that the BIOS info is up to date... OTOH, I'm not sure which is worse, bad data or no data. Jesse --
bad data is worse. And then there's the machines with duplicate slot names, how does this code handle PCI slots with that? I think some of the IBM machines had non-hotplug slots named the same as the hotplug slots, right? This stuff needs a _lot_ of testing on a lot of different machines, and a sane way to fall-back if there are errors to ensure that working machines don't break. And then there's the issue with userspace programs only expecting hotplugable slots in the slots/ directory... thanks, greg k-h --
But we don't want to throw the baby out with the bathwater; assuming there are lots of machines out there with good data (I really hope that's the case), Yeah, I think a good fallback is important. Might be good to have a blacklist along with a heuristic for detecting duplicate slot names... Anyway, yeah, testing will be huge here. Jesse --
On Tue, 4 Mar 2008 11:30:36 -0800 I too worry about interaction with other vendor's machines, so maybe we should make it easier to test by putting this series into linux-next? --
At one point, I had some code in there to stick the names of the slots into a linked list and walk through it to try and detect duplicate slot names, but after a few iterations, the cases I was dealing with, it turned out to be easier to refcount them. [my machines did not have colliding names between hp and non-hp slots, it was more like seeing the same SxFy object appear multiple times in the namespace and trying to create them multiple times.] It wasn't the IBM machine that was breaking; it was Fujitsu. They were returning an error code (the numerical value 1023) when I called the _SUN method on a slot object that existed in the ACPI namespace but was not present (as reported by the _STA method). By the time I got that error report, I'd already dropped the duplicate name detection code, and was letting the kobject infrastructure warn about duplicate names because for my test cases, refcounting was a better solution. [Kenji-san from Fujitsu seemed to be ok with the progress I'd made at the time, he can speak up if he's changed his mind ;)] But even that is not the error case you're describing, where there is clear name collision of two physical slots in the machine, one being hotplug, the other non-hotplug. Maybe I would have to add some duplicate name detection code back Yes -- totally agreed. And I'd like to see actual examples of name collisions or userspace breakage to get a better idea of how to handle real world problems rather than writing some crummy code based on what my limited imagination can think of. So how to get this test coverage? -mm? linux-next? Thanks. /ac --
Hi Alex-san, Unfortunatelly, I have not tried the new version of slot detection driver because of the lack of test environment. Maybe we need more several days to wait for test environment. BTW, does the new one fixes the issue I reported before? I could not find it in the changelog. IIRC, this issue was difficult to solve because the root cause of this issue is from the difference of interpretation of ACPI spec between HP and Fujitsu (I still don't think it's a good idea to evaluate _SUN for the device object whose _STA is 0). Anyway, Fujitsu servers which suffer from this issue doesn't need the slot detection driver because its PCI slots are all hot-pluggable and it already has /sys/bus/pci/slots/XXXX directory for all PCI slots. So one of the workaround is to not use slot detection driver. But when I tried it before, it was automatically loaded at boot up time and then I got strange slot named '1023' and many warning messages (stack traces). Thanks, Kenji Kaneshige --
Hi Kenji-san,
It looks like we disagree on how to interpret the spec (IBM
machines interpret the spec the same way HP machines do).
So given that it's two versus one, I modified my
drivers/acpi/pci_slot module to consider Fujitsu machines to be a
quirk. :)
Can you please test patches 1 and 2 that I sent out as v8 of my
series, but replace patch 3 with this patch?
Please note -- you will probably need to modify this block:
{
.callback = do_sta_before_sun,
.ident = "Fujitsu Limited Primequest",
.matches = {
DMI_MATCH(DMI_BIOS_VENDOR, "Fujitsu"),
DMI_MATCH(DMI_BIOS_VERSION, "2.35"),
},
},
To get the correct values for DMI_BIOS_VENDOR and
DMI_BIOS_VERSION, because I was just guessing.
Thanks!
/ac
From: Alex Chiang <achiang@hp.com>
Subject: [PATCH 3/3] ACPI PCI slot detection driver
Detect all physical PCI slots as described by ACPI, and create
entries in /sys/bus/pci/slots/.
Not all physical slots are hotpluggable, and the acpiphp module
does not detect them. Now we know the physical PCI geography of
our system, without caring about hotplug.
v8 -> v9:
Add DMI quirk for Fujitsu machines; eval _STA before _SUN
v6 -> v8:
No change
v5 -> v6:
Add debugging information.
v4 -> v5:
Convert to a tristate module.
Remove #ifdef CONFIG_ACPI_PCI_SLOT, as struct pci_slot
objects are properly refcounted, and multiple calls to
pci_create/destroy_slot work just fine.
Remove prior -EBUSY changes, as they have been folded
into the Introduce pci_slot patch.
v3 -> v4:
Always attempt to call pci_create_slot from pcihp_core to
cover cases of
a) user did not say Y to Kconfig option ACPI_PCI_SLOT
b) native PCIe hotplug driver registering on a
non-ACPI system.
Return -EBUSY if an hp driver attempts to register a slot
that is already registered to another driver. Do not
consider that to be an error condition in acpiphp and pciehp.
v2 -> v3:
Add Kconfig option to driver, allowing users to [de]config
this ...I like this idea. Nicely done. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." --
I don't have any better idea than this so far. I'll try suggested patch and send feedback as soon as possible. Thanks, Kenji Kaneshige --
Hi Alex-san,
I tried your patches, and I have two comments. I want 1) to be fixed
before merge to Greg's tree (or linux-next?), at least.
1) I checked ACPI spec again and again, but I could not find any
reason to add Fujitsu servers to quirks list. So I'd like you to
add HP servers to the quirks list. I'll send the following patches
followed by this e-mail.
- [PATCH 3/(3+1)] ACPI PCI slot detection driver
This is the updated version of ACPI PCI slot detection driver. I
changed your patch to evaluate _STA before evaluating _SUN.
- [PATCH 4/(3+1)] add quirks for ACPI PCI slot detection driver
Add quirks management code using DMI. Please update the
following part for HP servers.
static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
/*
* Ignore _STA if the hardware provides _STA to indicate the
* presence of PCI adapter card on PCI hotplug slot.
*/
/* Please add appropriate values for HP/IBM servers.
{
.callback = ignore_sta_before_sun,
.ident = "",
.matches = {
DMI_MATCH(DMI_BIOS_VENDOR, ""),
DMI_MATCH(DMI_BIOS_VERSION, ""),
},
},
*/
{}
};
2) The ACPI PCI slot detection driver would change the slot names of
some hotplug drivers (at least I checked shpchp and pciehp). And
the name of slots are depending on the order of driver loading.
For example, on my system which has several SHPCHP slots and
PCIEHP slots, the name of PCIEHP slots are changed as
follows. Please note that PCIEHP based slots are 0034_0027 and
0032_0026, and others are SHPCHP based slots.
- Without ACPI PCI slot detection driver
# ls /sys/bus/pci/slots/
0009_0016 0014_0018 0019_0020 0021_0022 0034_0027
...v9 -> v10: Remove DMI quirk for Fujitsu machines; eval _STA before _SUN. Skip device object whose _STA indicates not present. v8 -> v9: Add DMI quirk for Fujitsu machines; eval _STA before _SUN v6 -> v8: No change v5 -> v6: Add debugging information. v4 -> v5: Convert to a tristate module. Remove #ifdef CONFIG_ACPI_PCI_SLOT, as struct pci_slot objects are properly refcounted, and multiple calls to pci_create/destroy_slot work just fine. Remove prior -EBUSY changes, as they have been folded into the Introduce pci_slot patch. v3 -> v4: Always attempt to call pci_create_slot from pcihp_core to cover cases of a) user did not say Y to Kconfig option ACPI_PCI_SLOT b) native PCIe hotplug driver registering on a non-ACPI system. Return -EBUSY if an hp driver attempts to register a slot that is already registered to another driver. Do not consider that to be an error condition in acpiphp and pciehp. v2 -> v3: Add Kconfig option to driver, allowing users to [de]config this driver. If configured, take slightly different code paths in pci_hp_register and pci_hp_deregister. v1 -> v2: Now recursively discovering p2p bridges and slots underneath them. Hopefully, this will prevent us from trying to register the same slot multiple times. Signed-off-by: Alex Chiang <achiang@hp.com> Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> --- drivers/acpi/Kconfig | 9 drivers/acpi/Makefile | 1 drivers/acpi/pci_slot.c | 328 +++++++++++++++++++++++++++++++++ drivers/pci/hotplug/pci_hotplug_core.c | 11 + drivers/pci/hotplug/pciehp_core.c | 2 drivers/pci/hotplug/sgi_hotplug.c | 2 6 files changed, 350 insertions(+), 3 deletions(-) Index: linux-2.6.25-rc4/drivers/acpi/Kconfig =================================================================== --- linux-2.6.25-rc4.orig/drivers/acpi/Kconfig +++ linux-2.6.25-rc4/drivers/acpi/Kconfig @@ -347,6 ...
Previous one was destroyed by mailer. Resending it. Sorry. v9 -> v10: Remove DMI quirk for Fujitsu machines; eval _STA before _SUN Skip device object whose _STA indicates not present. v8 -> v9: Add DMI quirk for Fujitsu machines; eval _STA before _SUN v6 -> v8: No change v5 -> v6: Add debugging information. v4 -> v5: Convert to a tristate module. Remove #ifdef CONFIG_ACPI_PCI_SLOT, as struct pci_slot objects are properly refcounted, and multiple calls to pci_create/destroy_slot work just fine. Remove prior -EBUSY changes, as they have been folded into the Introduce pci_slot patch. v3 -> v4: Always attempt to call pci_create_slot from pcihp_core to cover cases of a) user did not say Y to Kconfig option ACPI_PCI_SLOT b) native PCIe hotplug driver registering on a non-ACPI system. Return -EBUSY if an hp driver attempts to register a slot that is already registered to another driver. Do not consider that to be an error condition in acpiphp and pciehp. v2 -> v3: Add Kconfig option to driver, allowing users to [de]config this driver. If configured, take slightly different code paths in pci_hp_register and pci_hp_deregister. v1 -> v2: Now recursively discovering p2p bridges and slots underneath them. Hopefully, this will prevent us from trying to register the same slot multiple times. Signed-off-by: Alex Chiang <achiang@hp.com> Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> --- drivers/acpi/Kconfig | 9 drivers/acpi/Makefile | 1 drivers/acpi/pci_slot.c | 328 +++++++++++++++++++++++++++++++++ drivers/pci/hotplug/pci_hotplug_core.c | 11 + drivers/pci/hotplug/pciehp_core.c | 2 drivers/pci/hotplug/sgi_hotplug.c | 2 6 files changed, 350 insertions(+), 3 deletions(-) Index: linux-2.6.25-rc4/drivers/acpi/Kconfig =================================================================== --- ...
ACPI spec says that OSPM checks with the bus driver for _ADR devices
to verify the presence of the device. But there are hardwares that
provide _STA for PCI hotplug slots to indicate the presence of the
device. For those hardwares, we want to ignore _STA and evaluate _SUN
by force, otherwise PCI hotplug slots would not be detected.
Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
---
drivers/acpi/pci_slot.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 43 insertions(+), 4 deletions(-)
Index: linux-2.6.25-rc4/drivers/acpi/pci_slot.c
===================================================================
--- linux-2.6.25-rc4.orig/drivers/acpi/pci_slot.c
+++ linux-2.6.25-rc4/drivers/acpi/pci_slot.c
@@ -31,6 +31,7 @@
#include <acpi/acpi_drivers.h>
static int debug;
+static int ignore_sta;
#define DRIVER_VERSION "0.1"
#define DRIVER_AUTHOR "Alex Chiang <achiang@hp.com>"
@@ -73,10 +74,20 @@ check_slot(acpi_handle handle, int *devi
acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
dbg("Checking slot on path: %s\n", (char *)buffer.pointer);
- status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
- if (ACPI_SUCCESS(status) && !(sta & ACPI_STA_DEVICE_PRESENT)) {
- retval = -1;
- goto out;
+ /*
+ * ACPI spec says that OSPM checks with the bus driver for
+ * _ADR devices to verify the presence of the device. But
+ * there are hardwares that provide _STA for PCI hotplug slots
+ * to indicate the presence of the device. For those
+ * hardwares, we want to ignore _STA and evaluate _SUN by
+ * force, otherwise PCI hotplug slots would not be detected.
+ */
+ if (!ignore_sta) {
+ status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+ if (ACPI_SUCCESS(status) && !(sta & ACPI_STA_DEVICE_PRESENT)) {
+ retval = -1;
+ goto out;
+ }
}
status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
@@ -311,9 +322,37 @@ acpi_pci_slot_remove(acpi_handle handle)
err("%s: unregister_slot failure - ...Alex pointed out that IBM interprets the spec the same way that HP does. Are there any other machines that follow the spec the same way that I hadn't realised that patch got in to put the bus name in as a uniquifier. I thought we'd rejected it because the problem only That is the plan. I'm not sure why the shpc slots aren't renamed in this revision of the patch -- maybe Alex dropped that part? -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." --
So Kenji is arguing that _STA should be called before _SUN (iow that the _SUN value is bogus unless _STA indicates that the slot is powered)? That seems harder for the user; often times you want to find a slot even if it's powered down & empty (say to add a new card)... It looks like the current code won't provide a slot name on Fujitsu machines in this case, or am I missing something? Thanks, Jesse --
Hi, Yes. And the another reason is _STA is not for representing the presence of PCI adapter card on the hot-plug slots because ACPI2.0 or later says "For _HID devices, OSPM evaluates the _STA method. For _ADR devices, OSPM checks with the bus driver for that device." in the explanation of _EJx. Since PCI adapter card on a hotplug slot is _ADR device, we must not provide _STA to represent the presence of PCI adapter card on a hotplug slot. Please look at the sample ASL codes for PCI hotplug slots in ACPI spec. There should be no _STA, of course. This is not described in ACPI1.0b spec and Alex's machine was based on ACPI1.0b. So I've suggested to check ACPI version in ACPI PCI slot detection driver. But it turned out that this suggestion didn't work No. Even if we evaluate _STA before _SUN, ACPI PCI slot detection driver detects all of the slot on Fujitsu machine regardless of slot status (present or not) because Fujitsu server never provide _STA for representing presence of PCI adapter card on the slot. Thanks, Kenji Kaneshige --
Hi, Maybe I've told you before :-) I also don't like to put bus into the names of those directories, because I think the names of those directories are used as physical slot identifier, not logical identifier, and neither PCI-to-PCI Bridge Architecture spec, PCI Hot-Plug spec, SHPC spec nor PCI Express spec allow putting bus number into physical slot identifier. Removing bus number from the names of those directories would fix the problem in a short term. But I think it is only a band-aid because all of those specifications allow using chassis number in the physical slot identifier. If any hotplug driver will put chassis number into slot names, the same problem will happen. I think it's the right direction to use combination of chassis and slot number as a slot name if we try to unify slot names among drivers, though there will be some challenges as follows. - how to handle the names of vendor specific hotplug drivers? - SHPC spec allows vendor specific names, IIRC. how to handle it? Maybe the reason is very simple. The SHPCHP driver was loaded first, and then ACPI PCI slot detection driver was loaded. Both are loaded automatically at boot time. If we changed the order, we would get another result. Thanks, Kenji Kaneshige --
On Tue, 11 Mar 2008 22:10:40 +0900 OK, let me see if I can understand what the issue is here. Please correct me if I'm wrong. The debate is about whether or not it is legitimate to call _SUN if _STA indicates that the device is not present and functional. I've checked ACPI 3.0b, and from what I've read, you may not evaluate _SUN until _INI is called. And _INI should not be called unless _STA indicates that a device is present and functional. "OSPM evaluates the _STA object before it evaluates a device _INI method. The return values of the Present and Functioning bits determines whether _INI should be evaluated and whether children of the device should be enumerated and initialized. See section 6.5.1, “_INI (Init)”." My interpretation of this is that if Alex's driver scans the Fujitsu machine and calls _STA on one of the slots and gets Bit 0 (present) and Bit 3 (functioning) set, then it is ok to enumerate it's children and evaluate _ADR and _SUN for the children. If they are returning 1023, that would mean you should not evaluate the children of that object since the functional bit is not set. Is this correct? If that is the case, I would say that Alex's driver needs to obey the spec - or else if the this is allowed in an earlier version of the spec then he needs to check for the version of the spec that was implemented and make some different rules for that. Thanks for the clarification, Kristen --
Hi Kenji, Kristen, This is also true, but I would like to point out that the *scope* of these control methods is confusing the issue slightly. On my machine, I have a namespace that looks like this: | | |-- L001 [6] | | | |-- _UID (0x100) [1] | | | |-- LMUT [9] | | | |-- _STA (0xf) [8] | | | |-- _BBN (0x1) [8] | | | |-- _HID (HWP0002) [8] | | | |-- _CID (PNP0A03) [8] | | | |-- _PRT [8] | | | |-- _CRS [8] | | | |-- RDFM [8] | | | |-- WRFM [8] | | | |-- _DSM [8] | | | |-- _OSC [8] | | | |-- _INI [8] | | | |-- S1F0 [6] | | | | |-- SLOT (0x0) [1] | | | | |-- FUNC (0x0) [1] | | | | |-- _UID (0x100) [8] | | | | |-- _ADR (0x10000) [8] | | | | |-- _STA (0x0) [8] | | | | |-- _SUN (0x9) [8] | | | | |-- _EJ0 [8] | | | | |-- _PS0 [8] | | | | |-- _PS3 [8] | | | | |-- ATNS [8] | | | | |-- ASTA [8] | | | | |-- PWRS [8] | | | | `-- PSTA [8] | | | |-- S1F1 [6] My L001 object is the PCI bridge. Note that it has _STA and _INI. My S1F0 object is the PCI slot. Note that it has a _STA and a _SUN, but it does *not* have an _INI. So on my machine, if L001._STA indicates "present", then we should evaluate L001._INI and examine the children for _INI methods. (according to acpi 3.0b spec). If the children have _INI, then we need to check child._STA before evaluating the child._INI. On my machine, it is legal to evaluate S1F0._SUN independent of S1F0._STA because L001._INI has already been evaluated. It would be helpful to know what Fujitsu's namespace looks like. If Fujitsu slot objects contain _STA and _INI, then I agree with Kenji-san -- I definitely need to check _STA before evaluating _SUN. But in any case, I think both HP and Fujitsu firmware are doing legal things -- neither firmware is breaking the spec. I should not have called Fujitsu's machine a quirk, so ...
Thank you for explanation. Maybe I understood the summary of implementation of HP firmware. But how to use or where to put _INI method in the ACPI namespace never becomes reasonable reason why My understanding of your explanation so far is: - evaluating _SUN without checking _STA doesn't cause problem, from the view point of HP's implementation. - some IBM machine is doing same as HP I never think those are reasonable reasons why ignoring _STA I don't want to consider "majority rule" before I understand why ignoring _STA is legal. Thanks, Kenji Kaneshige --
Hi Kenji-san, I think the piece that I did not explain clearly is that the spec does not require checking _STA immediately before _SUN. The spec says: - you must check _STA before calling _INI - _INI must be called to initialize _SUN - _INI is called once, when the table is loaded On HP's implementation, we do obey those rules. We call _INI on the PCI bridge during boot, which then initializes the children SxFy objects. From that point on, it is legal for us to call _SUN. The other issue is that the spec does not specify the *semantics* of _STA. P/IBM firmware engineers think _STA should indicate card presence, but Fujitsu firmware engineers think _STA should indicate slot presence. I don't think either firmware team is incorrect -- it is simply the case that the specification was not precise enough, to the point where separate teams following the same spec came up with implementations with different behaviors and different semantics. On HP and IBM machines, it *is* legal because we *did* call _STA, then _INI, then _SUN. That is one interpretation of the spec. On Fujitsu machines, the semantics of _STA are different, and it is not legal to ignore _STA. That is another interpretation of the spec. Because I think we both have compliant, legal firmware, I think the kernel should pick the most maintainable solution for the different implementations. In my opinion, writing code for "majority rule" and shorter DMI lists is the most maintainable solution. Let us try to find a compromise, ok? Right now, for the machines we know about, there are more implementations where _INI lives at the bridge level and correctly initialize _SUN, so it's easier to follow my original approach. If we can get this into linux-next and get more exposure on more platforms, and we find out that the other interpretation of the spec is more popular, then I will very happily ACK your patch, and we can have DMI calls to handle HP/IBM machines. Does that sound fair? [Let's ...
Hi Alex/Kenji-san,
Time for my 2 cents.
Based on my review of ACPI spec 3.0b I would add:
1. For a specific device that has only _STA and _SUN methods,
_SUN can be run and it's results can be trusted irrespective
of the _STA return value.
2. For a specific device that has _STA, _INI, and _SUN methods,
_SUN can be run and it's results can be trusted even if
_INI is not run because the device is absent. If the device
is present and _INI is run then _SUN cannot be run until
after _INI is run.
See ACPI spec 3.0b "6.1.8 _SUN (Slot User Number)" which
I checked this on the IBM x3850 and it appears to be true. I
instrumented register_slot() in drivers/pci/hotplug/acpiphp_glue.c
to print _STA and _SUN returns and got the following results with
slot 1 (PCI-X) populated, slot 2 (PCI-X) vacant, slot 3 (PCIe) vacant,
slot 4 (PCIe) populated, slot 5 (PCIe) vacant, and slot 6 (PCIe)
populated. When a card is present _STA returns non-zero status
for all functions, otherwise it returns zero. None of the SxFy
devices have an _INI method.
acpiphp: ACPI Hot Plug PCI Controller Driver version: 0.5
acpiphp_glue: found PCI host-bus bridge with hot-pluggable slots
acpiphp_glue: register_slot: \_SB_.VP02.S1F0 sta=0 sun=1
acpiphp_glue: found ACPI PCI Hotplug slot 1 at PCI 0000:02:01
acpiphp: Slot [1] registered
acpiphp_glue: register_slot: \_SB_.VP02.S1F1 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F2 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F3 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F4 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F5 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F6 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F7 sta=0 sun=1
acpiphp_glue: found PCI host-bus bridge with hot-pluggable slots
acpiphp_glue: register_slot: \_SB_.VP03.S2F0 sta=f sun=2
acpiphp_glue: found ACPI PCI Hotplug slot 2 at PCI 0000:06:01
acpiphp: Slot [2] registered
acpiphp_glue: register_slot: ...Alex-san, Gary-san and Kristen-san, Thank you for your explanation. Ok, I understood what you mean and I agreed both implementations are legal. Also I explained it to our firmware team, and they understood your situation. I don't know which implementation is more popular. But according to the current score (2 : 1), you win the "majority rule". Alex-san, could you send me the updated patches? I'll Ack the patch after adding Fujitsu firmware versions to the DMI list and testing it. Kristen-san, as I told you in another thread, we might need a same fix for acpiphp driver. I'll check it and send the patch. Thanks, Kenji Kaneshige --
Yep, and when we get more testing, if it turns out that more vendors follow the Fujitsu interpretation, then we can change the Yes, I'll send out a new patch series today that also fixes the shpchp/pciehp naming issue. Thanks. /ac --
Yes, it is one of the reason we should evaluate _STA before evaluating _STA. Maybe you're referring '1023' as a _STA value on Fujitsu server, but it is _SUN value. If a driver evaluate _SUN even if its corresponding _STA is 0, Fujitsu firmware returns '1023' as a _SUN value. The reason why Fujitsu firmware is doing so is because ACPI doesn't provide a way to Yes. I thought it's a goot idea too. But it doesn't work because ACPI2.0 based HP's machines also want Alex's driver to ignore _STA. Thanks, Kenji Kaneshige --
On Tue, 11 Mar 2008 22:10:40 +0900 I have followed up with a question on this on another thread, so I'll skip to #2... I think this should be done - if the pci slot driver detects that a hotplug driver is controlling a slot, it should allow that driver to I'm not sure if we can do this, since slot name might depend on what I think we should definitely implement this one ^^^. --
Yes, it is what I thought. I'm not sure too. But I think we can unify the slot names using the combination of chassis number and slot number among the controllers Yes. It's the simplest workaround. BTW, I'll be out of office tomorrow. So replying to this discussion will be delayed. I'm sorry about that. Thanks, Kenji Kaneshige --
Stronger guarantee here, since both pci_slot and <foo>hp driver We saw problems on Fujitsu machines, where they return an error code when the _SUN method is called on a slot that exists in the namespace but isn't actually present. After discussing with Kenji-san about specs, we came to the agreement that he was ok with this behavior because he had the option to not load pci_slot on his machines. I agree that there might be lots of buggy firmwares out there, but we won't know for certain until we get some exposure. And I think the upside is worth it. Kristen suggested the linux-next tree. That sounds viable to Of course we shouldn't break userspace, no one wants that. But nothing about that name (/sys/bus/pci/slots/) implies "hotplug only", and we have no idea how big the problem might be. Again, I'm thinking more exposure in linux-next might be a reasonable way for us to figure out how bad (or good) the situation might really be out there. Thanks. /ac --
But that is what the current code does, so I know a lot of userspace programs assume that all slots there are valid hotplug slots. I know I sure don't want to go fix up a lot of horrible Java Ok, care to resend them with the requested updates? thanks, greg k-h --
Hi Greg, all, Thanks, I need to review my patch that touches SGI after thinking about Prarit's comment some more. Look for v8 of series to come out later (and please ignore that stray patch I sent out a few minutes ago with subject [PATCH 3/4, v8]). Thanks. /ac --
Um, the obvious solution of providing a release function for these kobjects is somehow not correct? Please do that, otherwise the code is wrong (and yes, acpiphp might be wrong as well, I haven't seen that report yet.) thanks, greg k-h --
Hrm, maybe this is a false alarm? Those messages appear when doing an rmmod <foo>. Turns out you get those messages for lots of modules, for instance, uhci_ucd: [root@canola ~]# rmmod uhci_hcd kobject: 'uhci_hcd' (e0000005078e0000): kobject_cleanup kobject: 'uhci_hcd' (e0000005078e0000): auto cleanup 'remove' event kobject: 'uhci_hcd' (e0000005078e0000): kobject_uevent_env kobject: 'uhci_hcd' (e0000005078e0000): fill_kobj_path: path = '/bus/pci/drivers/uhci_hcd' kobject: 'uhci_hcd' (e0000005078e0000): auto cleanup kobject_del kobject: 'uhci_hcd' (e0000005078e0000): calling ktype release kobject: 'uhci_hcd': free name kobject: 'drivers' (e0000005065a0060): kobject_cleanup kobject: 'drivers' (e0000005065a0060): auto cleanup kobject_del kobject: 'drivers' (e0000005065a0060): calling ktype release kobject: (e0000005065a0060): dynamic_kobj_release kobject: 'drivers': free name kobject: 'holders' (e0000005065a0000): kobject_cleanup kobject: 'holders' (e0000005065a0000): auto cleanup kobject_del kobject: 'holders' (e0000005065a0000): calling ktype release kobject: (e0000005065a0000): dynamic_kobj_release kobject: 'holders': free name kobject: 'uhci_hcd' (a00000020427c8d0): kobject_cleanup kobject: 'uhci_hcd' (a00000020427c8d0): does not have a release() function, it is broken and must be fixed. kobject: 'uhci_hcd' (a00000020427c8d0): auto cleanup 'remove' event kobject: 'uhci_hcd' (a00000020427c8d0): kobject_uevent_env kobject: 'uhci_hcd' (a00000020427c8d0): fill_kobj_path: path = '/module/uhci_hcd' kobject: 'uhci_hcd' (a00000020427c8d0): auto cleanup kobject_del kobject: 'uhci_hcd': free name Are you saying that modules that call module_init/module_exit are supposed to supply a release() function? (those messages from my earlier email only came out when I did an rmmod <foo>, for foo = acpiphp, pci_slot) Thanks. /ac --
No, the module core needs to do this, as that is the owner of this kobject, not the module author. I'll add it to my todo list :( thanks, greg k-h --
