[v6,42/42] pci/hotplug: PowerPC PowerNV PCI hotplug driver

Commit Message

Gavin Shan Aug. 6, 2015, 4:11 a.m. UTC

The patch intends to add standalone driver to support PCI hotplug
for PowerPC PowerNV platform, which runs on top of skiboot firmware.
The firmware identified hotpluggable slots and marked their device
tree node with proper "ibm,slot-pluggable" and "ibm,reset-by-firmware".
The driver simply scans device-tree to create/register PCI hotplug slot
accordingly.

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 <[e-mail]>
Acked-by: Bjorn Helgaas <[e-mail]>
---
 MAINTAINERS                            |   6 +
 drivers/pci/hotplug/Kconfig            |  12 +
 drivers/pci/hotplug/Makefile           |   4 +
 drivers/pci/hotplug/powernv_php.c      | 140 +++++++
 drivers/pci/hotplug/powernv_php.h      |  92 +++++
 drivers/pci/hotplug/powernv_php_slot.c | 722 +++++++++++++++++++++++++++++++++
 6 files changed, 976 insertions(+)
 create mode 100644 drivers/pci/hotplug/powernv_php.c
 create mode 100644 drivers/pci/hotplug/powernv_php.h
 create mode 100644 drivers/pci/hotplug/powernv_php_slot.c

Comments

Alexey Kardashevskiy Aug. 15, 2015, 3:13 a.m. UTC | #1

On 08/06/2015 02:11 PM, Gavin Shan wrote:
> The patch intends to add standalone driver to support PCI hotplug
> for PowerPC PowerNV platform, which runs on top of skiboot firmware.
> The firmware identified hotpluggable slots and marked their device
> tree node with proper "ibm,slot-pluggable" and "ibm,reset-by-firmware".
> The driver simply scans device-tree to create/register PCI hotplug slot
> accordingly.
>
> 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 <[e-mail]>
> Acked-by: Bjorn Helgaas <[e-mail]>
> ---
>   MAINTAINERS                            |   6 +
>   drivers/pci/hotplug/Kconfig            |  12 +
>   drivers/pci/hotplug/Makefile           |   4 +
>   drivers/pci/hotplug/powernv_php.c      | 140 +++++++
>   drivers/pci/hotplug/powernv_php.h      |  92 +++++
>   drivers/pci/hotplug/powernv_php_slot.c | 722 +++++++++++++++++++++++++++++++++
>   6 files changed, 976 insertions(+)
>   create mode 100644 drivers/pci/hotplug/powernv_php.c
>   create mode 100644 drivers/pci/hotplug/powernv_php.h
>   create mode 100644 drivers/pci/hotplug/powernv_php_slot.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fd60784..3b75c92 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7747,6 +7747,12 @@ L:	[e-mail]
>   S:	Supported
>   F:	Documentation/PCI/pci-error-recovery.txt
>
> +PCI HOTPLUG DRIVER FOR POWERNV PLATFORM
> +M:	Gavin Shan <[e-mail]>
> +L:	[e-mail]
> +S:	Supported
> +F:	drivers/pci/hotplug/powernv_php*
> +
>   PCI SUBSYSTEM
>   M:	Bjorn Helgaas <[e-mail]>
>   L:	[e-mail]
> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
> index df8caec..ef55dae 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 powernv-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..fd51d65 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)	+= powernv-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,9 @@ ibmphp-objs		:=	ibmphp_core.o	\
>   acpiphp-objs		:=	acpiphp_core.o	\
>   				acpiphp_glue.o
>
> +powernv-php-objs	:=	powernv_php.o	\
> +				powernv_php_slot.o
> +
>   rpaphp-objs		:=	rpaphp_core.o	\
>   				rpaphp_pci.o	\
>   				rpaphp_slot.o
> diff --git a/drivers/pci/hotplug/powernv_php.c b/drivers/pci/hotplug/powernv_php.c
> new file mode 100644
> index 0000000..4cbff7a
> --- /dev/null
> +++ b/drivers/pci/hotplug/powernv_php.c
> @@ -0,0 +1,140 @@
> +/*
> + * 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/module.h>
> +
> +#include <asm/opal.h>
> +#include <asm/pnv-pci.h>
> +
> +#include "powernv_php.h"
> +
> +#define DRIVER_VERSION	"0.1"
> +#define DRIVER_AUTHOR	"Gavin Shan, IBM Corporation"
> +#define DRIVER_DESC	"PowerPC PowerNV PCI Hotplug Driver"


Align all or none.


> +
> +static struct notifier_block php_msg_nb = {
> +	.notifier_call	= powernv_php_msg_handler,
> +	.next		= NULL,
> +	.priority	= 0,
> +};
> +
> +static int powernv_php_register_one(struct device_node *dn)
> +{
> +	struct powernv_php_slot *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;
> +
> +	/* Allocate slot */
> +	slot = powernv_php_slot_alloc(dn);
> +	if (!slot)
> +		return -ENODEV;
> +
> +	/* Register it */
> +	ret = powernv_php_slot_register(slot);
> +	if (ret) {
> +		powernv_php_slot_put(slot);
> +		return ret;
> +	}
> +
> +	return powernv_php_slot_enable(slot->php_slot, false);


And if it fails, no unregister and cleanup is needed?


> +}
> +
> +int powernv_php_register(struct device_node *dn)
> +{
> +	struct device_node *child;
> +	int ret = 0;

@ret is not used below.

> +
> +	/*
> +	 * The parent slots should be registered before their
> +	 * child slots.
> +	 */
> +	for_each_child_of_node(dn, child) {
> +		powernv_php_register_one(child);
> +		powernv_php_register(child);
> +	}
> +
> +	return ret;
> +}
> +
> +static void powernv_php_unregister_one(struct device_node *dn)
> +{
> +	struct powernv_php_slot *slot;
> +
> +	slot = powernv_php_slot_find(dn);
> +	if (!slot)
> +		return;
> +
> +	pci_hp_deregister(slot->php_slot);
> +}
> +
> +void powernv_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) {
> +		powernv_php_unregister(child);
> +		powernv_php_unregister_one(child);
> +	}
> +}
> +
> +static int __init powernv_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")
> +		powernv_php_register(dn);
> +	for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
> +		powernv_php_register(dn);


May be move pnv_pci_hotplug_notifier_register() after 
powernv_php_register()? If not, then below (in powernv_php_exit()) move 
pnv_pci_hotplug_notifier_unregister() to the end?


> +
> +	return 0;
> +}
> +
> +static void __exit powernv_php_exit(void)
> +{
> +	struct device_node *dn;
> +
> +	pnv_pci_hotplug_notifier_unregister(&php_msg_nb);
> +
> +	for_each_compatible_node(dn, NULL, "ibm,ioda-phb")
> +		powernv_php_unregister(dn);
> +	for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
> +		powernv_php_unregister(dn);
> +}
> +
> +module_init(powernv_php_init);
> +module_exit(powernv_php_exit);
> +
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> diff --git a/drivers/pci/hotplug/powernv_php.h b/drivers/pci/hotplug/powernv_php.h
> new file mode 100644
> index 0000000..8034cc6
> --- /dev/null
> +++ b/drivers/pci/hotplug/powernv_php.h
> @@ -0,0 +1,92 @@
> +/*
> + * 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.
> + */
> +
> +#ifndef _POWERNV_PHP_H
> +#define _POWERNV_PHP_H
> +
> +#include <linux/list.h>
> +#include <linux/kref.h>
> +#include <linux/of.h>
> +#include <linux/pci.h>
> +#include <linux/pci_hotplug.h>
> +#include <linux/wait.h>
> +#include <linux/workqueue.h>
> +
> +#include <asm/opal-api.h>
> +
> +/* Slot power status */
> +#define POWERNV_PHP_SLOT_POWER_OFF	0
> +#define POWERNV_PHP_SLOT_POWER_ON	1
> +
> +/* Slot presence status */
> +#define POWERNV_PHP_SLOT_EMPTY		0
> +#define POWERNV_PHP_SLOT_PRESENT	1

These two are also only used in drivers/pci/hotplug/powernv_php_slot.c,
move them there at least. It also seems your PHP driver is the only one 
which uses flags for an adapter status, others return plain 0 or 1 (which 
are c-style "false" and "true", pretty much, so it is not the case of magic 
constants). Since you return these values from the hotplug_slot_ops 
callbacks to external code, you should probably do the same.

And exactly the same comment about POWERNV_PHP_SLOT_POWER_ON/OFF few lines 
above.



> +
> +/* Slot attention status */
> +#define POWERNV_PHP_SLOT_ATTEN_OFF	0
> +#define POWERNV_PHP_SLOT_ATTEN_ON	1
> +#define POWERNV_PHP_SLOT_ATTEN_IND	2
> +#define POWERNV_PHP_SLOT_ATTEN_ACT	3


These should go to drivers/pci/hotplug/powernv_php_slot.c. Where are these 
flags defined? Looks to me like there is a way to pass some status from the 
userspace via sysfs to OPAL so only OPAL is supposed to recognize and 
handle these. If so, these macros are missing "OPAL" in their names. I have 
one more comment below about it.


> +
> +struct powernv_php_slot {
> +	char			*name;
> +	struct device_node	*dn;
> +	struct pci_dev		*pdev;
> +	struct pci_bus		*bus;
> +	uint64_t		id;
> +	int			slot_no;
> +	struct kref		kref;
> +#define POWERNV_PHP_SLOT_STATE_INIT		0
> +#define POWERNV_PHP_SLOT_STATE_REGISTER		1
> +#define POWERNV_PHP_SLOT_STATE_POPULATED	2
> +	int			state;
> +	int			check_power_status;
> +	int			status_confirmed;

s/status_confirmed/power_status_confirmed/

What is this status? It can be 0, 1, 2 which seems to be 
UNCONFIRMED/INPROGRESS/CONFIRMED (does not need PNV/IODA prefixes as it is 
local to the powernv_php_slot.c file).


> +	struct opal_msg		*msg;
> +	void			*fdt;
> +	void			*dt;
> +	struct of_changeset	ocs;
> +	struct work_struct	work;
> +	wait_queue_head_t	queue;
> +	struct hotplug_slot	*php_slot;
> +	struct powernv_php_slot	*parent;
> +	struct list_head	children;
> +	struct list_head	link;
> +};

This should go to drivers/pci/hotplug/powernv_php_slot.c and this header 
should only have a forward declaration. After you move it there, you get 
better separation of the driver code from the slot code and only 2 changes 
will be needed:

1. powernv_php_slot_enable() should receive powernv_php_slot
2. add powernv_php_slot_unregister() (like powernv_php_slot_register()), 
this way you will have pairing pci_hp_register/pci_hp_deregister in the 
same file.


After you moved this struct to the source file, you could remove/shorten 
POWERNV_PHP_SLOT_STATE_ prefixes if you wished.

> +
> +int powernv_php_msg_handler(struct notifier_block *nb,
> +			    unsigned long type, void *message);
> +struct powernv_php_slot *powernv_php_slot_find(struct device_node *dn);
> +void powernv_php_slot_free(struct kref *kref);
> +struct powernv_php_slot *powernv_php_slot_alloc(struct device_node *dn);
> +int powernv_php_slot_register(struct powernv_php_slot *slot);
> +int powernv_php_slot_enable(struct hotplug_slot *php_slot, bool rescan);
> +int powernv_php_register(struct device_node *dn);
> +void powernv_php_unregister(struct device_node *dn);
> +
> +#define to_powernv_php_slot(kref) \
> +	container_of(kref, struct powernv_php_slot, kref)
> +
> +static inline void powernv_php_slot_get(struct powernv_php_slot *slot)
> +{
> +	if (slot)
> +		kref_get(&slot->kref);
> +}
> +
> +static inline int powernv_php_slot_put(struct powernv_php_slot *slot)
> +{
> +	if (slot)
> +		return kref_put(&slot->kref, powernv_php_slot_free);
> +
> +	return 0;
> +}

In these 2 helpers you do not have to check for @slof - it is checked in 
the callers pretty much always. Or it is not checked in php_slot_release() 
but dereferenced before you call powernv_php_slot_put(slot).

The only place you really want this check is 
powernv_php_slot_put(slot->parent) so just check it there. btw is it even 
possible for the slot not to have a parent?

So I'd ditch these helpers.


> +
> +#endif /* !_POWERNV_PHP_H */
> diff --git a/drivers/pci/hotplug/powernv_php_slot.c b/drivers/pci/hotplug/powernv_php_slot.c
> new file mode 100644
> index 0000000..73a93a2
> --- /dev/null
> +++ b/drivers/pci/hotplug/powernv_php_slot.c
> @@ -0,0 +1,722 @@
> +/*
> + * 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/module.h>
> +
> +#include <asm/opal.h>
> +#include <asm/pnv-pci.h>
> +#include <asm/ppc-pci.h>
> +
> +#include "powernv_php.h"
> +
> +static LIST_HEAD(php_slot_list);
> +static DEFINE_SPINLOCK(php_slot_lock);
> +
> +/*
> + * Remove firmware data for all child device nodes of the
> + * indicated one.
> + */
> +static void remove_child_pdn(struct device_node *np)
> +{
> +	struct device_node *child;
> +
> +	for_each_child_of_node(np, child) {
> +		/* In depth first */
> +		remove_child_pdn(child);
> +
> +		remove_pci_device_node_info(child);
> +	}
> +}
> +
> +/*
> + * Remove all subordinate device nodes of the indicated one.
> + * Those device nodes in deepest path should be released firstly.
> + */
> +static int remove_child_device_nodes(struct device_node *parent)
> +{
> +	struct device_node *np, *child;
> +	int ret = 0;
> +
> +	/* If the device node has children, remove them firstly */
> +	for_each_child_of_node(parent, np) {
> +		ret = remove_child_device_nodes(np);
> +		if (ret)
> +			return ret;
> +
> +		/* The device shouldn't have alive children */
> +		child = of_get_next_child(np, NULL);
> +		if (child) {
> +			of_node_put(child);
> +			of_node_put(np);
> +			pr_err("%s: Alive children of node <%s>\n",
> +			       __func__, of_node_full_name(np));
> +			return -EBUSY;
> +		}
> +
> +		/* Detach the device node */
> +		of_detach_node(np);
> +		of_node_put(np);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * The function processes the message sent by firmware
> + * to remove all device tree nodes beneath the slot's
> + * nodes, and the associated auxillary data.
> + */
> +static void slot_power_off_handler(struct powernv_php_slot *slot)
> +{
> +	int ret, status = 1;
> +
> +	/* Release the firmware data for the child device nodes */
> +	remove_child_pdn(slot->dn);
> +
> +	/*
> +	 * Release the child device nodes. If the sub-tree was
> +	 * built with the help of changeset, we just need destroy
> +	 * the changes.
> +	 */
> +	if (slot->fdt) {
> +		of_changeset_destroy(&slot->ocs);
> +		kfree(slot->dt);
> +		slot->dt = NULL;
> +		slot->dn->child = NULL;
> +		kfree(slot->fdt);
> +		slot->fdt = NULL;
> +	} else {
> +		ret = remove_child_device_nodes(slot->dn);
> +		if (ret) {
> +			status = 2;
> +			dev_warn(&slot->pdev->dev, "Error %d freeing nodes\n",
> +				 ret);
> +		}
> +	}
> +
> +	/* Confirm status change */
> +	slot->status_confirmed = status;
> +	wake_up_interruptible(&slot->queue);
> +}
> +
> +static int slot_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)
> +			return ret;
> +
> +		ret = slot_populate_changeset(ocs, child);
> +	}
> +
> +	return ret;
> +}
> +
> +static void slot_power_on_handler(struct powernv_php_slot *slot)
> +{
> +	void *fdt, *dt;
> +	uint64_t len;
> +	int ret, status = 1;
> +
> +	/* We don't know the FDT blob size. It tries with incremental
> +	 * sized memory chunk.
> +	 */


What is the real expected size? 0x10000 is just 64K, just allocate it and 
that's it.


> +	for (len = 0x2000; len <= 0x10000; len += 0x2000) {
> +		fdt = kzalloc(len, GFP_KERNEL);
> +		if (!fdt)
> +			break;
> +
> +		ret = pnv_pci_get_device_tree(slot->dn->phandle, fdt, len);
> +		if (!ret)
> +			break;
> +
> +		kfree(fdt);
> +	}
> +
> +	if (len > 0x10000) {
> +		dev_warn(&slot->pdev->dev, "Cannot alloc FDT blob\n");
> +		goto out;
> +	}
> +
> +	/* Unflatten device tree blob */
> +	dt = of_fdt_unflatten_tree(fdt, slot->dn, NULL);
> +	if (!dt) {
> +		dev_warn(&slot->pdev->dev, "Cannot unflatten FDT\n");
> +		goto free_fdt;
> +	}

Right here you could kfree(fdt) and not cache it in the slot struct at all. 
You do not use it later anyway.


> +
> +	/* Initialize and apply the changeset */
> +	of_changeset_init(&slot->ocs);
> +	ret = slot_populate_changeset(&slot->ocs, slot->dn);
> +	if (ret) {
> +		dev_warn(&slot->pdev->dev, "Error %d populating changeset\n",
> +			 ret);
> +		goto free_dt;
> +	}
> +
> +	slot->dn->child = NULL;
> +	ret = of_changeset_apply(&slot->ocs);
> +	if (ret) {
> +		dev_warn(&slot->pdev->dev, "Error %d applying changeset\n",
> +			 ret);
> +		goto destroy_changeset;
> +	}
> +
> +	/* Add device node firmware data */
> +	traverse_pci_device_nodes(slot->dn,
> +				  add_pci_device_node_info,
> +				  pci_bus_to_host(slot->bus));
> +	slot->fdt = fdt;
> +	slot->dt = dt;
> +	goto out;
> +
> +destroy_changeset:
> +	of_changeset_destroy(&slot->ocs);
> +free_dt:
> +	kfree(dt);
> +	slot->dn->child = NULL;

Can of_fdt_unflatten_tree() or of_changeset_init() or 
slot_populate_changeset() initialize dn->child? No kfree(slot->dn->child)?


> +free_fdt:
> +	kfree(fdt);
> +	status = 2;
> +out:
> +	/* Confirm status change */
> +	slot->status_confirmed = status;
> +	wake_up_interruptible(&slot->queue);
> +}
> +
> +static void powernv_php_slot_work(struct work_struct *data)
> +{
> +	struct powernv_php_slot *slot = container_of(data,
> +						     struct powernv_php_slot,
> +						     work);
> +	uint64_t php_event = be64_to_cpu(slot->msg->params[0]);
> +
> +	switch (php_event) {
> +	case 0: /* Slot power off */
> +		slot_power_off_handler(slot);
> +		break;
> +	case 1: /* Slot power on */
> +		slot_power_on_handler(slot);
> +		break;

These 0 and 1 are not the same 0 and 1 used for @val in get_power_status(), 
these are from OPAL so please define them.


> +	default:
> +		dev_warn(&slot->pdev->dev, "Unsupported hotplug event %lld\n",
> +			 php_event);
> +	}
> +
> +	of_node_put(slot->dn);
> +}
> +
> +int powernv_php_msg_handler(struct notifier_block *nb,
> +			    unsigned long type, void *message)
> +{
> +	phandle h;
> +	struct device_node *np;
> +	struct powernv_php_slot *slot;
> +	struct opal_msg *msg = message;
> +
> +	/* Check the message type */
> +	if (type != OPAL_MSG_PCI_HOTPLUG) {
> +		pr_warn("%s: Wrong message type %ld received!\n",
> +			__func__, type);
> +		return NOTIFY_DONE;
> +	}
> +
> +	/* Find the device node */
> +	h = (phandle)be64_to_cpu(msg->params[1]);
> +	np = of_find_node_by_phandle(h);
> +	if (!np) {
> +		pr_warn("%s: No device node for phandle 0x%08x\n",
> +			__func__, h);
> +		return NOTIFY_DONE;
> +	}
> +
> +	/* Find the slot */
> +	slot = powernv_php_slot_find(np);
> +	if (!slot) {
> +		pr_warn("%s: No slot found for node <%s>\n",
> +			__func__, of_node_full_name(np));
> +		of_node_put(np);
> +		return NOTIFY_DONE;
> +	}
> +
> +	/* Schedule the work */
> +	slot->msg = msg;
> +	schedule_work(&slot->work);
> +	return NOTIFY_OK;
> +}

This function belongs to drivers/pci/hotplug/powernv_php.c (searching a 
slot is powernv_php.c's scope) except these lines:

 > +	/* Schedule the work */
 > +	slot->msg = msg;
 > +	schedule_work(&slot->work);

These 3 lines should be in helper in drivers/pci/hotplug/powernv_php_slot.c.


> +
> +static int set_power_status(struct hotplug_slot *php_slot, u8 val)
> +{
> +	struct powernv_php_slot *slot = php_slot->private;
> +	int ret;
> +
> +	/* Set power status */
> +	slot->status_confirmed = 0;
> +	ret = pnv_pci_set_power_status(slot->id, val);
> +	if (ret) {
> +		dev_warn(&slot->pdev->dev, "Error %d powering %s slot\n",
> +			 ret, val ? "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 always waiting for that.
> +	 */
> +	if (slot->status_confirmed == 1)
> +		return 0;
> +	else if (slot->status_confirmed > 0)
> +		return -EBUSY;
> +
> +	ret = wait_event_timeout(slot->queue, slot->status_confirmed, 10 * HZ);
> +	if (!ret) {
> +		dev_warn(&slot->pdev->dev, "Error %d waiting for power-%s\n",
> +			 ret, val ? "on" : "off");
> +		return -EBUSY;
> +	}
> +
> +	/* Check the result */
> +	if (slot->status_confirmed == 1)
> +		return 0;
> +
> +	dev_warn(&slot->pdev->dev, "Error status %d for power-%s\n",
> +		 slot->status_confirmed, val ? "on" : "off");
> +	return -EBUSY;
> +}
> +
> +static int get_power_status(struct hotplug_slot *php_slot, u8 *val)
> +{
> +	struct powernv_php_slot *slot = php_slot->private;
> +	uint8_t state;
> +	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_status(slot->id, &state);
> +	if (ret) {
> +		*val = POWERNV_PHP_SLOT_POWER_ON;
> +		dev_warn(&slot->pdev->dev, "Error %d getting power status\n",
> +			 ret);
> +	} else {
> +		*val = state ? POWERNV_PHP_SLOT_POWER_ON :
> +			       POWERNV_PHP_SLOT_POWER_OFF;
> +		php_slot->info->power_status = *val;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_adapter_status(struct hotplug_slot *php_slot, u8 *val)
> +{
> +	struct powernv_php_slot *slot = php_slot->private;
> +	uint8_t state;
> +	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_status(slot->id, &state);
> +	if (ret >= 0) {
> +		ret = 0;
> +		*val = state ? POWERNV_PHP_SLOT_PRESENT :
> +			       POWERNV_PHP_SLOT_EMPTY;
> +		php_slot->info->adapter_status = *val;
> +		ret = 0;
> +	} else {
> +		*val = POWERNV_PHP_SLOT_EMPTY;
> +		dev_warn(&slot->pdev->dev, "Error %d getting presence\n",
> +			 ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int set_attention_status(struct hotplug_slot *php_slot, u8 val)
> +{
> +	struct powernv_php_slot *slot = php_slot->private;
> +
> +	/* The default operation would to turn on the attention */
> +	switch (val) {
> +	case POWERNV_PHP_SLOT_ATTEN_OFF:
> +	case POWERNV_PHP_SLOT_ATTEN_ON:
> +	case POWERNV_PHP_SLOT_ATTEN_IND:
> +	case POWERNV_PHP_SLOT_ATTEN_ACT:
> +		break;
> +	default:
> +		dev_warn(&slot->pdev->dev, "Invalid attention %d\n", val);
> +		return -EINVAL;
> +	}
> +
> +	/* FIXME: Make it real once firmware supports it */
> +	php_slot->info->attention_status = val;

Since firmware does not have an idea about these 
POWERNV_PHP_SLOT_ATTEN_xxx, just remove them. Later when the firmware will 
know about them, we will have to change this code anyway and by that time, 
the set of states may have changed.


> +
> +	return 0;
> +}
> +
> +int powernv_php_slot_enable(struct hotplug_slot *php_slot, bool rescan)


This should receive powernv_php_slot as described above.


> +{
> +	struct powernv_php_slot *slot = php_slot->private;
> +	uint8_t presence, power_status;
> +	int ret;
> +
> +	/* Check if the slot has been configured */
> +	if (slot->state != POWERNV_PHP_SLOT_STATE_REGISTER)
> +		return 0;
> +
> +	/* Retrieve slot presence status */
> +	ret = php_slot->ops->get_adapter_status(php_slot, &presence);
> +	if (ret)
> +		return ret;
> +
> +	/* Proceed if there have nothing behind the slot */
> +	if (presence == POWERNV_PHP_SLOT_EMPTY)
> +		goto scan;
> +
> +	/*
> +	 * If we don't detect something behind the slot, we need
> +	 * make sure the power suply to the slot is on. Otherwise,
> +	 * the slot downstream PCIe linkturn should be down.
> +	 *
> +	 * 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 (!slot->check_power_status) {
> +		slot->check_power_status = 1;
> +		goto scan;
> +	}
> +
> +	/* Check the power status. Scan the slot if that's already on */
> +	ret = php_slot->ops->get_power_status(php_slot, &power_status);
> +	if (ret)
> +		return ret;
> +
> +	if (power_status == POWERNV_PHP_SLOT_POWER_ON)
> +		goto scan;
> +
> +	/* Power is off, turn it on and then scan the slot */
> +	ret = set_power_status(php_slot, POWERNV_PHP_SLOT_POWER_ON);
> +	if (ret)
> +		return ret;
> +
> +scan:
> +	switch (presence) {
> +	case POWERNV_PHP_SLOT_PRESENT:
> +		if (rescan) {
> +			pci_lock_rescan_remove();
> +			pci_add_pci_devices(slot->bus);
> +			pci_unlock_rescan_remove();
> +		}
> +
> +		/* Rescan for child hotpluggable slots */
> +		slot->state = POWERNV_PHP_SLOT_STATE_POPULATED;
> +		if (rescan)
> +			powernv_php_register(slot->dn);
> +		break;
> +	case POWERNV_PHP_SLOT_EMPTY:
> +		slot->state = POWERNV_PHP_SLOT_STATE_POPULATED;
> +		break;
> +	default:
> +		dev_warn(&slot->pdev->dev, "Invalid presence status %d\n",
> +			 presence);
> +		return -EINVAL;

Neigher PHP driver will ever have presence other than 0 or 1. So this 
switch() is simple if(presence){}else{}.



> +	}
> +
> +	return 0;
> +}
> +
> +static int enable_slot(struct hotplug_slot *php_slot)
> +{
> +	return powernv_php_slot_enable(php_slot, true);
> +}
> +
> +static int disable_slot(struct hotplug_slot *php_slot)
> +{
> +	struct powernv_php_slot *slot = php_slot->private;
> +	uint8_t power_status;
> +	int ret;
> +
> +	if (slot->state != POWERNV_PHP_SLOT_STATE_POPULATED)
> +		return 0;
> +
> +	/* Remove all devices behind the slot */
> +	pci_lock_rescan_remove();
> +	pci_remove_pci_devices(slot->bus);
> +	pci_unlock_rescan_remove();
> +
> +	/* Detach the child hotpluggable slots */
> +	powernv_php_unregister(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 = php_slot->ops->get_power_status(php_slot, &power_status);
> +	if (ret || power_status == POWERNV_PHP_SLOT_POWER_ON) {
> +		ret = set_power_status(php_slot, POWERNV_PHP_SLOT_POWER_OFF);
> +		if (ret)
> +			dev_warn(&slot->pdev->dev, "Error %d powering off\n",
> +				 ret);
> +	}
> +
> +	/* Update slot state */
> +	slot->state = POWERNV_PHP_SLOT_STATE_REGISTER;
> +	return 0;
> +}
> +
> +static struct hotplug_slot_ops php_slot_ops = {
> +	.get_power_status	= get_power_status,
> +	.get_adapter_status	= get_adapter_status,
> +	.set_attention_status	= set_attention_status,
> +	.enable_slot		= enable_slot,
> +	.disable_slot		= disable_slot,
> +};
> +
> +static struct powernv_php_slot *php_slot_match(struct device_node *dn,
> +					       struct powernv_php_slot *slot)
> +{
> +	struct powernv_php_slot *target, *tmp;
> +
> +	if (slot->dn == dn)
> +		return slot;
> +
> +	list_for_each_entry(tmp, &slot->children, link) {
> +		target = php_slot_match(dn, tmp);
> +		if (target)
> +			return target;
> +	}
> +
> +	return NULL;
> +}
> +
> +struct powernv_php_slot *powernv_php_slot_find(struct device_node *dn)
> +{
> +	struct powernv_php_slot *slot, *tmp;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&php_slot_lock, flags);
> +	list_for_each_entry(tmp, &php_slot_list, link) {
> +		slot = php_slot_match(dn, tmp);
> +		if (slot) {
> +			spin_unlock_irqrestore(&php_slot_lock, flags);
> +			return slot;
> +		}
> +	}
> +	spin_unlock_irqrestore(&php_slot_lock, flags);
> +
> +	return NULL;
> +}
> +
> +void powernv_php_slot_free(struct kref *kref)
> +{
> +	struct powernv_php_slot *slot = to_powernv_php_slot(kref);
> +
> +	WARN_ON(!list_empty(&slot->children));
> +	kfree(slot->name);
> +	kfree(slot);
> +}
> +
> +static void php_slot_release(struct hotplug_slot *hp_slot)
> +{
> +	struct powernv_php_slot *slot = hp_slot->private;
> +	unsigned long flags;
> +
> +	/* Remove from global or child list */
> +	spin_lock_irqsave(&php_slot_lock, flags);
> +	list_del(&slot->link);
> +	spin_unlock_irqrestore(&php_slot_lock, flags);


This is a good example where RCU rules. powernv_php_slot_find() returns 
slot pointer and its use is not protected by spin_lock -> dangerous.

Remove spin_lock(), s/list_del/list_del_rcu/, and move bits below to 
call_rcu(), and s/list_for_each_entry/list_for_each_entry_rcu/.


> +
> +	/* Detach from parent */
> +	powernv_php_slot_put(slot);
> +	powernv_php_slot_put(slot->parent);
> +}
> +
> +static bool php_slot_get_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);
> +
> +	/* Bus/Slot/Function number */
> +	prop32 = of_get_property(dn, "reg", NULL);
> +	if (!prop32)
> +		return false;
> +	*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 false;
> +		}
> +
> +		*id |= be64_to_cpup(prop64);
> +		of_node_put(parent);
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +struct powernv_php_slot *powernv_php_slot_alloc(struct device_node *dn)
> +{
> +	struct eeh_dev *edev = pdn_to_eeh_dev(PCI_DN(dn));
> +	struct pci_bus *bus;
> +	struct powernv_php_slot *slot;
> +	const char *label;
> +	uint64_t id;
> +	int slot_no;
> +	size_t size;
> +	void *pmem;
> +
> +	/* Slot name */
> +	label = of_get_property(dn, "ibm,slot-label", NULL);
> +	if (!label)
> +		return NULL;
> +
> +	/* Slot identifier */
> +	if (!php_slot_get_id(dn, &id))
> +		return NULL;
> +
> +	/* PCI bus */
> +	bus = of_node_to_pci_bus(dn);
> +	if (!bus)
> +		return NULL;
> +
> +	/* Slot number */
> +	if (dn->child && PCI_DN(dn->child))
> +		slot_no = PCI_SLOT(PCI_DN(dn->child)->devfn);
> +	else
> +		slot_no = -1;

Not INVALID_SLOT and
#define INVALID_SLOT -1
? :)


> +
> +	/* Allocate slot */
> +	size = sizeof(struct powernv_php_slot) +
> +	       sizeof(struct hotplug_slot) +
> +	       sizeof(struct hotplug_slot_info);
> +	pmem = kzalloc(size, GFP_KERNEL);
> +	if (!pmem) {
> +		pr_warn("%s: Cannot allocate slot for node %s\n",
> +			__func__, dn->full_name);
> +		return NULL;
> +	}
> +
> +	/* Assign memory blocks */
> +	slot = pmem;
> +	slot->php_slot = pmem + sizeof(struct powernv_php_slot);
> +	slot->php_slot->info = pmem + sizeof(struct powernv_php_slot) +
> +			      sizeof(struct hotplug_slot);
> +	slot->name = kstrdup(label, GFP_KERNEL);
> +	if (!slot->name) {
> +		pr_warn("%s: Cannot populate name for node %s\n",
> +			__func__, dn->full_name);
> +		kfree(pmem);
> +		return NULL;
> +	}

Why not just embed structs one to another?

> +
> +	/* Initialize slot */
> +	kref_init(&slot->kref);
> +	slot->state = POWERNV_PHP_SLOT_STATE_INIT;
> +	slot->dn = dn;
> +	slot->pdev = eeh_dev_to_pci_dev(edev);
> +	slot->bus = bus;
> +	slot->id = id;
> +	slot->slot_no = slot_no;
> +	INIT_WORK(&slot->work, powernv_php_slot_work);
> +	init_waitqueue_head(&slot->queue);
> +	slot->check_power_status = 0;
> +	slot->status_confirmed = 0;
> +	slot->php_slot->ops = &php_slot_ops;
> +	slot->php_slot->release = php_slot_release;
> +	slot->php_slot->private = slot;
> +	INIT_LIST_HEAD(&slot->children);
> +	INIT_LIST_HEAD(&slot->link);
> +
> +	return slot;
> +}
> +
> +int powernv_php_slot_register(struct powernv_php_slot *slot)
> +{
> +	struct powernv_php_slot *parent;
> +	struct device_node *dn = slot->dn;
> +	unsigned long flags;
> +	int ret;
> +
> +	/* Avoid register same slot for twice */
> +	if (powernv_php_slot_find(slot->dn))
> +		return -EEXIST;
> +
> +	/* Register slot */
> +	ret = pci_hp_register(slot->php_slot, slot->bus,
> +			      slot->slot_no, slot->name);
> +	if (ret) {
> +		dev_warn(&slot->pdev->dev, "Error %d registering slot\n",
> +			 ret);
> +		return ret;
> +	}
> +
> +	/* Put into global or parent list */
> +	while ((dn = of_get_parent(dn))) {
> +		if (!PCI_DN(dn)) {
> +			of_node_put(dn);
> +			break;
> +		}
> +
> +		parent = powernv_php_slot_find(dn);
> +		if (parent) {
> +			of_node_put(dn);
> +			break;
> +		}
> +	}
> +
> +	spin_lock_irqsave(&php_slot_lock, flags);
> +	if (parent) {
> +		powernv_php_slot_get(parent);
> +		slot->parent = parent;
> +		list_add_tail(&slot->link, &parent->children);
> +	} else {
> +		list_add_tail(&slot->link, &php_slot_list);
> +	}
> +	spin_unlock_irqrestore(&php_slot_lock, flags);
> +
> +	/* Update slot state */
> +	slot->state = POWERNV_PHP_SLOT_STATE_REGISTER;
> +	return 0;
> +}
>


Now I finished with this patchset respin :)

Gavin Shan Aug. 15, 2015, 4:47 a.m. UTC | #2

On Sat, Aug 15, 2015 at 01:13:21PM +1000, Alexey Kardashevskiy wrote:
>On 08/06/2015 02:11 PM, Gavin Shan wrote:
>>The patch intends to add standalone driver to support PCI hotplug
>>for PowerPC PowerNV platform, which runs on top of skiboot firmware.
>>The firmware identified hotpluggable slots and marked their device
>>tree node with proper "ibm,slot-pluggable" and "ibm,reset-by-firmware".
>>The driver simply scans device-tree to create/register PCI hotplug slot
>>accordingly.
>>
>>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 <[e-mail]>
>>Acked-by: Bjorn Helgaas <[e-mail]>
>>---
>>  MAINTAINERS                            |   6 +
>>  drivers/pci/hotplug/Kconfig            |  12 +
>>  drivers/pci/hotplug/Makefile           |   4 +
>>  drivers/pci/hotplug/powernv_php.c      | 140 +++++++
>>  drivers/pci/hotplug/powernv_php.h      |  92 +++++
>>  drivers/pci/hotplug/powernv_php_slot.c | 722 +++++++++++++++++++++++++++++++++
>>  6 files changed, 976 insertions(+)
>>  create mode 100644 drivers/pci/hotplug/powernv_php.c
>>  create mode 100644 drivers/pci/hotplug/powernv_php.h
>>  create mode 100644 drivers/pci/hotplug/powernv_php_slot.c
>>
>>diff --git a/MAINTAINERS b/MAINTAINERS
>>index fd60784..3b75c92 100644
>>--- a/MAINTAINERS
>>+++ b/MAINTAINERS
>>@@ -7747,6 +7747,12 @@ L:	[e-mail]
>>  S:	Supported
>>  F:	Documentation/PCI/pci-error-recovery.txt
>>
>>+PCI HOTPLUG DRIVER FOR POWERNV PLATFORM
>>+M:	Gavin Shan <[e-mail]>
>>+L:	[e-mail]
>>+S:	Supported
>>+F:	drivers/pci/hotplug/powernv_php*
>>+
>>  PCI SUBSYSTEM
>>  M:	Bjorn Helgaas <[e-mail]>
>>  L:	[e-mail]
>>diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>>index df8caec..ef55dae 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 powernv-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..fd51d65 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)	+= powernv-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,9 @@ ibmphp-objs		:=	ibmphp_core.o	\
>>  acpiphp-objs		:=	acpiphp_core.o	\
>>  				acpiphp_glue.o
>>
>>+powernv-php-objs	:=	powernv_php.o	\
>>+				powernv_php_slot.o
>>+
>>  rpaphp-objs		:=	rpaphp_core.o	\
>>  				rpaphp_pci.o	\
>>  				rpaphp_slot.o
>>diff --git a/drivers/pci/hotplug/powernv_php.c b/drivers/pci/hotplug/powernv_php.c
>>new file mode 100644
>>index 0000000..4cbff7a
>>--- /dev/null
>>+++ b/drivers/pci/hotplug/powernv_php.c
>>@@ -0,0 +1,140 @@
>>+/*
>>+ * 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/module.h>
>>+
>>+#include <asm/opal.h>
>>+#include <asm/pnv-pci.h>
>>+
>>+#include "powernv_php.h"
>>+
>>+#define DRIVER_VERSION	"0.1"
>>+#define DRIVER_AUTHOR	"Gavin Shan, IBM Corporation"
>>+#define DRIVER_DESC	"PowerPC PowerNV PCI Hotplug Driver"
>
>
>Align all or none.
>

All of them are already aligned well. Please check your email setting.

>>+
>>+static struct notifier_block php_msg_nb = {
>>+	.notifier_call	= powernv_php_msg_handler,
>>+	.next		= NULL,
>>+	.priority	= 0,
>>+};
>>+
>>+static int powernv_php_register_one(struct device_node *dn)
>>+{
>>+	struct powernv_php_slot *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;
>>+
>>+	/* Allocate slot */
>>+	slot = powernv_php_slot_alloc(dn);
>>+	if (!slot)
>>+		return -ENODEV;
>>+
>>+	/* Register it */
>>+	ret = powernv_php_slot_register(slot);
>>+	if (ret) {
>>+		powernv_php_slot_put(slot);
>>+		return ret;
>>+	}
>>+
>>+	return powernv_php_slot_enable(slot->php_slot, false);
>
>
>And if it fails, no unregister and cleanup is needed?
>

You're right that it need care the failure cases. Will add something
in next revision.

>>+}
>>+
>>+int powernv_php_register(struct device_node *dn)
>>+{
>>+	struct device_node *child;
>>+	int ret = 0;
>
>@ret is not used below.
>

Ok. will remove it.

>>+
>>+	/*
>>+	 * The parent slots should be registered before their
>>+	 * child slots.
>>+	 */
>>+	for_each_child_of_node(dn, child) {
>>+		powernv_php_register_one(child);
>>+		powernv_php_register(child);
>>+	}
>>+
>>+	return ret;
>>+}
>>+
>>+static void powernv_php_unregister_one(struct device_node *dn)
>>+{
>>+	struct powernv_php_slot *slot;
>>+
>>+	slot = powernv_php_slot_find(dn);
>>+	if (!slot)
>>+		return;
>>+
>>+	pci_hp_deregister(slot->php_slot);
>>+}
>>+
>>+void powernv_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) {
>>+		powernv_php_unregister(child);
>>+		powernv_php_unregister_one(child);
>>+	}
>>+}
>>+
>>+static int __init powernv_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")
>>+		powernv_php_register(dn);
>>+	for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
>>+		powernv_php_register(dn);
>
>
>May be move pnv_pci_hotplug_notifier_register() after powernv_php_register()?
>If not, then below (in powernv_php_exit()) move
>pnv_pci_hotplug_notifier_unregister() to the end?
>
>

Ok. I'll move pnv_pci_hotplug_notifier_unregister() to end of powernv_php_exit().

>>+
>>+	return 0;
>>+}
>>+
>>+static void __exit powernv_php_exit(void)
>>+{
>>+	struct device_node *dn;
>>+
>>+	pnv_pci_hotplug_notifier_unregister(&php_msg_nb);
>>+
>>+	for_each_compatible_node(dn, NULL, "ibm,ioda-phb")
>>+		powernv_php_unregister(dn);
>>+	for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
>>+		powernv_php_unregister(dn);
>>+}
>>+
>>+module_init(powernv_php_init);
>>+module_exit(powernv_php_exit);
>>+
>>+MODULE_VERSION(DRIVER_VERSION);
>>+MODULE_LICENSE("GPL v2");
>>+MODULE_AUTHOR(DRIVER_AUTHOR);
>>+MODULE_DESCRIPTION(DRIVER_DESC);
>>diff --git a/drivers/pci/hotplug/powernv_php.h b/drivers/pci/hotplug/powernv_php.h
>>new file mode 100644
>>index 0000000..8034cc6
>>--- /dev/null
>>+++ b/drivers/pci/hotplug/powernv_php.h
>>@@ -0,0 +1,92 @@
>>+/*
>>+ * 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.
>>+ */
>>+
>>+#ifndef _POWERNV_PHP_H
>>+#define _POWERNV_PHP_H
>>+
>>+#include <linux/list.h>
>>+#include <linux/kref.h>
>>+#include <linux/of.h>
>>+#include <linux/pci.h>
>>+#include <linux/pci_hotplug.h>
>>+#include <linux/wait.h>
>>+#include <linux/workqueue.h>
>>+
>>+#include <asm/opal-api.h>
>>+
>>+/* Slot power status */
>>+#define POWERNV_PHP_SLOT_POWER_OFF	0
>>+#define POWERNV_PHP_SLOT_POWER_ON	1
>>+
>>+/* Slot presence status */
>>+#define POWERNV_PHP_SLOT_EMPTY		0
>>+#define POWERNV_PHP_SLOT_PRESENT	1
>
>These two are also only used in drivers/pci/hotplug/powernv_php_slot.c,
>move them there at least. It also seems your PHP driver is the only one which
>uses flags for an adapter status, others return plain 0 or 1 (which are
>c-style "false" and "true", pretty much, so it is not the case of magic
>constants). Since you return these values from the hotplug_slot_ops callbacks
>to external code, you should probably do the same.
>
>And exactly the same comment about POWERNV_PHP_SLOT_POWER_ON/OFF few lines
>above.
>

All those mcaroes are good enough to be put in this header file since they
are part of the slot's status. Yes, they're only used in powernv_php_slot.c
currently, but doesn't have to in future. I don't see why I need change them
to true/false, which just represents two states at most. Here, all states
are represented by numberic values (as POWERNV_PHP_SLOT_ATTEN_* as below).

>
>
>>+
>>+/* Slot attention status */
>>+#define POWERNV_PHP_SLOT_ATTEN_OFF	0
>>+#define POWERNV_PHP_SLOT_ATTEN_ON	1
>>+#define POWERNV_PHP_SLOT_ATTEN_IND	2
>>+#define POWERNV_PHP_SLOT_ATTEN_ACT	3
>
>
>These should go to drivers/pci/hotplug/powernv_php_slot.c. Where are these
>flags defined? Looks to me like there is a way to pass some status from the
>userspace via sysfs to OPAL so only OPAL is supposed to recognize and handle
>these. If so, these macros are missing "OPAL" in their names. I have one more
>comment below about it.
>

I prefer keep them in this header file as explained above. Those macros are
not connected/passed to OPAL directly. All OPAL calls have corresponding
wrappers in arch/powerpc/platforms/powernv/pci.c.

>>+
>>+struct powernv_php_slot {
>>+	char			*name;
>>+	struct device_node	*dn;
>>+	struct pci_dev		*pdev;
>>+	struct pci_bus		*bus;
>>+	uint64_t		id;
>>+	int			slot_no;
>>+	struct kref		kref;
>>+#define POWERNV_PHP_SLOT_STATE_INIT		0
>>+#define POWERNV_PHP_SLOT_STATE_REGISTER		1
>>+#define POWERNV_PHP_SLOT_STATE_POPULATED	2
>>+	int			state;
>>+	int			check_power_status;
>>+	int			status_confirmed;
>
>s/status_confirmed/power_status_confirmed/
>

Maybe, I think "status_confirmed" is enough here.

>What is this status? It can be 0, 1, 2 which seems to be
>UNCONFIRMED/INPROGRESS/CONFIRMED (does not need PNV/IODA prefixes as it is
>local to the powernv_php_slot.c file).
>

No, it only has two states: 0/1. Again, it's connected with any OPAL
calls directly. No need to have "IODA_" prefix at all.

>
>>+	struct opal_msg		*msg;
>>+	void			*fdt;
>>+	void			*dt;
>>+	struct of_changeset	ocs;
>>+	struct work_struct	work;
>>+	wait_queue_head_t	queue;
>>+	struct hotplug_slot	*php_slot;
>>+	struct powernv_php_slot	*parent;
>>+	struct list_head	children;
>>+	struct list_head	link;
>>+};
>
>This should go to drivers/pci/hotplug/powernv_php_slot.c and this header
>should only have a forward declaration. After you move it there, you get
>better separation of the driver code from the slot code and only 2 changes
>will be needed:
>
>1. powernv_php_slot_enable() should receive powernv_php_slot
>2. add powernv_php_slot_unregister() (like powernv_php_slot_register()), this
>way you will have pairing pci_hp_register/pci_hp_deregister in the same file.
>
>
>After you moved this struct to the source file, you could remove/shorten
>POWERNV_PHP_SLOT_STATE_ prefixes if you wished.
>

I don't see why it should go to drivers/pci/hotplug/powernv_php_slot.c. it's
the core data structure shared by multiple source files.

>>+
>>+int powernv_php_msg_handler(struct notifier_block *nb,
>>+			    unsigned long type, void *message);
>>+struct powernv_php_slot *powernv_php_slot_find(struct device_node *dn);
>>+void powernv_php_slot_free(struct kref *kref);
>>+struct powernv_php_slot *powernv_php_slot_alloc(struct device_node *dn);
>>+int powernv_php_slot_register(struct powernv_php_slot *slot);
>>+int powernv_php_slot_enable(struct hotplug_slot *php_slot, bool rescan);
>>+int powernv_php_register(struct device_node *dn);
>>+void powernv_php_unregister(struct device_node *dn);
>>+
>>+#define to_powernv_php_slot(kref) \
>>+	container_of(kref, struct powernv_php_slot, kref)
>>+
>>+static inline void powernv_php_slot_get(struct powernv_php_slot *slot)
>>+{
>>+	if (slot)
>>+		kref_get(&slot->kref);
>>+}
>>+
>>+static inline int powernv_php_slot_put(struct powernv_php_slot *slot)
>>+{
>>+	if (slot)
>>+		return kref_put(&slot->kref, powernv_php_slot_free);
>>+
>>+	return 0;
>>+}
>
>In these 2 helpers you do not have to check for @slof - it is checked in the
>callers pretty much always. Or it is not checked in php_slot_release() but
>dereferenced before you call powernv_php_slot_put(slot).
>
>The only place you really want this check is
>powernv_php_slot_put(slot->parent) so just check it there. btw is it even
>possible for the slot not to have a parent?
>

For most cases, slot doesn't have parent. That's why I had the check here.
Yes, if you really want me to drop the check, then I have to check slot's
validation in the callers. Also, those helpers are inline functions, no
too much difference actually.

>So I'd ditch these helpers.
>

I don't see the reason why these helpers need to be dropped. To use kref_{get,put}
directly?

>>+
>>+#endif /* !_POWERNV_PHP_H */
>>diff --git a/drivers/pci/hotplug/powernv_php_slot.c b/drivers/pci/hotplug/powernv_php_slot.c
>>new file mode 100644
>>index 0000000..73a93a2
>>--- /dev/null
>>+++ b/drivers/pci/hotplug/powernv_php_slot.c
>>@@ -0,0 +1,722 @@
>>+/*
>>+ * 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/module.h>
>>+
>>+#include <asm/opal.h>
>>+#include <asm/pnv-pci.h>
>>+#include <asm/ppc-pci.h>
>>+
>>+#include "powernv_php.h"
>>+
>>+static LIST_HEAD(php_slot_list);
>>+static DEFINE_SPINLOCK(php_slot_lock);
>>+
>>+/*
>>+ * Remove firmware data for all child device nodes of the
>>+ * indicated one.
>>+ */
>>+static void remove_child_pdn(struct device_node *np)
>>+{
>>+	struct device_node *child;
>>+
>>+	for_each_child_of_node(np, child) {
>>+		/* In depth first */
>>+		remove_child_pdn(child);
>>+
>>+		remove_pci_device_node_info(child);
>>+	}
>>+}
>>+
>>+/*
>>+ * Remove all subordinate device nodes of the indicated one.
>>+ * Those device nodes in deepest path should be released firstly.
>>+ */
>>+static int remove_child_device_nodes(struct device_node *parent)
>>+{
>>+	struct device_node *np, *child;
>>+	int ret = 0;
>>+
>>+	/* If the device node has children, remove them firstly */
>>+	for_each_child_of_node(parent, np) {
>>+		ret = remove_child_device_nodes(np);
>>+		if (ret)
>>+			return ret;
>>+
>>+		/* The device shouldn't have alive children */
>>+		child = of_get_next_child(np, NULL);
>>+		if (child) {
>>+			of_node_put(child);
>>+			of_node_put(np);
>>+			pr_err("%s: Alive children of node <%s>\n",
>>+			       __func__, of_node_full_name(np));
>>+			return -EBUSY;
>>+		}
>>+
>>+		/* Detach the device node */
>>+		of_detach_node(np);
>>+		of_node_put(np);
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+/*
>>+ * The function processes the message sent by firmware
>>+ * to remove all device tree nodes beneath the slot's
>>+ * nodes, and the associated auxillary data.
>>+ */
>>+static void slot_power_off_handler(struct powernv_php_slot *slot)
>>+{
>>+	int ret, status = 1;
>>+
>>+	/* Release the firmware data for the child device nodes */
>>+	remove_child_pdn(slot->dn);
>>+
>>+	/*
>>+	 * Release the child device nodes. If the sub-tree was
>>+	 * built with the help of changeset, we just need destroy
>>+	 * the changes.
>>+	 */
>>+	if (slot->fdt) {
>>+		of_changeset_destroy(&slot->ocs);
>>+		kfree(slot->dt);
>>+		slot->dt = NULL;
>>+		slot->dn->child = NULL;
>>+		kfree(slot->fdt);
>>+		slot->fdt = NULL;
>>+	} else {
>>+		ret = remove_child_device_nodes(slot->dn);
>>+		if (ret) {
>>+			status = 2;
>>+			dev_warn(&slot->pdev->dev, "Error %d freeing nodes\n",
>>+				 ret);
>>+		}
>>+	}
>>+
>>+	/* Confirm status change */
>>+	slot->status_confirmed = status;
>>+	wake_up_interruptible(&slot->queue);
>>+}
>>+
>>+static int slot_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)
>>+			return ret;
>>+
>>+		ret = slot_populate_changeset(ocs, child);
>>+	}
>>+
>>+	return ret;
>>+}
>>+
>>+static void slot_power_on_handler(struct powernv_php_slot *slot)
>>+{
>>+	void *fdt, *dt;
>>+	uint64_t len;
>>+	int ret, status = 1;
>>+
>>+	/* We don't know the FDT blob size. It tries with incremental
>>+	 * sized memory chunk.
>>+	 */
>
>
>What is the real expected size? 0x10000 is just 64K, just allocate it and
>that's it.
>

You can not know the size in advance. It depends on the size of the FDT
blob to be transfered. So the size has to be probed.

>>+	for (len = 0x2000; len <= 0x10000; len += 0x2000) {
>>+		fdt = kzalloc(len, GFP_KERNEL);
>>+		if (!fdt)
>>+			break;
>>+
>>+		ret = pnv_pci_get_device_tree(slot->dn->phandle, fdt, len);
>>+		if (!ret)
>>+			break;
>>+
>>+		kfree(fdt);
>>+	}
>>+
>>+	if (len > 0x10000) {
>>+		dev_warn(&slot->pdev->dev, "Cannot alloc FDT blob\n");
>>+		goto out;
>>+	}
>>+
>>+	/* Unflatten device tree blob */
>>+	dt = of_fdt_unflatten_tree(fdt, slot->dn, NULL);
>>+	if (!dt) {
>>+		dev_warn(&slot->pdev->dev, "Cannot unflatten FDT\n");
>>+		goto free_fdt;
>>+	}
>
>Right here you could kfree(fdt) and not cache it in the slot struct at all.
>You do not use it later anyway.
>

The "fdt" has been cached at end of this function and it can't be released
because the unflattened device-tree is still using data in the FDT blob (@fdt).

>
>>+
>>+	/* Initialize and apply the changeset */
>>+	of_changeset_init(&slot->ocs);
>>+	ret = slot_populate_changeset(&slot->ocs, slot->dn);
>>+	if (ret) {
>>+		dev_warn(&slot->pdev->dev, "Error %d populating changeset\n",
>>+			 ret);
>>+		goto free_dt;
>>+	}
>>+
>>+	slot->dn->child = NULL;
>>+	ret = of_changeset_apply(&slot->ocs);
>>+	if (ret) {
>>+		dev_warn(&slot->pdev->dev, "Error %d applying changeset\n",
>>+			 ret);
>>+		goto destroy_changeset;
>>+	}
>>+
>>+	/* Add device node firmware data */
>>+	traverse_pci_device_nodes(slot->dn,
>>+				  add_pci_device_node_info,
>>+				  pci_bus_to_host(slot->bus));
>>+	slot->fdt = fdt;
>>+	slot->dt = dt;
>>+	goto out;
>>+
>>+destroy_changeset:
>>+	of_changeset_destroy(&slot->ocs);
>>+free_dt:
>>+	kfree(dt);
>>+	slot->dn->child = NULL;
>
>Can of_fdt_unflatten_tree() or of_changeset_init() or
>slot_populate_changeset() initialize dn->child? No kfree(slot->dn->child)?
>

of_fdt_unflatten_tree() did. @dt is the unflattened device-tree here.
No need to free slot->dn->child.

>>+free_fdt:
>>+	kfree(fdt);
>>+	status = 2;
>>+out:
>>+	/* Confirm status change */
>>+	slot->status_confirmed = status;
>>+	wake_up_interruptible(&slot->queue);
>>+}
>>+
>>+static void powernv_php_slot_work(struct work_struct *data)
>>+{
>>+	struct powernv_php_slot *slot = container_of(data,
>>+						     struct powernv_php_slot,
>>+						     work);
>>+	uint64_t php_event = be64_to_cpu(slot->msg->params[0]);
>>+
>>+	switch (php_event) {
>>+	case 0: /* Slot power off */
>>+		slot_power_off_handler(slot);
>>+		break;
>>+	case 1: /* Slot power on */
>>+		slot_power_on_handler(slot);
>>+		break;
>
>These 0 and 1 are not the same 0 and 1 used for @val in get_power_status(),
>these are from OPAL so please define them.
>

Good point. I'll have following macros for them in opal-api.h:

#define OPAL_PCI_PHP_EVENT_POWER_OFF	0
#define OPAL_PCI_PHP_EVENT_POWER_ON	1

Or to use "enum" type.

>
>>+	default:
>>+		dev_warn(&slot->pdev->dev, "Unsupported hotplug event %lld\n",
>>+			 php_event);
>>+	}
>>+
>>+	of_node_put(slot->dn);
>>+}
>>+
>>+int powernv_php_msg_handler(struct notifier_block *nb,
>>+			    unsigned long type, void *message)
>>+{
>>+	phandle h;
>>+	struct device_node *np;
>>+	struct powernv_php_slot *slot;
>>+	struct opal_msg *msg = message;
>>+
>>+	/* Check the message type */
>>+	if (type != OPAL_MSG_PCI_HOTPLUG) {
>>+		pr_warn("%s: Wrong message type %ld received!\n",
>>+			__func__, type);
>>+		return NOTIFY_DONE;
>>+	}
>>+
>>+	/* Find the device node */
>>+	h = (phandle)be64_to_cpu(msg->params[1]);
>>+	np = of_find_node_by_phandle(h);
>>+	if (!np) {
>>+		pr_warn("%s: No device node for phandle 0x%08x\n",
>>+			__func__, h);
>>+		return NOTIFY_DONE;
>>+	}
>>+
>>+	/* Find the slot */
>>+	slot = powernv_php_slot_find(np);
>>+	if (!slot) {
>>+		pr_warn("%s: No slot found for node <%s>\n",
>>+			__func__, of_node_full_name(np));
>>+		of_node_put(np);
>>+		return NOTIFY_DONE;
>>+	}
>>+
>>+	/* Schedule the work */
>>+	slot->msg = msg;
>>+	schedule_work(&slot->work);
>>+	return NOTIFY_OK;
>>+}
>
>This function belongs to drivers/pci/hotplug/powernv_php.c (searching a slot
>is powernv_php.c's scope) except these lines:
>
>> +	/* Schedule the work */
>> +	slot->msg = msg;
>> +	schedule_work(&slot->work);
>
>These 3 lines should be in helper in drivers/pci/hotplug/powernv_php_slot.c.
>

It's no point to drop one helper and add another one. One thing I keep
in mind when writing the code: all hotplug logic (interacting with
OPAL firmware) is implemented in powernv_php_slot.c and powernv_php.c
just connect the helper functions with event.

>>+
>>+static int set_power_status(struct hotplug_slot *php_slot, u8 val)
>>+{
>>+	struct powernv_php_slot *slot = php_slot->private;
>>+	int ret;
>>+
>>+	/* Set power status */
>>+	slot->status_confirmed = 0;
>>+	ret = pnv_pci_set_power_status(slot->id, val);
>>+	if (ret) {
>>+		dev_warn(&slot->pdev->dev, "Error %d powering %s slot\n",
>>+			 ret, val ? "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 always waiting for that.
>>+	 */
>>+	if (slot->status_confirmed == 1)
>>+		return 0;
>>+	else if (slot->status_confirmed > 0)
>>+		return -EBUSY;
>>+
>>+	ret = wait_event_timeout(slot->queue, slot->status_confirmed, 10 * HZ);
>>+	if (!ret) {
>>+		dev_warn(&slot->pdev->dev, "Error %d waiting for power-%s\n",
>>+			 ret, val ? "on" : "off");
>>+		return -EBUSY;
>>+	}
>>+
>>+	/* Check the result */
>>+	if (slot->status_confirmed == 1)
>>+		return 0;
>>+
>>+	dev_warn(&slot->pdev->dev, "Error status %d for power-%s\n",
>>+		 slot->status_confirmed, val ? "on" : "off");
>>+	return -EBUSY;
>>+}
>>+
>>+static int get_power_status(struct hotplug_slot *php_slot, u8 *val)
>>+{
>>+	struct powernv_php_slot *slot = php_slot->private;
>>+	uint8_t state;
>>+	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_status(slot->id, &state);
>>+	if (ret) {
>>+		*val = POWERNV_PHP_SLOT_POWER_ON;
>>+		dev_warn(&slot->pdev->dev, "Error %d getting power status\n",
>>+			 ret);
>>+	} else {
>>+		*val = state ? POWERNV_PHP_SLOT_POWER_ON :
>>+			       POWERNV_PHP_SLOT_POWER_OFF;
>>+		php_slot->info->power_status = *val;
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static int get_adapter_status(struct hotplug_slot *php_slot, u8 *val)
>>+{
>>+	struct powernv_php_slot *slot = php_slot->private;
>>+	uint8_t state;
>>+	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_status(slot->id, &state);
>>+	if (ret >= 0) {
>>+		ret = 0;
>>+		*val = state ? POWERNV_PHP_SLOT_PRESENT :
>>+			       POWERNV_PHP_SLOT_EMPTY;
>>+		php_slot->info->adapter_status = *val;
>>+		ret = 0;
>>+	} else {
>>+		*val = POWERNV_PHP_SLOT_EMPTY;
>>+		dev_warn(&slot->pdev->dev, "Error %d getting presence\n",
>>+			 ret);
>>+	}
>>+
>>+	return ret;
>>+}
>>+
>>+static int set_attention_status(struct hotplug_slot *php_slot, u8 val)
>>+{
>>+	struct powernv_php_slot *slot = php_slot->private;
>>+
>>+	/* The default operation would to turn on the attention */
>>+	switch (val) {
>>+	case POWERNV_PHP_SLOT_ATTEN_OFF:
>>+	case POWERNV_PHP_SLOT_ATTEN_ON:
>>+	case POWERNV_PHP_SLOT_ATTEN_IND:
>>+	case POWERNV_PHP_SLOT_ATTEN_ACT:
>>+		break;
>>+	default:
>>+		dev_warn(&slot->pdev->dev, "Invalid attention %d\n", val);
>>+		return -EINVAL;
>>+	}
>>+
>>+	/* FIXME: Make it real once firmware supports it */
>>+	php_slot->info->attention_status = val;
>
>Since firmware does not have an idea about these POWERNV_PHP_SLOT_ATTEN_xxx,
>just remove them. Later when the firmware will know about them, we will have
>to change this code anyway and by that time, the set of states may have
>changed.
>

Sure, will do.

>>+
>>+	return 0;
>>+}
>>+
>>+int powernv_php_slot_enable(struct hotplug_slot *php_slot, bool rescan)
>
>
>This should receive powernv_php_slot as described above.
>

Good point, I'll change accordingly.

>>+{
>>+	struct powernv_php_slot *slot = php_slot->private;
>>+	uint8_t presence, power_status;
>>+	int ret;
>>+
>>+	/* Check if the slot has been configured */
>>+	if (slot->state != POWERNV_PHP_SLOT_STATE_REGISTER)
>>+		return 0;
>>+
>>+	/* Retrieve slot presence status */
>>+	ret = php_slot->ops->get_adapter_status(php_slot, &presence);
>>+	if (ret)
>>+		return ret;
>>+
>>+	/* Proceed if there have nothing behind the slot */
>>+	if (presence == POWERNV_PHP_SLOT_EMPTY)
>>+		goto scan;
>>+
>>+	/*
>>+	 * If we don't detect something behind the slot, we need
>>+	 * make sure the power suply to the slot is on. Otherwise,
>>+	 * the slot downstream PCIe linkturn should be down.
>>+	 *
>>+	 * 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 (!slot->check_power_status) {
>>+		slot->check_power_status = 1;
>>+		goto scan;
>>+	}
>>+
>>+	/* Check the power status. Scan the slot if that's already on */
>>+	ret = php_slot->ops->get_power_status(php_slot, &power_status);
>>+	if (ret)
>>+		return ret;
>>+
>>+	if (power_status == POWERNV_PHP_SLOT_POWER_ON)
>>+		goto scan;
>>+
>>+	/* Power is off, turn it on and then scan the slot */
>>+	ret = set_power_status(php_slot, POWERNV_PHP_SLOT_POWER_ON);
>>+	if (ret)
>>+		return ret;
>>+
>>+scan:
>>+	switch (presence) {
>>+	case POWERNV_PHP_SLOT_PRESENT:
>>+		if (rescan) {
>>+			pci_lock_rescan_remove();
>>+			pci_add_pci_devices(slot->bus);
>>+			pci_unlock_rescan_remove();
>>+		}
>>+
>>+		/* Rescan for child hotpluggable slots */
>>+		slot->state = POWERNV_PHP_SLOT_STATE_POPULATED;
>>+		if (rescan)
>>+			powernv_php_register(slot->dn);
>>+		break;
>>+	case POWERNV_PHP_SLOT_EMPTY:
>>+		slot->state = POWERNV_PHP_SLOT_STATE_POPULATED;
>>+		break;
>>+	default:
>>+		dev_warn(&slot->pdev->dev, "Invalid presence status %d\n",
>>+			 presence);
>>+		return -EINVAL;
>
>Neigher PHP driver will ever have presence other than 0 or 1. So this
>switch() is simple if(presence){}else{}.
>

Ok. Will use "if () {} else {}" then.

>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static int enable_slot(struct hotplug_slot *php_slot)
>>+{
>>+	return powernv_php_slot_enable(php_slot, true);
>>+}
>>+
>>+static int disable_slot(struct hotplug_slot *php_slot)
>>+{
>>+	struct powernv_php_slot *slot = php_slot->private;
>>+	uint8_t power_status;
>>+	int ret;
>>+
>>+	if (slot->state != POWERNV_PHP_SLOT_STATE_POPULATED)
>>+		return 0;
>>+
>>+	/* Remove all devices behind the slot */
>>+	pci_lock_rescan_remove();
>>+	pci_remove_pci_devices(slot->bus);
>>+	pci_unlock_rescan_remove();
>>+
>>+	/* Detach the child hotpluggable slots */
>>+	powernv_php_unregister(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 = php_slot->ops->get_power_status(php_slot, &power_status);
>>+	if (ret || power_status == POWERNV_PHP_SLOT_POWER_ON) {
>>+		ret = set_power_status(php_slot, POWERNV_PHP_SLOT_POWER_OFF);
>>+		if (ret)
>>+			dev_warn(&slot->pdev->dev, "Error %d powering off\n",
>>+				 ret);
>>+	}
>>+
>>+	/* Update slot state */
>>+	slot->state = POWERNV_PHP_SLOT_STATE_REGISTER;
>>+	return 0;
>>+}
>>+
>>+static struct hotplug_slot_ops php_slot_ops = {
>>+	.get_power_status	= get_power_status,
>>+	.get_adapter_status	= get_adapter_status,
>>+	.set_attention_status	= set_attention_status,
>>+	.enable_slot		= enable_slot,
>>+	.disable_slot		= disable_slot,
>>+};
>>+
>>+static struct powernv_php_slot *php_slot_match(struct device_node *dn,
>>+					       struct powernv_php_slot *slot)
>>+{
>>+	struct powernv_php_slot *target, *tmp;
>>+
>>+	if (slot->dn == dn)
>>+		return slot;
>>+
>>+	list_for_each_entry(tmp, &slot->children, link) {
>>+		target = php_slot_match(dn, tmp);
>>+		if (target)
>>+			return target;
>>+	}
>>+
>>+	return NULL;
>>+}
>>+
>>+struct powernv_php_slot *powernv_php_slot_find(struct device_node *dn)
>>+{
>>+	struct powernv_php_slot *slot, *tmp;
>>+	unsigned long flags;
>>+
>>+	spin_lock_irqsave(&php_slot_lock, flags);
>>+	list_for_each_entry(tmp, &php_slot_list, link) {
>>+		slot = php_slot_match(dn, tmp);
>>+		if (slot) {
>>+			spin_unlock_irqrestore(&php_slot_lock, flags);
>>+			return slot;
>>+		}
>>+	}
>>+	spin_unlock_irqrestore(&php_slot_lock, flags);
>>+
>>+	return NULL;
>>+}
>>+
>>+void powernv_php_slot_free(struct kref *kref)
>>+{
>>+	struct powernv_php_slot *slot = to_powernv_php_slot(kref);
>>+
>>+	WARN_ON(!list_empty(&slot->children));
>>+	kfree(slot->name);
>>+	kfree(slot);
>>+}
>>+
>>+static void php_slot_release(struct hotplug_slot *hp_slot)
>>+{
>>+	struct powernv_php_slot *slot = hp_slot->private;
>>+	unsigned long flags;
>>+
>>+	/* Remove from global or child list */
>>+	spin_lock_irqsave(&php_slot_lock, flags);
>>+	list_del(&slot->link);
>>+	spin_unlock_irqrestore(&php_slot_lock, flags);
>
>
>This is a good example where RCU rules. powernv_php_slot_find() returns slot
>pointer and its use is not protected by spin_lock -> dangerous.
>
>Remove spin_lock(), s/list_del/list_del_rcu/, and move bits below to
>call_rcu(), and s/list_for_each_entry/list_for_each_entry_rcu/.
>

Ok. It sounds RCU list might be better. I'll refactor it to use RCU list.

I think the spinlock is needed when adding one node to the RCU link list. If so,
the spinklock needn't to be removed?

>
>>+
>>+	/* Detach from parent */
>>+	powernv_php_slot_put(slot);
>>+	powernv_php_slot_put(slot->parent);
>>+}
>>+
>>+static bool php_slot_get_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);
>>+
>>+	/* Bus/Slot/Function number */
>>+	prop32 = of_get_property(dn, "reg", NULL);
>>+	if (!prop32)
>>+		return false;
>>+	*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 false;
>>+		}
>>+
>>+		*id |= be64_to_cpup(prop64);
>>+		of_node_put(parent);
>>+		return true;
>>+	}
>>+
>>+	return false;
>>+}
>>+
>>+struct powernv_php_slot *powernv_php_slot_alloc(struct device_node *dn)
>>+{
>>+	struct eeh_dev *edev = pdn_to_eeh_dev(PCI_DN(dn));
>>+	struct pci_bus *bus;
>>+	struct powernv_php_slot *slot;
>>+	const char *label;
>>+	uint64_t id;
>>+	int slot_no;
>>+	size_t size;
>>+	void *pmem;
>>+
>>+	/* Slot name */
>>+	label = of_get_property(dn, "ibm,slot-label", NULL);
>>+	if (!label)
>>+		return NULL;
>>+
>>+	/* Slot identifier */
>>+	if (!php_slot_get_id(dn, &id))
>>+		return NULL;
>>+
>>+	/* PCI bus */
>>+	bus = of_node_to_pci_bus(dn);
>>+	if (!bus)
>>+		return NULL;
>>+
>>+	/* Slot number */
>>+	if (dn->child && PCI_DN(dn->child))
>>+		slot_no = PCI_SLOT(PCI_DN(dn->child)->devfn);
>>+	else
>>+		slot_no = -1;
>
>Not INVALID_SLOT and
>#define INVALID_SLOT -1
>? :)
>

No need to do that. "-1" means it's a "placeholder" slot. None of
PCI devices will be attached with the slot. "-1" is defined by
PCI hotplug core and it doesn't have a macro yet.

>
>>+
>>+	/* Allocate slot */
>>+	size = sizeof(struct powernv_php_slot) +
>>+	       sizeof(struct hotplug_slot) +
>>+	       sizeof(struct hotplug_slot_info);
>>+	pmem = kzalloc(size, GFP_KERNEL);
>>+	if (!pmem) {
>>+		pr_warn("%s: Cannot allocate slot for node %s\n",
>>+			__func__, dn->full_name);
>>+		return NULL;
>>+	}
>>+
>>+	/* Assign memory blocks */
>>+	slot = pmem;
>>+	slot->php_slot = pmem + sizeof(struct powernv_php_slot);
>>+	slot->php_slot->info = pmem + sizeof(struct powernv_php_slot) +
>>+			      sizeof(struct hotplug_slot);
>>+	slot->name = kstrdup(label, GFP_KERNEL);
>>+	if (!slot->name) {
>>+		pr_warn("%s: Cannot populate name for node %s\n",
>>+			__func__, dn->full_name);
>>+		kfree(pmem);
>>+		return NULL;
>>+	}
>
>Why not just embed structs one to another?
>

Good point. I'll do.

>>+
>>+	/* Initialize slot */
>>+	kref_init(&slot->kref);
>>+	slot->state = POWERNV_PHP_SLOT_STATE_INIT;
>>+	slot->dn = dn;
>>+	slot->pdev = eeh_dev_to_pci_dev(edev);
>>+	slot->bus = bus;
>>+	slot->id = id;
>>+	slot->slot_no = slot_no;
>>+	INIT_WORK(&slot->work, powernv_php_slot_work);
>>+	init_waitqueue_head(&slot->queue);
>>+	slot->check_power_status = 0;
>>+	slot->status_confirmed = 0;
>>+	slot->php_slot->ops = &php_slot_ops;
>>+	slot->php_slot->release = php_slot_release;
>>+	slot->php_slot->private = slot;
>>+	INIT_LIST_HEAD(&slot->children);
>>+	INIT_LIST_HEAD(&slot->link);
>>+
>>+	return slot;
>>+}
>>+
>>+int powernv_php_slot_register(struct powernv_php_slot *slot)
>>+{
>>+	struct powernv_php_slot *parent;
>>+	struct device_node *dn = slot->dn;
>>+	unsigned long flags;
>>+	int ret;
>>+
>>+	/* Avoid register same slot for twice */
>>+	if (powernv_php_slot_find(slot->dn))
>>+		return -EEXIST;
>>+
>>+	/* Register slot */
>>+	ret = pci_hp_register(slot->php_slot, slot->bus,
>>+			      slot->slot_no, slot->name);
>>+	if (ret) {
>>+		dev_warn(&slot->pdev->dev, "Error %d registering slot\n",
>>+			 ret);
>>+		return ret;
>>+	}
>>+
>>+	/* Put into global or parent list */
>>+	while ((dn = of_get_parent(dn))) {
>>+		if (!PCI_DN(dn)) {
>>+			of_node_put(dn);
>>+			break;
>>+		}
>>+
>>+		parent = powernv_php_slot_find(dn);
>>+		if (parent) {
>>+			of_node_put(dn);
>>+			break;
>>+		}
>>+	}
>>+
>>+	spin_lock_irqsave(&php_slot_lock, flags);
>>+	if (parent) {
>>+		powernv_php_slot_get(parent);
>>+		slot->parent = parent;
>>+		list_add_tail(&slot->link, &parent->children);
>>+	} else {
>>+		list_add_tail(&slot->link, &php_slot_list);
>>+	}
>>+	spin_unlock_irqrestore(&php_slot_lock, flags);
>>+
>>+	/* Update slot state */
>>+	slot->state = POWERNV_PHP_SLOT_STATE_REGISTER;
>>+	return 0;
>>+}
>>
>
>
>Now I finished with this patchset respin :)
>

Ok. Appreciated for your time on this :-)

Thanks,
Gavin

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alexey Kardashevskiy Aug. 15, 2015, 9:15 a.m. UTC | #3

On 08/15/2015 02:47 PM, Gavin Shan wrote:
> On Sat, Aug 15, 2015 at 01:13:21PM +1000, Alexey Kardashevskiy wrote:
>> On 08/06/2015 02:11 PM, Gavin Shan wrote:
>>> The patch intends to add standalone driver to support PCI hotplug
>>> for PowerPC PowerNV platform, which runs on top of skiboot firmware.
>>> The firmware identified hotpluggable slots and marked their device
>>> tree node with proper "ibm,slot-pluggable" and "ibm,reset-by-firmware".
>>> The driver simply scans device-tree to create/register PCI hotplug slot
>>> accordingly.
>>>
>>> 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 <[e-mail]>
>>> Acked-by: Bjorn Helgaas <[e-mail]>
>>> ---
>>>   MAINTAINERS                            |   6 +
>>>   drivers/pci/hotplug/Kconfig            |  12 +
>>>   drivers/pci/hotplug/Makefile           |   4 +
>>>   drivers/pci/hotplug/powernv_php.c      | 140 +++++++
>>>   drivers/pci/hotplug/powernv_php.h      |  92 +++++
>>>   drivers/pci/hotplug/powernv_php_slot.c | 722 +++++++++++++++++++++++++++++++++
>>>   6 files changed, 976 insertions(+)
>>>   create mode 100644 drivers/pci/hotplug/powernv_php.c
>>>   create mode 100644 drivers/pci/hotplug/powernv_php.h
>>>   create mode 100644 drivers/pci/hotplug/powernv_php_slot.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index fd60784..3b75c92 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -7747,6 +7747,12 @@ L:	[e-mail]
>>>   S:	Supported
>>>   F:	Documentation/PCI/pci-error-recovery.txt
>>>
>>> +PCI HOTPLUG DRIVER FOR POWERNV PLATFORM
>>> +M:	Gavin Shan <[e-mail]>
>>> +L:	[e-mail]
>>> +S:	Supported
>>> +F:	drivers/pci/hotplug/powernv_php*
>>> +
>>>   PCI SUBSYSTEM
>>>   M:	Bjorn Helgaas <[e-mail]>
>>>   L:	[e-mail]
>>> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>>> index df8caec..ef55dae 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 powernv-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..fd51d65 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)	+= powernv-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,9 @@ ibmphp-objs		:=	ibmphp_core.o	\
>>>   acpiphp-objs		:=	acpiphp_core.o	\
>>>   				acpiphp_glue.o
>>>
>>> +powernv-php-objs	:=	powernv_php.o	\
>>> +				powernv_php_slot.o
>>> +
>>>   rpaphp-objs		:=	rpaphp_core.o	\
>>>   				rpaphp_pci.o	\
>>>   				rpaphp_slot.o
>>> diff --git a/drivers/pci/hotplug/powernv_php.c b/drivers/pci/hotplug/powernv_php.c
>>> new file mode 100644
>>> index 0000000..4cbff7a
>>> --- /dev/null
>>> +++ b/drivers/pci/hotplug/powernv_php.c
>>> @@ -0,0 +1,140 @@
>>> +/*
>>> + * 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/module.h>
>>> +
>>> +#include <asm/opal.h>
>>> +#include <asm/pnv-pci.h>
>>> +
>>> +#include "powernv_php.h"
>>> +
>>> +#define DRIVER_VERSION	"0.1"
>>> +#define DRIVER_AUTHOR	"Gavin Shan, IBM Corporation"
>>> +#define DRIVER_DESC	"PowerPC PowerNV PCI Hotplug Driver"
>>
>>
>> Align all or none.
>>
>
> All of them are already aligned well. Please check your email setting.

Right. tabs. My bad :)



>>> +
>>> +static struct notifier_block php_msg_nb = {
>>> +	.notifier_call	= powernv_php_msg_handler,
>>> +	.next		= NULL,
>>> +	.priority	= 0,
>>> +};
>>> +
>>> +static int powernv_php_register_one(struct device_node *dn)
>>> +{
>>> +	struct powernv_php_slot *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;
>>> +
>>> +	/* Allocate slot */
>>> +	slot = powernv_php_slot_alloc(dn);
>>> +	if (!slot)
>>> +		return -ENODEV;
>>> +
>>> +	/* Register it */
>>> +	ret = powernv_php_slot_register(slot);
>>> +	if (ret) {
>>> +		powernv_php_slot_put(slot);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return powernv_php_slot_enable(slot->php_slot, false);
>>
>>
>> And if it fails, no unregister and cleanup is needed?
>>
>
> You're right that it need care the failure cases. Will add something
> in next revision.
>
>>> +}
>>> +
>>> +int powernv_php_register(struct device_node *dn)
>>> +{
>>> +	struct device_node *child;
>>> +	int ret = 0;
>>
>> @ret is not used below.
>>
>
> Ok. will remove it.
>
>>> +
>>> +	/*
>>> +	 * The parent slots should be registered before their
>>> +	 * child slots.
>>> +	 */
>>> +	for_each_child_of_node(dn, child) {
>>> +		powernv_php_register_one(child);
>>> +		powernv_php_register(child);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void powernv_php_unregister_one(struct device_node *dn)
>>> +{
>>> +	struct powernv_php_slot *slot;
>>> +
>>> +	slot = powernv_php_slot_find(dn);
>>> +	if (!slot)
>>> +		return;
>>> +
>>> +	pci_hp_deregister(slot->php_slot);
>>> +}
>>> +
>>> +void powernv_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) {
>>> +		powernv_php_unregister(child);
>>> +		powernv_php_unregister_one(child);
>>> +	}
>>> +}
>>> +
>>> +static int __init powernv_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")
>>> +		powernv_php_register(dn);
>>> +	for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
>>> +		powernv_php_register(dn);
>>
>>
>> May be move pnv_pci_hotplug_notifier_register() after powernv_php_register()?
>> If not, then below (in powernv_php_exit()) move
>> pnv_pci_hotplug_notifier_unregister() to the end?
>>
>>
>
> Ok. I'll move pnv_pci_hotplug_notifier_unregister() to end of powernv_php_exit().


Sure? Just checking. If you do this, you will have situation when a 
notifier is enabled but there is no slot which does not seem dangerous but 
also not very accurate. I'd rather stop events from coming first, and then 
do cleanup.


>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void __exit powernv_php_exit(void)
>>> +{
>>> +	struct device_node *dn;
>>> +
>>> +	pnv_pci_hotplug_notifier_unregister(&php_msg_nb);
>>> +
>>> +	for_each_compatible_node(dn, NULL, "ibm,ioda-phb")
>>> +		powernv_php_unregister(dn);
>>> +	for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
>>> +		powernv_php_unregister(dn);
>>> +}
>>> +
>>> +module_init(powernv_php_init);
>>> +module_exit(powernv_php_exit);
>>> +
>>> +MODULE_VERSION(DRIVER_VERSION);
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>>> +MODULE_DESCRIPTION(DRIVER_DESC);
>>> diff --git a/drivers/pci/hotplug/powernv_php.h b/drivers/pci/hotplug/powernv_php.h
>>> new file mode 100644
>>> index 0000000..8034cc6
>>> --- /dev/null
>>> +++ b/drivers/pci/hotplug/powernv_php.h
>>> @@ -0,0 +1,92 @@
>>> +/*
>>> + * 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.
>>> + */
>>> +
>>> +#ifndef _POWERNV_PHP_H
>>> +#define _POWERNV_PHP_H
>>> +
>>> +#include <linux/list.h>
>>> +#include <linux/kref.h>
>>> +#include <linux/of.h>
>>> +#include <linux/pci.h>
>>> +#include <linux/pci_hotplug.h>
>>> +#include <linux/wait.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#include <asm/opal-api.h>
>>> +
>>> +/* Slot power status */
>>> +#define POWERNV_PHP_SLOT_POWER_OFF	0
>>> +#define POWERNV_PHP_SLOT_POWER_ON	1
>>> +
>>> +/* Slot presence status */
>>> +#define POWERNV_PHP_SLOT_EMPTY		0
>>> +#define POWERNV_PHP_SLOT_PRESENT	1
>>
>> These two are also only used in drivers/pci/hotplug/powernv_php_slot.c,
>> move them there at least. It also seems your PHP driver is the only one which
>> uses flags for an adapter status, others return plain 0 or 1 (which are
>> c-style "false" and "true", pretty much, so it is not the case of magic
>> constants). Since you return these values from the hotplug_slot_ops callbacks
>> to external code, you should probably do the same.
>>
>> And exactly the same comment about POWERNV_PHP_SLOT_POWER_ON/OFF few lines
>> above.
>>
>
> All those mcaroes are good enough

For the starter these macros are useless and should be removed, below is why ;)


> to be put in this header file since they
> are part of the slot's status.


They are part of the slot status. And the slot struct members should only 
be accessed by powernv_php_slot.c just because it is cleaner and easier to 
read/debug/support.


> Yes, they're only used in powernv_php_slot.c
> currently, but doesn't have to in future.

When/if this happens - you will move them to the header. But I seriously 
doubt this will ever happen.


> I don't see why I need change them
> to true/false, which just represents two states at most.

Because you use these macros to return status to pci_hotplug_core.c which 
is common code. Everybody else returns 0 and 1, this is an established API, 
if you implement hotplug_slot_ops, you should use what the caller expects, 
which is precisely 0 or 1. Readers of this code expect to see 0 and 1 too. 
If there were some macros defined in include/linux/pci_hotplug.h, you would 
use them but there is none (which is not very nice and probably needs 
fixing by introducing them across all PHB drivers but not in this patchset 
for sure).


> Here, all states
> are represented by numberic values (as POWERNV_PHP_SLOT_ATTEN_* as below).


You have different sets of states - some you report to pci_hotplug_core.c 
and you should follow pci_hotplug_core.c's rules/expectations; some are 
for/from OPAL, these you should define. It is not like "define everything" 
or "define nothing" :)


>
>>
>>
>>> +
>>> +/* Slot attention status */
>>> +#define POWERNV_PHP_SLOT_ATTEN_OFF	0
>>> +#define POWERNV_PHP_SLOT_ATTEN_ON	1
>>> +#define POWERNV_PHP_SLOT_ATTEN_IND	2
>>> +#define POWERNV_PHP_SLOT_ATTEN_ACT	3
>>
>>
>> These should go to drivers/pci/hotplug/powernv_php_slot.c. Where are these
>> flags defined? Looks to me like there is a way to pass some status from the
>> userspace via sysfs to OPAL so only OPAL is supposed to recognize and handle
>> these. If so, these macros are missing "OPAL" in their names. I have one more
>> comment below about it.
>>
>
> I prefer keep them in this header file as explained above. Those macros are
> not connected/passed to OPAL directly. All OPAL calls have corresponding
> wrappers in arch/powerpc/platforms/powernv/pci.c.


aaaand below you agreed to remove them for now so I'll skip this comment :)


>>> +
>>> +struct powernv_php_slot {
>>> +	char			*name;
>>> +	struct device_node	*dn;
>>> +	struct pci_dev		*pdev;
>>> +	struct pci_bus		*bus;
>>> +	uint64_t		id;
>>> +	int			slot_no;
>>> +	struct kref		kref;
>>> +#define POWERNV_PHP_SLOT_STATE_INIT		0
>>> +#define POWERNV_PHP_SLOT_STATE_REGISTER		1
>>> +#define POWERNV_PHP_SLOT_STATE_POPULATED	2
>>> +	int			state;
>>> +	int			check_power_status;
>>> +	int			status_confirmed;
>>
>> s/status_confirmed/power_status_confirmed/
>>
>
> Maybe, I think "status_confirmed" is enough here.
>
>> What is this status? It can be 0, 1, 2 which seems to be
>> UNCONFIRMED/INPROGRESS/CONFIRMED (does not need PNV/IODA prefixes as it is
>> local to the powernv_php_slot.c file).
>>
>
> No, it only has two states: 0/1.

No it is not. slot_power_on_handler() puts "2" to local variable @status 
and then stores it into slot->status_confirmed:


127 static void slot_power_on_handler(struct powernv_php_slot *slot)
[...]

190 free_fdt:
191         kfree(fdt);
192         status = 2;
193 out:
194         /* Confirm status change */
195         slot->status_confirmed = status;
196         wake_up_interruptible(&slot->queue);
197 }



> Again, it's connected with any OPAL
> calls directly. No need to have "IODA_" prefix at all.

Sorry, I do not follow you here.


>
>>
>>> +	struct opal_msg		*msg;
>>> +	void			*fdt;
>>> +	void			*dt;
>>> +	struct of_changeset	ocs;
>>> +	struct work_struct	work;
>>> +	wait_queue_head_t	queue;
>>> +	struct hotplug_slot	*php_slot;
>>> +	struct powernv_php_slot	*parent;
>>> +	struct list_head	children;
>>> +	struct list_head	link;
>>> +};
>>
>> This should go to drivers/pci/hotplug/powernv_php_slot.c and this header
>> should only have a forward declaration. After you move it there, you get
>> better separation of the driver code from the slot code and only 2 changes
>> will be needed:
>>
>> 1. powernv_php_slot_enable() should receive powernv_php_slot
>> 2. add powernv_php_slot_unregister() (like powernv_php_slot_register()), this
>> way you will have pairing pci_hp_register/pci_hp_deregister in the same file.
>>
>>
>> After you moved this struct to the source file, you could remove/shorten
>> POWERNV_PHP_SLOT_STATE_ prefixes if you wished.
>>
>
> I don't see why it should go to drivers/pci/hotplug/powernv_php_slot.c. it's
> the core data structure shared by multiple source files.


It is not shared - this is my point. The _only_ shared data is a php_slot 
pointer, the rest is never used outside of powernv_php_slot.c and even this 
does not need to be shared - and I above explained how.

powernv_php_slot.c handles a single slot. powernv_php.c handles all slots. 
That is the distinction and it is better to follow it while writing the 
code. It is always a good thing to restrict struct members visibility to a 
single file.

If you do not want to draw precise line between these 2 files (which is 
easy in this soecific case), just merge them into one.

btw #2 needs to be done in any case.



>>> +
>>> +int powernv_php_msg_handler(struct notifier_block *nb,
>>> +			    unsigned long type, void *message);
>>> +struct powernv_php_slot *powernv_php_slot_find(struct device_node *dn);
>>> +void powernv_php_slot_free(struct kref *kref);
>>> +struct powernv_php_slot *powernv_php_slot_alloc(struct device_node *dn);
>>> +int powernv_php_slot_register(struct powernv_php_slot *slot);
>>> +int powernv_php_slot_enable(struct hotplug_slot *php_slot, bool rescan);
>>> +int powernv_php_register(struct device_node *dn);
>>> +void powernv_php_unregister(struct device_node *dn);
>>> +
>>> +#define to_powernv_php_slot(kref) \
>>> +	container_of(kref, struct powernv_php_slot, kref)
>>> +
>>> +static inline void powernv_php_slot_get(struct powernv_php_slot *slot)
>>> +{
>>> +	if (slot)
>>> +		kref_get(&slot->kref);
>>> +}
>>> +
>>> +static inline int powernv_php_slot_put(struct powernv_php_slot *slot)
>>> +{
>>> +	if (slot)
>>> +		return kref_put(&slot->kref, powernv_php_slot_free);
>>> +
>>> +	return 0;
>>> +}
>>
>> In these 2 helpers you do not have to check for @slof - it is checked in the
>> callers pretty much always. Or it is not checked in php_slot_release() but
>> dereferenced before you call powernv_php_slot_put(slot).
>>
>> The only place you really want this check is
>> powernv_php_slot_put(slot->parent) so just check it there. btw is it even
>> possible for the slot not to have a parent?
>>
>
> For most cases, slot doesn't have parent. That's why I had the check here.
> Yes, if you really want me to drop the check, then I have to check slot's
> validation in the callers.

You already do this in all places except one. If you did not - then you 
could argue that it is useful but you do check already (except one case).

> Also, those helpers are inline functions, no
> too much difference actually.

Harder to read.

>> So I'd ditch these helpers.
>>
>
> I don't see the reason why these helpers need to be dropped. To use kref_{get,put}
> directly?

Yes.


>
>>> +
>>> +#endif /* !_POWERNV_PHP_H */
>>> diff --git a/drivers/pci/hotplug/powernv_php_slot.c b/drivers/pci/hotplug/powernv_php_slot.c
>>> new file mode 100644
>>> index 0000000..73a93a2
>>> --- /dev/null
>>> +++ b/drivers/pci/hotplug/powernv_php_slot.c
>>> @@ -0,0 +1,722 @@
>>> +/*
>>> + * 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/module.h>
>>> +
>>> +#include <asm/opal.h>
>>> +#include <asm/pnv-pci.h>
>>> +#include <asm/ppc-pci.h>
>>> +
>>> +#include "powernv_php.h"
>>> +
>>> +static LIST_HEAD(php_slot_list);
>>> +static DEFINE_SPINLOCK(php_slot_lock);
>>> +
>>> +/*
>>> + * Remove firmware data for all child device nodes of the
>>> + * indicated one.
>>> + */
>>> +static void remove_child_pdn(struct device_node *np)
>>> +{
>>> +	struct device_node *child;
>>> +
>>> +	for_each_child_of_node(np, child) {
>>> +		/* In depth first */
>>> +		remove_child_pdn(child);
>>> +
>>> +		remove_pci_device_node_info(child);
>>> +	}
>>> +}
>>> +
>>> +/*
>>> + * Remove all subordinate device nodes of the indicated one.
>>> + * Those device nodes in deepest path should be released firstly.
>>> + */
>>> +static int remove_child_device_nodes(struct device_node *parent)
>>> +{
>>> +	struct device_node *np, *child;
>>> +	int ret = 0;
>>> +
>>> +	/* If the device node has children, remove them firstly */
>>> +	for_each_child_of_node(parent, np) {
>>> +		ret = remove_child_device_nodes(np);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		/* The device shouldn't have alive children */
>>> +		child = of_get_next_child(np, NULL);
>>> +		if (child) {
>>> +			of_node_put(child);
>>> +			of_node_put(np);
>>> +			pr_err("%s: Alive children of node <%s>\n",
>>> +			       __func__, of_node_full_name(np));
>>> +			return -EBUSY;
>>> +		}
>>> +
>>> +		/* Detach the device node */
>>> +		of_detach_node(np);
>>> +		of_node_put(np);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * The function processes the message sent by firmware
>>> + * to remove all device tree nodes beneath the slot's
>>> + * nodes, and the associated auxillary data.
>>> + */
>>> +static void slot_power_off_handler(struct powernv_php_slot *slot)
>>> +{
>>> +	int ret, status = 1;
>>> +
>>> +	/* Release the firmware data for the child device nodes */
>>> +	remove_child_pdn(slot->dn);
>>> +
>>> +	/*
>>> +	 * Release the child device nodes. If the sub-tree was
>>> +	 * built with the help of changeset, we just need destroy
>>> +	 * the changes.
>>> +	 */
>>> +	if (slot->fdt) {
>>> +		of_changeset_destroy(&slot->ocs);
>>> +		kfree(slot->dt);
>>> +		slot->dt = NULL;
>>> +		slot->dn->child = NULL;
>>> +		kfree(slot->fdt);
>>> +		slot->fdt = NULL;
>>> +	} else {
>>> +		ret = remove_child_device_nodes(slot->dn);
>>> +		if (ret) {
>>> +			status = 2;
>>> +			dev_warn(&slot->pdev->dev, "Error %d freeing nodes\n",
>>> +				 ret);
>>> +		}
>>> +	}
>>> +
>>> +	/* Confirm status change */
>>> +	slot->status_confirmed = status;
>>> +	wake_up_interruptible(&slot->queue);
>>> +}
>>> +
>>> +static int slot_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)
>>> +			return ret;
>>> +
>>> +		ret = slot_populate_changeset(ocs, child);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void slot_power_on_handler(struct powernv_php_slot *slot)
>>> +{
>>> +	void *fdt, *dt;
>>> +	uint64_t len;
>>> +	int ret, status = 1;
>>> +
>>> +	/* We don't know the FDT blob size. It tries with incremental
>>> +	 * sized memory chunk.
>>> +	 */
>>
>>
>> What is the real expected size? 0x10000 is just 64K, just allocate it and
>> that's it.
>>
>
> You can not know the size in advance. It depends on the size of the FDT
> blob to be transfered. So the size has to be probed.

May be this is common practice with OPAL or linux, I do not know.

Usually when people implement API call which returns a blob and a size, it 
also has a way to signal that the buffer is not big enough and say what 
size is enough

And then the client would call it with blob=NULL (or small buf size, like a 
minumum device tree header which may have size already, do not know details 
so cannot tell exactly), a positive return value would be a required size - 
if it is bigger than the buffer size - you increase the buffer size, 
reallocate the blob and call again; if it is equal or less - then you got 
your tree completely already.

I am telling you all this because I cannot see OPAL_GET_DEVICE_TREE in 
https://github.com/open-power/hostboot so I assume it has not been pushed 
there yet and therefore can be fixed.



>
>>> +	for (len = 0x2000; len <= 0x10000; len += 0x2000) {
>>> +		fdt = kzalloc(len, GFP_KERNEL);
>>> +		if (!fdt)
>>> +			break;
>>> +
>>> +		ret = pnv_pci_get_device_tree(slot->dn->phandle, fdt, len);
>>> +		if (!ret)
>>> +			break;
>>> +
>>> +		kfree(fdt);
>>> +	}
>>> +
>>> +	if (len > 0x10000) {
>>> +		dev_warn(&slot->pdev->dev, "Cannot alloc FDT blob\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* Unflatten device tree blob */
>>> +	dt = of_fdt_unflatten_tree(fdt, slot->dn, NULL);
>>> +	if (!dt) {
>>> +		dev_warn(&slot->pdev->dev, "Cannot unflatten FDT\n");
>>> +		goto free_fdt;
>>> +	}
>>
>> Right here you could kfree(fdt) and not cache it in the slot struct at all.
>> You do not use it later anyway.
>>
>
> The "fdt" has been cached at end of this function and it can't be released
> because the unflattened device-tree is still using data in the FDT blob (@fdt).

Oh. Did not know that. Ok.


>
>>
>>> +
>>> +	/* Initialize and apply the changeset */
>>> +	of_changeset_init(&slot->ocs);
>>> +	ret = slot_populate_changeset(&slot->ocs, slot->dn);
>>> +	if (ret) {
>>> +		dev_warn(&slot->pdev->dev, "Error %d populating changeset\n",
>>> +			 ret);
>>> +		goto free_dt;
>>> +	}
>>> +
>>> +	slot->dn->child = NULL;
>>> +	ret = of_changeset_apply(&slot->ocs);
>>> +	if (ret) {
>>> +		dev_warn(&slot->pdev->dev, "Error %d applying changeset\n",
>>> +			 ret);
>>> +		goto destroy_changeset;
>>> +	}
>>> +
>>> +	/* Add device node firmware data */
>>> +	traverse_pci_device_nodes(slot->dn,
>>> +				  add_pci_device_node_info,
>>> +				  pci_bus_to_host(slot->bus));
>>> +	slot->fdt = fdt;
>>> +	slot->dt = dt;
>>> +	goto out;
>>> +
>>> +destroy_changeset:
>>> +	of_changeset_destroy(&slot->ocs);
>>> +free_dt:
>>> +	kfree(dt);
>>> +	slot->dn->child = NULL;
>>
>> Can of_fdt_unflatten_tree() or of_changeset_init() or
>> slot_populate_changeset() initialize dn->child? No kfree(slot->dn->child)?
>>
>
> of_fdt_unflatten_tree() did. @dt is the unflattened device-tree here.
> No need to free slot->dn->child.

Ok, good.


>
>>> +free_fdt:
>>> +	kfree(fdt);
>>> +	status = 2;
>>> +out:
>>> +	/* Confirm status change */
>>> +	slot->status_confirmed = status;
>>> +	wake_up_interruptible(&slot->queue);
>>> +}
>>> +
>>> +static void powernv_php_slot_work(struct work_struct *data)
>>> +{
>>> +	struct powernv_php_slot *slot = container_of(data,
>>> +						     struct powernv_php_slot,
>>> +						     work);
>>> +	uint64_t php_event = be64_to_cpu(slot->msg->params[0]);
>>> +
>>> +	switch (php_event) {
>>> +	case 0: /* Slot power off */
>>> +		slot_power_off_handler(slot);
>>> +		break;
>>> +	case 1: /* Slot power on */
>>> +		slot_power_on_handler(slot);
>>> +		break;
>>
>> These 0 and 1 are not the same 0 and 1 used for @val in get_power_status(),
>> these are from OPAL so please define them.
>>
>
> Good point. I'll have following macros for them in opal-api.h:
>
> #define OPAL_PCI_PHP_EVENT_POWER_OFF	0
> #define OPAL_PCI_PHP_EVENT_POWER_ON	1
>
> Or to use "enum" type.
>
>>
>>> +	default:
>>> +		dev_warn(&slot->pdev->dev, "Unsupported hotplug event %lld\n",
>>> +			 php_event);
>>> +	}
>>> +
>>> +	of_node_put(slot->dn);
>>> +}
>>> +
>>> +int powernv_php_msg_handler(struct notifier_block *nb,
>>> +			    unsigned long type, void *message)
>>> +{
>>> +	phandle h;
>>> +	struct device_node *np;
>>> +	struct powernv_php_slot *slot;
>>> +	struct opal_msg *msg = message;
>>> +
>>> +	/* Check the message type */
>>> +	if (type != OPAL_MSG_PCI_HOTPLUG) {
>>> +		pr_warn("%s: Wrong message type %ld received!\n",
>>> +			__func__, type);
>>> +		return NOTIFY_DONE;
>>> +	}
>>> +
>>> +	/* Find the device node */
>>> +	h = (phandle)be64_to_cpu(msg->params[1]);
>>> +	np = of_find_node_by_phandle(h);
>>> +	if (!np) {
>>> +		pr_warn("%s: No device node for phandle 0x%08x\n",
>>> +			__func__, h);
>>> +		return NOTIFY_DONE;
>>> +	}
>>> +
>>> +	/* Find the slot */
>>> +	slot = powernv_php_slot_find(np);
>>> +	if (!slot) {
>>> +		pr_warn("%s: No slot found for node <%s>\n",
>>> +			__func__, of_node_full_name(np));
>>> +		of_node_put(np);
>>> +		return NOTIFY_DONE;
>>> +	}
>>> +
>>> +	/* Schedule the work */
>>> +	slot->msg = msg;
>>> +	schedule_work(&slot->work);
>>> +	return NOTIFY_OK;
>>> +}
>>
>> This function belongs to drivers/pci/hotplug/powernv_php.c (searching a slot
>> is powernv_php.c's scope) except these lines:
>>
>>> +	/* Schedule the work */
>>> +	slot->msg = msg;
>>> +	schedule_work(&slot->work);
>>
>> These 3 lines should be in helper in drivers/pci/hotplug/powernv_php_slot.c.
>>
>
> It's no point to drop one helper and add another one. One thing I keep
> in mind when writing the code: all hotplug logic (interacting with
> OPAL firmware) is implemented in powernv_php_slot.c and powernv_php.c
> just connect the helper functions with event.


As I said - powernv_php_slot.c handles a single slot and powernv_php.c 
handles all slots so it makes sense to keep all operations which do not 
access slot data in powernv_php.c  and  move operations which access slot 
internal data to powernv_php_slot.c.


>>> +
>>> +static int set_power_status(struct hotplug_slot *php_slot, u8 val)
>>> +{
>>> +	struct powernv_php_slot *slot = php_slot->private;
>>> +	int ret;
>>> +
>>> +	/* Set power status */
>>> +	slot->status_confirmed = 0;
>>> +	ret = pnv_pci_set_power_status(slot->id, val);
>>> +	if (ret) {
>>> +		dev_warn(&slot->pdev->dev, "Error %d powering %s slot\n",
>>> +			 ret, val ? "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 always waiting for that.
>>> +	 */
>>> +	if (slot->status_confirmed == 1)
>>> +		return 0;
>>> +	else if (slot->status_confirmed > 0)
>>> +		return -EBUSY;
>>> +
>>> +	ret = wait_event_timeout(slot->queue, slot->status_confirmed, 10 * HZ);
>>> +	if (!ret) {
>>> +		dev_warn(&slot->pdev->dev, "Error %d waiting for power-%s\n",
>>> +			 ret, val ? "on" : "off");
>>> +		return -EBUSY;
>>> +	}
>>> +
>>> +	/* Check the result */
>>> +	if (slot->status_confirmed == 1)
>>> +		return 0;
>>> +
>>> +	dev_warn(&slot->pdev->dev, "Error status %d for power-%s\n",
>>> +		 slot->status_confirmed, val ? "on" : "off");
>>> +	return -EBUSY;
>>> +}
>>> +
>>> +static int get_power_status(struct hotplug_slot *php_slot, u8 *val)
>>> +{
>>> +	struct powernv_php_slot *slot = php_slot->private;
>>> +	uint8_t state;
>>> +	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_status(slot->id, &state);
>>> +	if (ret) {
>>> +		*val = POWERNV_PHP_SLOT_POWER_ON;
>>> +		dev_warn(&slot->pdev->dev, "Error %d getting power status\n",
>>> +			 ret);
>>> +	} else {
>>> +		*val = state ? POWERNV_PHP_SLOT_POWER_ON :
>>> +			       POWERNV_PHP_SLOT_POWER_OFF;
>>> +		php_slot->info->power_status = *val;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int get_adapter_status(struct hotplug_slot *php_slot, u8 *val)
>>> +{
>>> +	struct powernv_php_slot *slot = php_slot->private;
>>> +	uint8_t state;
>>> +	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_status(slot->id, &state);
>>> +	if (ret >= 0) {
>>> +		ret = 0;
>>> +		*val = state ? POWERNV_PHP_SLOT_PRESENT :
>>> +			       POWERNV_PHP_SLOT_EMPTY;
>>> +		php_slot->info->adapter_status = *val;
>>> +		ret = 0;
>>> +	} else {
>>> +		*val = POWERNV_PHP_SLOT_EMPTY;
>>> +		dev_warn(&slot->pdev->dev, "Error %d getting presence\n",
>>> +			 ret);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int set_attention_status(struct hotplug_slot *php_slot, u8 val)
>>> +{
>>> +	struct powernv_php_slot *slot = php_slot->private;
>>> +
>>> +	/* The default operation would to turn on the attention */
>>> +	switch (val) {
>>> +	case POWERNV_PHP_SLOT_ATTEN_OFF:
>>> +	case POWERNV_PHP_SLOT_ATTEN_ON:
>>> +	case POWERNV_PHP_SLOT_ATTEN_IND:
>>> +	case POWERNV_PHP_SLOT_ATTEN_ACT:
>>> +		break;
>>> +	default:
>>> +		dev_warn(&slot->pdev->dev, "Invalid attention %d\n", val);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	/* FIXME: Make it real once firmware supports it */
>>> +	php_slot->info->attention_status = val;
>>
>> Since firmware does not have an idea about these POWERNV_PHP_SLOT_ATTEN_xxx,
>> just remove them. Later when the firmware will know about them, we will have
>> to change this code anyway and by that time, the set of states may have
>> changed.
>>
>
> Sure, will do.
>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int powernv_php_slot_enable(struct hotplug_slot *php_slot, bool rescan)
>>
>>
>> This should receive powernv_php_slot as described above.
>>
>
> Good point, I'll change accordingly.
>
>>> +{
>>> +	struct powernv_php_slot *slot = php_slot->private;
>>> +	uint8_t presence, power_status;
>>> +	int ret;
>>> +
>>> +	/* Check if the slot has been configured */
>>> +	if (slot->state != POWERNV_PHP_SLOT_STATE_REGISTER)
>>> +		return 0;
>>> +
>>> +	/* Retrieve slot presence status */
>>> +	ret = php_slot->ops->get_adapter_status(php_slot, &presence);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Proceed if there have nothing behind the slot */
>>> +	if (presence == POWERNV_PHP_SLOT_EMPTY)
>>> +		goto scan;
>>> +
>>> +	/*
>>> +	 * If we don't detect something behind the slot, we need
>>> +	 * make sure the power suply to the slot is on. Otherwise,
>>> +	 * the slot downstream PCIe linkturn should be down.
>>> +	 *
>>> +	 * 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 (!slot->check_power_status) {
>>> +		slot->check_power_status = 1;
>>> +		goto scan;
>>> +	}
>>> +
>>> +	/* Check the power status. Scan the slot if that's already on */
>>> +	ret = php_slot->ops->get_power_status(php_slot, &power_status);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (power_status == POWERNV_PHP_SLOT_POWER_ON)
>>> +		goto scan;
>>> +
>>> +	/* Power is off, turn it on and then scan the slot */
>>> +	ret = set_power_status(php_slot, POWERNV_PHP_SLOT_POWER_ON);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +scan:
>>> +	switch (presence) {
>>> +	case POWERNV_PHP_SLOT_PRESENT:
>>> +		if (rescan) {
>>> +			pci_lock_rescan_remove();
>>> +			pci_add_pci_devices(slot->bus);
>>> +			pci_unlock_rescan_remove();
>>> +		}
>>> +
>>> +		/* Rescan for child hotpluggable slots */
>>> +		slot->state = POWERNV_PHP_SLOT_STATE_POPULATED;
>>> +		if (rescan)
>>> +			powernv_php_register(slot->dn);
>>> +		break;
>>> +	case POWERNV_PHP_SLOT_EMPTY:
>>> +		slot->state = POWERNV_PHP_SLOT_STATE_POPULATED;
>>> +		break;
>>> +	default:
>>> +		dev_warn(&slot->pdev->dev, "Invalid presence status %d\n",
>>> +			 presence);
>>> +		return -EINVAL;
>>
>> Neigher PHP driver will ever have presence other than 0 or 1. So this
>> switch() is simple if(presence){}else{}.
>>
>
> Ok. Will use "if () {} else {}" then.
>
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int enable_slot(struct hotplug_slot *php_slot)
>>> +{
>>> +	return powernv_php_slot_enable(php_slot, true);
>>> +}
>>> +
>>> +static int disable_slot(struct hotplug_slot *php_slot)
>>> +{
>>> +	struct powernv_php_slot *slot = php_slot->private;
>>> +	uint8_t power_status;
>>> +	int ret;
>>> +
>>> +	if (slot->state != POWERNV_PHP_SLOT_STATE_POPULATED)
>>> +		return 0;
>>> +
>>> +	/* Remove all devices behind the slot */
>>> +	pci_lock_rescan_remove();
>>> +	pci_remove_pci_devices(slot->bus);
>>> +	pci_unlock_rescan_remove();
>>> +
>>> +	/* Detach the child hotpluggable slots */
>>> +	powernv_php_unregister(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 = php_slot->ops->get_power_status(php_slot, &power_status);
>>> +	if (ret || power_status == POWERNV_PHP_SLOT_POWER_ON) {
>>> +		ret = set_power_status(php_slot, POWERNV_PHP_SLOT_POWER_OFF);
>>> +		if (ret)
>>> +			dev_warn(&slot->pdev->dev, "Error %d powering off\n",
>>> +				 ret);
>>> +	}
>>> +
>>> +	/* Update slot state */
>>> +	slot->state = POWERNV_PHP_SLOT_STATE_REGISTER;
>>> +	return 0;
>>> +}
>>> +
>>> +static struct hotplug_slot_ops php_slot_ops = {
>>> +	.get_power_status	= get_power_status,
>>> +	.get_adapter_status	= get_adapter_status,
>>> +	.set_attention_status	= set_attention_status,
>>> +	.enable_slot		= enable_slot,
>>> +	.disable_slot		= disable_slot,
>>> +};
>>> +
>>> +static struct powernv_php_slot *php_slot_match(struct device_node *dn,
>>> +					       struct powernv_php_slot *slot)
>>> +{
>>> +	struct powernv_php_slot *target, *tmp;
>>> +
>>> +	if (slot->dn == dn)
>>> +		return slot;
>>> +
>>> +	list_for_each_entry(tmp, &slot->children, link) {
>>> +		target = php_slot_match(dn, tmp);
>>> +		if (target)
>>> +			return target;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +struct powernv_php_slot *powernv_php_slot_find(struct device_node *dn)
>>> +{
>>> +	struct powernv_php_slot *slot, *tmp;
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&php_slot_lock, flags);
>>> +	list_for_each_entry(tmp, &php_slot_list, link) {
>>> +		slot = php_slot_match(dn, tmp);
>>> +		if (slot) {
>>> +			spin_unlock_irqrestore(&php_slot_lock, flags);
>>> +			return slot;
>>> +		}
>>> +	}
>>> +	spin_unlock_irqrestore(&php_slot_lock, flags);
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +void powernv_php_slot_free(struct kref *kref)
>>> +{
>>> +	struct powernv_php_slot *slot = to_powernv_php_slot(kref);
>>> +
>>> +	WARN_ON(!list_empty(&slot->children));
>>> +	kfree(slot->name);
>>> +	kfree(slot);
>>> +}
>>> +
>>> +static void php_slot_release(struct hotplug_slot *hp_slot)
>>> +{
>>> +	struct powernv_php_slot *slot = hp_slot->private;
>>> +	unsigned long flags;
>>> +
>>> +	/* Remove from global or child list */
>>> +	spin_lock_irqsave(&php_slot_lock, flags);
>>> +	list_del(&slot->link);
>>> +	spin_unlock_irqrestore(&php_slot_lock, flags);
>>
>>
>> This is a good example where RCU rules. powernv_php_slot_find() returns slot
>> pointer and its use is not protected by spin_lock -> dangerous.
>>
>> Remove spin_lock(), s/list_del/list_del_rcu/, and move bits below to
>> call_rcu(), and s/list_for_each_entry/list_for_each_entry_rcu/.
>>
>
> Ok. It sounds RCU list might be better. I'll refactor it to use RCU list.
>
> I think the spinlock is needed when adding one node to the RCU link list. If so,
> the spinklock needn't to be removed?

Yes, spin_lock() is still needed when adding.


>
>>
>>> +
>>> +	/* Detach from parent */
>>> +	powernv_php_slot_put(slot);
>>> +	powernv_php_slot_put(slot->parent);
>>> +}
>>> +
>>> +static bool php_slot_get_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);
>>> +
>>> +	/* Bus/Slot/Function number */
>>> +	prop32 = of_get_property(dn, "reg", NULL);
>>> +	if (!prop32)
>>> +		return false;
>>> +	*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 false;
>>> +		}
>>> +
>>> +		*id |= be64_to_cpup(prop64);
>>> +		of_node_put(parent);
>>> +		return true;
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>>> +struct powernv_php_slot *powernv_php_slot_alloc(struct device_node *dn)
>>> +{
>>> +	struct eeh_dev *edev = pdn_to_eeh_dev(PCI_DN(dn));
>>> +	struct pci_bus *bus;
>>> +	struct powernv_php_slot *slot;
>>> +	const char *label;
>>> +	uint64_t id;
>>> +	int slot_no;
>>> +	size_t size;
>>> +	void *pmem;
>>> +
>>> +	/* Slot name */
>>> +	label = of_get_property(dn, "ibm,slot-label", NULL);
>>> +	if (!label)
>>> +		return NULL;
>>> +
>>> +	/* Slot identifier */
>>> +	if (!php_slot_get_id(dn, &id))
>>> +		return NULL;
>>> +
>>> +	/* PCI bus */
>>> +	bus = of_node_to_pci_bus(dn);
>>> +	if (!bus)
>>> +		return NULL;
>>> +
>>> +	/* Slot number */
>>> +	if (dn->child && PCI_DN(dn->child))
>>> +		slot_no = PCI_SLOT(PCI_DN(dn->child)->devfn);
>>> +	else
>>> +		slot_no = -1;
>>
>> Not INVALID_SLOT and
>> #define INVALID_SLOT -1
>> ? :)
>>
>
> No need to do that. "-1" means it's a "placeholder" slot. None of
> PCI devices will be attached with the slot. "-1" is defined by
> PCI hotplug core

No, not really. drivers/pci/hotplug/pci_hotplug_core.c does not have "-1". 
Where is defined then? And this "slot_no" is a member of powernv_php_slot, 
not hotplug_slot.


> and it doesn't have a macro yet.
>
>>
>>> +
>>> +	/* Allocate slot */
>>> +	size = sizeof(struct powernv_php_slot) +
>>> +	       sizeof(struct hotplug_slot) +
>>> +	       sizeof(struct hotplug_slot_info);
>>> +	pmem = kzalloc(size, GFP_KERNEL);
>>> +	if (!pmem) {
>>> +		pr_warn("%s: Cannot allocate slot for node %s\n",
>>> +			__func__, dn->full_name);
>>> +		return NULL;
>>> +	}
>>> +
>>> +	/* Assign memory blocks */
>>> +	slot = pmem;
>>> +	slot->php_slot = pmem + sizeof(struct powernv_php_slot);
>>> +	slot->php_slot->info = pmem + sizeof(struct powernv_php_slot) +
>>> +			      sizeof(struct hotplug_slot);
>>> +	slot->name = kstrdup(label, GFP_KERNEL);
>>> +	if (!slot->name) {
>>> +		pr_warn("%s: Cannot populate name for node %s\n",
>>> +			__func__, dn->full_name);
>>> +		kfree(pmem);
>>> +		return NULL;
>>> +	}
>>
>> Why not just embed structs one to another?
>>
>
> Good point. I'll do.
>
>>> +
>>> +	/* Initialize slot */
>>> +	kref_init(&slot->kref);
>>> +	slot->state = POWERNV_PHP_SLOT_STATE_INIT;
>>> +	slot->dn = dn;
>>> +	slot->pdev = eeh_dev_to_pci_dev(edev);
>>> +	slot->bus = bus;
>>> +	slot->id = id;
>>> +	slot->slot_no = slot_no;
>>> +	INIT_WORK(&slot->work, powernv_php_slot_work);
>>> +	init_waitqueue_head(&slot->queue);
>>> +	slot->check_power_status = 0;
>>> +	slot->status_confirmed = 0;
>>> +	slot->php_slot->ops = &php_slot_ops;
>>> +	slot->php_slot->release = php_slot_release;
>>> +	slot->php_slot->private = slot;
>>> +	INIT_LIST_HEAD(&slot->children);
>>> +	INIT_LIST_HEAD(&slot->link);
>>> +
>>> +	return slot;
>>> +}
>>> +
>>> +int powernv_php_slot_register(struct powernv_php_slot *slot)
>>> +{
>>> +	struct powernv_php_slot *parent;
>>> +	struct device_node *dn = slot->dn;
>>> +	unsigned long flags;
>>> +	int ret;
>>> +
>>> +	/* Avoid register same slot for twice */
>>> +	if (powernv_php_slot_find(slot->dn))
>>> +		return -EEXIST;
>>> +
>>> +	/* Register slot */
>>> +	ret = pci_hp_register(slot->php_slot, slot->bus,
>>> +			      slot->slot_no, slot->name);
>>> +	if (ret) {
>>> +		dev_warn(&slot->pdev->dev, "Error %d registering slot\n",
>>> +			 ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* Put into global or parent list */
>>> +	while ((dn = of_get_parent(dn))) {
>>> +		if (!PCI_DN(dn)) {
>>> +			of_node_put(dn);
>>> +			break;
>>> +		}
>>> +
>>> +		parent = powernv_php_slot_find(dn);
>>> +		if (parent) {
>>> +			of_node_put(dn);
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	spin_lock_irqsave(&php_slot_lock, flags);
>>> +	if (parent) {
>>> +		powernv_php_slot_get(parent);
>>> +		slot->parent = parent;
>>> +		list_add_tail(&slot->link, &parent->children);
>>> +	} else {
>>> +		list_add_tail(&slot->link, &php_slot_list);
>>> +	}
>>> +	spin_unlock_irqrestore(&php_slot_lock, flags);
>>> +
>>> +	/* Update slot state */
>>> +	slot->state = POWERNV_PHP_SLOT_STATE_REGISTER;
>>> +	return 0;
>>> +}
>>>
>>
>>
>> Now I finished with this patchset respin :)
>>
>
> Ok. Appreciated for your time on this :-)

you're welcome :)

And since Bjorn ack'ed this patch already, you can probably ditch all my 
comments, his ack is cooler anyway :)

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index fd60784..3b75c92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7747,6 +7747,12 @@  L:	[e-mail]
 S:	Supported
 F:	Documentation/PCI/pci-error-recovery.txt
 
+PCI HOTPLUG DRIVER FOR POWERNV PLATFORM
+M:	Gavin Shan <[e-mail]>
+L:	[e-mail]
+S:	Supported
+F:	drivers/pci/hotplug/powernv_php*
+
 PCI SUBSYSTEM
 M:	Bjorn Helgaas <[e-mail]>
 L:	[e-mail]
diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
index df8caec..ef55dae 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 powernv-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..fd51d65 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)	+= powernv-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,9 @@  ibmphp-objs		:=	ibmphp_core.o	\
 acpiphp-objs		:=	acpiphp_core.o	\
 				acpiphp_glue.o
 
+powernv-php-objs	:=	powernv_php.o	\
+				powernv_php_slot.o
+
 rpaphp-objs		:=	rpaphp_core.o	\
 				rpaphp_pci.o	\
 				rpaphp_slot.o
diff --git a/drivers/pci/hotplug/powernv_php.c b/drivers/pci/hotplug/powernv_php.c
new file mode 100644
index 0000000..4cbff7a
--- /dev/null
+++ b/drivers/pci/hotplug/powernv_php.c
@@ -0,0 +1,140 @@ 
+/*
+ * 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/module.h>
+
+#include <asm/opal.h>
+#include <asm/pnv-pci.h>
+
+#include "powernv_php.h"
+
+#define DRIVER_VERSION	"0.1"
+#define DRIVER_AUTHOR	"Gavin Shan, IBM Corporation"
+#define DRIVER_DESC	"PowerPC PowerNV PCI Hotplug Driver"
+
+static struct notifier_block php_msg_nb = {
+	.notifier_call	= powernv_php_msg_handler,
+	.next		= NULL,
+	.priority	= 0,
+};
+
+static int powernv_php_register_one(struct device_node *dn)
+{
+	struct powernv_php_slot *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;
+
+	/* Allocate slot */
+	slot = powernv_php_slot_alloc(dn);
+	if (!slot)
+		return -ENODEV;
+
+	/* Register it */
+	ret = powernv_php_slot_register(slot);
+	if (ret) {
+		powernv_php_slot_put(slot);
+		return ret;
+	}
+
+	return powernv_php_slot_enable(slot->php_slot, false);
+}
+
+int powernv_php_register(struct device_node *dn)
+{
+	struct device_node *child;
+	int ret = 0;
+
+	/*
+	 * The parent slots should be registered before their
+	 * child slots.
+	 */
+	for_each_child_of_node(dn, child) {
+		powernv_php_register_one(child);
+		powernv_php_register(child);
+	}
+
+	return ret;
+}
+
+static void powernv_php_unregister_one(struct device_node *dn)
+{
+	struct powernv_php_slot *slot;
+
+	slot = powernv_php_slot_find(dn);
+	if (!slot)
+		return;
+
+	pci_hp_deregister(slot->php_slot);
+}
+
+void powernv_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) {
+		powernv_php_unregister(child);
+		powernv_php_unregister_one(child);
+	}
+}
+
+static int __init powernv_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")
+		powernv_php_register(dn);
+	for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
+		powernv_php_register(dn);
+
+	return 0;
+}
+
+static void __exit powernv_php_exit(void)
+{
+	struct device_node *dn;
+
+	pnv_pci_hotplug_notifier_unregister(&php_msg_nb);
+
+	for_each_compatible_node(dn, NULL, "ibm,ioda-phb")
+		powernv_php_unregister(dn);
+	for_each_compatible_node(dn, NULL, "ibm,ioda2-phb")
+		powernv_php_unregister(dn);
+}
+
+module_init(powernv_php_init);
+module_exit(powernv_php_exit);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
diff --git a/drivers/pci/hotplug/powernv_php.h b/drivers/pci/hotplug/powernv_php.h
new file mode 100644
index 0000000..8034cc6
--- /dev/null
+++ b/drivers/pci/hotplug/powernv_php.h
@@ -0,0 +1,92 @@ 
+/*
+ * 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.
+ */
+
+#ifndef _POWERNV_PHP_H
+#define _POWERNV_PHP_H
+
+#include <linux/list.h>
+#include <linux/kref.h>
+#include <linux/of.h>
+#include <linux/pci.h>
+#include <linux/pci_hotplug.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+#include <asm/opal-api.h>
+
+/* Slot power status */
+#define POWERNV_PHP_SLOT_POWER_OFF	0
+#define POWERNV_PHP_SLOT_POWER_ON	1
+
+/* Slot presence status */
+#define POWERNV_PHP_SLOT_EMPTY		0
+#define POWERNV_PHP_SLOT_PRESENT	1
+
+/* Slot attention status */
+#define POWERNV_PHP_SLOT_ATTEN_OFF	0
+#define POWERNV_PHP_SLOT_ATTEN_ON	1
+#define POWERNV_PHP_SLOT_ATTEN_IND	2
+#define POWERNV_PHP_SLOT_ATTEN_ACT	3
+
+struct powernv_php_slot {
+	char			*name;
+	struct device_node	*dn;
+	struct pci_dev		*pdev;
+	struct pci_bus		*bus;
+	uint64_t		id;
+	int			slot_no;
+	struct kref		kref;
+#define POWERNV_PHP_SLOT_STATE_INIT		0
+#define POWERNV_PHP_SLOT_STATE_REGISTER		1
+#define POWERNV_PHP_SLOT_STATE_POPULATED	2
+	int			state;
+	int			check_power_status;
+	int			status_confirmed;
+	struct opal_msg		*msg;
+	void			*fdt;
+	void			*dt;
+	struct of_changeset	ocs;
+	struct work_struct	work;
+	wait_queue_head_t	queue;
+	struct hotplug_slot	*php_slot;
+	struct powernv_php_slot	*parent;
+	struct list_head	children;
+	struct list_head	link;
+};
+
+int powernv_php_msg_handler(struct notifier_block *nb,
+			    unsigned long type, void *message);
+struct powernv_php_slot *powernv_php_slot_find(struct device_node *dn);
+void powernv_php_slot_free(struct kref *kref);
+struct powernv_php_slot *powernv_php_slot_alloc(struct device_node *dn);
+int powernv_php_slot_register(struct powernv_php_slot *slot);
+int powernv_php_slot_enable(struct hotplug_slot *php_slot, bool rescan);
+int powernv_php_register(struct device_node *dn);
+void powernv_php_unregister(struct device_node *dn);
+
+#define to_powernv_php_slot(kref) \
+	container_of(kref, struct powernv_php_slot, kref)
+
+static inline void powernv_php_slot_get(struct powernv_php_slot *slot)
+{
+	if (slot)
+		kref_get(&slot->kref);
+}
+
+static inline int powernv_php_slot_put(struct powernv_php_slot *slot)
+{
+	if (slot)
+		return kref_put(&slot->kref, powernv_php_slot_free);
+
+	return 0;
+}
+
+#endif /* !_POWERNV_PHP_H */
diff --git a/drivers/pci/hotplug/powernv_php_slot.c b/drivers/pci/hotplug/powernv_php_slot.c
new file mode 100644
index 0000000..73a93a2
--- /dev/null
+++ b/drivers/pci/hotplug/powernv_php_slot.c
@@ -0,0 +1,722 @@ 
+/*
+ * 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/module.h>
+
+#include <asm/opal.h>
+#include <asm/pnv-pci.h>
+#include <asm/ppc-pci.h>
+
+#include "powernv_php.h"
+
+static LIST_HEAD(php_slot_list);
+static DEFINE_SPINLOCK(php_slot_lock);
+
+/*
+ * Remove firmware data for all child device nodes of the
+ * indicated one.
+ */
+static void remove_child_pdn(struct device_node *np)
+{
+	struct device_node *child;
+
+	for_each_child_of_node(np, child) {
+		/* In depth first */
+		remove_child_pdn(child);
+
+		remove_pci_device_node_info(child);
+	}
+}
+
+/*
+ * Remove all subordinate device nodes of the indicated one.
+ * Those device nodes in deepest path should be released firstly.
+ */
+static int remove_child_device_nodes(struct device_node *parent)
+{
+	struct device_node *np, *child;
+	int ret = 0;
+
+	/* If the device node has children, remove them firstly */
+	for_each_child_of_node(parent, np) {
+		ret = remove_child_device_nodes(np);
+		if (ret)
+			return ret;
+
+		/* The device shouldn't have alive children */
+		child = of_get_next_child(np, NULL);
+		if (child) {
+			of_node_put(child);
+			of_node_put(np);
+			pr_err("%s: Alive children of node <%s>\n",
+			       __func__, of_node_full_name(np));
+			return -EBUSY;
+		}
+
+		/* Detach the device node */
+		of_detach_node(np);
+		of_node_put(np);
+	}
+
+	return 0;
+}
+
+/*
+ * The function processes the message sent by firmware
+ * to remove all device tree nodes beneath the slot's
+ * nodes, and the associated auxillary data.
+ */
+static void slot_power_off_handler(struct powernv_php_slot *slot)
+{
+	int ret, status = 1;
+
+	/* Release the firmware data for the child device nodes */
+	remove_child_pdn(slot->dn);
+
+	/*
+	 * Release the child device nodes. If the sub-tree was
+	 * built with the help of changeset, we just need destroy
+	 * the changes.
+	 */
+	if (slot->fdt) {
+		of_changeset_destroy(&slot->ocs);
+		kfree(slot->dt);
+		slot->dt = NULL;
+		slot->dn->child = NULL;
+		kfree(slot->fdt);
+		slot->fdt = NULL;
+	} else {
+		ret = remove_child_device_nodes(slot->dn);
+		if (ret) {
+			status = 2;
+			dev_warn(&slot->pdev->dev, "Error %d freeing nodes\n",
+				 ret);
+		}
+	}
+
+	/* Confirm status change */
+	slot->status_confirmed = status;
+	wake_up_interruptible(&slot->queue);
+}
+
+static int slot_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)
+			return ret;
+
+		ret = slot_populate_changeset(ocs, child);
+	}
+
+	return ret;
+}
+
+static void slot_power_on_handler(struct powernv_php_slot *slot)
+{
+	void *fdt, *dt;
+	uint64_t len;
+	int ret, status = 1;
+
+	/* We don't know the FDT blob size. It tries with incremental
+	 * sized memory chunk.
+	 */
+	for (len = 0x2000; len <= 0x10000; len += 0x2000) {
+		fdt = kzalloc(len, GFP_KERNEL);
+		if (!fdt)
+			break;
+
+		ret = pnv_pci_get_device_tree(slot->dn->phandle, fdt, len);
+		if (!ret)
+			break;
+
+		kfree(fdt);
+	}
+
+	if (len > 0x10000) {
+		dev_warn(&slot->pdev->dev, "Cannot alloc FDT blob\n");
+		goto out;
+	}
+
+	/* Unflatten device tree blob */
+	dt = of_fdt_unflatten_tree(fdt, slot->dn, NULL);
+	if (!dt) {
+		dev_warn(&slot->pdev->dev, "Cannot unflatten FDT\n");
+		goto free_fdt;
+	}
+
+	/* Initialize and apply the changeset */
+	of_changeset_init(&slot->ocs);
+	ret = slot_populate_changeset(&slot->ocs, slot->dn);
+	if (ret) {
+		dev_warn(&slot->pdev->dev, "Error %d populating changeset\n",
+			 ret);
+		goto free_dt;
+	}
+
+	slot->dn->child = NULL;
+	ret = of_changeset_apply(&slot->ocs);
+	if (ret) {
+		dev_warn(&slot->pdev->dev, "Error %d applying changeset\n",
+			 ret);
+		goto destroy_changeset;
+	}
+
+	/* Add device node firmware data */
+	traverse_pci_device_nodes(slot->dn,
+				  add_pci_device_node_info,
+				  pci_bus_to_host(slot->bus));
+	slot->fdt = fdt;
+	slot->dt = dt;
+	goto out;
+
+destroy_changeset:
+	of_changeset_destroy(&slot->ocs);
+free_dt:
+	kfree(dt);
+	slot->dn->child = NULL;
+free_fdt:
+	kfree(fdt);
+	status = 2;
+out:
+	/* Confirm status change */
+	slot->status_confirmed = status;
+	wake_up_interruptible(&slot->queue);
+}
+
+static void powernv_php_slot_work(struct work_struct *data)
+{
+	struct powernv_php_slot *slot = container_of(data,
+						     struct powernv_php_slot,
+						     work);
+	uint64_t php_event = be64_to_cpu(slot->msg->params[0]);
+
+	switch (php_event) {
+	case 0: /* Slot power off */
+		slot_power_off_handler(slot);
+		break;
+	case 1: /* Slot power on */
+		slot_power_on_handler(slot);
+		break;
+	default:
+		dev_warn(&slot->pdev->dev, "Unsupported hotplug event %lld\n",
+			 php_event);
+	}
+
+	of_node_put(slot->dn);
+}
+
+int powernv_php_msg_handler(struct notifier_block *nb,
+			    unsigned long type, void *message)
+{
+	phandle h;
+	struct device_node *np;
+	struct powernv_php_slot *slot;
+	struct opal_msg *msg = message;
+
+	/* Check the message type */
+	if (type != OPAL_MSG_PCI_HOTPLUG) {
+		pr_warn("%s: Wrong message type %ld received!\n",
+			__func__, type);
+		return NOTIFY_DONE;
+	}
+
+	/* Find the device node */
+	h = (phandle)be64_to_cpu(msg->params[1]);
+	np = of_find_node_by_phandle(h);
+	if (!np) {
+		pr_warn("%s: No device node for phandle 0x%08x\n",
+			__func__, h);
+		return NOTIFY_DONE;
+	}
+
+	/* Find the slot */
+	slot = powernv_php_slot_find(np);
+	if (!slot) {
+		pr_warn("%s: No slot found for node <%s>\n",
+			__func__, of_node_full_name(np));
+		of_node_put(np);
+		return NOTIFY_DONE;
+	}
+
+	/* Schedule the work */
+	slot->msg = msg;
+	schedule_work(&slot->work);
+	return NOTIFY_OK;
+}
+
+static int set_power_status(struct hotplug_slot *php_slot, u8 val)
+{
+	struct powernv_php_slot *slot = php_slot->private;
+	int ret;
+
+	/* Set power status */
+	slot->status_confirmed = 0;
+	ret = pnv_pci_set_power_status(slot->id, val);
+	if (ret) {
+		dev_warn(&slot->pdev->dev, "Error %d powering %s slot\n",
+			 ret, val ? "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 always waiting for that.
+	 */
+	if (slot->status_confirmed == 1)
+		return 0;
+	else if (slot->status_confirmed > 0)
+		return -EBUSY;
+
+	ret = wait_event_timeout(slot->queue, slot->status_confirmed, 10 * HZ);
+	if (!ret) {
+		dev_warn(&slot->pdev->dev, "Error %d waiting for power-%s\n",
+			 ret, val ? "on" : "off");
+		return -EBUSY;
+	}
+
+	/* Check the result */
+	if (slot->status_confirmed == 1)
+		return 0;
+
+	dev_warn(&slot->pdev->dev, "Error status %d for power-%s\n",
+		 slot->status_confirmed, val ? "on" : "off");
+	return -EBUSY;
+}
+
+static int get_power_status(struct hotplug_slot *php_slot, u8 *val)
+{
+	struct powernv_php_slot *slot = php_slot->private;
+	uint8_t state;
+	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_status(slot->id, &state);
+	if (ret) {
+		*val = POWERNV_PHP_SLOT_POWER_ON;
+		dev_warn(&slot->pdev->dev, "Error %d getting power status\n",
+			 ret);
+	} else {
+		*val = state ? POWERNV_PHP_SLOT_POWER_ON :
+			       POWERNV_PHP_SLOT_POWER_OFF;
+		php_slot->info->power_status = *val;
+	}
+
+	return 0;
+}
+
+static int get_adapter_status(struct hotplug_slot *php_slot, u8 *val)
+{
+	struct powernv_php_slot *slot = php_slot->private;
+	uint8_t state;
+	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_status(slot->id, &state);
+	if (ret >= 0) {
+		ret = 0;
+		*val = state ? POWERNV_PHP_SLOT_PRESENT :
+			       POWERNV_PHP_SLOT_EMPTY;
+		php_slot->info->adapter_status = *val;
+		ret = 0;
+	} else {
+		*val = POWERNV_PHP_SLOT_EMPTY;
+		dev_warn(&slot->pdev->dev, "Error %d getting presence\n",
+			 ret);
+	}
+
+	return ret;
+}
+
+static int set_attention_status(struct hotplug_slot *php_slot, u8 val)
+{
+	struct powernv_php_slot *slot = php_slot->private;
+
+	/* The default operation would to turn on the attention */
+	switch (val) {
+	case POWERNV_PHP_SLOT_ATTEN_OFF:
+	case POWERNV_PHP_SLOT_ATTEN_ON:
+	case POWERNV_PHP_SLOT_ATTEN_IND:
+	case POWERNV_PHP_SLOT_ATTEN_ACT:
+		break;
+	default:
+		dev_warn(&slot->pdev->dev, "Invalid attention %d\n", val);
+		return -EINVAL;
+	}
+
+	/* FIXME: Make it real once firmware supports it */
+	php_slot->info->attention_status = val;
+
+	return 0;
+}
+
+int powernv_php_slot_enable(struct hotplug_slot *php_slot, bool rescan)
+{
+	struct powernv_php_slot *slot = php_slot->private;
+	uint8_t presence, power_status;
+	int ret;
+
+	/* Check if the slot has been configured */
+	if (slot->state != POWERNV_PHP_SLOT_STATE_REGISTER)
+		return 0;
+
+	/* Retrieve slot presence status */
+	ret = php_slot->ops->get_adapter_status(php_slot, &presence);
+	if (ret)
+		return ret;
+
+	/* Proceed if there have nothing behind the slot */
+	if (presence == POWERNV_PHP_SLOT_EMPTY)
+		goto scan;
+
+	/*
+	 * If we don't detect something behind the slot, we need
+	 * make sure the power suply to the slot is on. Otherwise,
+	 * the slot downstream PCIe linkturn should be down.
+	 *
+	 * 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 (!slot->check_power_status) {
+		slot->check_power_status = 1;
+		goto scan;
+	}
+
+	/* Check the power status. Scan the slot if that's already on */
+	ret = php_slot->ops->get_power_status(php_slot, &power_status);
+	if (ret)
+		return ret;
+
+	if (power_status == POWERNV_PHP_SLOT_POWER_ON)
+		goto scan;
+
+	/* Power is off, turn it on and then scan the slot */
+	ret = set_power_status(php_slot, POWERNV_PHP_SLOT_POWER_ON);
+	if (ret)
+		return ret;
+
+scan:
+	switch (presence) {
+	case POWERNV_PHP_SLOT_PRESENT:
+		if (rescan) {
+			pci_lock_rescan_remove();
+			pci_add_pci_devices(slot->bus);
+			pci_unlock_rescan_remove();
+		}
+
+		/* Rescan for child hotpluggable slots */
+		slot->state = POWERNV_PHP_SLOT_STATE_POPULATED;
+		if (rescan)
+			powernv_php_register(slot->dn);
+		break;
+	case POWERNV_PHP_SLOT_EMPTY:
+		slot->state = POWERNV_PHP_SLOT_STATE_POPULATED;
+		break;
+	default:
+		dev_warn(&slot->pdev->dev, "Invalid presence status %d\n",
+			 presence);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int enable_slot(struct hotplug_slot *php_slot)
+{
+	return powernv_php_slot_enable(php_slot, true);
+}
+
+static int disable_slot(struct hotplug_slot *php_slot)
+{
+	struct powernv_php_slot *slot = php_slot->private;
+	uint8_t power_status;
+	int ret;
+
+	if (slot->state != POWERNV_PHP_SLOT_STATE_POPULATED)
+		return 0;
+
+	/* Remove all devices behind the slot */
+	pci_lock_rescan_remove();
+	pci_remove_pci_devices(slot->bus);
+	pci_unlock_rescan_remove();
+
+	/* Detach the child hotpluggable slots */
+	powernv_php_unregister(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 = php_slot->ops->get_power_status(php_slot, &power_status);
+	if (ret || power_status == POWERNV_PHP_SLOT_POWER_ON) {
+		ret = set_power_status(php_slot, POWERNV_PHP_SLOT_POWER_OFF);
+		if (ret)
+			dev_warn(&slot->pdev->dev, "Error %d powering off\n",
+				 ret);
+	}
+
+	/* Update slot state */
+	slot->state = POWERNV_PHP_SLOT_STATE_REGISTER;
+	return 0;
+}
+
+static struct hotplug_slot_ops php_slot_ops = {
+	.get_power_status	= get_power_status,
+	.get_adapter_status	= get_adapter_status,
+	.set_attention_status	= set_attention_status,
+	.enable_slot		= enable_slot,
+	.disable_slot		= disable_slot,
+};
+
+static struct powernv_php_slot *php_slot_match(struct device_node *dn,
+					       struct powernv_php_slot *slot)
+{
+	struct powernv_php_slot *target, *tmp;
+
+	if (slot->dn == dn)
+		return slot;
+
+	list_for_each_entry(tmp, &slot->children, link) {
+		target = php_slot_match(dn, tmp);
+		if (target)
+			return target;
+	}
+
+	return NULL;
+}
+
+struct powernv_php_slot *powernv_php_slot_find(struct device_node *dn)
+{
+	struct powernv_php_slot *slot, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&php_slot_lock, flags);
+	list_for_each_entry(tmp, &php_slot_list, link) {
+		slot = php_slot_match(dn, tmp);
+		if (slot) {
+			spin_unlock_irqrestore(&php_slot_lock, flags);
+			return slot;
+		}
+	}
+	spin_unlock_irqrestore(&php_slot_lock, flags);
+
+	return NULL;
+}
+
+void powernv_php_slot_free(struct kref *kref)
+{
+	struct powernv_php_slot *slot = to_powernv_php_slot(kref);
+
+	WARN_ON(!list_empty(&slot->children));
+	kfree(slot->name);
+	kfree(slot);
+}
+
+static void php_slot_release(struct hotplug_slot *hp_slot)
+{
+	struct powernv_php_slot *slot = hp_slot->private;
+	unsigned long flags;
+
+	/* Remove from global or child list */
+	spin_lock_irqsave(&php_slot_lock, flags);
+	list_del(&slot->link);
+	spin_unlock_irqrestore(&php_slot_lock, flags);
+
+	/* Detach from parent */
+	powernv_php_slot_put(slot);
+	powernv_php_slot_put(slot->parent);
+}
+
+static bool php_slot_get_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);
+
+	/* Bus/Slot/Function number */
+	prop32 = of_get_property(dn, "reg", NULL);
+	if (!prop32)
+		return false;
+	*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 false;
+		}
+
+		*id |= be64_to_cpup(prop64);
+		of_node_put(parent);
+		return true;
+	}
+
+	return false;
+}
+
+struct powernv_php_slot *powernv_php_slot_alloc(struct device_node *dn)
+{
+	struct eeh_dev *edev = pdn_to_eeh_dev(PCI_DN(dn));
+	struct pci_bus *bus;
+	struct powernv_php_slot *slot;
+	const char *label;
+	uint64_t id;
+	int slot_no;
+	size_t size;
+	void *pmem;
+
+	/* Slot name */
+	label = of_get_property(dn, "ibm,slot-label", NULL);
+	if (!label)
+		return NULL;
+
+	/* Slot identifier */
+	if (!php_slot_get_id(dn, &id))
+		return NULL;
+
+	/* PCI bus */
+	bus = of_node_to_pci_bus(dn);
+	if (!bus)
+		return NULL;
+
+	/* Slot number */
+	if (dn->child && PCI_DN(dn->child))
+		slot_no = PCI_SLOT(PCI_DN(dn->child)->devfn);
+	else
+		slot_no = -1;
+
+	/* Allocate slot */
+	size = sizeof(struct powernv_php_slot) +
+	       sizeof(struct hotplug_slot) +
+	       sizeof(struct hotplug_slot_info);
+	pmem = kzalloc(size, GFP_KERNEL);
+	if (!pmem) {
+		pr_warn("%s: Cannot allocate slot for node %s\n",
+			__func__, dn->full_name);
+		return NULL;
+	}
+
+	/* Assign memory blocks */
+	slot = pmem;
+	slot->php_slot = pmem + sizeof(struct powernv_php_slot);
+	slot->php_slot->info = pmem + sizeof(struct powernv_php_slot) +
+			      sizeof(struct hotplug_slot);
+	slot->name = kstrdup(label, GFP_KERNEL);
+	if (!slot->name) {
+		pr_warn("%s: Cannot populate name for node %s\n",
+			__func__, dn->full_name);
+		kfree(pmem);
+		return NULL;
+	}
+
+	/* Initialize slot */
+	kref_init(&slot->kref);
+	slot->state = POWERNV_PHP_SLOT_STATE_INIT;
+	slot->dn = dn;
+	slot->pdev = eeh_dev_to_pci_dev(edev);
+	slot->bus = bus;
+	slot->id = id;
+	slot->slot_no = slot_no;
+	INIT_WORK(&slot->work, powernv_php_slot_work);
+	init_waitqueue_head(&slot->queue);
+	slot->check_power_status = 0;
+	slot->status_confirmed = 0;
+	slot->php_slot->ops = &php_slot_ops;
+	slot->php_slot->release = php_slot_release;
+	slot->php_slot->private = slot;
+	INIT_LIST_HEAD(&slot->children);
+	INIT_LIST_HEAD(&slot->link);
+
+	return slot;
+}
+
+int powernv_php_slot_register(struct powernv_php_slot *slot)
+{
+	struct powernv_php_slot *parent;
+	struct device_node *dn = slot->dn;
+	unsigned long flags;
+	int ret;
+
+	/* Avoid register same slot for twice */
+	if (powernv_php_slot_find(slot->dn))
+		return -EEXIST;
+
+	/* Register slot */
+	ret = pci_hp_register(slot->php_slot, slot->bus,
+			      slot->slot_no, slot->name);
+	if (ret) {
+		dev_warn(&slot->pdev->dev, "Error %d registering slot\n",
+			 ret);
+		return ret;
+	}
+
+	/* Put into global or parent list */
+	while ((dn = of_get_parent(dn))) {
+		if (!PCI_DN(dn)) {
+			of_node_put(dn);
+			break;
+		}
+
+		parent = powernv_php_slot_find(dn);
+		if (parent) {
+			of_node_put(dn);
+			break;
+		}
+	}
+
+	spin_lock_irqsave(&php_slot_lock, flags);
+	if (parent) {
+		powernv_php_slot_get(parent);
+		slot->parent = parent;
+		list_add_tail(&slot->link, &parent->children);
+	} else {
+		list_add_tail(&slot->link, &php_slot_list);
+	}
+	spin_unlock_irqrestore(&php_slot_lock, flags);
+
+	/* Update slot state */
+	slot->state = POWERNV_PHP_SLOT_STATE_REGISTER;
+	return 0;
+}