On Mon, May 02, 2016 at 04:11:53PM +1000, Alexey Kardashevskiy wrote:
>On 05/02/2016 01:44 PM, Gavin Shan wrote:
>>On Tue, Apr 19, 2016 at 08:36:48PM +1000, Alexey Kardashevskiy wrote:
>>>On 02/17/2016 02:44 PM, Gavin Shan wrote:
>>>>This adds standalone driver to support PCI hotplug for PowerPC PowerNV
>>>>platform that runs on top of skiboot firmware. The firmware identifies
>>>>hotpluggable slots and marked their device tree node with proper
>>>>"ibm,slot-pluggable" and "ibm,reset-by-firmware". The driver scans
>>>>device tree nodes to create/register PCI hotplug slot accordingly.
>>>>
>>>>The PCI slots are organized in fashion of tree, which means one
>>>>PCI slot might have parent PCI slot and parent PCI slot possibly
>>>>contains multiple child PCI slots. At the plugging time, the parent
>>>>PCI slot is populated before its children. The child PCI slots are
>>>>removed before their parent PCI slot can be removed from the system.
>>>>
>>>>If the skiboot firmware doesn't support slot status retrieval, the PCI
>>>>slot device node shouldn't have property "ibm,reset-by-firmware". In
>>>>that case, none of valid PCI slots will be detected from device tree.
>>>>The skiboot firmware doesn't export the capability to access attention
>>>>LEDs yet and it's something for TBD.
>>>>
>>>>Signed-off-by: Gavin Shan <gws...@linux.vnet.ibm.com>
>>>>Acked-by: Bjorn Helgaas <bhelg...@google.com>
>>>>---
>>>> drivers/pci/hotplug/Kconfig   |  12 +
>>>> drivers/pci/hotplug/Makefile  |   3 +
>>>> drivers/pci/hotplug/pnv_php.c | 870 
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 885 insertions(+)
>>>> create mode 100644 drivers/pci/hotplug/pnv_php.c
>>>>
>>>>diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>>>>index df8caec..167c8ce 100644
>>>>--- a/drivers/pci/hotplug/Kconfig
>>>>+++ b/drivers/pci/hotplug/Kconfig
>>>>@@ -113,6 +113,18 @@ config HOTPLUG_PCI_SHPC
>>>>
>>>>      When in doubt, say N.
>>>>
>>>>+config HOTPLUG_PCI_POWERNV
>>>>+   tristate "PowerPC PowerNV PCI Hotplug driver"
>>>>+   depends on PPC_POWERNV && EEH
>>>>+   help
>>>>+     Say Y here if you run PowerPC PowerNV platform that supports
>>>>+     PCI Hotplug
>>>>+
>>>>+     To compile this driver as a module, choose M here: the
>>>>+     module will be called pnv-php.
>>>>+
>>>>+     When in doubt, say N.
>>>>+
>>>> config HOTPLUG_PCI_RPA
>>>>    tristate "RPA PCI Hotplug driver"
>>>>    depends on PPC_PSERIES && EEH
>>>>diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
>>>>index b616e75..e33cdda 100644
>>>>--- a/drivers/pci/hotplug/Makefile
>>>>+++ b/drivers/pci/hotplug/Makefile
>>>>@@ -14,6 +14,7 @@ obj-$(CONFIG_HOTPLUG_PCI_PCIE)            += pciehp.o
>>>> obj-$(CONFIG_HOTPLUG_PCI_CPCI_ZT5550)      += cpcihp_zt5550.o
>>>> obj-$(CONFIG_HOTPLUG_PCI_CPCI_GENERIC)     += cpcihp_generic.o
>>>> obj-$(CONFIG_HOTPLUG_PCI_SHPC)             += shpchp.o
>>>>+obj-$(CONFIG_HOTPLUG_PCI_POWERNV)  += pnv-php.o
>>>> obj-$(CONFIG_HOTPLUG_PCI_RPA)              += rpaphp.o
>>>> obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)        += rpadlpar_io.o
>>>> obj-$(CONFIG_HOTPLUG_PCI_SGI)              += sgi_hotplug.o
>>>>@@ -50,6 +51,8 @@ ibmphp-objs               :=      ibmphp_core.o   \
>>>> acpiphp-objs               :=      acpiphp_core.o  \
>>>>                            acpiphp_glue.o
>>>>
>>>>+pnv-php-objs               :=      pnv_php.o
>>>>+
>>>> rpaphp-objs                :=      rpaphp_core.o   \
>>>>                            rpaphp_pci.o    \
>>>>                            rpaphp_slot.o
>>>>diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
>>>>new file mode 100644
>>>>index 0000000..364ec36
>>>>--- /dev/null
>>>>+++ b/drivers/pci/hotplug/pnv_php.c
>>>>@@ -0,0 +1,870 @@
>>>>+/*
>>>>+ * PCI Hotplug Driver for PowerPC PowerNV platform.
>>>>+ *
>>>>+ * Copyright Gavin Shan, IBM Corporation 2015.
>>>>+ *
>>>>+ * This program is free software; you can redistribute it and/or modify
>>>>+ * it under the terms of the GNU General Public License as published by
>>>>+ * the Free Software Foundation; either version 2 of the License, or
>>>>+ * (at your option) any later version.
>>>>+ */
>>>>+
>>>>+#include <linux/libfdt.h>
>>>>+#include <linux/module.h>
>>>>+#include <linux/pci.h>
>>>>+#include <linux/pci_hotplug.h>
>>>>+
>>>>+#include <asm/opal.h>
>>>>+#include <asm/pnv-pci.h>
>>>>+#include <asm/ppc-pci.h>
>>>>+
>>>>+#define DRIVER_VERSION     "0.1"
>>>>+#define DRIVER_AUTHOR      "Gavin Shan, IBM Corporation"
>>>>+#define DRIVER_DESC        "PowerPC PowerNV PCI Hotplug Driver"
>>>>+
>>>>+struct pnv_php_slot {
>>>>+   struct hotplug_slot             slot;
>>>>+   struct hotplug_slot_info        slot_info;
>>>>+   uint64_t                        id;
>>>>+   char                            *name;
>>>>+   int                             slot_no;
>>>>+   struct kref                     kref;
>>>>+#define PNV_PHP_STATE_INITIALIZED  0
>>>>+#define PNV_PHP_STATE_REGISTERED   1
>>>>+#define PNV_PHP_STATE_POPULATED            2
>>>>+   int                             state;
>>>>+   struct device_node              *dn;
>>>>+   struct pci_dev                  *pdev;
>>>>+   struct pci_bus                  *bus;
>>>>+   bool                            power_state_check;
>>>>+   int                             power_state_confirmed;
>>>>+#define PNV_PHP_POWER_CONFIRMED_INVALID    0
>>>>+#define PNV_PHP_POWER_CONFIRMED_SUCCESS    1
>>>>+#define PNV_PHP_POWER_CONFIRMED_FAIL       2
>>>>+   struct opal_msg                 *msg;
>>>>+   void                            *fdt;
>>>>+   void                            *dt;
>>>>+   struct of_changeset             ocs;
>>>>+   struct work_struct              work;
>>>>+   wait_queue_head_t               queue;
>>>>+   struct pnv_php_slot             *parent;
>>>>+   struct list_head                children;
>>>>+   struct list_head                link;
>>>>+};
>>>>+
>>>>+static LIST_HEAD(pnv_php_slot_list);
>>>>+static DEFINE_SPINLOCK(pnv_php_lock);
>>>>+
>>>>+static void pnv_php_register(struct device_node *dn);
>>>>+static void pnv_php_unregister_one(struct device_node *dn);
>>>>+static void pnv_php_unregister(struct device_node *dn);
>>>
>>>
>>>The names confused me. I'd suggest pnv_php_scan(), pnv_php_unregister(),
>>>pnv_php_unregister_children() instead.
>>>
>>>
>>>Alistair, what do you reckon?
>>>
>>>
>>>>+
>>>>+static void pnv_php_free_slot(struct kref *kref)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot = container_of(kref,
>>>>+                                                struct pnv_php_slot,
>>>>+                                                kref);
>>>>+
>>>>+   WARN_ON(!list_empty(&php_slot->children));
>>>>+   kfree(php_slot->name);
>>>>+   kfree(php_slot);
>>>>+}
>>>>+
>>>>+static inline void pnv_php_put_slot(struct pnv_php_slot *php_slot)
>>>>+{
>>>>+   if (!php_slot)
>>>
>>>
>>>BUG_ON()?
>>>
>>
>>checkpatch.pl will report warning like below. Are you sure you need a 
>>BUG_ON()?
>
>
>No, I am not - this is why I asked. How possible is it to have here
>phb_slot==NULL? Can we recover from that? The options are -
>1) memory is corrupted (then we cannot and it has to be BUG_ON)
>2) broken/old OPAL returns unexpected error (then we can continue, I guess)
>3) there are ways (via sysfs in the userspace? no idea) to get
>pnv_php_put_slot() called with phb_slot.
>
>If only 1) is possible - then BUG_ON, if 2) - WARN_ON, if 3) - should be
>neither BUG_ON nor WARN_ON. You know the code better, you decide.
>

I will have a WARN_ON instead since it's not harmful.

>>
>>WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather 
>>than BUG() or BUG_ON()
>>#159: FILE: drivers/pci/hotplug/pnv_php.c:76:
>>+     BUG_ON(!php_slot);
>>
>>
>>>>+           return;
>>>>+
>>>>+   kref_put(&php_slot->kref, pnv_php_free_slot);
>>>>+}
>>>>+
>>>>+static struct pnv_php_slot *pnv_php_match(struct device_node *dn,
>>>>+                                     struct pnv_php_slot *php_slot)
>>>>+{
>>>>+   struct pnv_php_slot *target, *tmp;
>>>>+
>>>>+   if (php_slot->dn == dn) {
>>>>+           kref_get(&php_slot->kref);
>>>>+           return php_slot;
>>>>+   }
>>>>+
>>>>+   list_for_each_entry(tmp, &php_slot->children, link) {
>>>>+           target = pnv_php_match(dn, tmp);
>>>>+           if (target)
>>>>+                   return target;
>>>>+   }
>>>>+
>>>>+   return NULL;
>>>>+}
>>>>+
>>>>+static struct pnv_php_slot *pnv_php_find_slot(struct device_node *dn)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot, *tmp;
>>>>+   unsigned long flags;
>>>>+
>>>>+   spin_lock_irqsave(&pnv_php_lock, flags);
>>>>+   list_for_each_entry(tmp, &pnv_php_slot_list, link) {
>>>>+           php_slot = pnv_php_match(dn, tmp);
>>>>+           if (php_slot) {
>>>>+                   spin_unlock_irqrestore(&pnv_php_lock, flags);
>>>>+                   return php_slot;
>>>>+           }
>>>>+   }
>>>>+   spin_unlock_irqrestore(&pnv_php_lock, flags);
>>>>+
>>>>+   return NULL;
>>>>+}
>>>>+
>>>>+/*
>>>>+ * Remove pdn for all children of the indicated device node.
>>>>+ * The function should remove pdn in a depth-first manner.
>>>>+ */
>>>>+static void pnv_php_rmv_pdns(struct device_node *dn)
>>>>+{
>>>>+   struct device_node *child;
>>>>+
>>>>+   for_each_child_of_node(dn, child) {
>>>>+           pnv_php_rmv_pdns(child);
>>>>+
>>>>+           pci_remove_device_node_info(child);
>>>>+   }
>>>>+}
>>>>+
>>>>+/*
>>>>+ * Remove all child nodes of the indicated device nodes. The
>>>>+ * function should remove device nodes in depth-first manner.
>>>>+ */
>>>>+static int pnv_php_rmv_device_nodes(struct device_node *parent)
>>>>+{
>>>>+   struct device_node *dn, *child;
>>>>+   int ret = 0;
>>>>+
>>>>+   for_each_child_of_node(parent, dn) {
>>>>+           ret = pnv_php_rmv_device_nodes(dn);
>>>>+           if (ret)
>>>>+                   return ret;
>>>>+
>>>>+           child = of_get_next_child(dn, NULL);
>>>>+           if (child) {
>>>>+                   of_node_put(child);
>>>>+                   of_node_put(dn);
>>>>+                   pr_err("%s: Alive children of node <%s>\n",
>>>>+                          __func__, of_node_full_name(dn));
>>>>+                   return -EBUSY;
>>>>+           }
>>>>+
>>>>+           of_detach_node(dn);
>
>
>While playing with compiler options, I hit this:
>
>  MODPOST 248 modules
>ERROR: "of_detach_node" [drivers/pci/hotplug/pnv-php.ko] undefined!
>/home/aik/p/kernel-power8hp/scripts/Makefile.modpost:91: recipe for target
>'__modpost' failed
>make[2]: *** [__modpost] Error 1
>

It was a known issue and the patch has been piled there. I will include
it in next revision.

>
>I enabled pnv-php to compile as a module:
>CONFIG_HOTPLUG_PCI_POWERNV=m
>
>This is missing:
>
>diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>index c647bd1..75ce30d 100644
>--- a/drivers/of/dynamic.c
>+++ b/drivers/of/dynamic.c
>@@ -311,6 +311,7 @@ int of_detach_node(struct device_node *np)
>
>        return rc;
> }
>+EXPORT_SYMBOL_GPL(of_detach_node);
>
>
>
>
>>>>+           of_node_put(dn);
>>>>+   }
>>>>+
>>>>+   return 0;
>>>>+}
>>>>+
>>>>+/*
>>>>+ * The function processes the message sent by firmware
>>>>+ * to remove all device tree nodes beneath the slot's
>>>>+ * nodes and the associated auxiliary data.
>>>>+ */
>>>>+static void pnv_php_handle_poweroff(struct pnv_php_slot *php_slot)
>>>>+{
>>>>+   int ret;
>>>>+
>>>>+   pnv_php_rmv_pdns(php_slot->dn);
>>>>+
>>>>+   /*
>>>>+    * If the device sub-tree was created from OF changeset, simply
>>>>+    * to revert that. Otherwise, the device nodes in the sub-tree
>>>>+    * need to be iterated and detached.
>>>>+    */
>>>>+   if (php_slot->fdt) {
>>>>+           of_changeset_destroy(&php_slot->ocs);
>>>>+           kfree(php_slot->dt);
>>>>+           kfree(php_slot->fdt);
>>>>+           php_slot->dt        = NULL;
>>>>+           php_slot->dn->child = NULL;
>>>>+           php_slot->fdt       = NULL;
>>>>+           php_slot->power_state_confirmed =
>>>>+                   PNV_PHP_POWER_CONFIRMED_SUCCESS;
>>>>+           wake_up_interruptible(&php_slot->queue);
>>>>+           return;
>>>>+   }
>>>>+
>>>>+   ret = pnv_php_rmv_device_nodes(php_slot->dn);
>>>>+   if (!ret) {
>>>>+           php_slot->power_state_confirmed =
>>>>+                   PNV_PHP_POWER_CONFIRMED_SUCCESS;
>>>>+   } else {
>>>>+           php_slot->power_state_confirmed = PNV_PHP_POWER_CONFIRMED_FAIL;
>>>>+           dev_warn(&php_slot->pdev->dev, "Error %d freeing nodes\n", ret);
>>>>+   }
>>>>+
>>>>+   wake_up_interruptible(&php_slot->queue);
>>>
>>>
>>>I liked one wake_up_interruptible() better...
>>>
>>
>>Will fix in next revision.
>>
>>>>+}
>>>>+
>>>>+static int pnv_php_populate_changeset(struct of_changeset *ocs,
>>>>+                                 struct device_node *dn)
>>>>+{
>>>>+   struct device_node *child;
>>>>+   int ret = 0;
>>>>+
>>>>+   for_each_child_of_node(dn, child) {
>>>>+           ret = of_changeset_attach_node(ocs, child);
>>>>+           if (ret)
>>>>+                   break;
>>>>+
>>>>+           ret = pnv_php_populate_changeset(ocs, child);
>>>
>>>
>>>I asked in v7 - may be to add here "if (ret) break;"?
>>>
>>
>>Will add it in v9.
>>
>>>>+   }
>>>>+
>>>>+   return ret;
>>>>+}
>>>>+
>>>>+static void *pnv_php_add_one_pdn(struct device_node *dn, void *data)
>>>>+{
>>>>+   struct pci_controller *hose = (struct pci_controller *)data;
>>>>+   struct pci_dn *pdn;
>>>>+
>>>>+   pdn = pci_add_device_node_info(hose, dn);
>>>>+   if (!pdn)
>>>>+           return ERR_PTR(-ENOMEM);
>>>>+
>>>>+   return NULL;
>>>>+}
>>>>+
>>>>+static void pnv_php_add_pdns(struct pnv_php_slot *slot)
>>>>+{
>>>>+   struct pci_controller *hose = pci_bus_to_host(slot->bus);
>>>>+
>>>>+   pci_traverse_device_nodes(slot->dn, pnv_php_add_one_pdn, hose);
>>>>+}
>>>>+
>>>>+static void pnv_php_handle_poweron(struct pnv_php_slot *php_slot)
>>>>+{
>>>>+   void *fdt, *fdt1, *dt;
>>>>+   int confirm = PNV_PHP_POWER_CONFIRMED_SUCCESS;
>>>>+   int ret;
>>>>+
>>>>+   /* We don't know the FDT blob size. We try to get it through
>>>>+    * maximal memory chunk and then copy it to another chunk that
>>>>+    * fits the real size.
>>>>+    */
>>>>+   fdt1 = kzalloc(0x10000, GFP_KERNEL);
>>>>+   if (!fdt1)
>>>>+           goto error;
>>>>+
>>>>+   ret = pnv_pci_get_device_tree(php_slot->dn->phandle, fdt1, 0x10000);
>>>>+   if (ret)
>>>>+           goto free_fdt1;
>>>>+
>>>>+   fdt = kzalloc(fdt_totalsize(fdt1), GFP_KERNEL);
>>>>+   if (!fdt)
>>>>+           goto free_fdt1;
>>>>+
>>>>+   /* Unflatten device tree blob */
>>>>+   memcpy(fdt, fdt1, fdt_totalsize(fdt1));
>>>>+   dt = of_fdt_unflatten_tree(fdt, php_slot->dn, NULL);
>>>>+   if (!dt) {
>>>>+           dev_warn(&php_slot->pdev->dev, "Cannot unflatten FDT\n");
>>>>+           goto free_fdt;
>>>>+   }
>>>>+
>>>>+   /* Initialize and apply the changeset */
>>>>+   of_changeset_init(&php_slot->ocs);
>>>>+   ret = pnv_php_populate_changeset(&php_slot->ocs, php_slot->dn);
>>>>+   if (ret) {
>>>>+           dev_warn(&php_slot->pdev->dev, "Error %d populating 
>>>>changeset\n",
>>>>+                    ret);
>>>>+           goto free_dt;
>>>>+   }
>>>>+
>>>>+   php_slot->dn->child = NULL;
>>>>+   ret = of_changeset_apply(&php_slot->ocs);
>>>>+   if (ret) {
>>>>+           dev_warn(&php_slot->pdev->dev, "Error %d applying changeset\n",
>>>>+                    ret);
>>>>+           goto destroy_changeset;
>>>>+   }
>>>>+
>>>>+   /* Add device node firmware data */
>>>>+   pnv_php_add_pdns(php_slot);
>>>>+   php_slot->fdt = fdt;
>>>>+   php_slot->dt  = dt;
>>>>+   goto out;
>>>>+
>>>>+destroy_changeset:
>>>>+   of_changeset_destroy(&php_slot->ocs);
>>>>+free_dt:
>>>>+   kfree(dt);
>>>>+   php_slot->dn->child = NULL;
>>>>+free_fdt:
>>>>+   kfree(fdt);
>>>>+free_fdt1:
>>>>+   kfree(fdt1);
>>>>+error:
>>>>+   confirm = PNV_PHP_POWER_CONFIRMED_FAIL;
>>>>+out:
>>>>+   /* Confirm status change */
>>>>+   php_slot->power_state_confirmed = confirm;
>>>>+   wake_up_interruptible(&php_slot->queue);
>>>>+}
>>>>+
>>>>+static void pnv_php_work(struct work_struct *data)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot = container_of(data,
>>>>+                                                struct pnv_php_slot,
>>>>+                                                work);
>>>>+   uint64_t event = be64_to_cpu(php_slot->msg->params[0]);
>>>>+
>>>>+   if (event == OPAL_PCI_SLOT_POWER_OFF)
>>>>+           pnv_php_handle_poweroff(php_slot);
>>>>+   else
>>>>+           pnv_php_handle_poweron(php_slot);
>>>>+
>>>>+   pnv_php_put_slot(php_slot);
>>>>+}
>>>>+
>>>>+static int pnv_php_handle_msg(struct notifier_block *nb,
>>>>+                         unsigned long type,
>>>>+                         void *message)
>>>>+{
>>>>+   phandle h;
>>>>+   struct device_node *dn;
>>>>+   struct pnv_php_slot *php_slot;
>>>>+   struct opal_msg *msg = message;
>>>>+
>>>>+   if (type != OPAL_MSG_PCI_HOTPLUG) {
>>>>+           pr_warn("%s: Invalid message %ld received!\n",
>>>>+                   __func__, type);
>>>>+           return NOTIFY_DONE;
>>>>+   }
>>>>+
>>>>+   h = (phandle)be64_to_cpu(msg->params[1]);
>>>>+   dn = of_find_node_by_phandle(h);
>>>>+   if (!dn) {
>>>>+           pr_warn("%s: No device node for phandle 0x%x\n",
>>>>+                   __func__, h);
>>>>+           return NOTIFY_DONE;
>>>>+   }
>>>>+
>>>>+   php_slot = pnv_php_find_slot(dn);
>>>>+   if (!php_slot) {
>>>>+           pr_warn("%s: No slot found for node <%s>\n",
>>>>+                   __func__, of_node_full_name(dn));
>>>>+           of_node_put(dn);
>>>>+           return NOTIFY_DONE;
>>>>+   }
>>>>+
>>>>+   of_node_put(dn);
>>>>+   php_slot->msg = msg;
>>>>+   schedule_work(&php_slot->work);
>>>>+   return NOTIFY_OK;
>>>>+}
>>>>+
>>>>+static int pnv_php_set_power_state(struct hotplug_slot *slot, u8 state)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot = slot->private;
>>>>+   int ret;
>>>>+
>>>>+   php_slot->power_state_confirmed = PNV_PHP_POWER_CONFIRMED_INVALID;
>>>>+   ret = pnv_pci_set_power_state(php_slot->id, state);
>>>>+   if (ret) {
>>>>+           dev_warn(&php_slot->pdev->dev, "Error %d powering %s slot\n",
>>>>+                    ret, state ? "on" : "off");
>>>>+           return ret;
>>>>+   }
>>>>+
>>>>+   /* Continue to PCI probing after finalized device-tree. The
>>>>+    * device-tree might have been updated completely at this
>>>>+    * point. Thus we don't have to wait forever.
>>>>+    */
>>>>+   if (php_slot->power_state_confirmed == PNV_PHP_POWER_CONFIRMED_SUCCESS)
>>>>+           return 0;
>>>>+
>>>>+   if (php_slot->power_state_confirmed == PNV_PHP_POWER_CONFIRMED_FAIL)
>>>>+           return -EBUSY;
>>>>+
>>>>+   /* Wait for firmware to add or remove device sub-tree. When it's done,
>>>>+    * one signal is received from firmware.
>>>>+    */
>>>>+   ret = wait_event_timeout(php_slot->queue,
>>>>+                            php_slot->power_state_confirmed, 10 * HZ);
>>>>+   if (!ret) {
>>>>+           dev_warn(&php_slot->pdev->dev, "Error %d waiting for 
>>>>power-%s\n",
>>>>+                    ret, state ? "on" : "off");
>>>>+           return -EBUSY;
>>>>+   }
>>>>+
>>>>+   if (php_slot->power_state_confirmed == PNV_PHP_POWER_CONFIRMED_SUCCESS)
>>>>+           return 0;
>>>>+
>>>>+   dev_warn(&php_slot->pdev->dev, "Error status %d for power-%s\n",
>>>>+            php_slot->power_state_confirmed, state ? "on" : "off");
>>>>+   return -EBUSY;
>>>>+}
>>>>+
>>>>+static int pnv_php_get_power_state(struct hotplug_slot *slot, u8 *state)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot = slot->private;
>>>>+   uint8_t power_state;
>>>
>>>
>>>Uninitialized variable.
>>>
>>
>
>>When pnv_pci_get_power_state() fails to get the power state, it fails back to
>>default one (OPAL_PCI_SLOT_POWER_ON). Otherwise, it is set to the state 
>>returned
>>from pnv_pci_get_power_state(). The logic is complete.
>
>What does guarantee that if the corresponding OPAL call returned success,
>then all data pointers to which you passed to OPAL will point to correct
>values? For exampple, the new pnv_pci_poll() updates the state only in some
>cases.
>

Note that opal_pci_poll() is going to be replaced by opal_pci_poll2() as you
suggested in another thread. The later function accepts two arguments.

When the second argument to opal_pci_poll() isn't null, the result is returned.
Otherwise, the result won't be returned.

>
>> Also, I don't see building warning/error caused by this.
>
>You do not see them now with your current compiler which does not mean you
>will never see them.
>

hrm, I really don't catch the point. Obviously, I'm not able to know the
unpredictable thing. I will initialize @power_state to default (ON) in
next revision since you're insisting on it :)

>
>>>
>>>>+   int ret;
>>>>+
>>>>+   /*
>>>>+    * Retrieve power status from firmware. If we fail
>>>>+    * getting that, the power status fails back to
>>>>+    * be on.
>>>>+    */
>>>>+   ret = pnv_pci_get_power_state(php_slot->id, &power_state);
>>>>+   if (ret) {
>>>>+           *state = OPAL_PCI_SLOT_POWER_ON;
>>>>+           dev_warn(&php_slot->pdev->dev, "Error %d getting power 
>>>>status\n",
>>>>+                    ret);
>>>>+   } else {
>>>>+           *state = power_state;
>>>>+           slot->info->power_status = power_state;
>>>>+   }
>>>>+
>>>>+   return 0;
>>>>+}
>>>>+
>>>>+static int pnv_php_get_adapter_state(struct hotplug_slot *slot, u8 *state)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot = slot->private;
>>>>+   uint8_t presence;
>>>
>>>Uninitialized variable.
>>>
>>
>>Same as above.
>>
>>>>+   int ret;
>>>>+
>>>>+   /*
>>>>+    * Retrieve presence status from firmware. If we can't
>>>>+    * get that, it will fail back to be empty.
>>>>+    */
>>>>+   ret = pnv_pci_get_presence_state(php_slot->id, &presence);
>>>>+   if (ret >= 0) {
>>>>+           *state = presence;
>>>>+           slot->info->adapter_status = presence;
>>>>+           ret = 0;
>>>>+   } else {
>>>>+           *state = OPAL_PCI_SLOT_EMPTY;
>>>>+           dev_warn(&php_slot->pdev->dev, "Error %d getting presence\n",
>>>>+                    ret);
>>>>+   }
>>>>+
>>>>+   return ret;
>>>>+}
>>>>+
>>>>+static int pnv_php_set_attention_state(struct hotplug_slot *slot, u8 state)
>>>>+{
>>>>+   /* FIXME: Make it real once firmware supports it */
>>>
>>>It still does not?
>>>
>>>
>>>>+   slot->info->attention_status = state;
>>>>+
>>>>+   return 0;
>>>>+}
>>>>+
>>>>+static int pnv_php_enable(struct pnv_php_slot *php_slot, bool rescan)
>>>>+{
>>>>+   struct hotplug_slot *slot = &php_slot->slot;
>>>>+   uint8_t presence, power_status;
>>>
>>>
>>>Uninitialized variables.
>>>
>>>
>>
>>I will initialize them to default states in next revision.
>
>Thanks :)
>
>
>>
>>>>+   int ret;
>>>>+
>>>>+   /* Check if the slot has been configured */
>>>>+   if (php_slot->state != PNV_PHP_STATE_REGISTERED)
>>>>+           return 0;
>>>>+
>>>>+   /* Retrieve slot presence status */
>>>>+   ret = pnv_php_get_adapter_state(slot, &presence);
>>>>+   if (ret)
>>>>+           return ret;
>>>>+
>>>>+   /* Proceed if there have nothing behind the slot */
>>>>+   if (presence == OPAL_PCI_SLOT_EMPTY)
>>>>+           goto scan;
>>>>+
>>>>+   /*
>>>>+    * If the power suply to the slot is off, we can't detect
>>>
>>>s/suply/supply/
>>>
>>
>>Will fix in next revision.
>>
>>>>+    * adapter presence state. That means we have to turn the
>>>>+    * slot on before going to probe slot's presence state.
>>>>+    *
>>>>+    * On the first time, we don't change the power status to
>>>>+    * boost system boot with assumption that the firmware
>>>>+    * supplies consistent slot power status: empty slot always
>>>>+    * has its power off and non-empty slot has its power on.
>>>>+    */
>>>>+   if (!php_slot->power_state_check) {
>>>>+           php_slot->power_state_check = true;
>>>>+
>>>>+           ret = pnv_php_get_power_state(slot, &power_status);
>>>>+           if (ret)
>>>>+                   return ret;
>>>>+
>>>>+           if (power_status != OPAL_PCI_SLOT_POWER_ON)
>>>>+                   return 0;
>>>>+   }
>>>>+
>>>>+   /* Check the power status. Scan the slot if that's already on */
>>>
>>>
>>>s/that's/it is/
>>>
>>
>>I don't know the difference. Will fix it in next revision anyway.
>>
>>>
>>>>+   ret = pnv_php_get_power_state(slot, &power_status);
>>>>+   if (ret)
>>>>+           return ret;
>>>>+
>>>>+   if (power_status == OPAL_PCI_SLOT_POWER_ON)
>>>>+           goto scan;
>>>>+
>>>>+   /* Power is off, turn it on and then scan the slot */
>>>>+   ret = pnv_php_set_power_state(slot, OPAL_PCI_SLOT_POWER_ON);
>>>>+   if (ret)
>>>>+           return ret;
>>>>+
>>>>+scan:
>>>>+   if (presence == OPAL_PCI_SLOT_PRESENT) {
>>>>+           if (rescan) {
>>>>+                   pci_lock_rescan_remove();
>>>>+                   pci_add_pci_devices(php_slot->bus);
>>>>+                   pci_unlock_rescan_remove();
>>>>+           }
>>>>+
>>>>+           /* Rescan for child hotpluggable slots */
>>>>+           php_slot->state = PNV_PHP_STATE_POPULATED;
>>>>+           if (rescan)
>>>>+                   pnv_php_register(php_slot->dn);
>>>>+   } else {
>>>>+           php_slot->state = PNV_PHP_STATE_POPULATED;
>>>>+   }
>>>>+
>>>>+   return 0;
>>>>+}
>>>>+
>>>>+static int pnv_php_enable_slot(struct hotplug_slot *slot)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot = container_of(slot,
>>>>+                                                struct pnv_php_slot, slot);
>>>>+
>>>>+   return pnv_php_enable(php_slot, true);
>>>>+}
>>>>+
>>>>+static int pnv_php_disable_slot(struct hotplug_slot *slot)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot = slot->private;
>>>>+   uint8_t power_state;
>>>>+   int ret;
>>>>+
>>>>+   if (php_slot->state != PNV_PHP_STATE_POPULATED)
>>>>+           return 0;
>>>>+
>>>>+   /* Remove all devices behind the slot */
>>>>+   pci_lock_rescan_remove();
>>>>+   pci_remove_pci_devices(php_slot->bus);
>>>>+   pci_unlock_rescan_remove();
>>>>+
>>>>+   /* Detach the child hotpluggable slots */
>>>>+   pnv_php_unregister(php_slot->dn);
>>>>+
>>>>+   /*
>>>>+    * Check the power status and turn it off if necessary. If we
>>>>+    * fail to get the power status, the power will be forced to
>>>>+    * be off.
>>>>+    */
>>>>+   ret = pnv_php_get_power_state(slot, &power_state);
>>>>+   if (ret || power_state == OPAL_PCI_SLOT_POWER_ON) {
>>>>+           ret = pnv_php_set_power_state(slot, OPAL_PCI_SLOT_POWER_OFF);
>>>>+           if (ret)
>>>>+                   dev_warn(&php_slot->pdev->dev, "Error %d powering 
>>>>off\n",
>>>
>>>
>>>Long line, checkpatch.pl should have warned :)
>>>
>>
>>I didn't see the warning from checkpatch.pl.
>
>
>Cool, then never mind.
>
>
>>
>>>>+                            ret);
>>>>+   }
>>>>+
>>>>+   /* Update slot state */
>>>>+   php_slot->state = PNV_PHP_STATE_REGISTERED;
>>>>+   return 0;
>>>>+}
>>>>+
>>>>+static struct hotplug_slot_ops php_slot_ops = {
>>>>+   .get_power_status       = pnv_php_get_power_state,
>>>>+   .get_adapter_status     = pnv_php_get_adapter_state,
>>>>+   .set_attention_status   = pnv_php_set_attention_state,
>>>>+   .enable_slot            = pnv_php_enable_slot,
>>>>+   .disable_slot           = pnv_php_disable_slot,
>>>>+};
>>>>+
>>>>+static void pnv_php_release(struct hotplug_slot *slot)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot = slot->private;
>>>>+   unsigned long flags;
>>>>+
>>>>+   /* Remove from global or child list */
>>>>+   spin_lock_irqsave(&pnv_php_lock, flags);
>>>>+   list_del(&php_slot->link);
>>>>+   spin_unlock_irqrestore(&pnv_php_lock, flags);
>>>>+
>>>>+   /* Detach from parent */
>>>>+   pnv_php_put_slot(php_slot);
>>>>+   pnv_php_put_slot(php_slot->parent);
>>>>+}
>>>>+
>>>>+static int pnv_php_get_slot_id(struct device_node *dn, uint64_t *id)
>>>>+{
>>>>+   struct device_node *parent = dn;
>>>>+   const __be64 *prop64;
>>>>+   const __be32 *prop32;
>>>>+
>>>>+   /*
>>>>+    * The hotpluggable slot always has a compound Id, which
>>>>+    * consists of 16-bits PHB Id, 16 bits bus/slot/function
>>>>+    * number, and compound indicator
>>>>+    */
>>>>+   *id = (0x1ul << 63);
>>>
>>>
>>>Is this bit from the same space as 1<<60 as in pnv_eeh_bridge_reset()? If so,
>>>it would be great to have all these id bits defined in one place.
>>>
>>
>>Will have a macro (PCI_SLOT_ID) to produce the PCI slot ID in next revision.
>>
>>>
>>>>+
>>>>+   /* Bus/Slot/Function number */
>>>>+   prop32 = of_get_property(dn, "reg", NULL);
>>>>+   if (!prop32)
>>>>+           return -ENXIO;
>>>>+   *id |= ((of_read_number(prop32, 1) & 0x00ffff00) << 8);
>>>>+
>>>>+   /* PHB Id */
>>>>+   while ((parent = of_get_parent(parent))) {
>>>>+           if (!PCI_DN(parent)) {
>>>>+                   of_node_put(parent);
>>>>+                   break;
>>>>+           }
>>>>+
>>>>+           if (!of_device_is_compatible(parent, "ibm,ioda2-phb") &&
>>>>+               !of_device_is_compatible(parent, "ibm,ioda-phb")) {
>>>>+                   of_node_put(parent);
>>>>+                   continue;
>>>>+           }
>>>>+
>>>>+           prop64 = of_get_property(parent, "ibm,opal-phbid", NULL);
>>>>+           if (!prop64) {
>>>>+                   of_node_put(parent);
>>>>+                   return -ENXIO;
>>>>+           }
>>>>+
>>>>+           *id |= be64_to_cpup(prop64);
>>>>+           of_node_put(parent);
>>>>+           return 0;
>>>>+   }
>>>>+
>>>>+   return -ENODEV;
>>>>+}
>>>>+
>>>>+static struct pnv_php_slot *pnv_php_alloc_slot(struct device_node *dn)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot;
>>>>+   struct pci_bus *bus;
>>>>+   const char *label;
>>>>+   uint64_t id;
>>>>+
>>>>+   label = of_get_property(dn, "ibm,slot-label", NULL);
>>>>+   if (!label)
>>>>+           return NULL;
>>>>+
>>>>+   if (pnv_php_get_slot_id(dn, &id))
>>>>+           return NULL;
>>>>+
>>>>+   bus = pci_find_bus_by_node(dn);
>>>>+   if (!bus)
>>>>+           return NULL;
>>>>+
>>>>+   php_slot = kzalloc(sizeof(*php_slot), GFP_KERNEL);
>>>>+   if (!php_slot)
>>>>+           return NULL;
>>>>+
>>>>+   php_slot->name = kstrdup(label, GFP_KERNEL);
>>>>+   if (!php_slot->name) {
>>>>+           kfree(php_slot);
>>>>+           return NULL;
>>>>+   }
>>>>+
>>>>+   if (dn->child && PCI_DN(dn->child))
>>>>+           php_slot->slot_no = PCI_SLOT(PCI_DN(dn->child)->devfn);
>>>>+   else
>>>>+           php_slot->slot_no = -1;   /* Placeholder slot */
>>>>+
>>>>+   kref_init(&php_slot->kref);
>>>>+   php_slot->state                 = PNV_PHP_STATE_INITIALIZED;
>>>>+   php_slot->dn                    = dn;
>>>>+   php_slot->pdev                  = bus->self;
>>>>+   php_slot->bus                   = bus;
>>>>+   php_slot->id                    = id;
>>>>+   php_slot->power_state_check     = false;
>>>>+   php_slot->power_state_confirmed = PNV_PHP_POWER_CONFIRMED_INVALID;
>>>>+   php_slot->slot.ops              = &php_slot_ops;
>>>>+   php_slot->slot.info             = &php_slot->slot_info;
>>>>+   php_slot->slot.release          = pnv_php_release;
>>>>+   php_slot->slot.private          = php_slot;
>>>>+
>>>>+   INIT_WORK(&php_slot->work, pnv_php_work);
>>>>+   init_waitqueue_head(&php_slot->queue);
>>>>+   INIT_LIST_HEAD(&php_slot->children);
>>>>+   INIT_LIST_HEAD(&php_slot->link);
>>>>+
>>>>+   return php_slot;
>>>>+}
>>>>+
>>>>+static int pnv_php_register_slot(struct pnv_php_slot *php_slot)
>>>>+{
>>>>+   struct pnv_php_slot *parent;
>>>>+   struct device_node *dn = php_slot->dn;
>>>>+   unsigned long flags;
>>>>+   int ret;
>>>>+
>>>>+   /* Check if the slot is registered or not */
>>>>+   parent = pnv_php_find_slot(php_slot->dn);
>>>>+   if (parent) {
>>>>+           pnv_php_put_slot(parent);
>>>>+           return -EEXIST;
>>>>+   }
>>>>+
>>>>+   /* Register PCI slot */
>>>>+   ret = pci_hp_register(&php_slot->slot, php_slot->bus,
>>>>+                         php_slot->slot_no, php_slot->name);
>>>>+   if (ret) {
>>>>+           dev_warn(&php_slot->pdev->dev, "Error %d registering slot\n",
>>>>+                    ret);
>>>>+           return ret;
>>>>+   }
>>>>+
>>>>+   /* Attach to the parent's child list or global list */
>>>>+   while ((dn = of_get_parent(dn))) {
>>>>+           if (!PCI_DN(dn)) {
>>>>+                   of_node_put(dn);
>>>>+                   break;
>>>>+           }
>>>>+
>>>>+           parent = pnv_php_find_slot(dn);
>>>>+           if (parent) {
>>>>+                   of_node_put(dn);
>>>>+                   break;
>>>>+           }
>>>>+
>>>>+           of_node_put(dn);
>>>>+   }
>>>>+
>>>>+   spin_lock_irqsave(&pnv_php_lock, flags);
>>>>+   php_slot->parent = parent;
>>>>+   if (parent)
>>>>+           list_add_tail(&php_slot->link, &parent->children);
>>>>+   else
>>>>+           list_add_tail(&php_slot->link, &pnv_php_slot_list);
>>>>+   spin_unlock_irqrestore(&pnv_php_lock, flags);
>>>>+
>>>>+   php_slot->state = PNV_PHP_STATE_REGISTERED;
>>>>+   return 0;
>>>>+}
>>>>+
>>>>+static int pnv_php_register_one(struct device_node *dn)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot;
>>>>+   const __be32 *prop32;
>>>>+   int ret;
>>>>+
>>>>+   /* Check if it's hotpluggable slot */
>>>>+   prop32 = of_get_property(dn, "ibm,slot-pluggable", NULL);
>>>>+   if (!prop32 || !of_read_number(prop32, 1))
>>>>+           return -ENXIO;
>>>>+
>>>>+   prop32 = of_get_property(dn, "ibm,reset-by-firmware", NULL);
>>>>+   if (!prop32 || !of_read_number(prop32, 1))
>>>>+           return -ENXIO;
>>>>+
>>>>+   php_slot = pnv_php_alloc_slot(dn);
>>>>+   if (!php_slot)
>>>>+           return -ENODEV;
>>>>+
>>>>+   ret = pnv_php_register_slot(php_slot);
>>>>+   if (ret)
>>>>+           goto free_slot;
>>>>+
>>>>+   ret = pnv_php_enable(php_slot, false);
>>>>+   if (ret)
>>>>+           goto unregister_slot;
>>>>+
>>>>+   return 0;
>>>>+
>>>>+unregister_slot:
>>>>+   pnv_php_unregister_one(php_slot->dn);
>>>>+free_slot:
>>>>+   pnv_php_put_slot(php_slot);
>>>>+   return ret;
>>>>+}
>>>>+
>>>>+static void pnv_php_register(struct device_node *dn)
>>>>+{
>>>>+   struct device_node *child;
>>>>+
>>>>+   /*
>>>>+    * The parent slots should be registered before their
>>>>+    * child slots.
>>>>+    */
>>>>+   for_each_child_of_node(dn, child) {
>>>>+           pnv_php_register_one(child);
>>>>+           pnv_php_register(child);
>>>>+   }
>>>>+}
>>>>+
>>>>+static void pnv_php_unregister_one(struct device_node *dn)
>>>>+{
>>>>+   struct pnv_php_slot *php_slot;
>>>>+
>>>>+   php_slot = pnv_php_find_slot(dn);
>>>>+   if (!php_slot)
>>>>+           return;
>>>>+
>>>>+   pnv_php_put_slot(php_slot);
>>>>+   pci_hp_deregister(&php_slot->slot);
>>>>+}
>>>>+
>>>>+static void pnv_php_unregister(struct device_node *dn)
>>>>+{
>>>>+   struct device_node *child;
>>>>+
>>>>+   /* The child slots should go before their parent slots */
>>>>+   for_each_child_of_node(dn, child) {
>>>>+           pnv_php_unregister(child);
>>>>+           pnv_php_unregister_one(child);
>>>>+   }
>>>>+}
>>>>+
>>>>+static struct notifier_block php_msg_nb = {
>>>>+   .notifier_call  = pnv_php_handle_msg,
>>>>+   .next           = NULL,
>>>>+   .priority       = 0,
>>>>+};
>>>>+
>>>>+static int __init pnv_php_init(void)
>>>>+{
>>>>+   struct device_node *dn;
>>>>+   int ret;
>>>>+
>>>>+   pr_info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
>>>>+
>>>>+   /* Register hotplug message handler */
>>>>+   ret = pnv_pci_hotplug_notifier_register(&php_msg_nb);
>>>>+   if (ret) {
>>>>+           pr_warn("%s: Error %d registering hotplug notifier\n",
>>>>+                   __func__, ret);
>>>>+           return ret;
>>>>+   }
>>>>+
>>>>+   /* Scan PHB nodes and their children */
>>>>+   for_each_compatible_node(dn, NULL, "ibm,ioda-phb")
>>>>+           pnv_php_register(dn);
>>>>+   for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
>>>>+           pnv_php_register(dn);
>>>>+
>>>>+   return 0;
>>>>+}
>>>>+
>>>>+static void __exit pnv_php_exit(void)
>>>>+{
>>>>+   struct device_node *dn;
>>>>+
>>>>+   for_each_compatible_node(dn, NULL, "ibm,ioda-phb")
>>>>+           pnv_php_unregister(dn);
>>>>+   for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
>>>>+           pnv_php_unregister(dn);
>>>>+
>>>>+   pnv_pci_hotplug_notifier_unregister(&php_msg_nb);
>>>>+}
>>>>+
>>>>+module_init(pnv_php_init);
>>>>+module_exit(pnv_php_exit);
>>>>+
>>>>+MODULE_VERSION(DRIVER_VERSION);
>>>>+MODULE_LICENSE("GPL v2");
>>>>+MODULE_AUTHOR(DRIVER_AUTHOR);
>>>>+MODULE_DESCRIPTION(DRIVER_DESC);
>>>>

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to