[PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific
Sam Bobroff
sbobroff at linux.ibm.com
Fri Feb 7 13:24:13 AEDT 2020
On Mon, Feb 03, 2020 at 07:35:20PM +1100, Oliver O'Halloran wrote:
> The eeh_ops->probe() function is called from two different contexts:
>
> 1. On pseries, where set set EEH_PROBE_MODE_DEVTREE, it's called in
"set set" -> "we set"
> eeh_add_device_early() which is supposed to run before we create
> a pci_dev.
>
> 2. On PowerNV, where we set EEH_PROBE_MODE_DEV, it's called in
> eeh_device_add_late() which is supposed to run *after* the
> pci_dev is created.
>
> The "early" probe is required because PAPR requires that we perform an RTAS
> call to enable EEH support on a device before we start interacting with it
> via config space or MMIO. This requirement doesn't exist on PowerNV and
> shoehorning two completely separate initialisation paths into a common
> interface just results in a convoluted code everywhere.
>
> Additionally the early probe requires the probe function to take an pci_dn
> rather than a pci_dev argument. We'd like to make pci_dn a pseries specific
> data structure since there's no real requirement for them on PowerNV. To
> help both goals move the early probe into the pseries containment zone
> so the platform depedence is more explicit.
>
I had a look around near your comment:
> + // XXX: uh, do we have the rescan lock held here?
And we definitely don't have the lock when it gets called via the module
init path (as rpaphp is loaded) -- I tried it and there was no deadlock.
I don't think we have the lock in other situations but I haven't
unravelled it all enough yet to tell, either.
Regardless, good cleanup.
Reviewed-by: Sam Bobroff <sbobroff at linux.ibm.com>
> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
> ---
> arch/powerpc/include/asm/eeh.h | 14 +++---
> arch/powerpc/kernel/eeh.c | 46 --------------------
> arch/powerpc/kernel/of_platform.c | 6 +--
> arch/powerpc/platforms/powernv/eeh-powernv.c | 6 ---
> arch/powerpc/platforms/pseries/eeh_pseries.c | 65 ++++++++++++++++++++++------
> arch/powerpc/platforms/pseries/pci_dlpar.c | 2 +-
> drivers/pci/hotplug/rpadlpar_core.c | 2 +-
> drivers/pci/hotplug/rpaphp_core.c | 2 +-
> drivers/pci/hotplug/rpaphp_pci.c | 3 +-
> 9 files changed, 65 insertions(+), 81 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 5d10781..8580238 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -301,8 +301,6 @@ int __exit eeh_ops_unregister(const char *name);
> int eeh_check_failure(const volatile void __iomem *token);
> int eeh_dev_check_failure(struct eeh_dev *edev);
> void eeh_addr_cache_init(void);
> -void eeh_add_device_early(struct pci_dn *);
> -void eeh_add_device_tree_early(struct pci_dn *);
> void eeh_add_device_late(struct pci_dev *);
> void eeh_remove_device(struct pci_dev *);
> int eeh_unfreeze_pe(struct eeh_pe *pe);
> @@ -358,10 +356,6 @@ static inline int eeh_check_failure(const volatile void __iomem *token)
>
> static inline void eeh_addr_cache_init(void) { }
>
> -static inline void eeh_add_device_early(struct pci_dn *pdn) { }
> -
> -static inline void eeh_add_device_tree_early(struct pci_dn *pdn) { }
> -
> static inline void eeh_add_device_late(struct pci_dev *dev) { }
>
> static inline void eeh_remove_device(struct pci_dev *dev) { }
> @@ -370,6 +364,14 @@ static inline void eeh_remove_device(struct pci_dev *dev) { }
> #define EEH_IO_ERROR_VALUE(size) (-1UL)
> #endif /* CONFIG_EEH */
>
> +#if defined(CONFIG_PPC_PSERIES) && defined(CONFIG_EEH)
> +void pseries_eeh_init_edev(struct pci_dn *pdn);
> +void pseries_eeh_init_edev_recursive(struct pci_dn *pdn);
> +#else
> +static inline void pseries_eeh_add_device_early(struct pci_dn *pdn) { }
> +static inline void pseries_eeh_add_device_tree_early(struct pci_dn *pdn) { }
> +#endif
> +
> #ifdef CONFIG_PPC64
> /*
> * MMIO read/write operations with EEH support.
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index a9e4ca7..55d3ef6 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1107,52 +1107,6 @@ static int eeh_init(void)
> core_initcall_sync(eeh_init);
>
> /**
> - * eeh_add_device_early - Enable EEH for the indicated device node
> - * @pdn: PCI device node for which to set up EEH
> - *
> - * This routine must be used to perform EEH initialization for PCI
> - * devices that were added after system boot (e.g. hotplug, dlpar).
> - * This routine must be called before any i/o is performed to the
> - * adapter (inluding any config-space i/o).
> - * Whether this actually enables EEH or not for this device depends
> - * on the CEC architecture, type of the device, on earlier boot
> - * command-line arguments & etc.
> - */
> -void eeh_add_device_early(struct pci_dn *pdn)
> -{
> - struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> -
> - if (!edev)
> - return;
> -
> - if (!eeh_has_flag(EEH_PROBE_MODE_DEVTREE))
> - return;
> -
> - eeh_ops->probe(pdn, NULL);
> -}
> -
> -/**
> - * eeh_add_device_tree_early - Enable EEH for the indicated device
> - * @pdn: PCI device node
> - *
> - * This routine must be used to perform EEH initialization for the
> - * indicated PCI device that was added after system boot (e.g.
> - * hotplug, dlpar).
> - */
> -void eeh_add_device_tree_early(struct pci_dn *pdn)
> -{
> - struct pci_dn *n;
> -
> - if (!pdn)
> - return;
> -
> - list_for_each_entry(n, &pdn->child_list, list)
> - eeh_add_device_tree_early(n);
> - eeh_add_device_early(pdn);
> -}
> -EXPORT_SYMBOL_GPL(eeh_add_device_tree_early);
> -
> -/**
> * eeh_add_device_late - Perform EEH initialization for the indicated pci device
> * @dev: pci device for which to set up EEH
> *
> diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
> index 64edac81..71a3f97 100644
> --- a/arch/powerpc/kernel/of_platform.c
> +++ b/arch/powerpc/kernel/of_platform.c
> @@ -62,13 +62,9 @@ static int of_pci_phb_probe(struct platform_device *dev)
> /* Init pci_dn data structures */
> pci_devs_phb_init_dynamic(phb);
>
> - /* Create EEH devices for the PHB */
> + /* Create EEH PEs for the PHB */
> eeh_dev_phb_init_dynamic(phb);
>
> - /* Register devices with EEH */
> - if (dev->dev.of_node->child)
> - eeh_add_device_tree_early(PCI_DN(dev->dev.of_node));
> -
> /* Scan the bus */
> pcibios_scan_phb(phb);
> if (phb->bus == NULL)
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index ef727ec..eaa8dfe 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -40,13 +40,7 @@ static int eeh_event_irq = -EINVAL;
>
> void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
> {
> - struct pci_dn *pdn = pci_get_pdn(pdev);
> -
> - if (!pdn || eeh_has_flag(EEH_FORCE_DISABLED))
> - return;
> -
> dev_dbg(&pdev->dev, "EEH: Setting up device\n");
> - eeh_add_device_early(pdn);
> eeh_add_device_late(pdev);
> }
>
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 95bbf91..1ca7cf0 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -67,7 +67,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
> pdn->pe_number = physfn_pdn->pe_num_map[pdn->vf_index];
> }
> #endif
> - eeh_add_device_early(pdn);
> + pseries_eeh_init_edev(pdn);
> #ifdef CONFIG_PCI_IOV
> if (pdev->is_virtfn) {
> struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> @@ -221,15 +221,16 @@ static int pseries_eeh_find_ecap(struct pci_dn *pdn, int cap)
> }
>
> /**
> - * pseries_eeh_probe - EEH probe on the given device
> + * pseries_eeh_init_edev - initialise the eeh_dev and eeh_pe for a pci_dn
> + *
> * @pdn: PCI device node
> - * @data: Unused
> *
> - * When EEH module is installed during system boot, all PCI devices
> - * are checked one by one to see if it supports EEH. The function
> - * is introduced for the purpose.
> + * When we discover a new PCI device via the device-tree we create a
> + * corresponding pci_dn and we allocate, but don't initialise, an eeh_dev.
> + * This function takes care of the initialisation and inserts the eeh_dev
> + * into the correct eeh_pe. If no eeh_pe exists we'll allocate one.
> */
> -static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
> +void pseries_eeh_init_edev(struct pci_dn *pdn)
> {
> struct eeh_dev *edev;
> struct eeh_pe pe;
> @@ -237,18 +238,35 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
> int enable = 0;
> int ret;
>
> - /* Retrieve OF node and eeh device */
> + if (WARN_ON_ONCE(!eeh_has_flag(EEH_PROBE_MODE_DEVTREE)))
> + return;
> +
> + /*
> + * Find the eeh_dev for this pdn. The storage for the eeh_dev was
> + * allocated at the same time as the pci_dn.
> + *
> + * XXX: We should probably re-visit that.
> + */
> edev = pdn_to_eeh_dev(pdn);
> - if (!edev || edev->pe)
> - return NULL;
> + if (!edev)
> + return;
> +
> + /*
> + * If ->pe is set then we've already probed this device. We hit
> + * this path when a pci_dev is removed and rescanned while recovering
> + * a PE (i.e. for devices where the driver doesn't support error
> + * recovery).
> + */
> + if (edev->pe)
> + return;
>
> /* Check class/vendor/device IDs */
> if (!pdn->vendor_id || !pdn->device_id || !pdn->class_code)
> - return NULL;
> + return;
>
> /* Skip for PCI-ISA bridge */
> if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
> - return NULL;
> + return;
>
> eeh_edev_dbg(edev, "Probing device\n");
>
> @@ -315,9 +333,29 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
>
> /* Save memory bars */
> eeh_save_bars(edev);
> +}
> +
> +/**
> + * pseries_eeh_init_edev_recursive - Enable EEH for the indicated device
> + * @pdn: PCI device node
> + *
> + * This routine must be used to perform EEH initialization for the
> + * indicated PCI device that was added after system boot (e.g.
> + * hotplug, dlpar).
> + */
> +void pseries_eeh_init_edev_recursive(struct pci_dn *pdn)
> +{
> + struct pci_dn *n;
> +
> + if (!pdn)
> + return;
> +
> + list_for_each_entry(n, &pdn->child_list, list)
> + pseries_eeh_init_edev_recursive(n);
>
> - return NULL;
> + pseries_eeh_init_edev(pdn);
> }
> +EXPORT_SYMBOL_GPL(pseries_eeh_init_edev_recursive);
>
> /**
> * pseries_eeh_set_option - Initialize EEH or MMIO/DMA reenable
> @@ -775,7 +813,6 @@ static int pseries_notify_resume(struct pci_dn *pdn)
> static struct eeh_ops pseries_eeh_ops = {
> .name = "pseries",
> .init = pseries_eeh_init,
> - .probe = pseries_eeh_probe,
> .set_option = pseries_eeh_set_option,
> .get_pe_addr = pseries_eeh_get_pe_addr,
> .get_state = pseries_eeh_get_state,
> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
> index 361986e..b3a38f5 100644
> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
> @@ -37,7 +37,7 @@ struct pci_controller *init_phb_dynamic(struct device_node *dn)
> eeh_dev_phb_init_dynamic(phb);
>
> if (dn->child)
> - eeh_add_device_tree_early(PCI_DN(dn));
> + pseries_eeh_init_edev_recursive(PCI_DN(dn));
>
> pcibios_scan_phb(phb);
> pcibios_finish_adding_to_bus(phb->bus);
> diff --git a/drivers/pci/hotplug/rpadlpar_core.c b/drivers/pci/hotplug/rpadlpar_core.c
> index 977946e..c5eb509 100644
> --- a/drivers/pci/hotplug/rpadlpar_core.c
> +++ b/drivers/pci/hotplug/rpadlpar_core.c
> @@ -140,7 +140,7 @@ static void dlpar_pci_add_bus(struct device_node *dn)
> struct pci_controller *phb = pdn->phb;
> struct pci_dev *dev = NULL;
>
> - eeh_add_device_tree_early(pdn);
> + pseries_eeh_init_edev_recursive(pdn);
>
> /* Add EADS device to PHB bus, adding new entry to bus->devices */
> dev = of_create_pci_dev(dn, phb->bus, pdn->devfn);
> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> index 9c1e43e..b89d5ff 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -494,7 +494,7 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
> return retval;
>
> if (state == PRESENT) {
> - eeh_add_device_tree_early(PCI_DN(slot->dn));
> + pseries_eeh_init_edev_recursive(PCI_DN(slot->dn));
>
> pci_lock_rescan_remove();
> pci_hp_add_devices(slot->bus);
> diff --git a/drivers/pci/hotplug/rpaphp_pci.c b/drivers/pci/hotplug/rpaphp_pci.c
> index 61ebbd8..e116ffe 100644
> --- a/drivers/pci/hotplug/rpaphp_pci.c
> +++ b/drivers/pci/hotplug/rpaphp_pci.c
> @@ -96,7 +96,8 @@ int rpaphp_enable_slot(struct slot *slot)
> }
>
> if (list_empty(&bus->devices)) {
> - eeh_add_device_tree_early(PCI_DN(slot->dn));
> + // XXX: uh, do we have the rescan lock held here?
> + pseries_eeh_init_edev_recursive(PCI_DN(slot->dn));
> pci_hp_add_devices(bus);
> }
>
> --
> 2.9.5
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20200207/68b0768f/attachment.sig>
More information about the Linuxppc-dev
mailing list