[PATCH] PCI Hotplug: add address file and fix acpiphp bugs

This is the pending patch that adds 'address' file to show
PCI-address and a few other minor fixes.
As 2.6.0 is out, I'm resending the patch.
Would you mind taking this?

> > > Thanks.  I had a little time to try your patch today.  Sorry
> > > to report that it isn't working for me.
> > >
> > > I first powered off (successfully the 1st time) a populated slot
> > > and removed and reinserted the card into the same slot.  The slot
> > > powered back up but I was then unable to power it off.  I believe
> > > the following instruction that still exists in power_off_slot()
> > > may be preventing the slot from being powered off more than once.
> > >     func->flags &= (~FUNC_EXISTS);
> > >
> > > I then tried to insert an adapter in an un-populated slot.  For
> > > some reason (which I don't understand yet) there was an enabling
> > > error which I believe caused enable_device() to exit via a path
> > > that bypassed the instruction that sets the FUNC_EXISTS flag.
> > > I was then unable to power off the slot which I believe was due
> > > to the FUNC_EXISTS flag not being set.
> > >
> > > I didn't have time to definitely confirmed the above theories.
> > > I'll take a closer look at this tomorrow unless you are able
> > > to diagnose using my vague clues :)
> >
> > It turns out that both of the above mentioned problems happened
> > because the call to acpiphp_configure_slot() from enable_device()
> > failed after inserting the card.  When this happens enable_device()
> > exits without setting the FUNC_EXISTS flag for any of the slot
> > functions.  Subsequent attempts to power off the same slot fail
> > when power_off_slot() is unable to locate a function with both
> > FUNC_HAS_EJ0 and FUNC_EXISTS flags set.
> >
> > The patch works okay when using a card that allows
> > acpiphp_configure_slot() to succeed but I believe it should
> > be improved to allow the slot to be powered off following
> > device enablement errors.
>
> Thanks for testing and comments.
> I really appreciate it.
>
> This problem turned out to be somewhat fragile state
> transition:
>
> a lifecycle of a slot is (if there's no error)
>
>   function             state
> ----------------------------------------------------
> 0                      nothing
> 1  power_on_slot()  -> SLOT_POWERDON
> 2  enable_device()  -> SLOT_POWEREDON + SLOT_ENABLED
> 3  disable_device() -> SLOT_POWEREDON
> 4  power_off_slot() -> nothing
>
> but if any error occur during enable_device(), slot will remain
> SLOT_POWERDON, but some functions on the card may not have
> FUNC_EXISTS flags, which will eventually prevents powering
> off in power_off_slot(), state transition from 1 to 4 directly.
> I.e, the FUNC_EXISTS flag introduced more states to
> complicate things.
>
> The FUNC_EXISTS flag was introduced after some discussion
> between me and Irene Zubarev, but it has no more meaning
> than that the function has corresponding 'pci_dev' structure.
> So I eliminated the usage of FUNC_EXISTS and the result is
> the patches attached to this mail (for both 2.4 and 2.6.
> I think Greg already applied the 2.4 'cleanup' patch to his tree,
> but it's not in Marcelo's release so I'm re-attaching to
> this mail for anyone interested in this topic.  It's identical
> to the one I posted earlier).
> These patches don't include Gary's patch in his post last week,
> so please apply separately.
>
> Please note that current acpiphp driver cannot handle a
> PCI card that has a PCI-to-PCI bridge on it (support
> for such cards is incomplete).  But if it's treated as
> an error, it should be recoverable anyway.

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 0c41e66a10b63926b8797c57129d5456d92e435d..6d25ebf99fd5030f26d4deed632f66b09b993527 100644 (file)
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -201,7 +201,7 @@ struct acpiphp_func {
 
 #define SLOT_POWEREDON         (0x00000001)
 #define SLOT_ENABLED           (0x00000002)
-#define SLOT_MULTIFUNCTION     (x000000004)
+#define SLOT_MULTIFUNCTION     (0x00000004)
 
 /* function flags */
 
@@ -212,8 +212,6 @@ struct acpiphp_func {
 #define FUNC_HAS_PS2           (0x00000040)
 #define FUNC_HAS_PS3           (0x00000080)
 
-#define FUNC_EXISTS            (0x10000000) /* to make sure we call _EJ0 only for existing funcs */
-
 /* function prototypes */
 
 /* acpiphp_glue.c */
@@ -231,6 +229,7 @@ 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);
 
 /* acpiphp_pci.c */
 extern struct pci_dev *acpiphp_allocate_pcidev (struct pci_bus *pbus, int dev, int fn);

diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
index 544fb8976a0edbd0e4dbf2e4de69f2a487ff5a30..35f008c26b11e8746be196e631a5d4877d9f08ad 100644 (file)
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -30,14 +30,14 @@
  *
  */
 
-#include <linux/config.h>
-#include <linux/kernel.h>
+#include <linux/init.h>
 #include <linux/module.h>
+
+#include <linux/kernel.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/smp.h>
 #include <linux/smp_lock.h>
-#include <linux/init.h>
 #include "pci_hotplug.h"
 #include "acpiphp.h"
 
@@ -71,6 +71,7 @@ static int set_attention_status (struct hotplug_slot *slot, u8 value);
 static int hardware_test       (struct hotplug_slot *slot, u32 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);
 static int get_max_bus_speed   (struct hotplug_slot *hotplug_slot, enum pci_bus_speed *value);
@@ -86,6 +87,7 @@ 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,
        .get_max_bus_speed      = get_max_bus_speed,
        .get_cur_bus_speed      = get_cur_bus_speed,
 };
@@ -322,6 +324,28 @@ static int get_adapter_status (struct hotplug_slot *hotplug_slot, u8 *value)
 }
 
 
+/**
+ * get_address - get pci address of a slot
+ * @hotplug_slot: slot to get status
+ * @busdev: pointer to struct pci_busdev (seg, bus, dev)
+ *
+ */
+static int get_address (struct hotplug_slot *hotplug_slot, u32 *value)
+{
+       struct slot *slot = get_slot(hotplug_slot, __FUNCTION__);
+       int retval = 0;
+
+       if (slot == NULL)
+               return -ENODEV;
+
+       dbg("%s - physical_slot = %s\n", __FUNCTION__, hotplug_slot->name);
+
+       *value = acpiphp_get_address(slot->acpi_slot);
+
+       return retval;
+}
+
+
 /* return dummy value because ACPI doesn't provide any method... */
 static int get_max_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_speed *value)
 {

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 8e9c0036be167a404b9c6a6990c1f640bbd5a748..2ada5edfee0eaded8a878cbe1992948955edbdf5 100644 (file)
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -26,12 +26,12 @@
  *
  */
 
-#include <linux/config.h>
-#include <linux/kernel.h>
+#include <linux/init.h>
 #include <linux/module.h>
+
+#include <linux/kernel.h>
 #include <linux/pci.h>
 #include <linux/smp_lock.h>
-#include <linux/init.h>
 #include <asm/semaphore.h>
 
 #include "../pci.h"
@@ -686,7 +686,7 @@ static int power_on_slot (struct acpiphp_slot *slot)
        struct list_head *l;
        int retval = 0;
 
-       /* is this already enabled? */
+       /* if already enabled, just skip */
        if (slot->flags & SLOT_POWEREDON)
                goto err_exit;
 
@@ -724,14 +724,14 @@ static int power_off_slot (struct acpiphp_slot *slot)
 
        int retval = 0;
 
-       /* is this already enabled? */
+       /* if already disabled, just skip */
        if ((slot->flags & SLOT_POWEREDON) == 0)
                goto err_exit;
 
        list_for_each (l, &slot->funcs) {
                func = list_entry(l, struct acpiphp_func, sibling);
 
-               if (func->flags & (FUNC_HAS_PS3 | FUNC_EXISTS)) {
+               if (func->pci_dev && (func->flags & FUNC_HAS_PS3)) {
                        status = acpi_evaluate_object(func->handle, "_PS3", NULL, NULL);
                        if (ACPI_FAILURE(status)) {
                                warn("%s: _PS3 failed\n", __FUNCTION__);
@@ -745,7 +745,7 @@ static int power_off_slot (struct acpiphp_slot *slot)
                func = list_entry(l, struct acpiphp_func, sibling);
 
                /* We don't want to call _EJ0 on non-existing functions. */
-               if (func->flags & (FUNC_HAS_EJ0 | FUNC_EXISTS)) {
+               if (func->pci_dev && (func->flags & FUNC_HAS_EJ0)) {
                        /* _EJ0 method take one argument */
                        arg_list.count = 1;
                        arg_list.pointer = &arg;
@@ -758,7 +758,6 @@ static int power_off_slot (struct acpiphp_slot *slot)
                                retval = -1;
                                goto err_exit;
                        }
-                       func->flags &= (~FUNC_EXISTS);
                }
        }
 
@@ -838,8 +837,6 @@ static int enable_device (struct acpiphp_slot *slot)
                retval = acpiphp_configure_function(func);
                if (retval)
                        goto err_exit;
-
-               func->flags |= FUNC_EXISTS;
        }
 
        slot->flags |= SLOT_ENABLED;
@@ -1349,3 +1346,18 @@ 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;
+
+       address = ((slot->bridge->seg) << 16) |
+                 ((slot->bridge->bus) << 8) |
+                 slot->device;
+
+       return address;
+}

diff --git a/drivers/pci/hotplug/acpiphp_pci.c b/drivers/pci/hotplug/acpiphp_pci.c
index 93eb2eff595f03fa414181747d70203728b60d31..9d9dabdcee9c4305ff5113b84081aaf62116e13d 100644 (file)
--- a/drivers/pci/hotplug/acpiphp_pci.c
+++ b/drivers/pci/hotplug/acpiphp_pci.c
@@ -29,11 +29,11 @@
  *
  */
 
-#include <linux/config.h>
-#include <linux/kernel.h>
+#include <linux/init.h>
 #include <linux/module.h>
+
+#include <linux/kernel.h>
 #include <linux/pci.h>
-#include <linux/init.h>
 #include <linux/acpi.h>
 #include "../pci.h"
 #include "pci_hotplug.h"

diff --git a/drivers/pci/hotplug/acpiphp_res.c b/drivers/pci/hotplug/acpiphp_res.c
index 060095be323336490dcf7fa0db924515b9149916..ba9cbb995030a8420bf4c770f7699511948aaa96 100644 (file)
--- a/drivers/pci/hotplug/acpiphp_res.c
+++ b/drivers/pci/hotplug/acpiphp_res.c
@@ -29,7 +29,7 @@
  *
  */
 
-#include <linux/config.h>
+#include <linux/init.h>
 #include <linux/module.h>
 
 #include <linux/kernel.h>
@@ -39,7 +39,6 @@
 #include <linux/pci.h>
 #include <linux/smp.h>
 #include <linux/smp_lock.h>
-#include <linux/init.h>
 
 #include <linux/string.h>
 #include <linux/mm.h>

diff --git a/drivers/pci/hotplug/pci_hotplug.h b/drivers/pci/hotplug/pci_hotplug.h
index 0c003145dd767599ed5dae86051bc070dca1f9bf..77fed093c002627041430a73ae880cb591cba8c6 100644 (file)
--- a/drivers/pci/hotplug/pci_hotplug.h
+++ b/drivers/pci/hotplug/pci_hotplug.h
@@ -74,6 +74,9 @@ 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.
@@ -96,6 +99,7 @@ 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);
 };
@@ -106,6 +110,7 @@ struct hotplug_slot_ops {
  * @attention_status: if the attention light is enabled or not (1/0)
  * @latch_status: if the latch (if any) is open or closed (1/0)
  * @adapter_present: if there is a pci board present in the slot or not (1/0)
+ * @address: (domain << 16 | bus << 8 | dev)
  *
  * Used to notify the hotplug pci core of the status of a specific slot.
  */
@@ -114,6 +119,7 @@ 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;
 };

diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 6732c33a8b5166140f5f6a94926bfb239052dfba..64a5393387014a04e3e7ed80710aa53fa8af949a 100644 (file)
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -159,6 +159,7 @@ 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)
 
@@ -302,6 +303,28 @@ static struct hotplug_slot_attribute hotplug_slot_attr_presence = {
        .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)
@@ -425,6 +448,15 @@ 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)
 {
        if ((!slot) || (!slot->ops))
@@ -466,6 +498,9 @@ static int fs_add_slot (struct hotplug_slot *slot)
        if (has_adapter_file(slot) == 0)
                sysfs_create_file(&slot->kobj, &hotplug_slot_attr_presence.attr);
 
+       if (has_address_file(slot) == 0)
+               sysfs_create_file(&slot->kobj, &hotplug_slot_attr_address.attr);
+
        if (has_max_bus_speed_file(slot) == 0)
                sysfs_create_file(&slot->kobj, &hotplug_slot_attr_max_bus_speed.attr);
 
@@ -492,6 +527,9 @@ 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);
 
@@ -612,6 +650,10 @@ int pci_hp_change_slot_info (struct hotplug_slot *slot, struct hotplug_slot_info
            (slot->info->adapter_status != info->adapter_status))
                sysfs_update_file(&slot->kobj, &hotplug_slot_attr_presence.attr);
 
+       if ((has_address_file(slot) == 0) &&
+           (slot->info->address != info->address))
+               sysfs_update_file(&slot->kobj, &hotplug_slot_attr_address.attr);
+
        if ((has_max_bus_speed_file(slot) == 0) &&
            (slot->info->max_bus_speed != info->max_bus_speed))
                sysfs_update_file(&slot->kobj, &hotplug_slot_attr_max_bus_speed.attr);