[PATCH v8 45/45] PCI/hotplug: PowerPC PowerNV PCI hotplug driver

Alexey Kardashevskiy aik at ozlabs.ru
Tue May 3 10:44:17 AEST 2016


On 05/03/2016 09:41 AM, Gavin Shan wrote:
> On Wed, Apr 20, 2016 at 11:55:56AM +1000, Alistair Popple wrote:
>> On Tue, 19 Apr 2016 20:36:48 Alexey Kardashevskiy wrote:
>>> On 02/17/2016 02:44 PM, Gavin Shan wrote:
>>>> This adds standalone driver to support PCI hotplug for PowerPC PowerNV
>>>> platform that runs on top of skiboot firmware. The firmware identifies
>>>> hotpluggable slots and marked their device tree node with proper
>>>> "ibm,slot-pluggable" and "ibm,reset-by-firmware". The driver scans
>>>> device tree nodes to create/register PCI hotplug slot accordingly.
>>>>
>>>> The PCI slots are organized in fashion of tree, which means one
>>>> PCI slot might have parent PCI slot and parent PCI slot possibly
>>>> contains multiple child PCI slots. At the plugging time, the parent
>>>> PCI slot is populated before its children. The child PCI slots are
>>>> removed before their parent PCI slot can be removed from the system.
>>>>
>>>> If the skiboot firmware doesn't support slot status retrieval, the PCI
>>>> slot device node shouldn't have property "ibm,reset-by-firmware". In
>>>> that case, none of valid PCI slots will be detected from device tree.
>>>> The skiboot firmware doesn't export the capability to access attention
>>>> LEDs yet and it's something for TBD.
>>>>
>>>> Signed-off-by: Gavin Shan <gwshan at linux.vnet.ibm.com>
>>>> Acked-by: Bjorn Helgaas <bhelgaas at google.com>
>>>> ---
>>>>   drivers/pci/hotplug/Kconfig   |  12 +
>>>>   drivers/pci/hotplug/Makefile  |   3 +
>>>>   drivers/pci/hotplug/pnv_php.c | 870 ++++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 885 insertions(+)
>>>>   create mode 100644 drivers/pci/hotplug/pnv_php.c
>>>>
>>>> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>>>> index df8caec..167c8ce 100644
>>>> --- a/drivers/pci/hotplug/Kconfig
>>>> +++ b/drivers/pci/hotplug/Kconfig
>>>> @@ -113,6 +113,18 @@ config HOTPLUG_PCI_SHPC
>>>>
>>>>   	  When in doubt, say N.
>>>>
>>>> +config HOTPLUG_PCI_POWERNV
>>>> +	tristate "PowerPC PowerNV PCI Hotplug driver"
>>>> +	depends on PPC_POWERNV && EEH
>>>> +	help
>>>> +	  Say Y here if you run PowerPC PowerNV platform that supports
>>>> +	  PCI Hotplug
>>>> +
>>>> +	  To compile this driver as a module, choose M here: the
>>>> +	  module will be called pnv-php.
>>>> +
>>>> +	  When in doubt, say N.
>>>> +
>>>>   config HOTPLUG_PCI_RPA
>>>>   	tristate "RPA PCI Hotplug driver"
>>>>   	depends on PPC_PSERIES && EEH
>>>> diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
>>>> index b616e75..e33cdda 100644
>>>> --- a/drivers/pci/hotplug/Makefile
>>>> +++ b/drivers/pci/hotplug/Makefile
>>>> @@ -14,6 +14,7 @@ obj-$(CONFIG_HOTPLUG_PCI_PCIE)		+= pciehp.o
>>>>   obj-$(CONFIG_HOTPLUG_PCI_CPCI_ZT5550)	+= cpcihp_zt5550.o
>>>>   obj-$(CONFIG_HOTPLUG_PCI_CPCI_GENERIC)	+= cpcihp_generic.o
>>>>   obj-$(CONFIG_HOTPLUG_PCI_SHPC)		+= shpchp.o
>>>> +obj-$(CONFIG_HOTPLUG_PCI_POWERNV)	+= pnv-php.o
>>>>   obj-$(CONFIG_HOTPLUG_PCI_RPA)		+= rpaphp.o
>>>>   obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
>>>>   obj-$(CONFIG_HOTPLUG_PCI_SGI)		+= sgi_hotplug.o
>>>> @@ -50,6 +51,8 @@ ibmphp-objs		:=	ibmphp_core.o	\
>>>>   acpiphp-objs		:=	acpiphp_core.o	\
>>>>   				acpiphp_glue.o
>>>>
>>>> +pnv-php-objs		:=	pnv_php.o
>>>> +
>>>>   rpaphp-objs		:=	rpaphp_core.o	\
>>>>   				rpaphp_pci.o	\
>>>>   				rpaphp_slot.o
>>>> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
>>>> new file mode 100644
>>>> index 0000000..364ec36
>>>> --- /dev/null
>>>> +++ b/drivers/pci/hotplug/pnv_php.c
>>>> @@ -0,0 +1,870 @@
>>>> +/*
>>>> + * PCI Hotplug Driver for PowerPC PowerNV platform.
>>>> + *
>>>> + * Copyright Gavin Shan, IBM Corporation 2015.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + */
>>>> +
>>>> +#include <linux/libfdt.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/pci.h>
>>>> +#include <linux/pci_hotplug.h>
>>>> +
>>>> +#include <asm/opal.h>
>>>> +#include <asm/pnv-pci.h>
>>>> +#include <asm/ppc-pci.h>
>>>> +
>>>> +#define DRIVER_VERSION	"0.1"
>>>> +#define DRIVER_AUTHOR	"Gavin Shan, IBM Corporation"
>>>> +#define DRIVER_DESC	"PowerPC PowerNV PCI Hotplug Driver"
>>>> +
>>>> +struct pnv_php_slot {
>>>> +	struct hotplug_slot		slot;
>>>> +	struct hotplug_slot_info	slot_info;
>>>> +	uint64_t			id;
>>>> +	char				*name;
>>>> +	int				slot_no;
>>>> +	struct kref			kref;
>>>> +#define PNV_PHP_STATE_INITIALIZED	0
>>>> +#define PNV_PHP_STATE_REGISTERED	1
>>>> +#define PNV_PHP_STATE_POPULATED		2
>>>> +	int				state;
>>>> +	struct device_node		*dn;
>>>> +	struct pci_dev			*pdev;
>>>> +	struct pci_bus			*bus;
>>>> +	bool				power_state_check;
>>>> +	int				power_state_confirmed;
>>>> +#define PNV_PHP_POWER_CONFIRMED_INVALID	0
>>>> +#define PNV_PHP_POWER_CONFIRMED_SUCCESS	1
>>>> +#define PNV_PHP_POWER_CONFIRMED_FAIL	2
>>>> +	struct opal_msg			*msg;
>>>> +	void				*fdt;
>>>> +	void				*dt;
>>>> +	struct of_changeset		ocs;
>>>> +	struct work_struct		work;
>>>> +	wait_queue_head_t		queue;
>>>> +	struct pnv_php_slot		*parent;
>>>> +	struct list_head		children;
>>>> +	struct list_head		link;
>>>> +};
>>>> +
>>>> +static LIST_HEAD(pnv_php_slot_list);
>>>> +static DEFINE_SPINLOCK(pnv_php_lock);
>>>> +
>>>> +static void pnv_php_register(struct device_node *dn);
>>>> +static void pnv_php_unregister_one(struct device_node *dn);
>>>> +static void pnv_php_unregister(struct device_node *dn);
>>>
>>>
>>> The names confused me. I'd suggest pnv_php_scan(), pnv_php_unregister(),
>>> pnv_php_unregister_children() instead.
>>>
>>>
>>> Alistair, what do you reckon?
>>
>> To be honest I'm not sure the new names are necessarily any less confusing. I
>> will admit to having to read that code twice though so perhaps a short comment
>> describing what each of those functions does would be the best method for
>> reducing confusion.
>
> Alexey, Please confirm if I need rename those functions though I
> don't understand the confusion caused the function names.

Just add the comments.

I got confused because:

pnv_php_register() walks through nodes to find "ibm,slot-pluggable" - this 
is rather "scan" than "register" (which may not happen if the property is 
not there).

pnv_php_register_one() registers one what? slot? From the name I conclude 
that not a slot as there is pnv_php_register_slot() which does register one 
slot. So I suppose pnv_php_register_one() registers one _node_ (which may 
have multiple slots? there should be reason why it is a separate function). 
I do not know...



> [unrelated content removed]



-- 
Alexey


More information about the Linuxppc-dev mailing list