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 <[email protected]>
Signed-off-by: Matthew Wilcox <[email protected]>
---
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 = slot->hotplug_slot;
list_del(&((struct slot *)bss_hotplug_slot->private)->
hp_list);
- sysfs_remove_file(&bss_hotplug_slot->kobj,
- &sn_slot_path_attr.attr);
break;
}
return bss_hotplug_slot;
@@ -653,11 +628,6 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
rc = pci_hp_register(bss_hotplug_slot);
if (rc)
goto register_err;
-
- rc = sysfs_create_file(&bss_hotplug_slot->kobj,
- &sn_slot_path_attr.attr);
- if (rc)
- goto register_err;
}
dev_dbg(&pci_bus->self->dev, "Registered bus with hotplug\n");
return rc;
--
1.5.3.1.g1e61
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 | 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;
}
static void remove_slot(struct dummy_slot *dslot)
@@ -296,23 +299,9 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
return -ENODEV;
}
-/* find the hotplug_slot for the pci_dev */
-static struct hotplug_slot *get_slot_from_dev(struct pci_dev *dev)
-{
- struct dummy_slot *dslot;
-
- list_for_each_entry(dslot, &slot_list, node) {
- if (dslot->dev == dev)
- return dslot->slot;
- }
- return NULL;
-}
-
-
static int disable_slot(struct hotplug_slot *slot)
{
struct dummy_slot *dslot;
- struct hotplug_slot *hslot;
struct pci_dev *dev;
int func;
@@ -322,41 +311,27 @@ static int disable_slot(struct hotplug_slot *slot)
dbg("%s - physical_slot = %s\n", __FUNCTION__, slot->name);
- /* don't disable bridged devices just yet, we can't handle them easily... */
- if (dslot->dev->subordinate) {
- err("Can't remove PCI devices with other PCI devices behind it yet.\n");
- return -ENODEV;
- }
- if (test_and_set_bit(0, &dslot->removed)) {
- dbg("Slot already scheduled for removal\n");
- return -ENODEV;
- }
- /* search for subfunctions and disable them first */
- if (!(dslot->dev->devfn & 7)) {
- for (func = 1; func < 8; func++) {
- dev = pci_get_slot(dslot->dev->bus,
- dslot->dev->devfn + func);
- if (dev) {
- hslot = get_slot_from_dev(dev);
- if (hslot)
- disable_slot(hslot);
- else {
- err("Hotplug slot not found for subfunction of PCI device\n");
- return -ENODEV;
- }
- pci_dev_put(dev);
- } else
- dbg("No device in slot found\n");
+ for (func = 7; func >= 0; func--) {
+ dev = pci_get_slot(dslot->dev->bus, dslot->dev->devfn + func);
+ if (!dev)
+ continue;
+
+ if (test_and_set_bit(0, &dslot->removed)) {
+ dbg("Slot already scheduled for removal\n");
+ return -ENODEV;
}
- }
- /* remove the device from the pci core */
- pci_remove_bus_device(dslot->dev);
+ /* queue work item to blow away this sysfs entry and other
+ * parts.
+ */
+ INIT_WORK(&dslot->remove_work, remove_slot_worker);
+ queue_work(dummyphp_wq, &dslot->remove_work);
- /* queue work item to blow away this sysfs entry and other parts. */
- INIT_WORK(&dslot->remove_work, remove_slot_worker);
- queue_work(dummyphp_wq, &dslot->remove_work);
+ /* blow away this sysfs entry and other parts. */
+ remove_slot(dslot);
+ pci_dev_put(dev);
+ }
return 0;
}
--
1.5.3.1.g1e61
- 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 <[email protected]>
Signed-off-by: Matthew Wilcox <[email protected]>
---
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 | 224 ++++++++++++++++++++++++++++
include/linux/pci.h | 17 ++
include/linux/pci_hotplug.h | 12 +-
19 files changed, 386 insertions(+), 247 deletions(-)
create mode 100644 drivers/pci/slot.c
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 4d1ce2e..7d63f8c 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -2,7 +2,7 @@
# Makefile for the PCI bus specific drivers.
#
-obj-y += access.o bus.o probe.o remove.o pci.o quirks.o \
+obj-y += access.o bus.o probe.o remove.o pci.o quirks.o slot.o \
pci-driver.o search.o pci-sysfs.o rom.o setup-res.o
obj-$(CONFIG_PROC_FS) += proc.o
diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 7a29164..eecf7cb 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -215,7 +215,6 @@ extern u8 acpiphp_get_power_status (struct acpiphp_slot *slot);
extern u8 acpiphp_get_attention_status (struct acpiphp_slot *slot);
extern u8 acpiphp_get_latch_status (struct acpiphp_slot *slot);
extern u8 acpiphp_get_adapter_status (struct acpiphp_slot *slot);
-extern u32 acpiphp_get_address (struct acpiphp_slot *slot);
/* variables */
extern int acpiphp_debug;
diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
index 9279d5b..57319e6 100644
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -70,7 +70,6 @@ static int disable_slot (struct hotplug_slot *slot);
static int set_attention_status (struct hotplug_slot *slot, u8 value);
static int get_power_status (struct hotplug_slot *slot, u8 *value);
static int get_attention_status (struct hotplug_slot *slot, u8 *value);
-static int get_address (struct hotplug_slot *slot, u32 *value);
static int get_latch_status (struct hotplug_slot *slot, u8 *value);
static int get_adapter_status (struct hotplug_slot *slot, u8 *value);
@@ -83,7 +82,6 @@ static struct hotplug_slot_ops acpi_hotplug_slot_ops = {
.get_attention_status = get_attention_status,
.get_latch_status = get_latch_status,
.get_adapter_status = get_adapter_status,
- .get_address = get_address,
};
@@ -274,23 +272,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
return 0;
}
-
-/**
- * get_address - get pci address of a slot
- * @hotplug_slot: slot to get status
- * @value: pointer to struct pci_busdev (seg, bus, dev)
- */
-static int get_address(struct hotplug_slot *hotplug_slot, u32 *value)
-{
- struct slot *slot = hotplug_slot->private;
-
- dbg("%s - physical_slot = %s\n", __FUNCTION__, hotplug_slot->name);
-
- *value = acpiphp_get_address(slot->acpi_slot);
-
- return 0;
-}
-
static int __init init_acpi(void)
{
int retval;
@@ -357,7 +338,11 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
acpiphp_slot->slot = slot;
snprintf(slot->name, sizeof(slot->name), "%u", slot->acpi_slot->sun);
- retval = pci_hp_register(slot->hotplug_slot);
+ retval = pci_hp_register(slot->hotplug_slot,
+ acpiphp_slot->bridge->pci_bus,
+ acpiphp_slot->device);
+ if (retval == -EBUSY)
+ goto error_hpslot;
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
goto error_hpslot;
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index cf22f9e..f84c4bd 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -258,7 +258,12 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
bridge->pci_bus->number, slot->device);
retval = acpiphp_register_hotplug_slot(slot);
if (retval) {
- warn("acpiphp_register_hotplug_slot failed(err code = 0x%x)\n", retval);
+ if (retval == -EBUSY)
+ warn("Slot %d already registered by another "
+ "hotplug driver\n", slot->sun);
+ else
+ warn("acpiphp_register_hotplug_slot failed "
+ "(err code = 0x%x)\n", retval);
goto err_exit;
}
}
@@ -1867,19 +1872,3 @@ u8 acpiphp_get_adapter_status(struct acpiphp_slot *slot)
return (sta == 0) ? 0 : 1;
}
-
-
-/*
- * pci address (seg/bus/dev)
- */
-u32 acpiphp_get_address(struct acpiphp_slot *slot)
-{
- u32 address;
- struct pci_bus *pci_bus = slot->bridge->pci_bus;
-
- address = (pci_domain_nr(pci_bus) << 16) |
- (pci_bus->number << 8) |
- slot->device;
-
- return address;
-}
diff --git a/drivers/pci/hotplug/acpiphp_ibm.c b/drivers/pci/hotplug/acpiphp_ibm.c
index b0a22b9..ed76879 100644
--- a/drivers/pci/hotplug/acpiphp_ibm.c
+++ b/drivers/pci/hotplug/acpiphp_ibm.c
@@ -35,6 +35,7 @@
#include <linux/moduleparam.h>
#include "acpiphp.h"
+extern struct kset *pci_slots_kset;
#define DRIVER_VERSION "1.0.1"
#define DRIVER_AUTHOR "Irene Zubarev <[email protected]>, Vernon Mauery <[email protected]>"
@@ -430,7 +431,7 @@ static int __init ibm_acpiphp_init(void)
int retval = 0;
acpi_status status;
struct acpi_device *device;
- struct kobject *sysdir = &pci_hotplug_slots_kset->kobj;
+ struct kobject *sysdir = &pci_slots_kset->kobj;
dbg("%s\n", __FUNCTION__);
@@ -477,7 +478,7 @@ init_return:
static void __exit ibm_acpiphp_exit(void)
{
acpi_status status;
- struct kobject *sysdir = &pci_hotplug_slots_kset->kobj;
+ struct kobject *sysdir = &pci_slots_kset->kobj;
dbg("%s\n", __FUNCTION__);
diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c
index ed4d44e..aa47b80 100644
--- a/drivers/pci/hotplug/cpci_hotplug_core.c
+++ b/drivers/pci/hotplug/cpci_hotplug_core.c
@@ -285,7 +285,7 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
info->attention_status = cpci_get_attention_status(slot);
dbg("registering slot %s", slot->hotplug_slot->name);
- status = pci_hp_register(slot->hotplug_slot);
+ status = pci_hp_register(slot->hotplug_slot, bus, i);
if (status) {
err("pci_hp_register failed with error %d", status);
goto error_name;
diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c
index 7417887..2e0392e 100644
--- a/drivers/pci/hotplug/cpqphp_core.c
+++ b/drivers/pci/hotplug/cpqphp_core.c
@@ -434,7 +434,9 @@ static int ctrl_slot_setup(struct controller *ctrl,
slot->bus, slot->device,
slot->number, ctrl->slot_device_offset,
slot_number);
- result = pci_hp_register(hotplug_slot);
+ result = pci_hp_register(hotplug_slot,
+ ctrl->pci_dev->subordinate,
+ slot->device);
if (result) {
err("pci_hp_register failed with error %d\n", result);
goto error_name;
diff --git a/drivers/pci/hotplug/fakephp.c b/drivers/pci/hotplug/fakephp.c
index 6c14b4d..2d84755 100644
--- a/drivers/pci/hotplug/fakephp.c
+++ b/drivers/pci/hotplug/fakephp.c
@@ -127,7 +127,7 @@ static int add_slot(struct pci_dev *dev)
slot->release = &dummy_release;
slot->private = dslot;
- retval = pci_hp_register(slot);
+ retval = pci_hp_register(slot, dev->bus, PCI_SLOT(dev->devfn));
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
goto error_dslot;
diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c
index 600ed7b..eb7a1c0 100644
--- a/drivers/pci/hotplug/ibmphp_ebda.c
+++ b/drivers/pci/hotplug/ibmphp_ebda.c
@@ -1000,7 +1000,8 @@ static int __init ebda_rsrc_controller (void)
tmp_slot = list_entry (list, struct slot, ibm_slot_list);
snprintf (tmp_slot->hotplug_slot->name, 30, "%s", create_file_name (tmp_slot));
- pci_hp_register (tmp_slot->hotplug_slot);
+ pci_hp_register(tmp_slot->hotplug_slot,
+ pci_find_bus(0, tmp_slot->bus), tmp_slot->device);
}
print_ebda_hpc ();
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index dd59a05..21bbb5e 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -40,6 +40,7 @@
#include <linux/pci.h>
#include <linux/pci_hotplug.h>
#include <asm/uaccess.h>
+#include "../pci.h"
#define MY_NAME "pci_hotplug"
@@ -61,41 +62,6 @@ static int debug;
static LIST_HEAD(pci_hotplug_slot_list);
-struct kset *pci_hotplug_slots_kset;
-
-static ssize_t hotplug_slot_attr_show(struct kobject *kobj,
- struct attribute *attr, char *buf)
-{
- struct hotplug_slot *slot = to_hotplug_slot(kobj);
- struct hotplug_slot_attribute *attribute = to_hotplug_attr(attr);
- return attribute->show ? attribute->show(slot, buf) : -EIO;
-}
-
-static ssize_t hotplug_slot_attr_store(struct kobject *kobj,
- struct attribute *attr, const char *buf, size_t len)
-{
- struct hotplug_slot *slot = to_hotplug_slot(kobj);
- struct hotplug_slot_attribute *attribute = to_hotplug_attr(attr);
- return attribute->store ? attribute->store(slot, buf, len) : -EIO;
-}
-
-static struct sysfs_ops hotplug_slot_sysfs_ops = {
- .show = hotplug_slot_attr_show,
- .store = hotplug_slot_attr_store,
-};
-
-static void hotplug_slot_release(struct kobject *kobj)
-{
- struct hotplug_slot *slot = to_hotplug_slot(kobj);
- if (slot->release)
- slot->release(slot);
-}
-
-static struct kobj_type hotplug_slot_ktype = {
- .sysfs_ops = &hotplug_slot_sysfs_ops,
- .release = &hotplug_slot_release,
-};
-
/* these strings match up with the values in pci_bus_speed */
static char *pci_bus_speed_strings[] = {
"33 MHz PCI", /* 0x00 */
@@ -149,16 +115,15 @@ GET_STATUS(power_status, u8)
GET_STATUS(attention_status, u8)
GET_STATUS(latch_status, u8)
GET_STATUS(adapter_status, u8)
-GET_STATUS(address, u32)
GET_STATUS(max_bus_speed, enum pci_bus_speed)
GET_STATUS(cur_bus_speed, enum pci_bus_speed)
-static ssize_t power_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t power_read_file(struct pci_slot *slot, char *buf)
{
int retval;
u8 value;
- retval = get_power_status (slot, &value);
+ retval = get_power_status(slot->hotplug, &value);
if (retval)
goto exit;
retval = sprintf (buf, "%d\n", value);
@@ -166,9 +131,10 @@ exit:
return retval;
}
-static ssize_t power_write_file (struct hotplug_slot *slot, const char *buf,
+static ssize_t power_write_file(struct pci_slot *pci_slot, const char *buf,
size_t count)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
unsigned long lpower;
u8 power;
int retval = 0;
@@ -204,29 +170,30 @@ exit:
return count;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_power = {
+static struct pci_slot_attribute hotplug_slot_attr_power = {
.attr = {.name = "power", .mode = S_IFREG | S_IRUGO | S_IWUSR},
.show = power_read_file,
.store = power_write_file
};
-static ssize_t attention_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t attention_read_file(struct pci_slot *slot, char *buf)
{
int retval;
u8 value;
- retval = get_attention_status (slot, &value);
+ retval = get_attention_status(slot->hotplug, &value);
if (retval)
goto exit;
- retval = sprintf (buf, "%d\n", value);
+ retval = sprintf(buf, "%d\n", value);
exit:
return retval;
}
-static ssize_t attention_write_file (struct hotplug_slot *slot, const char *buf,
+static ssize_t attention_write_file(struct pci_slot *slot, const char *buf,
size_t count)
{
+ struct hotplug_slot_ops *ops = slot->hotplug->ops;
unsigned long lattention;
u8 attention;
int retval = 0;
@@ -235,13 +202,13 @@ static ssize_t attention_write_file (struct hotplug_slot *slot, const char *buf,
attention = (u8)(lattention & 0xff);
dbg (" - attention = %d\n", attention);
- if (!try_module_get(slot->ops->owner)) {
+ if (!try_module_get(ops->owner)) {
retval = -ENODEV;
goto exit;
}
- if (slot->ops->set_attention_status)
- retval = slot->ops->set_attention_status(slot, attention);
- module_put(slot->ops->owner);
+ if (ops->set_attention_status)
+ retval = ops->set_attention_status(slot->hotplug, attention);
+ module_put(ops->owner);
exit:
if (retval)
@@ -249,18 +216,18 @@ exit:
return count;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_attention = {
+static struct pci_slot_attribute hotplug_slot_attr_attention = {
.attr = {.name = "attention", .mode = S_IFREG | S_IRUGO | S_IWUSR},
.show = attention_read_file,
.store = attention_write_file
};
-static ssize_t latch_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t latch_read_file(struct pci_slot *slot, char *buf)
{
int retval;
u8 value;
- retval = get_latch_status (slot, &value);
+ retval = get_latch_status(slot->hotplug, &value);
if (retval)
goto exit;
retval = sprintf (buf, "%d\n", value);
@@ -269,17 +236,17 @@ exit:
return retval;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_latch = {
+static struct pci_slot_attribute hotplug_slot_attr_latch = {
.attr = {.name = "latch", .mode = S_IFREG | S_IRUGO},
.show = latch_read_file,
};
-static ssize_t presence_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t presence_read_file(struct pci_slot *slot, char *buf)
{
int retval;
u8 value;
- retval = get_adapter_status (slot, &value);
+ retval = get_adapter_status(slot->hotplug, &value);
if (retval)
goto exit;
retval = sprintf (buf, "%d\n", value);
@@ -288,42 +255,20 @@ exit:
return retval;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_presence = {
+static struct pci_slot_attribute hotplug_slot_attr_presence = {
.attr = {.name = "adapter", .mode = S_IFREG | S_IRUGO},
.show = presence_read_file,
};
-static ssize_t address_read_file (struct hotplug_slot *slot, char *buf)
-{
- int retval;
- u32 address;
-
- retval = get_address (slot, &address);
- if (retval)
- goto exit;
- retval = sprintf (buf, "%04x:%02x:%02x\n",
- (address >> 16) & 0xffff,
- (address >> 8) & 0xff,
- address & 0xff);
-
-exit:
- return retval;
-}
-
-static struct hotplug_slot_attribute hotplug_slot_attr_address = {
- .attr = {.name = "address", .mode = S_IFREG | S_IRUGO},
- .show = address_read_file,
-};
-
static char *unknown_speed = "Unknown bus speed";
-static ssize_t max_bus_speed_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t max_bus_speed_read_file(struct pci_slot *slot, char *buf)
{
char *speed_string;
int retval;
enum pci_bus_speed value;
- retval = get_max_bus_speed (slot, &value);
+ retval = get_max_bus_speed(slot->hotplug, &value);
if (retval)
goto exit;
@@ -338,18 +283,18 @@ exit:
return retval;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_max_bus_speed = {
+static struct pci_slot_attribute hotplug_slot_attr_max_bus_speed = {
.attr = {.name = "max_bus_speed", .mode = S_IFREG | S_IRUGO},
.show = max_bus_speed_read_file,
};
-static ssize_t cur_bus_speed_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t cur_bus_speed_read_file(struct pci_slot *slot, char *buf)
{
char *speed_string;
int retval;
enum pci_bus_speed value;
- retval = get_cur_bus_speed (slot, &value);
+ retval = get_cur_bus_speed(slot->hotplug, &value);
if (retval)
goto exit;
@@ -364,14 +309,15 @@ exit:
return retval;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_cur_bus_speed = {
+static struct pci_slot_attribute hotplug_slot_attr_cur_bus_speed = {
.attr = {.name = "cur_bus_speed", .mode = S_IFREG | S_IRUGO},
.show = cur_bus_speed_read_file,
};
-static ssize_t test_write_file (struct hotplug_slot *slot, const char *buf,
+static ssize_t test_write_file(struct pci_slot *pci_slot, const char *buf,
size_t count)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
unsigned long ltest;
u32 test;
int retval = 0;
@@ -394,13 +340,14 @@ exit:
return count;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_test = {
+static struct pci_slot_attribute hotplug_slot_attr_test = {
.attr = {.name = "test", .mode = S_IFREG | S_IRUGO | S_IWUSR},
.store = test_write_file
};
-static int has_power_file (struct hotplug_slot *slot)
+static int has_power_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if ((slot->ops->enable_slot) ||
@@ -410,8 +357,9 @@ static int has_power_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_attention_file (struct hotplug_slot *slot)
+static int has_attention_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if ((slot->ops->set_attention_status) ||
@@ -420,8 +368,9 @@ static int has_attention_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_latch_file (struct hotplug_slot *slot)
+static int has_latch_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->get_latch_status)
@@ -429,8 +378,9 @@ static int has_latch_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_adapter_file (struct hotplug_slot *slot)
+static int has_adapter_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->get_adapter_status)
@@ -438,17 +388,9 @@ static int has_adapter_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_address_file (struct hotplug_slot *slot)
-{
- if ((!slot) || (!slot->ops))
- return -ENODEV;
- if (slot->ops->get_address)
- return 0;
- return -ENOENT;
-}
-
-static int has_max_bus_speed_file (struct hotplug_slot *slot)
+static int has_max_bus_speed_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->get_max_bus_speed)
@@ -456,8 +398,9 @@ static int has_max_bus_speed_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_cur_bus_speed_file (struct hotplug_slot *slot)
+static int has_cur_bus_speed_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->get_cur_bus_speed)
@@ -465,8 +408,9 @@ static int has_cur_bus_speed_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_test_file (struct hotplug_slot *slot)
+static int has_test_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->hardware_test)
@@ -474,7 +418,7 @@ static int has_test_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int fs_add_slot (struct hotplug_slot *slot)
+static int fs_add_slot(struct pci_slot *slot)
{
int retval = 0;
@@ -505,13 +449,6 @@ static int fs_add_slot (struct hotplug_slot *slot)
goto exit_adapter;
}
- if (has_address_file(slot) == 0) {
- retval = sysfs_create_file(&slot->kobj,
- &hotplug_slot_attr_address.attr);
- if (retval)
- goto exit_address;
- }
-
if (has_max_bus_speed_file(slot) == 0) {
retval = sysfs_create_file(&slot->kobj,
&hotplug_slot_attr_max_bus_speed.attr);
@@ -544,10 +481,6 @@ exit_cur_speed:
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_max_bus_speed.attr);
exit_max_speed:
- if (has_address_file(slot) == 0)
- sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_address.attr);
-
-exit_address:
if (has_adapter_file(slot) == 0)
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_presence.attr);
@@ -567,7 +500,7 @@ exit:
return retval;
}
-static void fs_remove_slot (struct hotplug_slot *slot)
+static void fs_remove_slot(struct pci_slot *slot)
{
if (has_power_file(slot) == 0)
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_power.attr);
@@ -581,9 +514,6 @@ static void fs_remove_slot (struct hotplug_slot *slot)
if (has_adapter_file(slot) == 0)
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_presence.attr);
- if (has_address_file(slot) == 0)
- sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_address.attr);
-
if (has_max_bus_speed_file(slot) == 0)
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_max_bus_speed.attr);
@@ -607,6 +537,12 @@ static struct hotplug_slot *get_slot_from_name (const char *name)
return NULL;
}
+static void hotplug_release(struct pci_slot *slot)
+{
+ struct hotplug_slot *hotplug = slot->hotplug;
+ hotplug->release(hotplug);
+}
+
/**
* pci_hp_register - register a hotplug_slot with the PCI hotplug subsystem
* @slot: pointer to the &struct hotplug_slot to register
@@ -616,9 +552,10 @@ static struct hotplug_slot *get_slot_from_name (const char *name)
*
* Returns 0 if successful, anything else for an error.
*/
-int pci_hp_register (struct hotplug_slot *slot)
+int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
{
int result;
+ struct pci_slot *pci_slot;
if (slot == NULL)
return -ENODEV;
@@ -630,20 +567,24 @@ int pci_hp_register (struct hotplug_slot *slot)
return -EINVAL;
}
- /* this can fail if we have already registered a slot with the same name */
- slot->kobj.kset = pci_hotplug_slots_kset;
- result = kobject_init_and_add(&slot->kobj, &hotplug_slot_ktype, NULL,
- "%s", slot->name);
+ pci_slot = pci_create_slot(bus, slot_nr, slot->name);
+ if (IS_ERR(pci_slot))
+ return PTR_ERR(pci_slot);
+
+ result = pci_slot_add_hotplug(bus, slot_nr, hotplug_release);
if (result) {
- err("Unable to register kobject '%s'", slot->name);
- return -EINVAL;
+ pci_destroy_slot(pci_slot);
+ return result;
}
- list_add (&slot->slot_list, &pci_hotplug_slot_list);
+ slot->pci_slot = pci_slot;
+ pci_slot->hotplug = slot;
+
+ list_add(&slot->slot_list, &pci_hotplug_slot_list);
- result = fs_add_slot (slot);
- kobject_uevent(&slot->kobj, KOBJ_ADD);
- dbg ("Added slot %s to the list\n", slot->name);
+ result = fs_add_slot(pci_slot);
+ kobject_uevent(&pci_slot->kobj, KOBJ_ADD);
+ dbg("Added slot %s to the list\n", slot->name);
return result;
}
@@ -656,22 +597,24 @@ int pci_hp_register (struct hotplug_slot *slot)
*
* Returns 0 if successful, anything else for an error.
*/
-int pci_hp_deregister (struct hotplug_slot *slot)
+int pci_hp_deregister(struct hotplug_slot *hotplug)
{
struct hotplug_slot *temp;
+ struct pci_slot *slot;
- if (slot == NULL)
+ if (!hotplug)
return -ENODEV;
- temp = get_slot_from_name (slot->name);
- if (temp != slot) {
+ temp = get_slot_from_name(hotplug->name);
+ if (temp != hotplug)
return -ENODEV;
- }
- list_del (&slot->slot_list);
- fs_remove_slot (slot);
- dbg ("Removed slot %s from the list\n", slot->name);
- kobject_put(&slot->kobj);
+ list_del(&hotplug->slot_list);
+
+ slot = hotplug->pci_slot;
+ fs_remove_slot(slot);
+ pci_destroy_slot(slot);
+ dbg("Removed slot %s from the list\n", hotplug->name);
return 0;
}
@@ -685,13 +628,15 @@ int pci_hp_deregister (struct hotplug_slot *slot)
*
* Returns 0 if successful, anything else for an error.
*/
-int __must_check pci_hp_change_slot_info(struct hotplug_slot *slot,
+int __must_check pci_hp_change_slot_info(struct hotplug_slot *hotplug,
struct hotplug_slot_info *info)
{
- if ((slot == NULL) || (info == NULL))
+ struct pci_slot *slot;
+ if (!hotplug || !info)
return -ENODEV;
+ slot = hotplug->pci_slot;
- memcpy (slot->info, info, sizeof (struct hotplug_slot_info));
+ memcpy(hotplug->info, info, sizeof(struct hotplug_slot_info));
return 0;
}
@@ -699,36 +644,22 @@ int __must_check pci_hp_change_slot_info(struct hotplug_slot *slot,
static int __init pci_hotplug_init (void)
{
int result;
- struct kset *pci_bus_kset;
- pci_bus_kset = bus_get_kset(&pci_bus_type);
-
- pci_hotplug_slots_kset = kset_create_and_add("slots", NULL,
- &pci_bus_kset->kobj);
- if (!pci_hotplug_slots_kset) {
- result = -ENOMEM;
- err("Register subsys error\n");
- goto exit;
- }
result = cpci_hotplug_init(debug);
if (result) {
err ("cpci_hotplug_init with error %d\n", result);
- goto err_subsys;
+ goto err_cpci;
}
info (DRIVER_DESC " version: " DRIVER_VERSION "\n");
- goto exit;
-err_subsys:
- kset_unregister(pci_hotplug_slots_kset);
-exit:
+err_cpci:
return result;
}
static void __exit pci_hotplug_exit (void)
{
cpci_hotplug_exit();
- kset_unregister(pci_hotplug_slots_kset);
}
module_init(pci_hotplug_init);
@@ -740,7 +671,6 @@ MODULE_LICENSE("GPL");
module_param(debug, bool, 0644);
MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
-EXPORT_SYMBOL_GPL(pci_hotplug_slots_kset);
EXPORT_SYMBOL_GPL(pci_hp_register);
EXPORT_SYMBOL_GPL(pci_hp_deregister);
EXPORT_SYMBOL_GPL(pci_hp_change_slot_info);
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 7f4836b..ab45b69 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -69,7 +69,6 @@ static int get_power_status (struct hotplug_slot *slot, u8 *value);
static int get_attention_status (struct hotplug_slot *slot, u8 *value);
static int get_latch_status (struct hotplug_slot *slot, u8 *value);
static int get_adapter_status (struct hotplug_slot *slot, u8 *value);
-static int get_address (struct hotplug_slot *slot, u32 *value);
static int get_max_bus_speed (struct hotplug_slot *slot, enum pci_bus_speed *value);
static int get_cur_bus_speed (struct hotplug_slot *slot, enum pci_bus_speed *value);
@@ -82,7 +81,6 @@ static struct hotplug_slot_ops pciehp_hotplug_slot_ops = {
.get_attention_status = get_attention_status,
.get_latch_status = get_latch_status,
.get_adapter_status = get_adapter_status,
- .get_address = get_address,
.get_max_bus_speed = get_max_bus_speed,
.get_cur_bus_speed = get_cur_bus_speed,
};
@@ -245,14 +243,18 @@ static int init_slots(struct controller *ctrl)
dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
"slot_device_offset=%x\n", slot->bus, slot->device,
slot->hp_slot, slot->number, ctrl->slot_device_offset);
- retval = pci_hp_register(hotplug_slot);
+ retval = pci_hp_register(hotplug_slot,
+ ctrl->pci_dev->subordinate,
+ slot->device);
+ if (retval == -EBUSY)
+ goto error_info;
if (retval) {
err ("pci_hp_register failed with error %d\n", retval);
goto error_info;
}
/* create additional sysfs entries */
if (EMI(ctrl->ctrlcap)) {
- retval = sysfs_create_file(&hotplug_slot->kobj,
+ retval = sysfs_create_file(&hotplug_slot->pci_slot->kobj,
&hotplug_slot_attr_lock.attr);
if (retval) {
pci_hp_deregister(hotplug_slot);
@@ -285,7 +287,7 @@ static void cleanup_slots(struct controller *ctrl)
slot = list_entry(tmp, struct slot, slot_list);
list_del(&slot->slot_list);
if (EMI(ctrl->ctrlcap))
- sysfs_remove_file(&slot->hotplug_slot->kobj,
+ sysfs_remove_file(&slot->hotplug_slot->pci_slot->kobj,
&hotplug_slot_attr_lock.attr);
cancel_delayed_work(&slot->work);
flush_scheduled_work();
@@ -387,18 +389,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
return 0;
}
-static int get_address(struct hotplug_slot *hotplug_slot, u32 *value)
-{
- struct slot *slot = hotplug_slot->private;
- struct pci_bus *bus = slot->ctrl->pci_dev->subordinate;
-
- dbg("%s - physical_slot = %s\n", __FUNCTION__, hotplug_slot->name);
-
- *value = (pci_domain_nr(bus) << 16) | (slot->bus << 8) | slot->device;
-
- return 0;
-}
-
static int get_max_bus_speed(struct hotplug_slot *hotplug_slot, enum pci_bus_speed *value)
{
struct slot *slot = hotplug_slot->private;
@@ -460,7 +450,12 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
/* Setup the slot information structures */
rc = init_slots(ctrl);
if (rc) {
- err("%s: slot initialization failed\n", PCIE_MODULE_NAME);
+ if (rc == -EBUSY)
+ warn("%s: slot already registered by another "
+ "hotplug driver\n", PCIE_MODULE_NAME);
+ else
+ err("%s: slot initialization failed\n",
+ PCIE_MODULE_NAME);
goto err_out_release_ctlr;
}
diff --git a/drivers/pci/hotplug/rpadlpar_sysfs.c b/drivers/pci/hotplug/rpadlpar_sysfs.c
index e32148a..1a13703 100644
--- a/drivers/pci/hotplug/rpadlpar_sysfs.c
+++ b/drivers/pci/hotplug/rpadlpar_sysfs.c
@@ -23,6 +23,7 @@
#define MAX_DRC_NAME_LEN 64
+extern struct kset *pci_slots_kset;
static ssize_t add_slot_store(struct kobject *kobj, struct kobj_attribute *attr,
const char *buf, size_t nbytes)
@@ -108,7 +109,7 @@ int dlpar_sysfs_init(void)
int error;
dlpar_kobj = kobject_create_and_add(DLPAR_KOBJ_NAME,
- &pci_hotplug_slots_kset->kobj);
+ &pci_slots_kset->kobj);
if (!dlpar_kobj)
return -EINVAL;
diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c
index 8ad3deb..8e5fff0 100644
--- a/drivers/pci/hotplug/rpaphp_slot.c
+++ b/drivers/pci/hotplug/rpaphp_slot.c
@@ -162,7 +162,8 @@ int rpaphp_register_slot(struct slot *slot)
return -EAGAIN;
}
- retval = pci_hp_register(php_slot);
+ retval = pci_hp_register(php_slot, slot->bus,
+ PCI_SLOT(PCI_DN(slot->dn->child)->devfn));
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
return retval;
diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
index 693519e..cc74602 100644
--- a/drivers/pci/hotplug/sgi_hotplug.c
+++ b/drivers/pci/hotplug/sgi_hotplug.c
@@ -625,7 +625,7 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
bss_hotplug_slot->ops = &sn_hotplug_slot_ops;
bss_hotplug_slot->release = &sn_release_slot;
- rc = pci_hp_register(bss_hotplug_slot);
+ rc = pci_hp_register(bss_hotplug_slot, pci_bus, device);
if (rc)
goto register_err;
}
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index 80dec97..22c4d2e 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -65,7 +65,6 @@ static int get_power_status (struct hotplug_slot *slot, u8 *value);
static int get_attention_status (struct hotplug_slot *slot, u8 *value);
static int get_latch_status (struct hotplug_slot *slot, u8 *value);
static int get_adapter_status (struct hotplug_slot *slot, u8 *value);
-static int get_address (struct hotplug_slot *slot, u32 *value);
static int get_max_bus_speed (struct hotplug_slot *slot, enum pci_bus_speed *value);
static int get_cur_bus_speed (struct hotplug_slot *slot, enum pci_bus_speed *value);
@@ -78,7 +77,6 @@ static struct hotplug_slot_ops shpchp_hotplug_slot_ops = {
.get_attention_status = get_attention_status,
.get_latch_status = get_latch_status,
.get_adapter_status = get_adapter_status,
- .get_address = get_address,
.get_max_bus_speed = get_max_bus_speed,
.get_cur_bus_speed = get_cur_bus_speed,
};
@@ -152,7 +150,8 @@ static int init_slots(struct controller *ctrl)
dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
"slot_device_offset=%x\n", slot->bus, slot->device,
slot->hp_slot, slot->number, ctrl->slot_device_offset);
- retval = pci_hp_register(slot->hotplug_slot);
+ retval = pci_hp_register(slot->hotplug_slot,
+ ctrl->pci_dev->subordinate, slot->device);
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
goto error_info;
@@ -277,18 +276,6 @@ static int get_adapter_status (struct hotplug_slot *hotplug_slot, u8 *value)
return 0;
}
-static int get_address (struct hotplug_slot *hotplug_slot, u32 *value)
-{
- struct slot *slot = get_slot(hotplug_slot);
- struct pci_bus *bus = slot->ctrl->pci_dev->subordinate;
-
- dbg("%s - physical_slot = %s\n", __FUNCTION__, hotplug_slot->name);
-
- *value = (pci_domain_nr(bus) << 16) | (slot->bus << 8) | slot->device;
-
- return 0;
-}
-
static int get_max_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_speed *value)
{
struct slot *slot = get_slot(hotplug_slot);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index eabeb1f..61bb743 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -87,3 +87,16 @@ pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
}
struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
+
+/* PCI slot sysfs helper code */
+#define to_pci_slot(s) container_of(s, struct pci_slot, kobj)
+
+extern struct kset *pci_slots_kset;
+
+struct pci_slot_attribute {
+ struct attribute attr;
+ ssize_t (*show)(struct pci_slot *, char *);
+ ssize_t (*store)(struct pci_slot *, const char *, size_t);
+};
+#define to_pci_slot_attr(s) container_of(s, struct pci_slot_attribute, attr)
+
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
new file mode 100644
index 0000000..5b71b9f
--- /dev/null
+++ b/drivers/pci/slot.c
@@ -0,0 +1,224 @@
+/*
+ * drivers/pci/slot.c
+ * Copyright (C) 2006 Matthew Wilcox <[email protected]>
+ * Copyright (C) 2006,2007 Hewlett-Packard Development Company, L.P.
+ * Copyright (C) 2007 Alex Chiang <[email protected]>
+ */
+
+#include <linux/kobject.h>
+#include <linux/pci.h>
+#include "pci.h"
+
+static int pci_slot_debug;
+#define MY_NAME "slot"
+#define dbg(format, arg...) \
+ do { \
+ if (pci_slot_debug) \
+ printk(KERN_DEBUG "%s: " format, \
+ MY_NAME , ## arg); \
+ } while (0)
+
+struct kset *pci_slots_kset;
+EXPORT_SYMBOL_GPL(pci_slots_kset);
+
+static ssize_t pci_slot_attr_show(struct kobject *kobj,
+ struct attribute *attr, char *buf)
+{
+ struct pci_slot *slot = to_pci_slot(kobj);
+ struct pci_slot_attribute *attribute = to_pci_slot_attr(attr);
+ return attribute->show ? attribute->show(slot, buf) : -EIO;
+}
+
+static ssize_t pci_slot_attr_store(struct kobject *kobj,
+ struct attribute *attr, const char *buf, size_t len)
+{
+ struct pci_slot *slot = to_pci_slot(kobj);
+ struct pci_slot_attribute *attribute = to_pci_slot_attr(attr);
+ return attribute->store ? attribute->store(slot, buf, len) : -EIO;
+}
+
+static struct sysfs_ops pci_slot_sysfs_ops = {
+ .show = pci_slot_attr_show,
+ .store = pci_slot_attr_store,
+};
+
+static ssize_t address_read_file(struct pci_slot *slot, char *buf)
+{
+ return sprintf(buf, "%04x:%02x:%02x\n", pci_domain_nr(slot->bus),
+ slot->bus->number, slot->number);
+}
+
+static struct pci_slot_attribute pci_slot_attr_address = {
+ .attr = { .name = "address", .mode = S_IFREG | S_IRUGO },
+ .show = address_read_file,
+};
+
+static void remove_sysfs_files(struct pci_slot *slot)
+{
+ sysfs_remove_file(&slot->kobj, &pci_slot_attr_address.attr);
+}
+
+static int create_sysfs_files(struct pci_slot *slot)
+{
+ int result;
+
+ result = sysfs_create_file(&slot->kobj, &pci_slot_attr_address.attr);
+
+ return result;
+}
+
+static void pci_slot_release(struct kobject *kobj)
+{
+ struct pci_slot **pprev;
+ struct pci_slot *slot = to_pci_slot(kobj);
+
+ dbg("%s: releasing pci_slot on %x:%d\n", __FUNCTION__,
+ slot->bus->number, slot->number);
+
+ for (pprev = &slot->bus->slot; *pprev; pprev = &(*pprev)->next) {
+ if (*pprev == slot) {
+ *pprev = slot->next;
+ break;
+ }
+ }
+
+ if (slot->release)
+ slot->release(slot);
+
+ remove_sysfs_files(slot);
+ kfree(slot);
+}
+
+static struct kobj_type pci_slot_ktype = {
+ .sysfs_ops = &pci_slot_sysfs_ops,
+ .release = &pci_slot_release,
+};
+
+int pci_slot_add_hotplug(struct pci_bus *parent, int slot_nr,
+ void (*release)(struct pci_slot *))
+{
+ struct pci_slot *slot;
+ int retval, found;
+
+ retval = found = 0;
+
+ down_write(&pci_bus_sem);
+
+ /* This slot should have already been created, so look for it. If
+ * we can't find it, return -EEXIST.
+ */
+ for (slot = parent->slot; slot; slot = slot->next)
+ if (slot->number == slot_nr) {
+ found = 1;
+ break;
+ }
+
+ if (!found) {
+ dbg("%s: slot not found\n", __FUNCTION__);
+ retval = -EEXIST;
+ goto out;
+ }
+
+ if (slot->release) {
+ dbg("%s: already claimed\n", __FUNCTION__);
+ retval = -EBUSY;
+ goto out;
+ }
+
+ dbg("%s: adding release function to %x:%d\n",
+ __FUNCTION__, parent->number, slot_nr);
+ slot->release = release;
+ out:
+ up_write(&pci_bus_sem);
+ return retval;
+}
+EXPORT_SYMBOL_GPL(pci_slot_add_hotplug);
+
+struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
+ const char *name)
+{
+ struct pci_slot *slot;
+ int err;
+
+ down_write(&pci_bus_sem);
+
+ /* If we've already created this slot, bump refcount and return. */
+ for (slot = parent->slot; slot; slot = slot->next) {
+ if (slot->number == slot_nr) {
+ kobject_get(&slot->kobj);
+ dbg("%s: bumped refcount to %d on %x:%d\n",
+ __FUNCTION__, slot->kobj.kref.refcount,
+ parent->number, slot_nr);
+ goto out;
+ }
+ }
+
+ slot = kzalloc(sizeof(*slot), GFP_KERNEL);
+ if (!slot) {
+ slot = ERR_PTR(-ENOMEM);
+ goto out;
+ }
+
+ slot->bus = parent;
+ slot->number = slot_nr;
+
+ slot->kobj.kset = pci_slots_kset;
+ err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
+ "%s", name);
+ if (err) {
+ printk(KERN_ERR "Unable to register kobject %s", name);
+ err = -EINVAL;
+ goto err;
+ }
+
+ err = create_sysfs_files(slot);
+ if (err)
+ goto unregister;
+
+ slot->next = parent->slot;
+ parent->slot = slot;
+
+ dbg("%s: created pci_slot on %x:%d\n",
+ __FUNCTION__, parent->number, slot_nr);
+
+ out:
+ up_write(&pci_bus_sem);
+ return slot;
+
+ unregister:
+ kobject_put(&slot->kobj);
+ err:
+ kfree(slot);
+ slot = ERR_PTR(err);
+ goto out;
+}
+EXPORT_SYMBOL_GPL(pci_create_slot);
+
+int pci_destroy_slot(struct pci_slot *slot)
+{
+ kobject_put(&slot->kobj);
+
+ dbg("%s: decreased refcount to %d on %x:%d\n", __FUNCTION__,
+ slot->kobj.kref.refcount, slot->bus->number, slot->number);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pci_destroy_slot);
+
+static int pci_slot_init(void)
+{
+ int result = 0;
+ struct kset *pci_bus_kset;
+
+ pci_bus_kset = bus_get_kset(&pci_bus_type);
+
+ pci_slots_kset = kset_create_and_add("slots", NULL,
+ &pci_bus_kset->kobj);
+ if (!pci_slots_kset) {
+ result = -ENOMEM;
+ printk(KERN_ERR "PCI: Slot initialisation failure (%d)",
+ result);
+ }
+ return result;
+}
+
+subsys_initcall(pci_slot_init);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 87195b6..2b3ada0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -128,6 +128,16 @@ struct pci_cap_saved_state {
u32 data[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) */
+ unsigned char number; /* PCI_SLOT(pci_dev->devfn) */
+ struct kobject kobj;
+ void (*release)(struct pci_slot *);
+};
+
/*
* The pci_dev structure is used to describe PCI devices.
*/
@@ -139,6 +149,7 @@ struct pci_dev {
void *sysdata; /* hook for sys-specific extension */
struct proc_dir_entry *procent; /* device entry in /proc/bus/pci */
+ struct pci_slot *slot; /* Physical slot this device is in */
unsigned int devfn; /* encoded device & function index */
unsigned short vendor;
@@ -258,6 +269,7 @@ struct pci_bus {
struct list_head children; /* list of child buses */
struct list_head devices; /* list of devices on this bus */
struct pci_dev *self; /* bridge device as seen by parent */
+ struct pci_slot *slot; /* First physical slot on this bus */
struct resource *resource[PCI_BUS_NUM_RESOURCES];
/* address space routed to this bus */
@@ -470,6 +482,11 @@ struct pci_bus *pci_create_bus(struct device *parent, int bus,
struct pci_ops *ops, void *sysdata);
struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev,
int busnr);
+struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
+ const char *name);
+int pci_slot_add_hotplug(struct pci_bus *parent, int slot_nr,
+ void (*release)(struct pci_slot *));
+int pci_destroy_slot(struct pci_slot *slot);
int pci_scan_slot(struct pci_bus *bus, int devfn);
struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn);
void pci_device_add(struct pci_dev *dev, struct pci_bus *bus);
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 8f67e8f..bb36c59 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -95,9 +95,6 @@ struct hotplug_slot_attribute {
* @get_adapter_status: Called to get see if an adapter is present in the slot or not.
* If this field is NULL, the value passed in the struct hotplug_slot_info
* will be used when this value is requested by a user.
- * @get_address: Called to get pci address of a slot.
- * If this field is NULL, the value passed in the struct hotplug_slot_info
- * will be used when this value is requested by a user.
* @get_max_bus_speed: Called to get the max bus speed for a slot.
* If this field is NULL, the value passed in the struct hotplug_slot_info
* will be used when this value is requested by a user.
@@ -120,7 +117,6 @@ struct hotplug_slot_ops {
int (*get_attention_status) (struct hotplug_slot *slot, u8 *value);
int (*get_latch_status) (struct hotplug_slot *slot, u8 *value);
int (*get_adapter_status) (struct hotplug_slot *slot, u8 *value);
- int (*get_address) (struct hotplug_slot *slot, u32 *value);
int (*get_max_bus_speed) (struct hotplug_slot *slot, enum pci_bus_speed *value);
int (*get_cur_bus_speed) (struct hotplug_slot *slot, enum pci_bus_speed *value);
};
@@ -140,7 +136,6 @@ struct hotplug_slot_info {
u8 attention_status;
u8 latch_status;
u8 adapter_status;
- u32 address;
enum pci_bus_speed max_bus_speed;
enum pci_bus_speed cur_bus_speed;
};
@@ -166,15 +161,14 @@ struct hotplug_slot {
/* Variables below this are for use only by the hotplug pci core. */
struct list_head slot_list;
- struct kobject kobj;
+ struct pci_slot *pci_slot;
};
#define to_hotplug_slot(n) container_of(n, struct hotplug_slot, kobj)
-extern int pci_hp_register (struct hotplug_slot *slot);
-extern int pci_hp_deregister (struct hotplug_slot *slot);
+extern int pci_hp_register(struct hotplug_slot *, struct pci_bus *, int nr);
+extern int pci_hp_deregister(struct hotplug_slot *slot);
extern int __must_check pci_hp_change_slot_info (struct hotplug_slot *slot,
struct hotplug_slot_info *info);
-extern struct kset *pci_hotplug_slots_kset;
/* PCI Setting Record (Type 0) */
struct hpp_type0 {
--
1.5.3.1.g1e61
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 <[email protected]>
---
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 battery and thermal drivers. If you are compiling for a
mobile system, say Y.
+config ACPI_PCI_SLOT
+ tristate "PCI slot detection driver"
+ default n
+ help
+ This driver will attempt to discover all PCI slots in your system,
+ and creates entries in /sys/bus/pci/slots/. This feature can
+ help you correlate PCI bus addresses with the physical geography
+ of your slots. If you are unsure, say N.
+
config ACPI_POWER
bool
default y
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 40b0fca..579c29c 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_ACPI_DOCK) += dock.o
obj-$(CONFIG_ACPI_BAY) += bay.o
obj-$(CONFIG_ACPI_VIDEO) += video.o
obj-y += pci_root.o pci_link.o pci_irq.o pci_bind.o
+obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o
obj-$(CONFIG_ACPI_POWER) += power.o
obj-$(CONFIG_ACPI_PROCESSOR) += processor.o
obj-$(CONFIG_ACPI_CONTAINER) += container.o
diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
new file mode 100644
index 0000000..24c6b4a
--- /dev/null
+++ b/drivers/acpi/pci_slot.c
@@ -0,0 +1,322 @@
+/*
+ * pci_slot.c - ACPI PCI Slot Driver
+ *
+ * The code here is heavily leveraged from the acpiphp module.
+ * Thanks to Matthew Wilcox <[email protected]> for much guidance.
+ *
+ * Copyright (C) 2007 Alex Chiang <[email protected]>
+ * Copyright (C) 2007 Hewlett-Packard Development Company, L.P.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+static int debug;
+
+#define DRIVER_VERSION "0.1"
+#define DRIVER_AUTHOR "Alex Chiang <[email protected]>"
+#define DRIVER_DESC "ACPI PCI Slot Detection Driver"
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
+MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
+module_param(debug, bool, 0644);
+
+#define _COMPONENT ACPI_PCI_COMPONENT
+ACPI_MODULE_NAME("pci_slot");
+
+#define MY_NAME "pci_slot"
+#define err(format, arg...) printk(KERN_ERR "%s: " format , MY_NAME , ## arg)
+#define info(format, arg...) printk(KERN_INFO "%s: " format , MY_NAME , ## arg)
+#define dbg(format, arg...) \
+ do { \
+ if (debug) \
+ printk(KERN_DEBUG "%s: " format, \
+ MY_NAME , ## arg); \
+ } while (0)
+
+static int acpi_pci_slot_add(acpi_handle handle);
+static void acpi_pci_slot_remove(acpi_handle handle);
+
+static struct acpi_pci_driver acpi_pci_slot_driver = {
+ .add = acpi_pci_slot_add,
+ .remove = acpi_pci_slot_remove,
+};
+
+static int
+check_slot(acpi_handle handle, int *device, unsigned long *sun)
+{
+ int retval = 0;
+ unsigned long adr;
+ acpi_status status;
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+
+ acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+ dbg("Checking slot on path: %s\n", (char *)buffer.pointer);
+
+ status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
+ if (ACPI_FAILURE(status)) {
+ dbg("_ADR returned %d on %s\n", status, (char *)buffer.pointer);
+ retval = -1;
+ goto out;
+ }
+
+ *device = (adr >> 16) & 0xffff;
+
+ /* No _SUN == not a slot == bail */
+ status = acpi_evaluate_integer(handle, "_SUN", NULL, sun);
+ if (ACPI_FAILURE(status)) {
+ dbg("_SUN returned %d on %s\n", status, (char *)buffer.pointer);
+ retval = -1;
+ goto out;
+ }
+
+out:
+ kfree(buffer.pointer);
+ return retval;
+}
+
+/*
+ * unregister_slot
+ *
+ * Called once for each SxFy object in the namespace. Each call to
+ * pci_destroy_slot decrements the refcount on the pci_slot, and
+ * eventually calls kobject_unregister at the appropriate time.
+ */
+static acpi_status
+unregister_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device;
+ unsigned long sun;
+ struct pci_slot *slot;
+ struct pci_bus *pci_bus = context;
+
+ if (check_slot(handle, &device, &sun))
+ return AE_OK;
+
+ for (slot = pci_bus->slot; slot; slot = slot->next) {
+ if (slot->number == device)
+ pci_destroy_slot(slot);
+ }
+
+ return AE_OK;
+}
+
+/*
+ * register_slot
+ *
+ * Called once for each SxFy object in the namespace. Don't worry about
+ * calling pci_create_slot multiple times for the same pci_bus:device,
+ * since each subsequent call simply bumps the refcount on the pci_slot.
+ *
+ * The number of calls to pci_destroy_slot from unregister_slot is
+ * symmetrical.
+ */
+static acpi_status
+register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device;
+ unsigned long sun;
+ char name[KOBJ_NAME_LEN];
+
+ struct pci_slot *pci_slot;
+ struct pci_bus *pci_bus = context;
+
+ if (check_slot(handle, &device, &sun))
+ return AE_OK;
+
+ snprintf(name, sizeof(name), "%u", (u32)sun);
+ pci_slot = pci_create_slot(pci_bus, device, name);
+ if (IS_ERR(pci_slot))
+ err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
+
+ dbg("pci_slot: %#lx, pci_bus: %x, device: %d, name: %s\n",
+ (uint64_t)pci_slot, pci_bus->number, device, name);
+
+ return AE_OK;
+}
+
+struct p2p_bridge_context
+{
+ acpi_walk_callback user_function;
+ struct pci_bus *pci_bus;
+};
+
+/*
+ * walk_p2p_bridge - discover and walk p2p bridges
+ * @handle: points to an acpi_pci_root
+ * @context: p2p_bridge_context pointer
+ *
+ * Note that when we call ourselves recursively, we pass a different
+ * value of pci_bus in the child_context.
+ */
+static acpi_status
+walk_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device, function;
+ unsigned long adr;
+ acpi_status status;
+ acpi_handle dummy_handle;
+ acpi_walk_callback user_function;
+
+ struct pci_dev *dev;
+ struct pci_bus *pci_bus;
+ struct p2p_bridge_context child_context;
+ struct p2p_bridge_context *parent_context = context;
+
+ pci_bus = parent_context->pci_bus;
+ user_function = parent_context->user_function;
+
+ status = acpi_get_handle(handle, "_ADR", &dummy_handle);
+ if (ACPI_FAILURE(status))
+ return AE_OK;
+
+ status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return AE_OK;
+
+ device = (adr >> 16) & 0xffff;
+ function = adr & 0xffff;
+
+ dev = pci_get_slot(pci_bus, PCI_DEVFN(device, function));
+ if (!dev || !dev->subordinate)
+ goto out;
+
+ dbg("p2p bridge walk, pci_bus = %x\n", dev->subordinate->number);
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ user_function, dev->subordinate, NULL);
+ if (ACPI_FAILURE(status))
+ goto out;
+
+ child_context.pci_bus = dev->subordinate;
+ child_context.user_function = user_function;
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ walk_p2p_bridge, &child_context, NULL);
+out:
+ pci_dev_put(dev);
+ return AE_OK;
+}
+
+#define ACPI_STA_FUNCTIONING (0x00000008)
+
+/*
+ * walk_root_bridge - generic root bridge walker
+ * @handle: points to an acpi_pci_root
+ * @user_function: user callback for slot objects
+ *
+ * Call user_function for all objects underneath this root bridge.
+ * Walk p2p bridges underneath us and call user_function on those too.
+ */
+static int
+walk_root_bridge(acpi_handle handle, acpi_walk_callback user_function)
+{
+ int seg, bus;
+ unsigned long tmp;
+ acpi_status status;
+ acpi_handle dummy_handle;
+ struct pci_bus *pci_bus;
+ struct p2p_bridge_context context;
+
+ /* If the bridge doesn't have _STA, we assume it is always there */
+ status = acpi_get_handle(handle, "_STA", &dummy_handle);
+ if (ACPI_SUCCESS(status)) {
+ status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
+ if (ACPI_FAILURE(status)) {
+ info("%s: _STA evaluation failure\n", __FUNCTION__);
+ return 0;
+ }
+ if ((tmp & ACPI_STA_FUNCTIONING) == 0)
+ /* don't register this object */
+ return 0;
+ }
+
+ status = acpi_evaluate_integer(handle, "_SEG", NULL, &tmp);
+ seg = ACPI_SUCCESS(status) ? tmp : 0;
+
+ status = acpi_evaluate_integer(handle, "_BBN", NULL, &tmp);
+ bus = ACPI_SUCCESS(status) ? tmp : 0;
+
+ pci_bus = pci_find_bus(seg, bus);
+ if (!pci_bus)
+ return 0;
+
+ dbg("root bridge walk, pci_bus = %x\n", pci_bus->number);
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ user_function, pci_bus, NULL);
+ if (ACPI_FAILURE(status))
+ return status;
+
+ context.pci_bus = pci_bus;
+ context.user_function = user_function;
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ walk_p2p_bridge, &context, NULL);
+ if (ACPI_FAILURE(status))
+ err("%s: walk_p2p_bridge failure - %d\n", __FUNCTION__, status);
+
+ return status;
+}
+
+/*
+ * acpi_pci_slot_add
+ * @handle: points to an acpi_pci_root
+ */
+static int
+acpi_pci_slot_add(acpi_handle handle)
+{
+ acpi_status status;
+
+ status = walk_root_bridge(handle, register_slot);
+ if (ACPI_FAILURE(status))
+ err("%s: register_slot failure - %d\n", __FUNCTION__, status);
+
+ return status;
+}
+
+/*
+ * acpi_pci_slot_remove
+ * @handle: points to an acpi_pci_root
+ */
+static void
+acpi_pci_slot_remove(acpi_handle handle)
+{
+ acpi_status status;
+
+ status = walk_root_bridge(handle, unregister_slot);
+ if (ACPI_FAILURE(status))
+ err("%s: unregister_slot failure - %d\n", __FUNCTION__, status);
+}
+
+static int __init
+acpi_pci_slot_init(void)
+{
+ acpi_pci_register_driver(&acpi_pci_slot_driver);
+ return 0;
+}
+
+static void __exit
+acpi_pci_slot_exit(void)
+{
+ acpi_pci_unregister_driver(&acpi_pci_slot_driver);
+}
+
+module_init(acpi_pci_slot_init);
+module_exit(acpi_pci_slot_exit);
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 21bbb5e..b857dd0 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -567,6 +567,11 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
return -EINVAL;
}
+ /*
+ * No problems if we call this interface from both ACPI_PCI_SLOT
+ * driver and call it here again. If we've already created the
+ * pci_slot, the interface will simply bump the refcount.
+ */
pci_slot = pci_create_slot(bus, slot_nr, slot->name);
if (IS_ERR(pci_slot))
return PTR_ERR(pci_slot);
@@ -613,8 +618,12 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
slot = hotplug->pci_slot;
fs_remove_slot(slot);
- pci_destroy_slot(slot);
dbg("Removed slot %s from the list\n", hotplug->name);
+
+ hotplug_release(slot);
+ slot->release = NULL;
+ pci_destroy_slot(slot);
+
return 0;
}
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index ab45b69..9096908 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -249,7 +249,7 @@ static int init_slots(struct controller *ctrl)
if (retval == -EBUSY)
goto error_info;
if (retval) {
- err ("pci_hp_register failed with error %d\n", retval);
+ err("pci_hp_register failed with error %d\n", retval);
goto error_info;
}
/* create additional sysfs entries */
diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
index cc74602..094b9e9 100644
--- a/drivers/pci/hotplug/sgi_hotplug.c
+++ b/drivers/pci/hotplug/sgi_hotplug.c
@@ -634,7 +634,7 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
register_err:
dev_dbg(&pci_bus->self->dev, "bus failed to register with err = %d\n",
- rc);
+ rc);
alloc_err:
if (rc == -ENOMEM)
--
1.5.3.1.g1e61
On Thu, Feb 28, 2008 at 05:23:41PM -0700, Alex Chiang wrote:
> 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. :)
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
On Thu, Feb 28, 2008 at 05:28:55PM -0700, Alex Chiang wrote:
> - 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.
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
On Thu, Feb 28, 2008 at 05:29:38PM -0700, Alex Chiang wrote:
> 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.
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
On Fri, Feb 29, 2008 at 09:25:42PM -0800, Greg KH wrote:
> What is the guarantee that the names of these slots are correct and do
> not happen to be the same as the hotpluggable ones?
That would be a bug -- and yes, bugs happen, and we have to deal with
them.
> Why show this information on machines that can not do anything with
> these slots at all? Will that not just confuse people?
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."
[Adding Prarit to cc since I think this is his code.]
On Thursday, February 28, 2008 4:26 pm Alex Chiang wrote:
> 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 <[email protected]>
> Signed-off-by: Matthew Wilcox <[email protected]>
> ---
> 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 = slot->hotplug_slot;
> list_del(&((struct slot *)bss_hotplug_slot->private)->
> hp_list);
> - sysfs_remove_file(&bss_hotplug_slot->kobj,
> - &sn_slot_path_attr.attr);
> break;
> }
> return bss_hotplug_slot;
> @@ -653,11 +628,6 @@ static int sn_hotplug_slot_register(struct pci_bus
> *pci_bus) rc = pci_hp_register(bss_hotplug_slot);
> if (rc)
> goto register_err;
> -
> - rc = sysfs_create_file(&bss_hotplug_slot->kobj,
> - &sn_slot_path_attr.attr);
> - if (rc)
> - goto register_err;
> }
> dev_dbg(&pci_bus->self->dev, "Registered bus with hotplug\n");
> return rc;
>> - 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);
>>
>>
>
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.
Hi Greg,
* Greg KH <[email protected]>:
> On Thu, Feb 28, 2008 at 05:28:55PM -0700, Alex Chiang wrote:
> > - 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.
>
> 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
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
now handled by pci_slot directly.
> and how you are handling machines that do not export this
> information at all?
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
before (when they are loaded).
> oh, and don't put "extern" in a .c file,
Fixed in next version.
> 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...
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, right?
This kinda seems like a stupid design, but the next patch in the
series adds another callsite for pci_create_slot. The next patch
is detecting physical slots described by ACPI, but doesn't know
(or care) about their hotplug capabilities.
I don't think it makes sense to be emitting uevents simply upon
detecting a physical slot.
[over time, I hope to add more functionality to pci_slot, such as
displaying speed, width, etc., but right now, we only get the
address]
One alternative I can think of -- which would further complicate
this model that I'm introducing -- would be to make hotplug_slot
a kobject too, and then let pci_slot emit a uevent upon physical
slot detection, and then let pci_hp_register emit another uevent
when the hotplug_slot is created / added to sysfs. But I must
admit, I don't really like that alternative.
Your thoughts?
Thanks.
/ac
* Greg KH <[email protected]>:
> On Thu, Feb 28, 2008 at 05:23:41PM -0700, Alex Chiang wrote:
> >
> > 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. :)
>
> 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.)
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
On Sat, Mar 01, 2008 at 07:43:07AM -0700, Matthew Wilcox wrote:
> On Fri, Feb 29, 2008 at 09:25:42PM -0800, Greg KH wrote:
> > What is the guarantee that the names of these slots are correct and do
> > not happen to be the same as the hotpluggable ones?
>
> That would be a bug -- and yes, bugs happen, and we have to deal with
> them.
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
confirmed by some initial testing of IBM systems, right?
> > Why show this information on machines that can not do anything with
> > these slots at all? Will that not just confuse people?
>
> 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).
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
On Mon, Mar 03, 2008 at 04:35:56PM -0700, Alex Chiang wrote:
> * Greg KH <[email protected]>:
> > On Thu, Feb 28, 2008 at 05:23:41PM -0700, Alex Chiang wrote:
> > >
> > > 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. :)
> >
> > 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.)
>
> 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:
No, they are correct, and need to be fixed.
> 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?
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
On Mon, Mar 03, 2008 at 01:56:21PM -0700, Alex Chiang wrote:
> Hi Greg,
>
> * Greg KH <[email protected]>:
> > On Thu, Feb 28, 2008 at 05:28:55PM -0700, Alex Chiang wrote:
> > > - 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.
> >
> > 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
>
> 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.
Ok, that's a bit better, please include it in the changelog information
in the patch next time :)
> - 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.
>
> > and how you are handling machines that do not export this
> > information at all?
>
> 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
> before (when they are loaded).
The issue is for non-hotpluggable slots, right? That's a big behavior
change, especially for userspace tools not expecting that.
> > 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...
>
> 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, right?
That is correct, but the location seemed a bit "odd", next time around
I'll review it again to be sure.
> This kinda seems like a stupid design, but the next patch in the
> series adds another callsite for pci_create_slot. The next patch
> is detecting physical slots described by ACPI, but doesn't know
> (or care) about their hotplug capabilities.
>
> I don't think it makes sense to be emitting uevents simply upon
> detecting a physical slot.
>
> [over time, I hope to add more functionality to pci_slot, such as
> displaying speed, width, etc., but right now, we only get the
> address]
>
> One alternative I can think of -- which would further complicate
> this model that I'm introducing -- would be to make hotplug_slot
> a kobject too, and then let pci_slot emit a uevent upon physical
> slot detection, and then let pci_hp_register emit another uevent
> when the hotplug_slot is created / added to sysfs. But I must
> admit, I don't really like that alternative.
No, that's a mess, don't do that :)
thanks,
greg k-h
On Monday, March 03, 2008 9:49 pm Greg KH wrote:
> On Sat, Mar 01, 2008 at 07:43:07AM -0700, Matthew Wilcox wrote:
> > On Fri, Feb 29, 2008 at 09:25:42PM -0800, Greg KH wrote:
> > > What is the guarantee that the names of these slots are correct and do
> > > not happen to be the same as the hotpluggable ones?
> >
> > That would be a bug -- and yes, bugs happen, and we have to deal with
> > them.
>
> 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
> confirmed by some initial testing of IBM systems, right?
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
On Tue, Mar 04, 2008 at 10:18:28AM -0800, Jesse Barnes wrote:
> On Monday, March 03, 2008 9:49 pm Greg KH wrote:
> > On Sat, Mar 01, 2008 at 07:43:07AM -0700, Matthew Wilcox wrote:
> > > On Fri, Feb 29, 2008 at 09:25:42PM -0800, Greg KH wrote:
> > > > What is the guarantee that the names of these slots are correct and do
> > > > not happen to be the same as the hotpluggable ones?
> > >
> > > That would be a bug -- and yes, bugs happen, and we have to deal with
> > > them.
> >
> > 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
> > confirmed by some initial testing of IBM systems, right?
>
> 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.
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
On Tuesday, March 04, 2008 11:30 am Greg KH wrote:
> bad data is worse.
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),
this feature seems like a really good idea.
> 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.
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
Greg KH <[email protected]> wrote:
> On Tue, Mar 04, 2008 at 10:18:28AM -0800, Jesse Barnes wrote:
> > On Monday, March 03, 2008 9:49 pm Greg KH wrote:
> > > On Sat, Mar 01, 2008 at 07:43:07AM -0700, Matthew Wilcox wrote:
> > > > On Fri, Feb 29, 2008 at 09:25:42PM -0800, Greg KH wrote:
> > > > > What is the guarantee that the names of these slots are
> > > > > correct and do not happen to be the same as the hotpluggable
> > > > > ones?
> > > >
> > > > That would be a bug -- and yes, bugs happen, and we have to
> > > > deal with them.
> > >
> > > 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 confirmed by some initial testing of IBM systems, right?
> >
> > 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.
>
> 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
>
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?
* Greg KH <[email protected]>:
> On Sat, Mar 01, 2008 at 07:43:07AM -0700, Matthew Wilcox wrote:
> > On Fri, Feb 29, 2008 at 09:25:42PM -0800, Greg KH wrote:
> > > What is the guarantee that the names of these slots are correct
No guarantee there. We report whatever firmware tells us.
> > > and do not happen to be the same as the hotpluggable ones?
Stronger guarantee here, since both pci_slot and <foo>hp driver
will be getting the name of the slot from the same place.
> > That would be a bug -- and yes, bugs happen, and we have to deal with
> > them.
>
> 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
> confirmed by some initial testing of IBM systems, right?
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
me...
> > > Why show this information on machines that can not do
> > > anything with these slots at all? Will that not just
> > > confuse people?
> >
> > 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).
>
> 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?
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
* Greg KH <[email protected]>:
> On Tue, Mar 04, 2008 at 10:18:28AM -0800, Jesse Barnes wrote:
> > On Monday, March 03, 2008 9:49 pm Greg KH wrote:
> > > On Sat, Mar 01, 2008 at 07:43:07AM -0700, Matthew Wilcox wrote:
> > > > On Fri, Feb 29, 2008 at 09:25:42PM -0800, Greg KH wrote:
> > > > > What is the guarantee that the names of these slots are correct and do
> > > > > not happen to be the same as the hotpluggable ones?
> > > >
> > > > That would be a bug -- and yes, bugs happen, and we have to deal with
> > > > them.
> > >
> > > 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
> > > confirmed by some initial testing of IBM systems, right?
> >
> > 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.
>
> 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?
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
in there but...
> 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...
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
On Tue, Mar 04, 2008 at 03:58:30PM -0700, Alex Chiang wrote:
> * Greg KH <[email protected]>:
> > On Sat, Mar 01, 2008 at 07:43:07AM -0700, Matthew Wilcox wrote:
> > > On Fri, Feb 29, 2008 at 09:25:42PM -0800, Greg KH wrote:
> > > > What is the guarantee that the names of these slots are correct
>
> No guarantee there. We report whatever firmware tells us.
>
> > > > and do not happen to be the same as the hotpluggable ones?
>
> Stronger guarantee here, since both pci_slot and <foo>hp driver
> will be getting the name of the slot from the same place.
>
> > > That would be a bug -- and yes, bugs happen, and we have to deal with
> > > them.
> >
> > 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
> > confirmed by some initial testing of IBM systems, right?
>
> 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
> me...
>
> > > > Why show this information on machines that can not do
> > > > anything with these slots at all? Will that not just
> > > > confuse people?
> > >
> > > 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).
> >
> > 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?
>
> 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.
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
closed-source code in IBM's huge system management toolkits :(
> 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.
Ok, care to resend them with the requested updates?
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 <[email protected]>
Signed-off-by: Matthew Wilcox <[email protected]>
---
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 | 6 +-
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 | 5 +-
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 | 224 ++++++++++++++++++++++++++++
include/linux/pci.h | 17 ++
include/linux/pci_hotplug.h | 12 +-
19 files changed, 388 insertions(+), 248 deletions(-)
create mode 100644 drivers/pci/slot.c
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 4d1ce2e..7d63f8c 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -2,7 +2,7 @@
# Makefile for the PCI bus specific drivers.
#
-obj-y += access.o bus.o probe.o remove.o pci.o quirks.o \
+obj-y += access.o bus.o probe.o remove.o pci.o quirks.o slot.o \
pci-driver.o search.o pci-sysfs.o rom.o setup-res.o
obj-$(CONFIG_PROC_FS) += proc.o
diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 7a29164..eecf7cb 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -215,7 +215,6 @@ extern u8 acpiphp_get_power_status (struct acpiphp_slot *slot);
extern u8 acpiphp_get_attention_status (struct acpiphp_slot *slot);
extern u8 acpiphp_get_latch_status (struct acpiphp_slot *slot);
extern u8 acpiphp_get_adapter_status (struct acpiphp_slot *slot);
-extern u32 acpiphp_get_address (struct acpiphp_slot *slot);
/* variables */
extern int acpiphp_debug;
diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
index 9279d5b..57319e6 100644
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -70,7 +70,6 @@ static int disable_slot (struct hotplug_slot *slot);
static int set_attention_status (struct hotplug_slot *slot, u8 value);
static int get_power_status (struct hotplug_slot *slot, u8 *value);
static int get_attention_status (struct hotplug_slot *slot, u8 *value);
-static int get_address (struct hotplug_slot *slot, u32 *value);
static int get_latch_status (struct hotplug_slot *slot, u8 *value);
static int get_adapter_status (struct hotplug_slot *slot, u8 *value);
@@ -83,7 +82,6 @@ static struct hotplug_slot_ops acpi_hotplug_slot_ops = {
.get_attention_status = get_attention_status,
.get_latch_status = get_latch_status,
.get_adapter_status = get_adapter_status,
- .get_address = get_address,
};
@@ -274,23 +272,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
return 0;
}
-
-/**
- * get_address - get pci address of a slot
- * @hotplug_slot: slot to get status
- * @value: pointer to struct pci_busdev (seg, bus, dev)
- */
-static int get_address(struct hotplug_slot *hotplug_slot, u32 *value)
-{
- struct slot *slot = hotplug_slot->private;
-
- dbg("%s - physical_slot = %s\n", __FUNCTION__, hotplug_slot->name);
-
- *value = acpiphp_get_address(slot->acpi_slot);
-
- return 0;
-}
-
static int __init init_acpi(void)
{
int retval;
@@ -357,7 +338,11 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
acpiphp_slot->slot = slot;
snprintf(slot->name, sizeof(slot->name), "%u", slot->acpi_slot->sun);
- retval = pci_hp_register(slot->hotplug_slot);
+ retval = pci_hp_register(slot->hotplug_slot,
+ acpiphp_slot->bridge->pci_bus,
+ acpiphp_slot->device);
+ if (retval == -EBUSY)
+ goto error_hpslot;
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
goto error_hpslot;
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index cf22f9e..f84c4bd 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -258,7 +258,12 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
bridge->pci_bus->number, slot->device);
retval = acpiphp_register_hotplug_slot(slot);
if (retval) {
- warn("acpiphp_register_hotplug_slot failed(err code = 0x%x)\n", retval);
+ if (retval == -EBUSY)
+ warn("Slot %d already registered by another "
+ "hotplug driver\n", slot->sun);
+ else
+ warn("acpiphp_register_hotplug_slot failed "
+ "(err code = 0x%x)\n", retval);
goto err_exit;
}
}
@@ -1867,19 +1872,3 @@ u8 acpiphp_get_adapter_status(struct acpiphp_slot *slot)
return (sta == 0) ? 0 : 1;
}
-
-
-/*
- * pci address (seg/bus/dev)
- */
-u32 acpiphp_get_address(struct acpiphp_slot *slot)
-{
- u32 address;
- struct pci_bus *pci_bus = slot->bridge->pci_bus;
-
- address = (pci_domain_nr(pci_bus) << 16) |
- (pci_bus->number << 8) |
- slot->device;
-
- return address;
-}
diff --git a/drivers/pci/hotplug/acpiphp_ibm.c b/drivers/pci/hotplug/acpiphp_ibm.c
index b0a22b9..34a708f 100644
--- a/drivers/pci/hotplug/acpiphp_ibm.c
+++ b/drivers/pci/hotplug/acpiphp_ibm.c
@@ -33,8 +33,10 @@
#include <linux/kobject.h>
#include <asm/uaccess.h>
#include <linux/moduleparam.h>
+#include <linux/pci.h>
#include "acpiphp.h"
+#include "../pci.h"
#define DRIVER_VERSION "1.0.1"
#define DRIVER_AUTHOR "Irene Zubarev <[email protected]>, Vernon Mauery <[email protected]>"
@@ -430,7 +432,7 @@ static int __init ibm_acpiphp_init(void)
int retval = 0;
acpi_status status;
struct acpi_device *device;
- struct kobject *sysdir = &pci_hotplug_slots_kset->kobj;
+ struct kobject *sysdir = &pci_slots_kset->kobj;
dbg("%s\n", __FUNCTION__);
@@ -477,7 +479,7 @@ init_return:
static void __exit ibm_acpiphp_exit(void)
{
acpi_status status;
- struct kobject *sysdir = &pci_hotplug_slots_kset->kobj;
+ struct kobject *sysdir = &pci_slots_kset->kobj;
dbg("%s\n", __FUNCTION__);
diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c
index ed4d44e..aa47b80 100644
--- a/drivers/pci/hotplug/cpci_hotplug_core.c
+++ b/drivers/pci/hotplug/cpci_hotplug_core.c
@@ -285,7 +285,7 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
info->attention_status = cpci_get_attention_status(slot);
dbg("registering slot %s", slot->hotplug_slot->name);
- status = pci_hp_register(slot->hotplug_slot);
+ status = pci_hp_register(slot->hotplug_slot, bus, i);
if (status) {
err("pci_hp_register failed with error %d", status);
goto error_name;
diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c
index 7417887..2e0392e 100644
--- a/drivers/pci/hotplug/cpqphp_core.c
+++ b/drivers/pci/hotplug/cpqphp_core.c
@@ -434,7 +434,9 @@ static int ctrl_slot_setup(struct controller *ctrl,
slot->bus, slot->device,
slot->number, ctrl->slot_device_offset,
slot_number);
- result = pci_hp_register(hotplug_slot);
+ result = pci_hp_register(hotplug_slot,
+ ctrl->pci_dev->subordinate,
+ slot->device);
if (result) {
err("pci_hp_register failed with error %d\n", result);
goto error_name;
diff --git a/drivers/pci/hotplug/fakephp.c b/drivers/pci/hotplug/fakephp.c
index 6c14b4d..2d84755 100644
--- a/drivers/pci/hotplug/fakephp.c
+++ b/drivers/pci/hotplug/fakephp.c
@@ -127,7 +127,7 @@ static int add_slot(struct pci_dev *dev)
slot->release = &dummy_release;
slot->private = dslot;
- retval = pci_hp_register(slot);
+ retval = pci_hp_register(slot, dev->bus, PCI_SLOT(dev->devfn));
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
goto error_dslot;
diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c
index 600ed7b..eb7a1c0 100644
--- a/drivers/pci/hotplug/ibmphp_ebda.c
+++ b/drivers/pci/hotplug/ibmphp_ebda.c
@@ -1000,7 +1000,8 @@ static int __init ebda_rsrc_controller (void)
tmp_slot = list_entry (list, struct slot, ibm_slot_list);
snprintf (tmp_slot->hotplug_slot->name, 30, "%s", create_file_name (tmp_slot));
- pci_hp_register (tmp_slot->hotplug_slot);
+ pci_hp_register(tmp_slot->hotplug_slot,
+ pci_find_bus(0, tmp_slot->bus), tmp_slot->device);
}
print_ebda_hpc ();
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index dd59a05..21bbb5e 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -40,6 +40,7 @@
#include <linux/pci.h>
#include <linux/pci_hotplug.h>
#include <asm/uaccess.h>
+#include "../pci.h"
#define MY_NAME "pci_hotplug"
@@ -61,41 +62,6 @@ static int debug;
static LIST_HEAD(pci_hotplug_slot_list);
-struct kset *pci_hotplug_slots_kset;
-
-static ssize_t hotplug_slot_attr_show(struct kobject *kobj,
- struct attribute *attr, char *buf)
-{
- struct hotplug_slot *slot = to_hotplug_slot(kobj);
- struct hotplug_slot_attribute *attribute = to_hotplug_attr(attr);
- return attribute->show ? attribute->show(slot, buf) : -EIO;
-}
-
-static ssize_t hotplug_slot_attr_store(struct kobject *kobj,
- struct attribute *attr, const char *buf, size_t len)
-{
- struct hotplug_slot *slot = to_hotplug_slot(kobj);
- struct hotplug_slot_attribute *attribute = to_hotplug_attr(attr);
- return attribute->store ? attribute->store(slot, buf, len) : -EIO;
-}
-
-static struct sysfs_ops hotplug_slot_sysfs_ops = {
- .show = hotplug_slot_attr_show,
- .store = hotplug_slot_attr_store,
-};
-
-static void hotplug_slot_release(struct kobject *kobj)
-{
- struct hotplug_slot *slot = to_hotplug_slot(kobj);
- if (slot->release)
- slot->release(slot);
-}
-
-static struct kobj_type hotplug_slot_ktype = {
- .sysfs_ops = &hotplug_slot_sysfs_ops,
- .release = &hotplug_slot_release,
-};
-
/* these strings match up with the values in pci_bus_speed */
static char *pci_bus_speed_strings[] = {
"33 MHz PCI", /* 0x00 */
@@ -149,16 +115,15 @@ GET_STATUS(power_status, u8)
GET_STATUS(attention_status, u8)
GET_STATUS(latch_status, u8)
GET_STATUS(adapter_status, u8)
-GET_STATUS(address, u32)
GET_STATUS(max_bus_speed, enum pci_bus_speed)
GET_STATUS(cur_bus_speed, enum pci_bus_speed)
-static ssize_t power_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t power_read_file(struct pci_slot *slot, char *buf)
{
int retval;
u8 value;
- retval = get_power_status (slot, &value);
+ retval = get_power_status(slot->hotplug, &value);
if (retval)
goto exit;
retval = sprintf (buf, "%d\n", value);
@@ -166,9 +131,10 @@ exit:
return retval;
}
-static ssize_t power_write_file (struct hotplug_slot *slot, const char *buf,
+static ssize_t power_write_file(struct pci_slot *pci_slot, const char *buf,
size_t count)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
unsigned long lpower;
u8 power;
int retval = 0;
@@ -204,29 +170,30 @@ exit:
return count;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_power = {
+static struct pci_slot_attribute hotplug_slot_attr_power = {
.attr = {.name = "power", .mode = S_IFREG | S_IRUGO | S_IWUSR},
.show = power_read_file,
.store = power_write_file
};
-static ssize_t attention_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t attention_read_file(struct pci_slot *slot, char *buf)
{
int retval;
u8 value;
- retval = get_attention_status (slot, &value);
+ retval = get_attention_status(slot->hotplug, &value);
if (retval)
goto exit;
- retval = sprintf (buf, "%d\n", value);
+ retval = sprintf(buf, "%d\n", value);
exit:
return retval;
}
-static ssize_t attention_write_file (struct hotplug_slot *slot, const char *buf,
+static ssize_t attention_write_file(struct pci_slot *slot, const char *buf,
size_t count)
{
+ struct hotplug_slot_ops *ops = slot->hotplug->ops;
unsigned long lattention;
u8 attention;
int retval = 0;
@@ -235,13 +202,13 @@ static ssize_t attention_write_file (struct hotplug_slot *slot, const char *buf,
attention = (u8)(lattention & 0xff);
dbg (" - attention = %d\n", attention);
- if (!try_module_get(slot->ops->owner)) {
+ if (!try_module_get(ops->owner)) {
retval = -ENODEV;
goto exit;
}
- if (slot->ops->set_attention_status)
- retval = slot->ops->set_attention_status(slot, attention);
- module_put(slot->ops->owner);
+ if (ops->set_attention_status)
+ retval = ops->set_attention_status(slot->hotplug, attention);
+ module_put(ops->owner);
exit:
if (retval)
@@ -249,18 +216,18 @@ exit:
return count;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_attention = {
+static struct pci_slot_attribute hotplug_slot_attr_attention = {
.attr = {.name = "attention", .mode = S_IFREG | S_IRUGO | S_IWUSR},
.show = attention_read_file,
.store = attention_write_file
};
-static ssize_t latch_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t latch_read_file(struct pci_slot *slot, char *buf)
{
int retval;
u8 value;
- retval = get_latch_status (slot, &value);
+ retval = get_latch_status(slot->hotplug, &value);
if (retval)
goto exit;
retval = sprintf (buf, "%d\n", value);
@@ -269,17 +236,17 @@ exit:
return retval;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_latch = {
+static struct pci_slot_attribute hotplug_slot_attr_latch = {
.attr = {.name = "latch", .mode = S_IFREG | S_IRUGO},
.show = latch_read_file,
};
-static ssize_t presence_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t presence_read_file(struct pci_slot *slot, char *buf)
{
int retval;
u8 value;
- retval = get_adapter_status (slot, &value);
+ retval = get_adapter_status(slot->hotplug, &value);
if (retval)
goto exit;
retval = sprintf (buf, "%d\n", value);
@@ -288,42 +255,20 @@ exit:
return retval;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_presence = {
+static struct pci_slot_attribute hotplug_slot_attr_presence = {
.attr = {.name = "adapter", .mode = S_IFREG | S_IRUGO},
.show = presence_read_file,
};
-static ssize_t address_read_file (struct hotplug_slot *slot, char *buf)
-{
- int retval;
- u32 address;
-
- retval = get_address (slot, &address);
- if (retval)
- goto exit;
- retval = sprintf (buf, "%04x:%02x:%02x\n",
- (address >> 16) & 0xffff,
- (address >> 8) & 0xff,
- address & 0xff);
-
-exit:
- return retval;
-}
-
-static struct hotplug_slot_attribute hotplug_slot_attr_address = {
- .attr = {.name = "address", .mode = S_IFREG | S_IRUGO},
- .show = address_read_file,
-};
-
static char *unknown_speed = "Unknown bus speed";
-static ssize_t max_bus_speed_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t max_bus_speed_read_file(struct pci_slot *slot, char *buf)
{
char *speed_string;
int retval;
enum pci_bus_speed value;
- retval = get_max_bus_speed (slot, &value);
+ retval = get_max_bus_speed(slot->hotplug, &value);
if (retval)
goto exit;
@@ -338,18 +283,18 @@ exit:
return retval;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_max_bus_speed = {
+static struct pci_slot_attribute hotplug_slot_attr_max_bus_speed = {
.attr = {.name = "max_bus_speed", .mode = S_IFREG | S_IRUGO},
.show = max_bus_speed_read_file,
};
-static ssize_t cur_bus_speed_read_file (struct hotplug_slot *slot, char *buf)
+static ssize_t cur_bus_speed_read_file(struct pci_slot *slot, char *buf)
{
char *speed_string;
int retval;
enum pci_bus_speed value;
- retval = get_cur_bus_speed (slot, &value);
+ retval = get_cur_bus_speed(slot->hotplug, &value);
if (retval)
goto exit;
@@ -364,14 +309,15 @@ exit:
return retval;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_cur_bus_speed = {
+static struct pci_slot_attribute hotplug_slot_attr_cur_bus_speed = {
.attr = {.name = "cur_bus_speed", .mode = S_IFREG | S_IRUGO},
.show = cur_bus_speed_read_file,
};
-static ssize_t test_write_file (struct hotplug_slot *slot, const char *buf,
+static ssize_t test_write_file(struct pci_slot *pci_slot, const char *buf,
size_t count)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
unsigned long ltest;
u32 test;
int retval = 0;
@@ -394,13 +340,14 @@ exit:
return count;
}
-static struct hotplug_slot_attribute hotplug_slot_attr_test = {
+static struct pci_slot_attribute hotplug_slot_attr_test = {
.attr = {.name = "test", .mode = S_IFREG | S_IRUGO | S_IWUSR},
.store = test_write_file
};
-static int has_power_file (struct hotplug_slot *slot)
+static int has_power_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if ((slot->ops->enable_slot) ||
@@ -410,8 +357,9 @@ static int has_power_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_attention_file (struct hotplug_slot *slot)
+static int has_attention_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if ((slot->ops->set_attention_status) ||
@@ -420,8 +368,9 @@ static int has_attention_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_latch_file (struct hotplug_slot *slot)
+static int has_latch_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->get_latch_status)
@@ -429,8 +378,9 @@ static int has_latch_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_adapter_file (struct hotplug_slot *slot)
+static int has_adapter_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->get_adapter_status)
@@ -438,17 +388,9 @@ static int has_adapter_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_address_file (struct hotplug_slot *slot)
-{
- if ((!slot) || (!slot->ops))
- return -ENODEV;
- if (slot->ops->get_address)
- return 0;
- return -ENOENT;
-}
-
-static int has_max_bus_speed_file (struct hotplug_slot *slot)
+static int has_max_bus_speed_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->get_max_bus_speed)
@@ -456,8 +398,9 @@ static int has_max_bus_speed_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_cur_bus_speed_file (struct hotplug_slot *slot)
+static int has_cur_bus_speed_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->get_cur_bus_speed)
@@ -465,8 +408,9 @@ static int has_cur_bus_speed_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int has_test_file (struct hotplug_slot *slot)
+static int has_test_file(struct pci_slot *pci_slot)
{
+ struct hotplug_slot *slot = pci_slot->hotplug;
if ((!slot) || (!slot->ops))
return -ENODEV;
if (slot->ops->hardware_test)
@@ -474,7 +418,7 @@ static int has_test_file (struct hotplug_slot *slot)
return -ENOENT;
}
-static int fs_add_slot (struct hotplug_slot *slot)
+static int fs_add_slot(struct pci_slot *slot)
{
int retval = 0;
@@ -505,13 +449,6 @@ static int fs_add_slot (struct hotplug_slot *slot)
goto exit_adapter;
}
- if (has_address_file(slot) == 0) {
- retval = sysfs_create_file(&slot->kobj,
- &hotplug_slot_attr_address.attr);
- if (retval)
- goto exit_address;
- }
-
if (has_max_bus_speed_file(slot) == 0) {
retval = sysfs_create_file(&slot->kobj,
&hotplug_slot_attr_max_bus_speed.attr);
@@ -544,10 +481,6 @@ exit_cur_speed:
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_max_bus_speed.attr);
exit_max_speed:
- if (has_address_file(slot) == 0)
- sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_address.attr);
-
-exit_address:
if (has_adapter_file(slot) == 0)
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_presence.attr);
@@ -567,7 +500,7 @@ exit:
return retval;
}
-static void fs_remove_slot (struct hotplug_slot *slot)
+static void fs_remove_slot(struct pci_slot *slot)
{
if (has_power_file(slot) == 0)
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_power.attr);
@@ -581,9 +514,6 @@ static void fs_remove_slot (struct hotplug_slot *slot)
if (has_adapter_file(slot) == 0)
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_presence.attr);
- if (has_address_file(slot) == 0)
- sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_address.attr);
-
if (has_max_bus_speed_file(slot) == 0)
sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_max_bus_speed.attr);
@@ -607,6 +537,12 @@ static struct hotplug_slot *get_slot_from_name (const char *name)
return NULL;
}
+static void hotplug_release(struct pci_slot *slot)
+{
+ struct hotplug_slot *hotplug = slot->hotplug;
+ hotplug->release(hotplug);
+}
+
/**
* pci_hp_register - register a hotplug_slot with the PCI hotplug subsystem
* @slot: pointer to the &struct hotplug_slot to register
@@ -616,9 +552,10 @@ static struct hotplug_slot *get_slot_from_name (const char *name)
*
* Returns 0 if successful, anything else for an error.
*/
-int pci_hp_register (struct hotplug_slot *slot)
+int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
{
int result;
+ struct pci_slot *pci_slot;
if (slot == NULL)
return -ENODEV;
@@ -630,20 +567,24 @@ int pci_hp_register (struct hotplug_slot *slot)
return -EINVAL;
}
- /* this can fail if we have already registered a slot with the same name */
- slot->kobj.kset = pci_hotplug_slots_kset;
- result = kobject_init_and_add(&slot->kobj, &hotplug_slot_ktype, NULL,
- "%s", slot->name);
+ pci_slot = pci_create_slot(bus, slot_nr, slot->name);
+ if (IS_ERR(pci_slot))
+ return PTR_ERR(pci_slot);
+
+ result = pci_slot_add_hotplug(bus, slot_nr, hotplug_release);
if (result) {
- err("Unable to register kobject '%s'", slot->name);
- return -EINVAL;
+ pci_destroy_slot(pci_slot);
+ return result;
}
- list_add (&slot->slot_list, &pci_hotplug_slot_list);
+ slot->pci_slot = pci_slot;
+ pci_slot->hotplug = slot;
+
+ list_add(&slot->slot_list, &pci_hotplug_slot_list);
- result = fs_add_slot (slot);
- kobject_uevent(&slot->kobj, KOBJ_ADD);
- dbg ("Added slot %s to the list\n", slot->name);
+ result = fs_add_slot(pci_slot);
+ kobject_uevent(&pci_slot->kobj, KOBJ_ADD);
+ dbg("Added slot %s to the list\n", slot->name);
return result;
}
@@ -656,22 +597,24 @@ int pci_hp_register (struct hotplug_slot *slot)
*
* Returns 0 if successful, anything else for an error.
*/
-int pci_hp_deregister (struct hotplug_slot *slot)
+int pci_hp_deregister(struct hotplug_slot *hotplug)
{
struct hotplug_slot *temp;
+ struct pci_slot *slot;
- if (slot == NULL)
+ if (!hotplug)
return -ENODEV;
- temp = get_slot_from_name (slot->name);
- if (temp != slot) {
+ temp = get_slot_from_name(hotplug->name);
+ if (temp != hotplug)
return -ENODEV;
- }
- list_del (&slot->slot_list);
- fs_remove_slot (slot);
- dbg ("Removed slot %s from the list\n", slot->name);
- kobject_put(&slot->kobj);
+ list_del(&hotplug->slot_list);
+
+ slot = hotplug->pci_slot;
+ fs_remove_slot(slot);
+ pci_destroy_slot(slot);
+ dbg("Removed slot %s from the list\n", hotplug->name);
return 0;
}
@@ -685,13 +628,15 @@ int pci_hp_deregister (struct hotplug_slot *slot)
*
* Returns 0 if successful, anything else for an error.
*/
-int __must_check pci_hp_change_slot_info(struct hotplug_slot *slot,
+int __must_check pci_hp_change_slot_info(struct hotplug_slot *hotplug,
struct hotplug_slot_info *info)
{
- if ((slot == NULL) || (info == NULL))
+ struct pci_slot *slot;
+ if (!hotplug || !info)
return -ENODEV;
+ slot = hotplug->pci_slot;
- memcpy (slot->info, info, sizeof (struct hotplug_slot_info));
+ memcpy(hotplug->info, info, sizeof(struct hotplug_slot_info));
return 0;
}
@@ -699,36 +644,22 @@ int __must_check pci_hp_change_slot_info(struct hotplug_slot *slot,
static int __init pci_hotplug_init (void)
{
int result;
- struct kset *pci_bus_kset;
- pci_bus_kset = bus_get_kset(&pci_bus_type);
-
- pci_hotplug_slots_kset = kset_create_and_add("slots", NULL,
- &pci_bus_kset->kobj);
- if (!pci_hotplug_slots_kset) {
- result = -ENOMEM;
- err("Register subsys error\n");
- goto exit;
- }
result = cpci_hotplug_init(debug);
if (result) {
err ("cpci_hotplug_init with error %d\n", result);
- goto err_subsys;
+ goto err_cpci;
}
info (DRIVER_DESC " version: " DRIVER_VERSION "\n");
- goto exit;
-err_subsys:
- kset_unregister(pci_hotplug_slots_kset);
-exit:
+err_cpci:
return result;
}
static void __exit pci_hotplug_exit (void)
{
cpci_hotplug_exit();
- kset_unregister(pci_hotplug_slots_kset);
}
module_init(pci_hotplug_init);
@@ -740,7 +671,6 @@ MODULE_LICENSE("GPL");
module_param(debug, bool, 0644);
MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
-EXPORT_SYMBOL_GPL(pci_hotplug_slots_kset);
EXPORT_SYMBOL_GPL(pci_hp_register);
EXPORT_SYMBOL_GPL(pci_hp_deregister);
EXPORT_SYMBOL_GPL(pci_hp_change_slot_info);
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 7f4836b..ab45b69 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -69,7 +69,6 @@ static int get_power_status (struct hotplug_slot *slot, u8 *value);
static int get_attention_status (struct hotplug_slot *slot, u8 *value);
static int get_latch_status (struct hotplug_slot *slot, u8 *value);
static int get_adapter_status (struct hotplug_slot *slot, u8 *value);
-static int get_address (struct hotplug_slot *slot, u32 *value);
static int get_max_bus_speed (struct hotplug_slot *slot, enum pci_bus_speed *value);
static int get_cur_bus_speed (struct hotplug_slot *slot, enum pci_bus_speed *value);
@@ -82,7 +81,6 @@ static struct hotplug_slot_ops pciehp_hotplug_slot_ops = {
.get_attention_status = get_attention_status,
.get_latch_status = get_latch_status,
.get_adapter_status = get_adapter_status,
- .get_address = get_address,
.get_max_bus_speed = get_max_bus_speed,
.get_cur_bus_speed = get_cur_bus_speed,
};
@@ -245,14 +243,18 @@ static int init_slots(struct controller *ctrl)
dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
"slot_device_offset=%x\n", slot->bus, slot->device,
slot->hp_slot, slot->number, ctrl->slot_device_offset);
- retval = pci_hp_register(hotplug_slot);
+ retval = pci_hp_register(hotplug_slot,
+ ctrl->pci_dev->subordinate,
+ slot->device);
+ if (retval == -EBUSY)
+ goto error_info;
if (retval) {
err ("pci_hp_register failed with error %d\n", retval);
goto error_info;
}
/* create additional sysfs entries */
if (EMI(ctrl->ctrlcap)) {
- retval = sysfs_create_file(&hotplug_slot->kobj,
+ retval = sysfs_create_file(&hotplug_slot->pci_slot->kobj,
&hotplug_slot_attr_lock.attr);
if (retval) {
pci_hp_deregister(hotplug_slot);
@@ -285,7 +287,7 @@ static void cleanup_slots(struct controller *ctrl)
slot = list_entry(tmp, struct slot, slot_list);
list_del(&slot->slot_list);
if (EMI(ctrl->ctrlcap))
- sysfs_remove_file(&slot->hotplug_slot->kobj,
+ sysfs_remove_file(&slot->hotplug_slot->pci_slot->kobj,
&hotplug_slot_attr_lock.attr);
cancel_delayed_work(&slot->work);
flush_scheduled_work();
@@ -387,18 +389,6 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
return 0;
}
-static int get_address(struct hotplug_slot *hotplug_slot, u32 *value)
-{
- struct slot *slot = hotplug_slot->private;
- struct pci_bus *bus = slot->ctrl->pci_dev->subordinate;
-
- dbg("%s - physical_slot = %s\n", __FUNCTION__, hotplug_slot->name);
-
- *value = (pci_domain_nr(bus) << 16) | (slot->bus << 8) | slot->device;
-
- return 0;
-}
-
static int get_max_bus_speed(struct hotplug_slot *hotplug_slot, enum pci_bus_speed *value)
{
struct slot *slot = hotplug_slot->private;
@@ -460,7 +450,12 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
/* Setup the slot information structures */
rc = init_slots(ctrl);
if (rc) {
- err("%s: slot initialization failed\n", PCIE_MODULE_NAME);
+ if (rc == -EBUSY)
+ warn("%s: slot already registered by another "
+ "hotplug driver\n", PCIE_MODULE_NAME);
+ else
+ err("%s: slot initialization failed\n",
+ PCIE_MODULE_NAME);
goto err_out_release_ctlr;
}
diff --git a/drivers/pci/hotplug/rpadlpar_sysfs.c b/drivers/pci/hotplug/rpadlpar_sysfs.c
index e32148a..399b196 100644
--- a/drivers/pci/hotplug/rpadlpar_sysfs.c
+++ b/drivers/pci/hotplug/rpadlpar_sysfs.c
@@ -14,8 +14,10 @@
*/
#include <linux/kobject.h>
#include <linux/string.h>
+#include <linux/pci.h>
#include <linux/pci_hotplug.h>
#include "rpadlpar.h"
+#include "../pcih."
#define DLPAR_KOBJ_NAME "control"
#define ADD_SLOT_ATTR_NAME "add_slot"
@@ -23,7 +25,6 @@
#define MAX_DRC_NAME_LEN 64
-
static ssize_t add_slot_store(struct kobject *kobj, struct kobj_attribute *attr,
const char *buf, size_t nbytes)
{
@@ -108,7 +109,7 @@ int dlpar_sysfs_init(void)
int error;
dlpar_kobj = kobject_create_and_add(DLPAR_KOBJ_NAME,
- &pci_hotplug_slots_kset->kobj);
+ &pci_slots_kset->kobj);
if (!dlpar_kobj)
return -EINVAL;
diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c
index 8ad3deb..8e5fff0 100644
--- a/drivers/pci/hotplug/rpaphp_slot.c
+++ b/drivers/pci/hotplug/rpaphp_slot.c
@@ -162,7 +162,8 @@ int rpaphp_register_slot(struct slot *slot)
return -EAGAIN;
}
- retval = pci_hp_register(php_slot);
+ retval = pci_hp_register(php_slot, slot->bus,
+ PCI_SLOT(PCI_DN(slot->dn->child)->devfn));
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
return retval;
diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
index 693519e..cc74602 100644
--- a/drivers/pci/hotplug/sgi_hotplug.c
+++ b/drivers/pci/hotplug/sgi_hotplug.c
@@ -625,7 +625,7 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
bss_hotplug_slot->ops = &sn_hotplug_slot_ops;
bss_hotplug_slot->release = &sn_release_slot;
- rc = pci_hp_register(bss_hotplug_slot);
+ rc = pci_hp_register(bss_hotplug_slot, pci_bus, device);
if (rc)
goto register_err;
}
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index 80dec97..22c4d2e 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -65,7 +65,6 @@ static int get_power_status (struct hotplug_slot *slot, u8 *value);
static int get_attention_status (struct hotplug_slot *slot, u8 *value);
static int get_latch_status (struct hotplug_slot *slot, u8 *value);
static int get_adapter_status (struct hotplug_slot *slot, u8 *value);
-static int get_address (struct hotplug_slot *slot, u32 *value);
static int get_max_bus_speed (struct hotplug_slot *slot, enum pci_bus_speed *value);
static int get_cur_bus_speed (struct hotplug_slot *slot, enum pci_bus_speed *value);
@@ -78,7 +77,6 @@ static struct hotplug_slot_ops shpchp_hotplug_slot_ops = {
.get_attention_status = get_attention_status,
.get_latch_status = get_latch_status,
.get_adapter_status = get_adapter_status,
- .get_address = get_address,
.get_max_bus_speed = get_max_bus_speed,
.get_cur_bus_speed = get_cur_bus_speed,
};
@@ -152,7 +150,8 @@ static int init_slots(struct controller *ctrl)
dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
"slot_device_offset=%x\n", slot->bus, slot->device,
slot->hp_slot, slot->number, ctrl->slot_device_offset);
- retval = pci_hp_register(slot->hotplug_slot);
+ retval = pci_hp_register(slot->hotplug_slot,
+ ctrl->pci_dev->subordinate, slot->device);
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
goto error_info;
@@ -277,18 +276,6 @@ static int get_adapter_status (struct hotplug_slot *hotplug_slot, u8 *value)
return 0;
}
-static int get_address (struct hotplug_slot *hotplug_slot, u32 *value)
-{
- struct slot *slot = get_slot(hotplug_slot);
- struct pci_bus *bus = slot->ctrl->pci_dev->subordinate;
-
- dbg("%s - physical_slot = %s\n", __FUNCTION__, hotplug_slot->name);
-
- *value = (pci_domain_nr(bus) << 16) | (slot->bus << 8) | slot->device;
-
- return 0;
-}
-
static int get_max_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_speed *value)
{
struct slot *slot = get_slot(hotplug_slot);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index eabeb1f..61bb743 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -87,3 +87,16 @@ pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
}
struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
+
+/* PCI slot sysfs helper code */
+#define to_pci_slot(s) container_of(s, struct pci_slot, kobj)
+
+extern struct kset *pci_slots_kset;
+
+struct pci_slot_attribute {
+ struct attribute attr;
+ ssize_t (*show)(struct pci_slot *, char *);
+ ssize_t (*store)(struct pci_slot *, const char *, size_t);
+};
+#define to_pci_slot_attr(s) container_of(s, struct pci_slot_attribute, attr)
+
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
new file mode 100644
index 0000000..5b71b9f
--- /dev/null
+++ b/drivers/pci/slot.c
@@ -0,0 +1,224 @@
+/*
+ * drivers/pci/slot.c
+ * Copyright (C) 2006 Matthew Wilcox <[email protected]>
+ * Copyright (C) 2006,2007 Hewlett-Packard Development Company, L.P.
+ * Copyright (C) 2007 Alex Chiang <[email protected]>
+ */
+
+#include <linux/kobject.h>
+#include <linux/pci.h>
+#include "pci.h"
+
+static int pci_slot_debug;
+#define MY_NAME "slot"
+#define dbg(format, arg...) \
+ do { \
+ if (pci_slot_debug) \
+ printk(KERN_DEBUG "%s: " format, \
+ MY_NAME , ## arg); \
+ } while (0)
+
+struct kset *pci_slots_kset;
+EXPORT_SYMBOL_GPL(pci_slots_kset);
+
+static ssize_t pci_slot_attr_show(struct kobject *kobj,
+ struct attribute *attr, char *buf)
+{
+ struct pci_slot *slot = to_pci_slot(kobj);
+ struct pci_slot_attribute *attribute = to_pci_slot_attr(attr);
+ return attribute->show ? attribute->show(slot, buf) : -EIO;
+}
+
+static ssize_t pci_slot_attr_store(struct kobject *kobj,
+ struct attribute *attr, const char *buf, size_t len)
+{
+ struct pci_slot *slot = to_pci_slot(kobj);
+ struct pci_slot_attribute *attribute = to_pci_slot_attr(attr);
+ return attribute->store ? attribute->store(slot, buf, len) : -EIO;
+}
+
+static struct sysfs_ops pci_slot_sysfs_ops = {
+ .show = pci_slot_attr_show,
+ .store = pci_slot_attr_store,
+};
+
+static ssize_t address_read_file(struct pci_slot *slot, char *buf)
+{
+ return sprintf(buf, "%04x:%02x:%02x\n", pci_domain_nr(slot->bus),
+ slot->bus->number, slot->number);
+}
+
+static struct pci_slot_attribute pci_slot_attr_address = {
+ .attr = { .name = "address", .mode = S_IFREG | S_IRUGO },
+ .show = address_read_file,
+};
+
+static void remove_sysfs_files(struct pci_slot *slot)
+{
+ sysfs_remove_file(&slot->kobj, &pci_slot_attr_address.attr);
+}
+
+static int create_sysfs_files(struct pci_slot *slot)
+{
+ int result;
+
+ result = sysfs_create_file(&slot->kobj, &pci_slot_attr_address.attr);
+
+ return result;
+}
+
+static void pci_slot_release(struct kobject *kobj)
+{
+ struct pci_slot **pprev;
+ struct pci_slot *slot = to_pci_slot(kobj);
+
+ dbg("%s: releasing pci_slot on %x:%d\n", __FUNCTION__,
+ slot->bus->number, slot->number);
+
+ for (pprev = &slot->bus->slot; *pprev; pprev = &(*pprev)->next) {
+ if (*pprev == slot) {
+ *pprev = slot->next;
+ break;
+ }
+ }
+
+ if (slot->release)
+ slot->release(slot);
+
+ remove_sysfs_files(slot);
+ kfree(slot);
+}
+
+static struct kobj_type pci_slot_ktype = {
+ .sysfs_ops = &pci_slot_sysfs_ops,
+ .release = &pci_slot_release,
+};
+
+int pci_slot_add_hotplug(struct pci_bus *parent, int slot_nr,
+ void (*release)(struct pci_slot *))
+{
+ struct pci_slot *slot;
+ int retval, found;
+
+ retval = found = 0;
+
+ down_write(&pci_bus_sem);
+
+ /* This slot should have already been created, so look for it. If
+ * we can't find it, return -EEXIST.
+ */
+ for (slot = parent->slot; slot; slot = slot->next)
+ if (slot->number == slot_nr) {
+ found = 1;
+ break;
+ }
+
+ if (!found) {
+ dbg("%s: slot not found\n", __FUNCTION__);
+ retval = -EEXIST;
+ goto out;
+ }
+
+ if (slot->release) {
+ dbg("%s: already claimed\n", __FUNCTION__);
+ retval = -EBUSY;
+ goto out;
+ }
+
+ dbg("%s: adding release function to %x:%d\n",
+ __FUNCTION__, parent->number, slot_nr);
+ slot->release = release;
+ out:
+ up_write(&pci_bus_sem);
+ return retval;
+}
+EXPORT_SYMBOL_GPL(pci_slot_add_hotplug);
+
+struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
+ const char *name)
+{
+ struct pci_slot *slot;
+ int err;
+
+ down_write(&pci_bus_sem);
+
+ /* If we've already created this slot, bump refcount and return. */
+ for (slot = parent->slot; slot; slot = slot->next) {
+ if (slot->number == slot_nr) {
+ kobject_get(&slot->kobj);
+ dbg("%s: bumped refcount to %d on %x:%d\n",
+ __FUNCTION__, slot->kobj.kref.refcount,
+ parent->number, slot_nr);
+ goto out;
+ }
+ }
+
+ slot = kzalloc(sizeof(*slot), GFP_KERNEL);
+ if (!slot) {
+ slot = ERR_PTR(-ENOMEM);
+ goto out;
+ }
+
+ slot->bus = parent;
+ slot->number = slot_nr;
+
+ slot->kobj.kset = pci_slots_kset;
+ err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
+ "%s", name);
+ if (err) {
+ printk(KERN_ERR "Unable to register kobject %s", name);
+ err = -EINVAL;
+ goto err;
+ }
+
+ err = create_sysfs_files(slot);
+ if (err)
+ goto unregister;
+
+ slot->next = parent->slot;
+ parent->slot = slot;
+
+ dbg("%s: created pci_slot on %x:%d\n",
+ __FUNCTION__, parent->number, slot_nr);
+
+ out:
+ up_write(&pci_bus_sem);
+ return slot;
+
+ unregister:
+ kobject_put(&slot->kobj);
+ err:
+ kfree(slot);
+ slot = ERR_PTR(err);
+ goto out;
+}
+EXPORT_SYMBOL_GPL(pci_create_slot);
+
+int pci_destroy_slot(struct pci_slot *slot)
+{
+ kobject_put(&slot->kobj);
+
+ dbg("%s: decreased refcount to %d on %x:%d\n", __FUNCTION__,
+ slot->kobj.kref.refcount, slot->bus->number, slot->number);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pci_destroy_slot);
+
+static int pci_slot_init(void)
+{
+ int result = 0;
+ struct kset *pci_bus_kset;
+
+ pci_bus_kset = bus_get_kset(&pci_bus_type);
+
+ pci_slots_kset = kset_create_and_add("slots", NULL,
+ &pci_bus_kset->kobj);
+ if (!pci_slots_kset) {
+ result = -ENOMEM;
+ printk(KERN_ERR "PCI: Slot initialisation failure (%d)",
+ result);
+ }
+ return result;
+}
+
+subsys_initcall(pci_slot_init);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 87195b6..2b3ada0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -128,6 +128,16 @@ struct pci_cap_saved_state {
u32 data[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) */
+ unsigned char number; /* PCI_SLOT(pci_dev->devfn) */
+ struct kobject kobj;
+ void (*release)(struct pci_slot *);
+};
+
/*
* The pci_dev structure is used to describe PCI devices.
*/
@@ -139,6 +149,7 @@ struct pci_dev {
void *sysdata; /* hook for sys-specific extension */
struct proc_dir_entry *procent; /* device entry in /proc/bus/pci */
+ struct pci_slot *slot; /* Physical slot this device is in */
unsigned int devfn; /* encoded device & function index */
unsigned short vendor;
@@ -258,6 +269,7 @@ struct pci_bus {
struct list_head children; /* list of child buses */
struct list_head devices; /* list of devices on this bus */
struct pci_dev *self; /* bridge device as seen by parent */
+ struct pci_slot *slot; /* First physical slot on this bus */
struct resource *resource[PCI_BUS_NUM_RESOURCES];
/* address space routed to this bus */
@@ -470,6 +482,11 @@ struct pci_bus *pci_create_bus(struct device *parent, int bus,
struct pci_ops *ops, void *sysdata);
struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev,
int busnr);
+struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
+ const char *name);
+int pci_slot_add_hotplug(struct pci_bus *parent, int slot_nr,
+ void (*release)(struct pci_slot *));
+int pci_destroy_slot(struct pci_slot *slot);
int pci_scan_slot(struct pci_bus *bus, int devfn);
struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn);
void pci_device_add(struct pci_dev *dev, struct pci_bus *bus);
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 8f67e8f..bb36c59 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -95,9 +95,6 @@ struct hotplug_slot_attribute {
* @get_adapter_status: Called to get see if an adapter is present in the slot or not.
* If this field is NULL, the value passed in the struct hotplug_slot_info
* will be used when this value is requested by a user.
- * @get_address: Called to get pci address of a slot.
- * If this field is NULL, the value passed in the struct hotplug_slot_info
- * will be used when this value is requested by a user.
* @get_max_bus_speed: Called to get the max bus speed for a slot.
* If this field is NULL, the value passed in the struct hotplug_slot_info
* will be used when this value is requested by a user.
@@ -120,7 +117,6 @@ struct hotplug_slot_ops {
int (*get_attention_status) (struct hotplug_slot *slot, u8 *value);
int (*get_latch_status) (struct hotplug_slot *slot, u8 *value);
int (*get_adapter_status) (struct hotplug_slot *slot, u8 *value);
- int (*get_address) (struct hotplug_slot *slot, u32 *value);
int (*get_max_bus_speed) (struct hotplug_slot *slot, enum pci_bus_speed *value);
int (*get_cur_bus_speed) (struct hotplug_slot *slot, enum pci_bus_speed *value);
};
@@ -140,7 +136,6 @@ struct hotplug_slot_info {
u8 attention_status;
u8 latch_status;
u8 adapter_status;
- u32 address;
enum pci_bus_speed max_bus_speed;
enum pci_bus_speed cur_bus_speed;
};
@@ -166,15 +161,14 @@ struct hotplug_slot {
/* Variables below this are for use only by the hotplug pci core. */
struct list_head slot_list;
- struct kobject kobj;
+ struct pci_slot *pci_slot;
};
#define to_hotplug_slot(n) container_of(n, struct hotplug_slot, kobj)
-extern int pci_hp_register (struct hotplug_slot *slot);
-extern int pci_hp_deregister (struct hotplug_slot *slot);
+extern int pci_hp_register(struct hotplug_slot *, struct pci_bus *, int nr);
+extern int pci_hp_deregister(struct hotplug_slot *slot);
extern int __must_check pci_hp_change_slot_info (struct hotplug_slot *slot,
struct hotplug_slot_info *info);
-extern struct kset *pci_hotplug_slots_kset;
/* PCI Setting Record (Type 0) */
struct hpp_type0 {
--
1.5.3.1.g1e61
Hi Greg, all,
* Greg KH <[email protected]>:
> On Tue, Mar 04, 2008 at 03:58:30PM -0700, Alex Chiang wrote:
>
> > 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.
>
> Ok, care to resend them with the requested updates?
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
* Prarit Bhargava <[email protected]>:
>
>>> - 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);
>>>
>>>
>>
> 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 ....
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
Hi Alex-san,
Sorry for the delay in responding.
> 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 ;)]
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
Alex Chiang wrote:
> * Greg KH <[email protected]>:
>> On Tue, Mar 04, 2008 at 10:18:28AM -0800, Jesse Barnes wrote:
>>> On Monday, March 03, 2008 9:49 pm Greg KH wrote:
>>>> On Sat, Mar 01, 2008 at 07:43:07AM -0700, Matthew Wilcox wrote:
>>>>> On Fri, Feb 29, 2008 at 09:25:42PM -0800, Greg KH wrote:
>>>>>> What is the guarantee that the names of these slots are correct and do
>>>>>> not happen to be the same as the hotpluggable ones?
>>>>> That would be a bug -- and yes, bugs happen, and we have to deal with
>>>>> them.
>>>> 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
>>>> confirmed by some initial testing of IBM systems, right?
>>> 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.
>> 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?
>
> 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
> in there but...
>
>> 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...
>
> 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 Kenji-san,
* Kenji Kaneshige <[email protected]>:
> Hi Alex-san,
>
> Sorry for the delay in responding.
>
>> 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 ;)]
>
> 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).
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 <[email protected]>
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 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 <[email protected]>
---
drivers/acpi/Kconfig | 9 +
drivers/acpi/Makefile | 1 +
drivers/acpi/pci_slot.c | 361 ++++++++++++++++++++++++++++++++
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, 383 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 battery and thermal drivers. If you are compiling for a
mobile system, say Y.
+config ACPI_PCI_SLOT
+ tristate "PCI slot detection driver"
+ default n
+ help
+ This driver will attempt to discover all PCI slots in your system,
+ and creates entries in /sys/bus/pci/slots/. This feature can
+ help you correlate PCI bus addresses with the physical geography
+ of your slots. If you are unsure, say N.
+
config ACPI_POWER
bool
default y
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 40b0fca..579c29c 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_ACPI_DOCK) += dock.o
obj-$(CONFIG_ACPI_BAY) += bay.o
obj-$(CONFIG_ACPI_VIDEO) += video.o
obj-y += pci_root.o pci_link.o pci_irq.o pci_bind.o
+obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o
obj-$(CONFIG_ACPI_POWER) += power.o
obj-$(CONFIG_ACPI_PROCESSOR) += processor.o
obj-$(CONFIG_ACPI_CONTAINER) += container.o
diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
new file mode 100644
index 0000000..869e99f
--- /dev/null
+++ b/drivers/acpi/pci_slot.c
@@ -0,0 +1,361 @@
+/*
+ * pci_slot.c - ACPI PCI Slot Driver
+ *
+ * The code here is heavily leveraged from the acpiphp module.
+ * Thanks to Matthew Wilcox <[email protected]> for much guidance.
+ *
+ * Copyright (C) 2007 Alex Chiang <[email protected]>
+ * Copyright (C) 2007 Hewlett-Packard Development Company, L.P.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+static int debug;
+static int check_sta_before_sun;
+
+#define DRIVER_VERSION "0.1"
+#define DRIVER_AUTHOR "Alex Chiang <[email protected]>"
+#define DRIVER_DESC "ACPI PCI Slot Detection Driver"
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
+MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
+module_param(debug, bool, 0644);
+
+#define _COMPONENT ACPI_PCI_COMPONENT
+ACPI_MODULE_NAME("pci_slot");
+
+#define MY_NAME "pci_slot"
+#define err(format, arg...) printk(KERN_ERR "%s: " format , MY_NAME , ## arg)
+#define info(format, arg...) printk(KERN_INFO "%s: " format , MY_NAME , ## arg)
+#define dbg(format, arg...) \
+ do { \
+ if (debug) \
+ printk(KERN_DEBUG "%s: " format, \
+ MY_NAME , ## arg); \
+ } while (0)
+
+static int acpi_pci_slot_add(acpi_handle handle);
+static void acpi_pci_slot_remove(acpi_handle handle);
+
+static struct acpi_pci_driver acpi_pci_slot_driver = {
+ .add = acpi_pci_slot_add,
+ .remove = acpi_pci_slot_remove,
+};
+
+static int
+check_slot(acpi_handle handle, int *device, unsigned long *sun)
+{
+ int retval = 0;
+ unsigned long adr, sta;
+ acpi_status status;
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+
+ acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+ dbg("Checking slot on path: %s\n", (char *)buffer.pointer);
+
+ if (check_sta_before_sun) {
+ /* If SxFy doesn't have _STA, we just assume it's there */
+ acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+
+ if (!(sta & ACPI_STA_DEVICE_PRESENT)) {
+ retval = -1;
+ goto out;
+ }
+ }
+
+ status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
+ if (ACPI_FAILURE(status)) {
+ dbg("_ADR returned %d on %s\n", status, (char *)buffer.pointer);
+ retval = -1;
+ goto out;
+ }
+
+ *device = (adr >> 16) & 0xffff;
+
+ /* No _SUN == not a slot == bail */
+ status = acpi_evaluate_integer(handle, "_SUN", NULL, sun);
+ if (ACPI_FAILURE(status)) {
+ dbg("_SUN returned %d on %s\n", status, (char *)buffer.pointer);
+ retval = -1;
+ goto out;
+ }
+
+out:
+ kfree(buffer.pointer);
+ return retval;
+}
+
+/*
+ * unregister_slot
+ *
+ * Called once for each SxFy object in the namespace. Each call to
+ * pci_destroy_slot decrements the refcount on the pci_slot, and
+ * eventually calls kobject_unregister at the appropriate time.
+ */
+static acpi_status
+unregister_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device;
+ unsigned long sun;
+ struct pci_slot *slot;
+ struct pci_bus *pci_bus = context;
+
+ if (check_slot(handle, &device, &sun))
+ return AE_OK;
+
+ for (slot = pci_bus->slot; slot; slot = slot->next) {
+ if (slot->number == device)
+ pci_destroy_slot(slot);
+ }
+
+ return AE_OK;
+}
+
+/*
+ * register_slot
+ *
+ * Called once for each SxFy object in the namespace. Don't worry about
+ * calling pci_create_slot multiple times for the same pci_bus:device,
+ * since each subsequent call simply bumps the refcount on the pci_slot.
+ *
+ * The number of calls to pci_destroy_slot from unregister_slot is
+ * symmetrical.
+ */
+static acpi_status
+register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device;
+ unsigned long sun;
+ char name[KOBJ_NAME_LEN];
+
+ struct pci_slot *pci_slot;
+ struct pci_bus *pci_bus = context;
+
+ if (check_slot(handle, &device, &sun))
+ return AE_OK;
+
+ snprintf(name, sizeof(name), "%u", (u32)sun);
+ pci_slot = pci_create_slot(pci_bus, device, name);
+ if (IS_ERR(pci_slot))
+ err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
+
+ dbg("pci_slot: %#lx, pci_bus: %x, device: %d, name: %s\n",
+ (uint64_t)pci_slot, pci_bus->number, device, name);
+
+ return AE_OK;
+}
+
+struct p2p_bridge_context
+{
+ acpi_walk_callback user_function;
+ struct pci_bus *pci_bus;
+};
+
+/*
+ * walk_p2p_bridge - discover and walk p2p bridges
+ * @handle: points to an acpi_pci_root
+ * @context: p2p_bridge_context pointer
+ *
+ * Note that when we call ourselves recursively, we pass a different
+ * value of pci_bus in the child_context.
+ */
+static acpi_status
+walk_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device, function;
+ unsigned long adr;
+ acpi_status status;
+ acpi_handle dummy_handle;
+ acpi_walk_callback user_function;
+
+ struct pci_dev *dev;
+ struct pci_bus *pci_bus;
+ struct p2p_bridge_context child_context;
+ struct p2p_bridge_context *parent_context = context;
+
+ pci_bus = parent_context->pci_bus;
+ user_function = parent_context->user_function;
+
+ status = acpi_get_handle(handle, "_ADR", &dummy_handle);
+ if (ACPI_FAILURE(status))
+ return AE_OK;
+
+ status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return AE_OK;
+
+ device = (adr >> 16) & 0xffff;
+ function = adr & 0xffff;
+
+ dev = pci_get_slot(pci_bus, PCI_DEVFN(device, function));
+ if (!dev || !dev->subordinate)
+ goto out;
+
+ dbg("p2p bridge walk, pci_bus = %x\n", dev->subordinate->number);
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ user_function, dev->subordinate, NULL);
+ if (ACPI_FAILURE(status))
+ goto out;
+
+ child_context.pci_bus = dev->subordinate;
+ child_context.user_function = user_function;
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ walk_p2p_bridge, &child_context, NULL);
+out:
+ pci_dev_put(dev);
+ return AE_OK;
+}
+
+#define ACPI_STA_FUNCTIONING (0x00000008)
+
+/*
+ * walk_root_bridge - generic root bridge walker
+ * @handle: points to an acpi_pci_root
+ * @user_function: user callback for slot objects
+ *
+ * Call user_function for all objects underneath this root bridge.
+ * Walk p2p bridges underneath us and call user_function on those too.
+ */
+static int
+walk_root_bridge(acpi_handle handle, acpi_walk_callback user_function)
+{
+ int seg, bus;
+ unsigned long tmp;
+ acpi_status status;
+ acpi_handle dummy_handle;
+ struct pci_bus *pci_bus;
+ struct p2p_bridge_context context;
+
+ /* If the bridge doesn't have _STA, we assume it is always there */
+ status = acpi_get_handle(handle, "_STA", &dummy_handle);
+ if (ACPI_SUCCESS(status)) {
+ status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
+ if (ACPI_FAILURE(status)) {
+ info("%s: _STA evaluation failure\n", __FUNCTION__);
+ return 0;
+ }
+ if ((tmp & ACPI_STA_FUNCTIONING) == 0)
+ /* don't register this object */
+ return 0;
+ }
+
+ status = acpi_evaluate_integer(handle, "_SEG", NULL, &tmp);
+ seg = ACPI_SUCCESS(status) ? tmp : 0;
+
+ status = acpi_evaluate_integer(handle, "_BBN", NULL, &tmp);
+ bus = ACPI_SUCCESS(status) ? tmp : 0;
+
+ pci_bus = pci_find_bus(seg, bus);
+ if (!pci_bus)
+ return 0;
+
+ dbg("root bridge walk, pci_bus = %x\n", pci_bus->number);
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ user_function, pci_bus, NULL);
+ if (ACPI_FAILURE(status))
+ return status;
+
+ context.pci_bus = pci_bus;
+ context.user_function = user_function;
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ walk_p2p_bridge, &context, NULL);
+ if (ACPI_FAILURE(status))
+ err("%s: walk_p2p_bridge failure - %d\n", __FUNCTION__, status);
+
+ return status;
+}
+
+/*
+ * acpi_pci_slot_add
+ * @handle: points to an acpi_pci_root
+ */
+static int
+acpi_pci_slot_add(acpi_handle handle)
+{
+ acpi_status status;
+
+ status = walk_root_bridge(handle, register_slot);
+ if (ACPI_FAILURE(status))
+ err("%s: register_slot failure - %d\n", __FUNCTION__, status);
+
+ return status;
+}
+
+/*
+ * acpi_pci_slot_remove
+ * @handle: points to an acpi_pci_root
+ */
+static void
+acpi_pci_slot_remove(acpi_handle handle)
+{
+ acpi_status status;
+
+ status = walk_root_bridge(handle, unregister_slot);
+ if (ACPI_FAILURE(status))
+ err("%s: unregister_slot failure - %d\n", __FUNCTION__, status);
+}
+
+#ifdef CONFIG_DMI
+static int do_sta_before_sun(const struct dmi_system_id *d)
+{
+ info("%s detected: will evaluate _STA before calling _SUN\n", d->ident);
+ check_sta_before_sun = 1;
+ return 0;
+}
+
+static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
+ /*
+ * Fujitsu Primequest machines will return 1023 to indicate an
+ * error if the _SUN method is evaluated on SxFy objects that
+ * are not present (as indicated by _STA), so for those machines,
+ * we want to check _STA before evaluating _SUN.
+ */
+ {
+ .callback = do_sta_before_sun,
+ .ident = "Fujitsu Limited Primequest",
+ .matches = {
+ DMI_MATCH(DMI_BIOS_VENDOR, ""),
+ DMI_MATCH(DMI_BIOS_VERSION, ""),
+ },
+ },
+ {}
+};
+#endif /* CONFIG_DMI */
+
+static int __init
+acpi_pci_slot_init(void)
+{
+ dmi_check_system(acpi_pci_slot_dmi_table);
+ acpi_pci_register_driver(&acpi_pci_slot_driver);
+ return 0;
+}
+
+static void __exit
+acpi_pci_slot_exit(void)
+{
+ acpi_pci_unregister_driver(&acpi_pci_slot_driver);
+}
+
+module_init(acpi_pci_slot_init);
+module_exit(acpi_pci_slot_exit);
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 21bbb5e..b857dd0 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -567,6 +567,11 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
return -EINVAL;
}
+ /*
+ * No problems if we call this interface from both ACPI_PCI_SLOT
+ * driver and call it here again. If we've already created the
+ * pci_slot, the interface will simply bump the refcount.
+ */
pci_slot = pci_create_slot(bus, slot_nr, slot->name);
if (IS_ERR(pci_slot))
return PTR_ERR(pci_slot);
@@ -613,8 +618,12 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
slot = hotplug->pci_slot;
fs_remove_slot(slot);
- pci_destroy_slot(slot);
dbg("Removed slot %s from the list\n", hotplug->name);
+
+ hotplug_release(slot);
+ slot->release = NULL;
+ pci_destroy_slot(slot);
+
return 0;
}
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index ab45b69..9096908 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -249,7 +249,7 @@ static int init_slots(struct controller *ctrl)
if (retval == -EBUSY)
goto error_info;
if (retval) {
- err ("pci_hp_register failed with error %d\n", retval);
+ err("pci_hp_register failed with error %d\n", retval);
goto error_info;
}
/* create additional sysfs entries */
diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
index 8908834..5fd6887 100644
--- a/drivers/pci/hotplug/sgi_hotplug.c
+++ b/drivers/pci/hotplug/sgi_hotplug.c
@@ -668,7 +668,7 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
register_err:
dev_dbg(&pci_bus->self->dev, "bus failed to register with err = %d\n",
- rc);
+ rc);
alloc_err:
if (rc == -ENOMEM)
--
1.5.3.1.g1e61
On Wed, Mar 05, 2008 at 01:20:52PM -0700, Alex Chiang wrote:
> 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?
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."
Hi Alex-san,
> 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?
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,
Alex Chiang wrote:
>>> 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 ;)]
>> 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).
>
> 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.
>
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
0011_0017 0016_0019 0021_0021 0032_0026
- With ACPI PCI slot detection driver
# ls /sys/bus/pci/slots/
0009_0016 0014_0018 0019_0020 0021_0022 27
0011_0017 0016_0019 0021_0021 26
I had thought it is not a big problem before because people who
don't like new names would not load the PCI slot driver. But since
it is loaded automatically at boot time, I'm wondering that it
would be a problem. For example, some platform, not fujitsu,
depends on the old slot name to work, IIRC (Maybe Kristen knows
the background about it). And I don't think the fact that slot
names are changed depending on the order of driver loading is
acceptable by system management people/software, though I don't
have such software.
Though I don't have any specific idea about this, folliwings might
be candidates.
- Override slot names with hotplug driver's slot names
- Unify slot names among all hotplug drivers
- Stop automatic loading of ACPI PCI slot driver
Anyway, the naming should be considered in the next enhancement.
Thanks,
Kenji Kaneshige
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 <[email protected]>
Signed-off-by: Kenji Kaneshige <[email protected]>
---
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 +347,15 @@ config ACPI_EC
the battery and thermal drivers. If you are compiling for a
mobile system, say Y.
+config ACPI_PCI_SLOT
+ tristate "PCI slot detection driver"
+ default n
+ help
+ This driver will attempt to discover all PCI slots in your system,
+ and creates entries in /sys/bus/pci/slots/. This feature can
+ help you correlate PCI bus addresses with the physical geography
+ of your slots. If you are unsure, say N.
+
config ACPI_POWER
bool
default y
Index: linux-2.6.25-rc4/drivers/acpi/Makefile
===================================================================
--- linux-2.6.25-rc4.orig/drivers/acpi/Makefile
+++ linux-2.6.25-rc4/drivers/acpi/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_ACPI_DOCK) += dock.o
obj-$(CONFIG_ACPI_BAY) += bay.o
obj-$(CONFIG_ACPI_VIDEO) += video.o
obj-y += pci_root.o pci_link.o pci_irq.o pci_bind.o
+obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o
obj-$(CONFIG_ACPI_POWER) += power.o
obj-$(CONFIG_ACPI_PROCESSOR) += processor.o
obj-$(CONFIG_ACPI_CONTAINER) += container.o
Index: linux-2.6.25-rc4/drivers/acpi/pci_slot.c
===================================================================
--- /dev/null
+++ linux-2.6.25-rc4/drivers/acpi/pci_slot.c
@@ -0,0 +1,328 @@
+/*
+ * pci_slot.c - ACPI PCI Slot Driver
+ *
+ * The code here is heavily leveraged from the acpiphp module.
+ * Thanks to Matthew Wilcox <[email protected]> for much guidance.
+ *
+ * Copyright (C) 2007 Alex Chiang <[email protected]>
+ * Copyright (C) 2007 Hewlett-Packard Development Company, L.P.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+static int debug;
+
+#define DRIVER_VERSION "0.1"
+#define DRIVER_AUTHOR "Alex Chiang <[email protected]>"
+#define DRIVER_DESC "ACPI PCI Slot Detection Driver"
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
+MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
+module_param(debug, bool, 0644);
+
+#define _COMPONENT ACPI_PCI_COMPONENT
+ACPI_MODULE_NAME("pci_slot");
+
+#define MY_NAME "pci_slot"
+#define err(format, arg...) printk(KERN_ERR "%s: " format , MY_NAME , ## arg)
+#define info(format, arg...) printk(KERN_INFO "%s: " format , MY_NAME , ## arg)
+#define dbg(format, arg...) \
+ do { \
+ if (debug) \
+ printk(KERN_DEBUG "%s: " format, \
+ MY_NAME , ## arg); \
+ } while (0)
+
+static int acpi_pci_slot_add(acpi_handle handle);
+static void acpi_pci_slot_remove(acpi_handle handle);
+
+static struct acpi_pci_driver acpi_pci_slot_driver = {
+ .add = acpi_pci_slot_add,
+ .remove = acpi_pci_slot_remove,
+};
+
+static int
+check_slot(acpi_handle handle, int *device, unsigned long *sun)
+{
+ int retval = 0;
+ unsigned long adr, sta;
+ acpi_status status;
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+
+ 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;
+ }
+
+ status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
+ if (ACPI_FAILURE(status)) {
+ dbg("_ADR returned %d on %s\n", status, (char *)buffer.pointer);
+ retval = -1;
+ goto out;
+ }
+
+ *device = (adr >> 16) & 0xffff;
+
+ /* No _SUN == not a slot == bail */
+ status = acpi_evaluate_integer(handle, "_SUN", NULL, sun);
+ if (ACPI_FAILURE(status)) {
+ dbg("_SUN returned %d on %s\n", status, (char *)buffer.pointer);
+ retval = -1;
+ goto out;
+ }
+
+out:
+ kfree(buffer.pointer);
+ return retval;
+}
+
+/*
+ * unregister_slot
+ *
+ * Called once for each SxFy object in the namespace. Each call to
+ * pci_destroy_slot decrements the refcount on the pci_slot, and
+ * eventually calls kobject_unregister at the appropriate time.
+ */
+static acpi_status
+unregister_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device;
+ unsigned long sun;
+ struct pci_slot *slot;
+ struct pci_bus *pci_bus = context;
+
+ if (check_slot(handle, &device, &sun))
+ return AE_OK;
+
+ for (slot = pci_bus->slot; slot; slot = slot->next) {
+ if (slot->number == device)
+ pci_destroy_slot(slot);
+ }
+
+ return AE_OK;
+}
+
+/*
+ * register_slot
+ *
+ * Called once for each SxFy object in the namespace. Don't worry about
+ * calling pci_create_slot multiple times for the same pci_bus:device,
+ * since each subsequent call simply bumps the refcount on the pci_slot.
+ *
+ * The number of calls to pci_destroy_slot from unregister_slot is
+ * symmetrical.
+ */
+static acpi_status
+register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device;
+ unsigned long sun;
+ char name[KOBJ_NAME_LEN];
+
+ struct pci_slot *pci_slot;
+ struct pci_bus *pci_bus = context;
+
+ if (check_slot(handle, &device, &sun))
+ return AE_OK;
+
+ snprintf(name, sizeof(name), "%u", (u32)sun);
+ pci_slot = pci_create_slot(pci_bus, device, name);
+ if (IS_ERR(pci_slot))
+ err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
+
+ dbg("pci_slot: %#lx, pci_bus: %x, device: %d, name: %s\n",
+ (uint64_t)pci_slot, pci_bus->number, device, name);
+
+ return AE_OK;
+}
+
+struct p2p_bridge_context
+{
+ acpi_walk_callback user_function;
+ struct pci_bus *pci_bus;
+};
+
+/*
+ * walk_p2p_bridge - discover and walk p2p bridges
+ * @handle: points to an acpi_pci_root
+ * @context: p2p_bridge_context pointer
+ *
+ * Note that when we call ourselves recursively, we pass a different
+ * value of pci_bus in the child_context.
+ */
+static acpi_status
+walk_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device, function;
+ unsigned long adr;
+ acpi_status status;
+ acpi_handle dummy_handle;
+ acpi_walk_callback user_function;
+
+ struct pci_dev *dev;
+ struct pci_bus *pci_bus;
+ struct p2p_bridge_context child_context;
+ struct p2p_bridge_context *parent_context = context;
+
+ pci_bus = parent_context->pci_bus;
+ user_function = parent_context->user_function;
+
+ status = acpi_get_handle(handle, "_ADR", &dummy_handle);
+ if (ACPI_FAILURE(status))
+ return AE_OK;
+
+ status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return AE_OK;
+
+ device = (adr >> 16) & 0xffff;
+ function = adr & 0xffff;
+
+ dev = pci_get_slot(pci_bus, PCI_DEVFN(device, function));
+ if (!dev || !dev->subordinate)
+ goto out;
+
+ dbg("p2p bridge walk, pci_bus = %x\n", dev->subordinate->number);
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ user_function, dev->subordinate, NULL);
+ if (ACPI_FAILURE(status))
+ goto out;
+
+ child_context.pci_bus = dev->subordinate;
+ child_context.user_function = user_function;
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ walk_p2p_bridge, &child_context, NULL);
+out:
+ pci_dev_put(dev);
+ return AE_OK;
+}
+
+#define ACPI_STA_FUNCTIONING (0x00000008)
+
+/*
+ * walk_root_bridge - generic root bridge walker
+ * @handle: points to an acpi_pci_root
+ * @user_function: user callback for slot objects
+ *
+ * Call user_function for all objects underneath this root bridge.
+ * Walk p2p bridges underneath us and call user_function on those too.
+ */
+static int
+walk_root_bridge(acpi_handle handle, acpi_walk_callback user_function)
+{
+ int seg, bus;
+ unsigned long tmp;
+ acpi_status status;
+ acpi_handle dummy_handle;
+ struct pci_bus *pci_bus;
+ struct p2p_bridge_context context;
+
+ /* If the bridge doesn't have _STA, we assume it is always there */
+ status = acpi_get_handle(handle, "_STA", &dummy_handle);
+ if (ACPI_SUCCESS(status)) {
+ status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
+ if (ACPI_FAILURE(status)) {
+ info("%s: _STA evaluation failure\n", __FUNCTION__);
+ return 0;
+ }
+ if ((tmp & ACPI_STA_FUNCTIONING) == 0)
+ /* don't register this object */
+ return 0;
+ }
+
+ status = acpi_evaluate_integer(handle, "_SEG", NULL, &tmp);
+ seg = ACPI_SUCCESS(status) ? tmp : 0;
+
+ status = acpi_evaluate_integer(handle, "_BBN", NULL, &tmp);
+ bus = ACPI_SUCCESS(status) ? tmp : 0;
+
+ pci_bus = pci_find_bus(seg, bus);
+ if (!pci_bus)
+ return 0;
+
+ dbg("root bridge walk, pci_bus = %x\n", pci_bus->number);
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ user_function, pci_bus, NULL);
+ if (ACPI_FAILURE(status))
+ return status;
+
+ context.pci_bus = pci_bus;
+ context.user_function = user_function;
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ walk_p2p_bridge, &context, NULL);
+ if (ACPI_FAILURE(status))
+ err("%s: walk_p2p_bridge failure - %d\n", __FUNCTION__, status);
+
+ return status;
+}
+
+/*
+ * acpi_pci_slot_add
+ * @handle: points to an acpi_pci_root
+ */
+static int
+acpi_pci_slot_add(acpi_handle handle)
+{
+ acpi_status status;
+
+ status = walk_root_bridge(handle, register_slot);
+ if (ACPI_FAILURE(status))
+ err("%s: register_slot failure - %d\n", __FUNCTION__, status);
+
+ return status;
+}
+
+/*
+ * acpi_pci_slot_remove
+ * @handle: points to an acpi_pci_root
+ */
+static void
+acpi_pci_slot_remove(acpi_handle handle)
+{
+ acpi_status status;
+
+ status = walk_root_bridge(handle, unregister_slot);
+ if (ACPI_FAILURE(status))
+ err("%s: unregister_slot failure - %d\n", __FUNCTION__, status);
+}
+
+static int __init
+acpi_pci_slot_init(void)
+{
+ acpi_pci_register_driver(&acpi_pci_slot_driver);
+ return 0;
+}
+
+static void __exit
+acpi_pci_slot_exit(void)
+{
+ acpi_pci_unregister_driver(&acpi_pci_slot_driver);
+}
+
+module_init(acpi_pci_slot_init);
+module_exit(acpi_pci_slot_exit);
Index: linux-2.6.25-rc4/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================
--- linux-2.6.25-rc4.orig/drivers/pci/hotplug/pci_hotplug_core.c
+++ linux-2.6.25-rc4/drivers/pci/hotplug/pci_hotplug_core.c
@@ -567,6 +567,11 @@ int pci_hp_register(struct hotplug_slot
return -EINVAL;
}
+ /*
+ * No problems if we call this interface from both ACPI_PCI_SLOT
+ * driver and call it here again. If we've already created the
+ * pci_slot, the interface will simply bump the refcount.
+ */
pci_slot = pci_create_slot(bus, slot_nr, slot->name);
if (IS_ERR(pci_slot))
return PTR_ERR(pci_slot);
@@ -613,8 +618,12 @@ int pci_hp_deregister(struct hotplug_slo
slot = hotplug->pci_slot;
fs_remove_slot(slot);
- pci_destroy_slot(slot);
dbg("Removed slot %s from the list\n", hotplug->name);
+
+ hotplug_release(slot);
+ slot->release = NULL;
+ pci_destroy_slot(slot);
+
return 0;
}
Index: linux-2.6.25-rc4/drivers/pci/hotplug/pciehp_core.c
===================================================================
--- linux-2.6.25-rc4.orig/drivers/pci/hotplug/pciehp_core.c
+++ linux-2.6.25-rc4/drivers/pci/hotplug/pciehp_core.c
@@ -249,7 +249,7 @@ static int init_slots(struct controller
if (retval == -EBUSY)
goto error_info;
if (retval) {
- err ("pci_hp_register failed with error %d\n", retval);
+ err("pci_hp_register failed with error %d\n", retval);
goto error_info;
}
/* create additional sysfs entries */
Index: linux-2.6.25-rc4/drivers/pci/hotplug/sgi_hotplug.c
===================================================================
--- linux-2.6.25-rc4.orig/drivers/pci/hotplug/sgi_hotplug.c
+++ linux-2.6.25-rc4/drivers/pci/hotplug/sgi_hotplug.c
@@ -668,7 +668,7 @@ static int sn_hotplug_slot_register(stru
register_err:
dev_dbg(&pci_bus->self->dev, "bus failed to register with err = %d\n",
- rc);
+ rc);
alloc_err:
if (rc == -ENOMEM)
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 <[email protected]>
---
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 <[email protected]>"
@@ -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 - %d\n", __FUNCTION__, status);
}
+#ifdef CONFIG_DMI
+static int ignore_sta_before_sun(const struct dmi_system_id *d)
+{
+ info("%s detected: will ignore _STA before calling _SUN\n", d->ident);
+ ignore_sta = 1;
+ return 0;
+}
+
+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, ""),
+ },
+ },
+ */
+ {}
+};
+#endif /* CONFIG_DMI */
+
static int __init
acpi_pci_slot_init(void)
{
+ dmi_check_system(acpi_pci_slot_dmi_table);
acpi_pci_register_driver(&acpi_pci_slot_driver);
return 0;
}
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 <[email protected]>
Signed-off-by: Kenji Kaneshige <[email protected]>
---
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 +347,15 @@ config ACPI_EC
the battery and thermal drivers. If you are compiling for a
mobile system, say Y.
+config ACPI_PCI_SLOT
+ tristate "PCI slot detection driver"
+ default n
+ help
+ This driver will attempt to discover all PCI slots in your system,
+ and creates entries in /sys/bus/pci/slots/. This feature can
+ help you correlate PCI bus addresses with the physical geography
+ of your slots. If you are unsure, say N.
+
config ACPI_POWER
bool
default y
Index: linux-2.6.25-rc4/drivers/acpi/Makefile
===================================================================
--- linux-2.6.25-rc4.orig/drivers/acpi/Makefile
+++ linux-2.6.25-rc4/drivers/acpi/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_ACPI_DOCK) += dock.o
obj-$(CONFIG_ACPI_BAY) += bay.o
obj-$(CONFIG_ACPI_VIDEO) += video.o
obj-y += pci_root.o pci_link.o pci_irq.o pci_bind.o
+obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o
obj-$(CONFIG_ACPI_POWER) += power.o
obj-$(CONFIG_ACPI_PROCESSOR) += processor.o
obj-$(CONFIG_ACPI_CONTAINER) += container.o
Index: linux-2.6.25-rc4/drivers/acpi/pci_slot.c
===================================================================
--- /dev/null
+++ linux-2.6.25-rc4/drivers/acpi/pci_slot.c
@@ -0,0 +1,328 @@
+/*
+ * pci_slot.c - ACPI PCI Slot Driver
+ *
+ * The code here is heavily leveraged from the acpiphp module.
+ * Thanks to Matthew Wilcox <[email protected]> for much guidance.
+ *
+ * Copyright (C) 2007 Alex Chiang <[email protected]>
+ * Copyright (C) 2007 Hewlett-Packard Development Company, L.P.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+static int debug;
+
+#define DRIVER_VERSION "0.1"
+#define DRIVER_AUTHOR "Alex Chiang <[email protected]>"
+#define DRIVER_DESC "ACPI PCI Slot Detection Driver"
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
+MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
+module_param(debug, bool, 0644);
+
+#define _COMPONENT ACPI_PCI_COMPONENT
+ACPI_MODULE_NAME("pci_slot");
+
+#define MY_NAME "pci_slot"
+#define err(format, arg...) printk(KERN_ERR "%s: " format , MY_NAME , ## arg)
+#define info(format, arg...) printk(KERN_INFO "%s: " format , MY_NAME , ## arg)
+#define dbg(format, arg...) \
+ do { \
+ if (debug) \
+ printk(KERN_DEBUG "%s: " format, \
+ MY_NAME , ## arg); \
+ } while (0)
+
+static int acpi_pci_slot_add(acpi_handle handle);
+static void acpi_pci_slot_remove(acpi_handle handle);
+
+static struct acpi_pci_driver acpi_pci_slot_driver = {
+ .add = acpi_pci_slot_add,
+ .remove = acpi_pci_slot_remove,
+};
+
+static int
+check_slot(acpi_handle handle, int *device, unsigned long *sun)
+{
+ int retval = 0;
+ unsigned long adr, sta;
+ acpi_status status;
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+
+ 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;
+ }
+
+ status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
+ if (ACPI_FAILURE(status)) {
+ dbg("_ADR returned %d on %s\n", status, (char *)buffer.pointer);
+ retval = -1;
+ goto out;
+ }
+
+ *device = (adr >> 16) & 0xffff;
+
+ /* No _SUN == not a slot == bail */
+ status = acpi_evaluate_integer(handle, "_SUN", NULL, sun);
+ if (ACPI_FAILURE(status)) {
+ dbg("_SUN returned %d on %s\n", status, (char *)buffer.pointer);
+ retval = -1;
+ goto out;
+ }
+
+out:
+ kfree(buffer.pointer);
+ return retval;
+}
+
+/*
+ * unregister_slot
+ *
+ * Called once for each SxFy object in the namespace. Each call to
+ * pci_destroy_slot decrements the refcount on the pci_slot, and
+ * eventually calls kobject_unregister at the appropriate time.
+ */
+static acpi_status
+unregister_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device;
+ unsigned long sun;
+ struct pci_slot *slot;
+ struct pci_bus *pci_bus = context;
+
+ if (check_slot(handle, &device, &sun))
+ return AE_OK;
+
+ for (slot = pci_bus->slot; slot; slot = slot->next) {
+ if (slot->number == device)
+ pci_destroy_slot(slot);
+ }
+
+ return AE_OK;
+}
+
+/*
+ * register_slot
+ *
+ * Called once for each SxFy object in the namespace. Don't worry about
+ * calling pci_create_slot multiple times for the same pci_bus:device,
+ * since each subsequent call simply bumps the refcount on the pci_slot.
+ *
+ * The number of calls to pci_destroy_slot from unregister_slot is
+ * symmetrical.
+ */
+static acpi_status
+register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device;
+ unsigned long sun;
+ char name[KOBJ_NAME_LEN];
+
+ struct pci_slot *pci_slot;
+ struct pci_bus *pci_bus = context;
+
+ if (check_slot(handle, &device, &sun))
+ return AE_OK;
+
+ snprintf(name, sizeof(name), "%u", (u32)sun);
+ pci_slot = pci_create_slot(pci_bus, device, name);
+ if (IS_ERR(pci_slot))
+ err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
+
+ dbg("pci_slot: %#lx, pci_bus: %x, device: %d, name: %s\n",
+ (uint64_t)pci_slot, pci_bus->number, device, name);
+
+ return AE_OK;
+}
+
+struct p2p_bridge_context
+{
+ acpi_walk_callback user_function;
+ struct pci_bus *pci_bus;
+};
+
+/*
+ * walk_p2p_bridge - discover and walk p2p bridges
+ * @handle: points to an acpi_pci_root
+ * @context: p2p_bridge_context pointer
+ *
+ * Note that when we call ourselves recursively, we pass a different
+ * value of pci_bus in the child_context.
+ */
+static acpi_status
+walk_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+ int device, function;
+ unsigned long adr;
+ acpi_status status;
+ acpi_handle dummy_handle;
+ acpi_walk_callback user_function;
+
+ struct pci_dev *dev;
+ struct pci_bus *pci_bus;
+ struct p2p_bridge_context child_context;
+ struct p2p_bridge_context *parent_context = context;
+
+ pci_bus = parent_context->pci_bus;
+ user_function = parent_context->user_function;
+
+ status = acpi_get_handle(handle, "_ADR", &dummy_handle);
+ if (ACPI_FAILURE(status))
+ return AE_OK;
+
+ status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return AE_OK;
+
+ device = (adr >> 16) & 0xffff;
+ function = adr & 0xffff;
+
+ dev = pci_get_slot(pci_bus, PCI_DEVFN(device, function));
+ if (!dev || !dev->subordinate)
+ goto out;
+
+ dbg("p2p bridge walk, pci_bus = %x\n", dev->subordinate->number);
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ user_function, dev->subordinate, NULL);
+ if (ACPI_FAILURE(status))
+ goto out;
+
+ child_context.pci_bus = dev->subordinate;
+ child_context.user_function = user_function;
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ walk_p2p_bridge, &child_context, NULL);
+out:
+ pci_dev_put(dev);
+ return AE_OK;
+}
+
+#define ACPI_STA_FUNCTIONING (0x00000008)
+
+/*
+ * walk_root_bridge - generic root bridge walker
+ * @handle: points to an acpi_pci_root
+ * @user_function: user callback for slot objects
+ *
+ * Call user_function for all objects underneath this root bridge.
+ * Walk p2p bridges underneath us and call user_function on those too.
+ */
+static int
+walk_root_bridge(acpi_handle handle, acpi_walk_callback user_function)
+{
+ int seg, bus;
+ unsigned long tmp;
+ acpi_status status;
+ acpi_handle dummy_handle;
+ struct pci_bus *pci_bus;
+ struct p2p_bridge_context context;
+
+ /* If the bridge doesn't have _STA, we assume it is always there */
+ status = acpi_get_handle(handle, "_STA", &dummy_handle);
+ if (ACPI_SUCCESS(status)) {
+ status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
+ if (ACPI_FAILURE(status)) {
+ info("%s: _STA evaluation failure\n", __FUNCTION__);
+ return 0;
+ }
+ if ((tmp & ACPI_STA_FUNCTIONING) == 0)
+ /* don't register this object */
+ return 0;
+ }
+
+ status = acpi_evaluate_integer(handle, "_SEG", NULL, &tmp);
+ seg = ACPI_SUCCESS(status) ? tmp : 0;
+
+ status = acpi_evaluate_integer(handle, "_BBN", NULL, &tmp);
+ bus = ACPI_SUCCESS(status) ? tmp : 0;
+
+ pci_bus = pci_find_bus(seg, bus);
+ if (!pci_bus)
+ return 0;
+
+ dbg("root bridge walk, pci_bus = %x\n", pci_bus->number);
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ user_function, pci_bus, NULL);
+ if (ACPI_FAILURE(status))
+ return status;
+
+ context.pci_bus = pci_bus;
+ context.user_function = user_function;
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
+ walk_p2p_bridge, &context, NULL);
+ if (ACPI_FAILURE(status))
+ err("%s: walk_p2p_bridge failure - %d\n", __FUNCTION__, status);
+
+ return status;
+}
+
+/*
+ * acpi_pci_slot_add
+ * @handle: points to an acpi_pci_root
+ */
+static int
+acpi_pci_slot_add(acpi_handle handle)
+{
+ acpi_status status;
+
+ status = walk_root_bridge(handle, register_slot);
+ if (ACPI_FAILURE(status))
+ err("%s: register_slot failure - %d\n", __FUNCTION__, status);
+
+ return status;
+}
+
+/*
+ * acpi_pci_slot_remove
+ * @handle: points to an acpi_pci_root
+ */
+static void
+acpi_pci_slot_remove(acpi_handle handle)
+{
+ acpi_status status;
+
+ status = walk_root_bridge(handle, unregister_slot);
+ if (ACPI_FAILURE(status))
+ err("%s: unregister_slot failure - %d\n", __FUNCTION__, status);
+}
+
+static int __init
+acpi_pci_slot_init(void)
+{
+ acpi_pci_register_driver(&acpi_pci_slot_driver);
+ return 0;
+}
+
+static void __exit
+acpi_pci_slot_exit(void)
+{
+ acpi_pci_unregister_driver(&acpi_pci_slot_driver);
+}
+
+module_init(acpi_pci_slot_init);
+module_exit(acpi_pci_slot_exit);
Index: linux-2.6.25-rc4/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================
--- linux-2.6.25-rc4.orig/drivers/pci/hotplug/pci_hotplug_core.c
+++ linux-2.6.25-rc4/drivers/pci/hotplug/pci_hotplug_core.c
@@ -567,6 +567,11 @@ int pci_hp_register(struct hotplug_slot
return -EINVAL;
}
+ /*
+ * No problems if we call this interface from both ACPI_PCI_SLOT
+ * driver and call it here again. If we've already created the
+ * pci_slot, the interface will simply bump the refcount.
+ */
pci_slot = pci_create_slot(bus, slot_nr, slot->name);
if (IS_ERR(pci_slot))
return PTR_ERR(pci_slot);
@@ -613,8 +618,12 @@ int pci_hp_deregister(struct hotplug_slo
slot = hotplug->pci_slot;
fs_remove_slot(slot);
- pci_destroy_slot(slot);
dbg("Removed slot %s from the list\n", hotplug->name);
+
+ hotplug_release(slot);
+ slot->release = NULL;
+ pci_destroy_slot(slot);
+
return 0;
}
Index: linux-2.6.25-rc4/drivers/pci/hotplug/pciehp_core.c
===================================================================
--- linux-2.6.25-rc4.orig/drivers/pci/hotplug/pciehp_core.c
+++ linux-2.6.25-rc4/drivers/pci/hotplug/pciehp_core.c
@@ -249,7 +249,7 @@ static int init_slots(struct controller
if (retval == -EBUSY)
goto error_info;
if (retval) {
- err ("pci_hp_register failed with error %d\n", retval);
+ err("pci_hp_register failed with error %d\n", retval);
goto error_info;
}
/* create additional sysfs entries */
Index: linux-2.6.25-rc4/drivers/pci/hotplug/sgi_hotplug.c
===================================================================
--- linux-2.6.25-rc4.orig/drivers/pci/hotplug/sgi_hotplug.c
+++ linux-2.6.25-rc4/drivers/pci/hotplug/sgi_hotplug.c
@@ -668,7 +668,7 @@ static int sn_hotplug_slot_register(stru
register_err:
dev_dbg(&pci_bus->self->dev, "bus failed to register with err = %d\n",
- rc);
+ rc);
alloc_err:
if (rc == -ENOMEM)
On Tue, Mar 11, 2008 at 10:10:40PM +0900, Kenji Kaneshige wrote:
> 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.
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
Fujitsu does?
> 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
> 0011_0017 0016_0019 0021_0021 0032_0026
>
> - With ACPI PCI slot detection driver
> # ls /sys/bus/pci/slots/
> 0009_0016 0014_0018 0019_0020 0021_0022 27
> 0011_0017 0016_0019 0021_0021 26
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
occurred on one box with bad firmware.
> - Unify slot names among all hotplug drivers
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."
On Tuesday, March 11, 2008 6:28 am Matthew Wilcox wrote:
> On Tue, Mar 11, 2008 at 10:10:40PM +0900, Kenji Kaneshige wrote:
> > 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.
>
> 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
> Fujitsu does?
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
On Tue, 11 Mar 2008 22:10:40 +0900
Kenji Kaneshige <[email protected]> wrote:
> Hi Alex-san,
>
> Alex Chiang wrote:
> >>> 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 ;)]
> >> 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).
> >
> > It looks like we disagree on how to interpret the spec (IBM
> > machines interpret the spec the same way HP machines do).
> >
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,
* Kristen Carlson Accardi <[email protected]>:
> On Tue, 11 Mar 2008 22:10:40 +0900
> Kenji Kaneshige <[email protected]> wrote:
>
> > Hi Alex-san,
> >
> > Alex Chiang wrote:
> > >>> 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 ;)]
> > >> 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).
> > >
> > > It looks like we disagree on how to interpret the spec (IBM
> > > machines interpret the spec the same way HP machines do).
> > >
>
> 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.
Yes, I agree that is what we are discussing here.
> 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.
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 I
apologize for that. I can change the comments to remove that
language.
I do think that what we should do is see which list is easier to
maintain, based on "what interpretation is being implemented on
most platforms".
If one list is shorter than the other, then that should be the
list to put in the kernel, and the default behavior should be
"majority rule".
Thanks.
/ac
On Tue, 11 Mar 2008 22:10:40 +0900
Kenji Kaneshige <[email protected]> wrote:
> 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.
yes, we will not merge these patches without your ack.
>
> 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.
I have followed up with a question on this on another thread, so I'll
skip to #2...
<snip>
>
> 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
> 0011_0017 0016_0019 0021_0021 0032_0026
>
> - With ACPI PCI slot detection driver
> # ls /sys/bus/pci/slots/
> 0009_0016 0014_0018 0019_0020 0021_0022 27
> 0011_0017 0016_0019 0021_0021 26
>
> I had thought it is not a big problem before because people who
> don't like new names would not load the PCI slot driver. But since
> it is loaded automatically at boot time, I'm wondering that it
> would be a problem. For example, some platform, not fujitsu,
> depends on the old slot name to work, IIRC (Maybe Kristen knows
> the background about it). And I don't think the fact that slot
> names are changed depending on the order of driver loading is
> acceptable by system management people/software, though I don't
> have such software.
>
> Though I don't have any specific idea about this, folliwings might
> be candidates.
>
> - Override slot names with hotplug driver's slot names
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
set the name of the slot.
> - Unify slot names among all hotplug drivers
I'm not sure if we can do this, since slot name might depend on what
spec people are implementing.
> - Stop automatic loading of ACPI PCI slot driver
I think we should definitely implement this one ^^^.
Hi,
Matthew Wilcox wrote:
> On Tue, Mar 11, 2008 at 10:10:40PM +0900, Kenji Kaneshige wrote:
>> 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.
>
> 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
> Fujitsu does?
>
I don't know.
>> 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
>> 0011_0017 0016_0019 0021_0021 0032_0026
>>
>> - With ACPI PCI slot detection driver
>> # ls /sys/bus/pci/slots/
>> 0009_0016 0014_0018 0019_0020 0021_0022 27
>> 0011_0017 0016_0019 0021_0021 26
>
> 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
> occurred on one box with bad firmware.
>
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?
- and so on.
>> - Unify slot names among all hotplug drivers
>
> 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?
>
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
Hi,
Jesse Barnes wrote:
> On Tuesday, March 11, 2008 6:28 am Matthew Wilcox wrote:
>> On Tue, Mar 11, 2008 at 10:10:40PM +0900, Kenji Kaneshige wrote:
>>> 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.
>> 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
>> Fujitsu does?
>
> 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)?
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
because HP's ACPI2.0 based machine is doing same as ACPI1.0b based one.
> 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?
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,
> 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?
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
return errors in this situation.
> 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.
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
Kristen Carlson Accardi wrote:
> On Tue, 11 Mar 2008 22:10:40 +0900
> Kenji Kaneshige <[email protected]> wrote:
>
>> Hi Alex-san,
>>
>> Alex Chiang wrote:
>>>>> 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 ;)]
>>>> 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).
>>> It looks like we disagree on how to interpret the spec (IBM
>>> machines interpret the spec the same way HP machines do).
>>>
>
> 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 Alex-san,
> 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.
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
your driver may ignore _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.
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
before evaluating _SUN is legal. Am I missing something?
> If one list is shorter than the other, then that should be the
> list to put in the kernel, and the default behavior should be
> "majority rule".
I don't want to consider "majority rule" before I understand why
ignoring _STA is legal.
Thanks,
Kenji Kaneshige
Kristen Carlson Accardi wrote:
> On Tue, 11 Mar 2008 22:10:40 +0900
> Kenji Kaneshige <[email protected]> wrote:
>
>> 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.
>
> yes, we will not merge these patches without your ack.
>
>> 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.
>
> I have followed up with a question on this on another thread, so I'll
> skip to #2...
> <snip>
>
>> 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
>> 0011_0017 0016_0019 0021_0021 0032_0026
>>
>> - With ACPI PCI slot detection driver
>> # ls /sys/bus/pci/slots/
>> 0009_0016 0014_0018 0019_0020 0021_0022 27
>> 0011_0017 0016_0019 0021_0021 26
>>
>> I had thought it is not a big problem before because people who
>> don't like new names would not load the PCI slot driver. But since
>> it is loaded automatically at boot time, I'm wondering that it
>> would be a problem. For example, some platform, not fujitsu,
>> depends on the old slot name to work, IIRC (Maybe Kristen knows
>> the background about it). And I don't think the fact that slot
>> names are changed depending on the order of driver loading is
>> acceptable by system management people/software, though I don't
>> have such software.
>>
>> Though I don't have any specific idea about this, folliwings might
>> be candidates.
>>
>> - Override slot names with hotplug driver's slot names
>
> 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
> set the name of the slot.
>
Yes, it is what I thought.
But maybe we should try unifying slots names first, I think.
>> - Unify slot names among all hotplug drivers
>
> I'm not sure if we can do this, since slot name might depend on what
> spec people are implementing.
>
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
which is based on PCI Hot-Plug spec.
>> - Stop automatic loading of ACPI PCI slot driver
>
> I think we should definitely implement this one ^^^.
>
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
Hi Kenji-san,
* Kenji Kaneshige <[email protected]>:
> Hi Alex-san,
>
>> 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.
>
> 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
> your driver may ignore _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.
>
> 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
> before evaluating _SUN is legal. Am I missing something?
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.
I believe that we both have compliant, legal firmware.
>> If one list is shorter than the other, then that should be the
>> list to put in the kernel, and the default behavior should be
>> "majority rule".
>
> I don't want to consider "majority rule" before I understand why
> ignoring _STA is legal.
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 try and work out an answer here before working on shpchp
naming issues; I already agree that I need to fix that issue
before going upstream, but I want to focus here first.]
Thanks,
/ac
Hi Alex/Kenji-san,
Time for my 2 cents.
On Wed, Mar 12, 2008 at 09:24:10PM -0600, Alex Chiang wrote:
> Hi Kenji-san,
>
> * Kenji Kaneshige <[email protected]>:
> > Hi Alex-san,
> >
> >> 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.
> >
> > 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
> > your driver may ignore _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.
> >
> > 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
> > before evaluating _SUN is legal. Am I missing something?
>
> 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
Agreed.
> - _INI must be called to initialize _SUN
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
says nothing about a required presence of the device.
> - _INI is called once, when the table is loaded
Agreed.
>
> 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,
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: \_SB_.VP03.S2F1 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F2 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F3 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F4 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F5 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F6 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F7 sta=f sun=2
acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:0a:00.0
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F0 sta=0 sun=3
acpiphp_glue: found ACPI PCI Hotplug slot 3 at PCI 0000:0b:00
acpiphp: Slot [3] registered
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F1 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F2 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F3 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F4 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F5 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F6 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F7 sta=0 sun=3
acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:0f:00.0
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F0 sta=f sun=4
acpiphp_glue: found ACPI PCI Hotplug slot 4 at PCI 0000:10:00
acpiphp: Slot [4] registered
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F1 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F2 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F3 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F4 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F5 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F6 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F7 sta=f sun=4
acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:14:00.0
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F0 sta=0 sun=5
acpiphp_glue: found ACPI PCI Hotplug slot 5 at PCI 0000:15:00
acpiphp: Slot [5] registered
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F1 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F2 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F3 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F4 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F5 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F6 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F7 sta=0 sun=5
acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:19:00.0
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F0 sta=f sun=6
acpiphp_glue: found ACPI PCI Hotplug slot 6 at PCI 0000:1a:00
acpiphp: Slot [6] registered
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F1 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F2 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F3 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F4 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F5 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F6 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F7 sta=f sun=6
acpiphp_glue: Bus 0000:1a has 1 slot
acpiphp_glue: Bus 0000:15 has 1 slot
acpiphp_glue: Bus 0000:10 has 1 slot
acpiphp_glue: Bus 0000:0b has 1 slot
acpiphp_glue: Bus 0000:06 has 1 slot
acpiphp_glue: Bus 0000:02 has 1 slot
acpiphp_glue: Total 6 slots
> 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.
>
> I believe that we both have compliant, legal firmware.
>
> >> If one list is shorter than the other, then that should be the
> >> list to put in the kernel, and the default behavior should be
> >> "majority rule".
> >
> > I don't want to consider "majority rule" before I understand why
> > ignoring _STA is legal.
>
> 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.
I am not convinced that this a correct interpretation. I believe
that the ACPI spec indicates that it is legal to call _SUN and
trust it's results no matter what _STA returns. I believe the
only constraint on running _SUN is that it be run after _INI is run
if _INI exists for the same device and _STA for that device
indicates that it should be run.
Gary
--
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503 IBM T/L: 775-4503
[email protected]
http://www.ibm.com/linux/ltc
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
Gary Hade wrote:
> Hi Alex/Kenji-san,
>
> Time for my 2 cents.
>
> On Wed, Mar 12, 2008 at 09:24:10PM -0600, Alex Chiang wrote:
>> Hi Kenji-san,
>>
>> * Kenji Kaneshige <[email protected]>:
>>> Hi Alex-san,
>>>
>>>> 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.
>>> 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
>>> your driver may ignore _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.
>>> 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
>>> before evaluating _SUN is legal. Am I missing something?
>> 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
> Agreed.
>
>> - _INI must be called to initialize _SUN
> 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
> says nothing about a required presence of the device.
>
>> - _INI is called once, when the table is loaded
> Agreed.
>
>> 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,
>
> 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: \_SB_.VP03.S2F1 sta=f sun=2
> acpiphp_glue: register_slot: \_SB_.VP03.S2F2 sta=f sun=2
> acpiphp_glue: register_slot: \_SB_.VP03.S2F3 sta=f sun=2
> acpiphp_glue: register_slot: \_SB_.VP03.S2F4 sta=f sun=2
> acpiphp_glue: register_slot: \_SB_.VP03.S2F5 sta=f sun=2
> acpiphp_glue: register_slot: \_SB_.VP03.S2F6 sta=f sun=2
> acpiphp_glue: register_slot: \_SB_.VP03.S2F7 sta=f sun=2
> acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:0a:00.0
> acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F0 sta=0 sun=3
> acpiphp_glue: found ACPI PCI Hotplug slot 3 at PCI 0000:0b:00
> acpiphp: Slot [3] registered
> acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F1 sta=0 sun=3
> acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F2 sta=0 sun=3
> acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F3 sta=0 sun=3
> acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F4 sta=0 sun=3
> acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F5 sta=0 sun=3
> acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F6 sta=0 sun=3
> acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F7 sta=0 sun=3
> acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:0f:00.0
> acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F0 sta=f sun=4
> acpiphp_glue: found ACPI PCI Hotplug slot 4 at PCI 0000:10:00
> acpiphp: Slot [4] registered
> acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F1 sta=f sun=4
> acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F2 sta=f sun=4
> acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F3 sta=f sun=4
> acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F4 sta=f sun=4
> acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F5 sta=f sun=4
> acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F6 sta=f sun=4
> acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F7 sta=f sun=4
> acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:14:00.0
> acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F0 sta=0 sun=5
> acpiphp_glue: found ACPI PCI Hotplug slot 5 at PCI 0000:15:00
> acpiphp: Slot [5] registered
> acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F1 sta=0 sun=5
> acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F2 sta=0 sun=5
> acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F3 sta=0 sun=5
> acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F4 sta=0 sun=5
> acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F5 sta=0 sun=5
> acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F6 sta=0 sun=5
> acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F7 sta=0 sun=5
> acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:19:00.0
> acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F0 sta=f sun=6
> acpiphp_glue: found ACPI PCI Hotplug slot 6 at PCI 0000:1a:00
> acpiphp: Slot [6] registered
> acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F1 sta=f sun=6
> acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F2 sta=f sun=6
> acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F3 sta=f sun=6
> acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F4 sta=f sun=6
> acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F5 sta=f sun=6
> acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F6 sta=f sun=6
> acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F7 sta=f sun=6
> acpiphp_glue: Bus 0000:1a has 1 slot
> acpiphp_glue: Bus 0000:15 has 1 slot
> acpiphp_glue: Bus 0000:10 has 1 slot
> acpiphp_glue: Bus 0000:0b has 1 slot
> acpiphp_glue: Bus 0000:06 has 1 slot
> acpiphp_glue: Bus 0000:02 has 1 slot
> acpiphp_glue: Total 6 slots
>
>> 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.
>>
>> I believe that we both have compliant, legal firmware.
>>
>>>> If one list is shorter than the other, then that should be the
>>>> list to put in the kernel, and the default behavior should be
>>>> "majority rule".
>>> I don't want to consider "majority rule" before I understand why
>>> ignoring _STA is legal.
>> 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.
>
> I am not convinced that this a correct interpretation. I believe
> that the ACPI spec indicates that it is legal to call _SUN and
> trust it's results no matter what _STA returns. I believe the
> only constraint on running _SUN is that it be run after _INI is run
> if _INI exists for the same device and _STA for that device
> indicates that it should be run.
>
> Gary
>
Hi Kenji-san,
> 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.
Great, thanks.
> I don't know which implementation is more popular. But according
> to the current score (2 : 1), you win the "majority rule".
Yep, and when we get more testing, if it turns out that more
vendors follow the Fujitsu interpretation, then we can change the
default behavior.
> 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.
Yes, I'll send out a new patch series today that also fixes the
shpchp/pciehp naming issue.
Thanks.
/ac