EEH init cleanup

EEH init cleanup

[Oliver O'Halloran]

This series reworks how EEH devices are initialised. This mainly affects
pseries since it moves the "early" EEH probe out of shared code and into
pseries platform code. The goal here is to make the platform
dependencies more explicit and to allow PowerNV to implement its own
pci_dev <-> eeh_dev mapping that doesn't require a PCI_DN.

[PATCH 1/6] powerpc/eeh: Add sysfs files in late probe

[Oliver O'Halloran]

Move creating the EEH specific sysfs files into eeh_add_device_late()
rather than being open-coded all over the place. Calling the function is
generally done immediately after calling eeh_add_device_late() anyway. The
two cases where it's not done there (OF based PCI probing and the pseries
VFs) don't seem to have any issues with the re-ordering.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/include/asm/eeh.h               |  3 ---
 arch/powerpc/kernel/eeh.c                    | 24 +-----------------------
 arch/powerpc/kernel/of_platform.c            |  3 ---
 arch/powerpc/kernel/pci-common.c             |  3 ---
 arch/powerpc/platforms/powernv/eeh-powernv.c |  1 -
 arch/powerpc/platforms/pseries/eeh_pseries.c |  3 +--
 6 files changed, 2 insertions(+), 35 deletions(-)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 6f9b2a1..5a34907 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -305,7 +305,6 @@ 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_add_device_tree_late(struct pci_bus *);
-void eeh_add_sysfs_files(struct pci_bus *);
 void eeh_remove_device(struct pci_dev *);
 int eeh_unfreeze_pe(struct eeh_pe *pe);
 int eeh_pe_reset_and_recover(struct eeh_pe *pe);
@@ -368,8 +367,6 @@ static inline void eeh_add_device_late(struct pci_dev *dev) { }
 
 static inline void eeh_add_device_tree_late(struct pci_bus *bus) { }
 
-static inline void eeh_add_sysfs_files(struct pci_bus *bus) { }
-
 static inline void eeh_remove_device(struct pci_dev *dev) { }
 
 #define EEH_POSSIBLE_ERROR(val, type) (0)
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 17cb3e9..0878912 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1210,6 +1210,7 @@ void eeh_add_device_late(struct pci_dev *dev)
 	dev->dev.archdata.edev = edev;
 
 	eeh_addr_cache_insert_dev(dev);
+	eeh_sysfs_add_device(dev);
 }
 
 /**
@@ -1238,29 +1239,6 @@ void eeh_add_device_tree_late(struct pci_bus *bus)
 EXPORT_SYMBOL_GPL(eeh_add_device_tree_late);
 
 /**
- * eeh_add_sysfs_files - Add EEH sysfs files for the indicated PCI bus
- * @bus: PCI bus
- *
- * This routine must be used to add EEH sysfs files for PCI
- * devices which are attached to the indicated PCI bus. The PCI bus
- * is added after system boot through hotplug or dlpar.
- */
-void eeh_add_sysfs_files(struct pci_bus *bus)
-{
-	struct pci_dev *dev;
-
-	list_for_each_entry(dev, &bus->devices, bus_list) {
-		eeh_sysfs_add_device(dev);
-		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
-			struct pci_bus *subbus = dev->subordinate;
-			if (subbus)
-				eeh_add_sysfs_files(subbus);
-		}
-	}
-}
-EXPORT_SYMBOL_GPL(eeh_add_sysfs_files);
-
-/**
  * eeh_remove_device - Undo EEH setup for the indicated pci device
  * @dev: pci device to be removed
  *
diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
index 427fc22..cb68800 100644
--- a/arch/powerpc/kernel/of_platform.c
+++ b/arch/powerpc/kernel/of_platform.c
@@ -86,9 +86,6 @@ static int of_pci_phb_probe(struct platform_device *dev)
 	/* Add probed PCI devices to the device model */
 	pci_bus_add_devices(phb->bus);
 
-	/* sysfs files should only be added after devices are added */
-	eeh_add_sysfs_files(phb->bus);
-
 	return 0;
 }
 
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index c6c0341..3d2b1cf 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1404,9 +1404,6 @@ void pcibios_finish_adding_to_bus(struct pci_bus *bus)
 
 	/* Add new devices to global lists.  Register in proc, sysfs. */
 	pci_bus_add_devices(bus);
-
-	/* sysfs files should only be added after devices are added */
-	eeh_add_sysfs_files(bus);
 }
 EXPORT_SYMBOL_GPL(pcibios_finish_adding_to_bus);
 
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index 6f300ab..ef727ec 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -48,7 +48,6 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
 	dev_dbg(&pdev->dev, "EEH: Setting up device\n");
 	eeh_add_device_early(pdn);
 	eeh_add_device_late(pdev);
-	eeh_sysfs_add_device(pdev);
 }
 
 static int pnv_eeh_init(void)
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 893ba3f..95bbf91 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -68,7 +68,6 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
 	}
 #endif
 	eeh_add_device_early(pdn);
-	eeh_add_device_late(pdev);
 #ifdef CONFIG_PCI_IOV
 	if (pdev->is_virtfn) {
 		struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
@@ -78,7 +77,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
 		eeh_add_to_parent_pe(edev);   /* Add as VF PE type */
 	}
 #endif
-	eeh_sysfs_add_device(pdev);
+	eeh_add_device_late(pdev);
 }
 
 /*
-- 
2.9.5

[PATCH 2/6] powerpc/eeh: Remove eeh_add_device_tree_late()

[Oliver O'Halloran]

On pseries and PowerNV pcibios_bus_add_device() calls eeh_add_device_late()
so there's no need to do a separate tree traversal to bind the eeh_dev and
pci_dev together setting up the PHB at boot. As a result we can remove
eeh_add_device_tree_late().

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/include/asm/eeh.h    |  3 ---
 arch/powerpc/kernel/eeh.c         | 25 -------------------------
 arch/powerpc/kernel/of_platform.c |  3 ---
 arch/powerpc/kernel/pci-common.c  |  3 ---
 4 files changed, 34 deletions(-)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 5a34907..5d10781 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -304,7 +304,6 @@ 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_add_device_tree_late(struct pci_bus *);
 void eeh_remove_device(struct pci_dev *);
 int eeh_unfreeze_pe(struct eeh_pe *pe);
 int eeh_pe_reset_and_recover(struct eeh_pe *pe);
@@ -365,8 +364,6 @@ 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_add_device_tree_late(struct pci_bus *bus) { }
-
 static inline void eeh_remove_device(struct pci_dev *dev) { }
 
 #define EEH_POSSIBLE_ERROR(val, type) (0)
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 0878912..9cb3370 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1214,31 +1214,6 @@ void eeh_add_device_late(struct pci_dev *dev)
 }
 
 /**
- * eeh_add_device_tree_late - Perform EEH initialization for the indicated PCI bus
- * @bus: PCI bus
- *
- * This routine must be used to perform EEH initialization for PCI
- * devices which are attached to the indicated PCI bus. The PCI bus
- * is added after system boot through hotplug or dlpar.
- */
-void eeh_add_device_tree_late(struct pci_bus *bus)
-{
-	struct pci_dev *dev;
-
-	if (eeh_has_flag(EEH_FORCE_DISABLED))
-		return;
-	list_for_each_entry(dev, &bus->devices, bus_list) {
-		eeh_add_device_late(dev);
-		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
-			struct pci_bus *subbus = dev->subordinate;
-			if (subbus)
-				eeh_add_device_tree_late(subbus);
-		}
-	}
-}
-EXPORT_SYMBOL_GPL(eeh_add_device_tree_late);
-
-/**
  * eeh_remove_device - Undo EEH setup for the indicated pci device
  * @dev: pci device to be removed
  *
diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
index cb68800..64edac81 100644
--- a/arch/powerpc/kernel/of_platform.c
+++ b/arch/powerpc/kernel/of_platform.c
@@ -80,9 +80,6 @@ static int of_pci_phb_probe(struct platform_device *dev)
 	 */
 	pcibios_claim_one_bus(phb->bus);
 
-	/* Finish EEH setup */
-	eeh_add_device_tree_late(phb->bus);
-
 	/* Add probed PCI devices to the device model */
 	pci_bus_add_devices(phb->bus);
 
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 3d2b1cf..8983afa 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1399,9 +1399,6 @@ void pcibios_finish_adding_to_bus(struct pci_bus *bus)
 			pci_assign_unassigned_bus_resources(bus);
 	}
 
-	/* Fixup EEH */
-	eeh_add_device_tree_late(bus);
-
 	/* Add new devices to global lists.  Register in proc, sysfs. */
 	pci_bus_add_devices(bus);
 }
-- 
2.9.5

[PATCH 3/6] powerpc/eeh: Do early EEH init only when required

[Oliver O'Halloran]

The pci hotplug helper (pci_hp_add_devices()) calls
eeh_add_device_tree_early() to scan the device-tree for new PCI devices and
do the early EEH probe before the device is scanned. This early probe is a
no-op in a lot of cases because:

a) The early init is only required to satisfy a PAPR requirement that EEH
   be configured before we start doing config accesses. On PowerNV it is
   a no-op.

b) It's a no-op for devices that have already had their eeh_dev
   initialised.

There are four callers of pci_hp_add_devices():

1. arch/powerpc/kernel/eeh_driver.c
	Here the hotplug helper is called when re-scanning pci_devs that
	were removed during an EEH recovery pass. The EEH stat for each
	removed device (the eeh_dev) is retained across a recovery pass
	so the early init is a no-op in this case.

2. drivers/pci/hotplug/pnv_php.c
	This is also a no-op since the PowerNV hotplug driver is, suprisingly,
	PowerNV specific.

3. drivers/pci/hotplug/rpaphp_core.c
4. drivers/pci/hotplug/rpaphp_pci.c
	In these two cases new devices have been hotplugged and FW has
	provided new DT nodes for each. These are the only two cases where
	the EEH we might have new PCI device nodes in the DT so these are
	the only two cases where the early EEH probe needs to be done.

We can move the calls to eeh_add_device_tree_early() to the locations where
it's needed and remove it from the generic path. This is preparation for
making the early EEH probe pseries specific.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/kernel/pci-hotplug.c | 2 --
 drivers/pci/hotplug/rpaphp_core.c | 2 ++
 drivers/pci/hotplug/rpaphp_pci.c  | 4 +++-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
index d6a67f8..bf83f76 100644
--- a/arch/powerpc/kernel/pci-hotplug.c
+++ b/arch/powerpc/kernel/pci-hotplug.c
@@ -112,8 +112,6 @@ void pci_hp_add_devices(struct pci_bus *bus)
 	struct pci_controller *phb;
 	struct device_node *dn = pci_bus_to_OF_node(bus);
 
-	eeh_add_device_tree_early(PCI_DN(dn));
-
 	phb = pci_bus_to_host(bus);
 
 	mode = PCI_PROBE_NORMAL;
diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index e408e40..9c1e43e 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -494,6 +494,8 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
 		return retval;
 
 	if (state == PRESENT) {
+		eeh_add_device_tree_early(PCI_DN(slot->dn));
+
 		pci_lock_rescan_remove();
 		pci_hp_add_devices(slot->bus);
 		pci_unlock_rescan_remove();
diff --git a/drivers/pci/hotplug/rpaphp_pci.c b/drivers/pci/hotplug/rpaphp_pci.c
index beca61b..61ebbd8 100644
--- a/drivers/pci/hotplug/rpaphp_pci.c
+++ b/drivers/pci/hotplug/rpaphp_pci.c
@@ -95,8 +95,10 @@ int rpaphp_enable_slot(struct slot *slot)
 			return -EINVAL;
 		}
 
-		if (list_empty(&bus->devices))
+		if (list_empty(&bus->devices)) {
+			eeh_add_device_tree_early(PCI_DN(slot->dn));
 			pci_hp_add_devices(bus);
+		}
 
 		if (!list_empty(&bus->devices)) {
 			slot->state = CONFIGURED;
-- 
2.9.5

[PATCH 4/6] powerpc/eeh: Remove PHB check in probe

[Oliver O'Halloran]

This check for a missing PHB has existing in various forms since the
initial PPC64 port was upstreamed in 2002. The idea seems to be that we
need to guard against creating pci-specific data structures for the non-pci
children of a PCI device tree node (e.g. USB devices). However, we only
create pci_dn structures for DT nodes that correspond to PCI devices so
there's not much point in doing this check in the eeh_probe path.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/kernel/eeh.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 9cb3370..a9e4ca7 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1120,7 +1120,6 @@ core_initcall_sync(eeh_init);
  */
 void eeh_add_device_early(struct pci_dn *pdn)
 {
-	struct pci_controller *phb = pdn ? pdn->phb : NULL;
 	struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
 
 	if (!edev)
@@ -1129,11 +1128,6 @@ void eeh_add_device_early(struct pci_dn *pdn)
 	if (!eeh_has_flag(EEH_PROBE_MODE_DEVTREE))
 		return;
 
-	/* USB Bus children of PCI devices will not have BUID's */
-	if (NULL == phb ||
-	    (eeh_has_flag(EEH_PROBE_MODE_DEVTREE) && 0 == phb->buid))
-		return;
-
 	eeh_ops->probe(pdn, NULL);
 }
 
-- 
2.9.5

[PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific

[Oliver O'Halloran]

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
   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.

Signed-off-by: Oliver O'Halloran <oohall@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

[PATCH 6/6] powerpc/eeh: Rework eeh_ops->probe()

[Oliver O'Halloran]

With the EEH early probe now being pseries specific there's no need for
eeh_ops->probe() to take a pci_dn. Instead, we can make it take a pci_dev
and use the probe function to map a pci_dev to an eeh_dev. This allows
the platform to implement it's own method for finding (or creating) an
eeh_dev for a given pci_dev which also removes a use of pci_dn in
generic EEH code.

This patch also renames eeh_device_add_late() to eeh_device_probe(). This
better reflects what it does does and removes the last vestiges of the
early/late EEH probe split.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/include/asm/eeh.h               |  6 ++--
 arch/powerpc/kernel/eeh.c                    | 42 +++++++++++++++-------------
 arch/powerpc/platforms/powernv/eeh-powernv.c | 30 ++++++++++----------
 arch/powerpc/platforms/pseries/eeh_pseries.c | 23 ++++++++++++++-
 4 files changed, 61 insertions(+), 40 deletions(-)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 8580238..964a542 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -215,7 +215,7 @@ enum {
 struct eeh_ops {
 	char *name;
 	int (*init)(void);
-	void* (*probe)(struct pci_dn *pdn, void *data);
+	struct eeh_dev *(*probe)(struct pci_dev *pdev);
 	int (*set_option)(struct eeh_pe *pe, int option);
 	int (*get_pe_addr)(struct eeh_pe *pe);
 	int (*get_state)(struct eeh_pe *pe, int *delay);
@@ -301,7 +301,7 @@ 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_late(struct pci_dev *);
+void eeh_probe_device(struct pci_dev *pdev);
 void eeh_remove_device(struct pci_dev *);
 int eeh_unfreeze_pe(struct eeh_pe *pe);
 int eeh_pe_reset_and_recover(struct eeh_pe *pe);
@@ -356,7 +356,7 @@ 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_late(struct pci_dev *dev) { }
+static inline void eeh_probe_device(struct pci_dev *dev) { }
 
 static inline void eeh_remove_device(struct pci_dev *dev) { }
 
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 55d3ef6..2c5f7a6 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1113,29 +1113,37 @@ core_initcall_sync(eeh_init);
  * This routine must be used to complete EEH initialization for PCI
  * devices that were added after system boot (e.g. hotplug, dlpar).
  */
-void eeh_add_device_late(struct pci_dev *dev)
+void eeh_probe_device(struct pci_dev *dev)
 {
-	struct pci_dn *pdn;
 	struct eeh_dev *edev;
 
-	if (!dev)
+	pr_debug("EEH: Adding device %s\n", pci_name(dev));
+
+	/*
+	 * pci_dev_to_eeh_dev() can only work if eeh_probe_dev() was
+	 * already called for this device.
+	 */
+	if (WARN_ON_ONCE(pci_dev_to_eeh_dev(dev))) {
+		eeh_edev_dbg(edev, "Already bound to an eeh_dev!\n");
 		return;
+	}
 
-	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
-	edev = pdn_to_eeh_dev(pdn);
-	eeh_edev_dbg(edev, "Adding device\n");
-	if (edev->pdev == dev) {
-		eeh_edev_dbg(edev, "Device already referenced!\n");
+	edev = eeh_ops->probe(dev);
+	if (!edev) {
+		pr_debug("EEH: Adding device failed\n");
 		return;
 	}
 
 	/*
-	 * The EEH cache might not be removed correctly because of
-	 * unbalanced kref to the device during unplug time, which
-	 * relies on pcibios_release_device(). So we have to remove
-	 * that here explicitly.
+	 * FIXME: We rely on pcibios_release_device() to remove the
+	 * existing EEH state. The release function is only called if
+	 * the pci_dev's refcount drops to zero so if something is
+	 * keeping a ref to a device (e.g. a filesystem) we need to
+	 * remove the old EEH state.
+	 *
+	 * FIXME: HEY MA, LOOK AT ME, NO LOCKING!
 	 */
-	if (edev->pdev) {
+	if (edev->pdev && edev->pdev != dev) {
 		eeh_rmv_from_parent_pe(edev);
 		eeh_addr_cache_rmv_dev(edev->pdev);
 		eeh_sysfs_remove_device(edev->pdev);
@@ -1146,17 +1154,11 @@ void eeh_add_device_late(struct pci_dev *dev)
 		 * into error handler afterwards.
 		 */
 		edev->mode |= EEH_DEV_NO_HANDLER;
-
-		edev->pdev = NULL;
-		dev->dev.archdata.edev = NULL;
 	}
 
-	if (eeh_has_flag(EEH_PROBE_MODE_DEV))
-		eeh_ops->probe(pdn, NULL);
-
+	/* bind the pdev and the edev together */
 	edev->pdev = dev;
 	dev->dev.archdata.edev = edev;
-
 	eeh_addr_cache_insert_dev(dev);
 	eeh_sysfs_add_device(dev);
 }
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index eaa8dfe..79409e0 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -41,7 +41,7 @@ static int eeh_event_irq = -EINVAL;
 void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
 {
 	dev_dbg(&pdev->dev, "EEH: Setting up device\n");
-	eeh_add_device_late(pdev);
+	eeh_probe_device(pdev);
 }
 
 static int pnv_eeh_init(void)
@@ -340,23 +340,13 @@ static int pnv_eeh_find_ecap(struct pci_dn *pdn, int cap)
 
 /**
  * pnv_eeh_probe - Do probe on PCI device
- * @pdn: PCI device node
- * @data: unused
+ * @pdev: pci_dev to probe
  *
- * 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. By default, EEH has been enabled
- * on all PCI devices. That's to say, we only need do necessary
- * initialization on the corresponding eeh device and create PE
- * accordingly.
- *
- * It's notable that's unsafe to retrieve the EEH device through
- * the corresponding PCI device. During the PCI device hotplug, which
- * was possiblly triggered by EEH core, the binding between EEH device
- * and the PCI device isn't built yet.
+ * Create, or find the existing, eeh_dev for this pci_dev.
  */
-static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
+static struct eeh_dev *pnv_eeh_probe(struct pci_dev *pdev)
 {
+	struct pci_dn *pdn = pci_get_pdn(pdev);
 	struct pci_controller *hose = pdn->phb;
 	struct pnv_phb *phb = hose->private_data;
 	struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
@@ -373,6 +363,14 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
 	if (!edev || edev->pe)
 		return NULL;
 
+	/* already configured? */
+	if (edev->pdev) {
+		pr_debug("%s: found existing edev for %04x:%02x:%02x.%01x\n",
+			__func__, hose->global_number, config_addr >> 8,
+			PCI_SLOT(config_addr), PCI_FUNC(config_addr));
+		return edev;
+	}
+
 	/* Skip for PCI-ISA bridge */
 	if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
 		return NULL;
@@ -464,7 +462,7 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
 
 	eeh_edev_dbg(edev, "EEH enabled on device\n");
 
-	return NULL;
+	return edev;
 }
 
 /**
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 1ca7cf0..8453428 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -77,7 +77,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
 		eeh_add_to_parent_pe(edev);   /* Add as VF PE type */
 	}
 #endif
-	eeh_add_device_late(pdev);
+	eeh_probe_device(pdev);
 }
 
 /*
@@ -335,6 +335,26 @@ void pseries_eeh_init_edev(struct pci_dn *pdn)
 	eeh_save_bars(edev);
 }
 
+static struct eeh_dev *pseries_eeh_probe(struct pci_dev *pdev)
+{
+	struct eeh_dev *edev;
+	struct pci_dn *pdn;
+
+	pdn = pci_get_pdn_by_devfn(pdev->bus, pdev->devfn);
+	if (!pdn)
+		return NULL;
+
+	/*
+	 * If the system supports EEH on this device then the eeh_dev was
+	 * configured and inserted into a PE in pseries_eeh_init_edev()
+	 */
+	edev = pdn_to_eeh_dev(pdn);
+	if (!edev || !edev->pe)
+		return NULL;
+
+	return edev;
+}
+
 /**
  * pseries_eeh_init_edev_recursive - Enable EEH for the indicated device
  * @pdn: PCI device node
@@ -813,6 +833,7 @@ 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,
-- 
2.9.5

Re: [PATCH 1/6] powerpc/eeh: Add sysfs files in late probe

[Sam Bobroff]

[-- Attachment #1: Type: text/plain, Size: 6492 bytes --]

On Mon, Feb 03, 2020 at 07:35:16PM +1100, Oliver O'Halloran wrote:
> Move creating the EEH specific sysfs files into eeh_add_device_late()
> rather than being open-coded all over the place. Calling the function is
> generally done immediately after calling eeh_add_device_late() anyway. The
> two cases where it's not done there (OF based PCI probing and the pseries
> VFs) don't seem to have any issues with the re-ordering.

I haven't tested it explicitly, but I suspect the re-ordering will
actually improve things: in some error cases it will no longer add sysfs
files for devices that have failed to init, because bailing out in
eeh_add_device_late() (or eeh_probve_device()) will now prevent
eeh_sysfs_add_device() from being called.

Nice cleanup.

Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>

> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
>  arch/powerpc/include/asm/eeh.h               |  3 ---
>  arch/powerpc/kernel/eeh.c                    | 24 +-----------------------
>  arch/powerpc/kernel/of_platform.c            |  3 ---
>  arch/powerpc/kernel/pci-common.c             |  3 ---
>  arch/powerpc/platforms/powernv/eeh-powernv.c |  1 -
>  arch/powerpc/platforms/pseries/eeh_pseries.c |  3 +--
>  6 files changed, 2 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 6f9b2a1..5a34907 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -305,7 +305,6 @@ 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_add_device_tree_late(struct pci_bus *);
> -void eeh_add_sysfs_files(struct pci_bus *);
>  void eeh_remove_device(struct pci_dev *);
>  int eeh_unfreeze_pe(struct eeh_pe *pe);
>  int eeh_pe_reset_and_recover(struct eeh_pe *pe);
> @@ -368,8 +367,6 @@ static inline void eeh_add_device_late(struct pci_dev *dev) { }
>  
>  static inline void eeh_add_device_tree_late(struct pci_bus *bus) { }
>  
> -static inline void eeh_add_sysfs_files(struct pci_bus *bus) { }
> -
>  static inline void eeh_remove_device(struct pci_dev *dev) { }
>  
>  #define EEH_POSSIBLE_ERROR(val, type) (0)
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 17cb3e9..0878912 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1210,6 +1210,7 @@ void eeh_add_device_late(struct pci_dev *dev)
>  	dev->dev.archdata.edev = edev;
>  
>  	eeh_addr_cache_insert_dev(dev);
> +	eeh_sysfs_add_device(dev);
>  }
>  
>  /**
> @@ -1238,29 +1239,6 @@ void eeh_add_device_tree_late(struct pci_bus *bus)
>  EXPORT_SYMBOL_GPL(eeh_add_device_tree_late);
>  
>  /**
> - * eeh_add_sysfs_files - Add EEH sysfs files for the indicated PCI bus
> - * @bus: PCI bus
> - *
> - * This routine must be used to add EEH sysfs files for PCI
> - * devices which are attached to the indicated PCI bus. The PCI bus
> - * is added after system boot through hotplug or dlpar.
> - */
> -void eeh_add_sysfs_files(struct pci_bus *bus)
> -{
> -	struct pci_dev *dev;
> -
> -	list_for_each_entry(dev, &bus->devices, bus_list) {
> -		eeh_sysfs_add_device(dev);
> -		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> -			struct pci_bus *subbus = dev->subordinate;
> -			if (subbus)
> -				eeh_add_sysfs_files(subbus);
> -		}
> -	}
> -}
> -EXPORT_SYMBOL_GPL(eeh_add_sysfs_files);
> -
> -/**
>   * eeh_remove_device - Undo EEH setup for the indicated pci device
>   * @dev: pci device to be removed
>   *
> diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
> index 427fc22..cb68800 100644
> --- a/arch/powerpc/kernel/of_platform.c
> +++ b/arch/powerpc/kernel/of_platform.c
> @@ -86,9 +86,6 @@ static int of_pci_phb_probe(struct platform_device *dev)
>  	/* Add probed PCI devices to the device model */
>  	pci_bus_add_devices(phb->bus);
>  
> -	/* sysfs files should only be added after devices are added */
> -	eeh_add_sysfs_files(phb->bus);
> -

So for this case, the sysfs files are added by pci_bus_add_devices(),
via...
	pci_bus_add_devices() (loops over devices) ->
	pci_bus_add_device() ->
	pcibios_bus_add_device() ->
	ppc_md.pcibios_bus_add_device() ->
	{pseries,pnv}_pcibios_bus_add_device() ->
	eeh_add_device_late() ->
	eeh_sysfs_add_device().

>  	return 0;
>  }
>  
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index c6c0341..3d2b1cf 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1404,9 +1404,6 @@ void pcibios_finish_adding_to_bus(struct pci_bus *bus)
>  
>  	/* Add new devices to global lists.  Register in proc, sysfs. */
>  	pci_bus_add_devices(bus);
> -
> -	/* sysfs files should only be added after devices are added */
> -	eeh_add_sysfs_files(bus);

As above.

>  }
>  EXPORT_SYMBOL_GPL(pcibios_finish_adding_to_bus);
>  
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index 6f300ab..ef727ec 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -48,7 +48,6 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
>  	dev_dbg(&pdev->dev, "EEH: Setting up device\n");
>  	eeh_add_device_early(pdn);
>  	eeh_add_device_late(pdev);
> -	eeh_sysfs_add_device(pdev);

So for this case, the sysfs files are added by eeh_add_device_late(),
via...
	eeh_add_device_late() ->
	eeh_sysfs_add_device().

>  }
>  
>  static int pnv_eeh_init(void)
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 893ba3f..95bbf91 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -68,7 +68,6 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
>  	}
>  #endif
>  	eeh_add_device_early(pdn);
> -	eeh_add_device_late(pdev);
>  #ifdef CONFIG_PCI_IOV
>  	if (pdev->is_virtfn) {
>  		struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> @@ -78,7 +77,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
>  		eeh_add_to_parent_pe(edev);   /* Add as VF PE type */
>  	}
>  #endif
> -	eeh_sysfs_add_device(pdev);
> +	eeh_add_device_late(pdev);

This is just a re-ordering.

>  }
>  
>  /*
> -- 
> 2.9.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/6] powerpc/eeh: Remove eeh_add_device_tree_late()

[Sam Bobroff]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]

On Mon, Feb 03, 2020 at 07:35:17PM +1100, Oliver O'Halloran wrote:
> On pseries and PowerNV pcibios_bus_add_device() calls eeh_add_device_late()
> so there's no need to do a separate tree traversal to bind the eeh_dev and
> pci_dev together setting up the PHB at boot. As a result we can remove
> eeh_add_device_tree_late().
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

... with pcibios_bus_add_device() being called from
pci_bus_add_devices(), in this case.

Looks good.

Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>

> ---
>  arch/powerpc/include/asm/eeh.h    |  3 ---
>  arch/powerpc/kernel/eeh.c         | 25 -------------------------
>  arch/powerpc/kernel/of_platform.c |  3 ---
>  arch/powerpc/kernel/pci-common.c  |  3 ---
>  4 files changed, 34 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 5a34907..5d10781 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -304,7 +304,6 @@ 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_add_device_tree_late(struct pci_bus *);
>  void eeh_remove_device(struct pci_dev *);
>  int eeh_unfreeze_pe(struct eeh_pe *pe);
>  int eeh_pe_reset_and_recover(struct eeh_pe *pe);
> @@ -365,8 +364,6 @@ 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_add_device_tree_late(struct pci_bus *bus) { }
> -
>  static inline void eeh_remove_device(struct pci_dev *dev) { }
>  
>  #define EEH_POSSIBLE_ERROR(val, type) (0)
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 0878912..9cb3370 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1214,31 +1214,6 @@ void eeh_add_device_late(struct pci_dev *dev)
>  }
>  
>  /**
> - * eeh_add_device_tree_late - Perform EEH initialization for the indicated PCI bus
> - * @bus: PCI bus
> - *
> - * This routine must be used to perform EEH initialization for PCI
> - * devices which are attached to the indicated PCI bus. The PCI bus
> - * is added after system boot through hotplug or dlpar.
> - */
> -void eeh_add_device_tree_late(struct pci_bus *bus)
> -{
> -	struct pci_dev *dev;
> -
> -	if (eeh_has_flag(EEH_FORCE_DISABLED))
> -		return;
> -	list_for_each_entry(dev, &bus->devices, bus_list) {
> -		eeh_add_device_late(dev);
> -		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> -			struct pci_bus *subbus = dev->subordinate;
> -			if (subbus)
> -				eeh_add_device_tree_late(subbus);
> -		}
> -	}
> -}
> -EXPORT_SYMBOL_GPL(eeh_add_device_tree_late);
> -
> -/**
>   * eeh_remove_device - Undo EEH setup for the indicated pci device
>   * @dev: pci device to be removed
>   *
> diff --git a/arch/powerpc/kernel/of_platform.c b/arch/powerpc/kernel/of_platform.c
> index cb68800..64edac81 100644
> --- a/arch/powerpc/kernel/of_platform.c
> +++ b/arch/powerpc/kernel/of_platform.c
> @@ -80,9 +80,6 @@ static int of_pci_phb_probe(struct platform_device *dev)
>  	 */
>  	pcibios_claim_one_bus(phb->bus);
>  
> -	/* Finish EEH setup */
> -	eeh_add_device_tree_late(phb->bus);
> -
>  	/* Add probed PCI devices to the device model */
>  	pci_bus_add_devices(phb->bus);
>  
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 3d2b1cf..8983afa 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1399,9 +1399,6 @@ void pcibios_finish_adding_to_bus(struct pci_bus *bus)
>  			pci_assign_unassigned_bus_resources(bus);
>  	}
>  
> -	/* Fixup EEH */
> -	eeh_add_device_tree_late(bus);
> -
>  	/* Add new devices to global lists.  Register in proc, sysfs. */
>  	pci_bus_add_devices(bus);
>  }
> -- 
> 2.9.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/6] powerpc/eeh: Do early EEH init only when required

[Sam Bobroff]

[-- Attachment #1: Type: text/plain, Size: 3515 bytes --]

On Mon, Feb 03, 2020 at 07:35:18PM +1100, Oliver O'Halloran wrote:
> The pci hotplug helper (pci_hp_add_devices()) calls
> eeh_add_device_tree_early() to scan the device-tree for new PCI devices and
> do the early EEH probe before the device is scanned. This early probe is a
> no-op in a lot of cases because:
> 
> a) The early init is only required to satisfy a PAPR requirement that EEH
>    be configured before we start doing config accesses. On PowerNV it is
>    a no-op.
> 
> b) It's a no-op for devices that have already had their eeh_dev
>    initialised.
> 
> There are four callers of pci_hp_add_devices():
> 
> 1. arch/powerpc/kernel/eeh_driver.c
> 	Here the hotplug helper is called when re-scanning pci_devs that
> 	were removed during an EEH recovery pass. The EEH stat for each
> 	removed device (the eeh_dev) is retained across a recovery pass
> 	so the early init is a no-op in this case.
> 
> 2. drivers/pci/hotplug/pnv_php.c
> 	This is also a no-op since the PowerNV hotplug driver is, suprisingly,
> 	PowerNV specific.
> 
> 3. drivers/pci/hotplug/rpaphp_core.c
> 4. drivers/pci/hotplug/rpaphp_pci.c
> 	In these two cases new devices have been hotplugged and FW has
> 	provided new DT nodes for each. These are the only two cases where
> 	the EEH we might have new PCI device nodes in the DT so these are
> 	the only two cases where the early EEH probe needs to be done.
> 
> We can move the calls to eeh_add_device_tree_early() to the locations where
> it's needed and remove it from the generic path. This is preparation for
> making the early EEH probe pseries specific.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Makes sense to me.
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>

> ---
>  arch/powerpc/kernel/pci-hotplug.c | 2 --
>  drivers/pci/hotplug/rpaphp_core.c | 2 ++
>  drivers/pci/hotplug/rpaphp_pci.c  | 4 +++-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
> index d6a67f8..bf83f76 100644
> --- a/arch/powerpc/kernel/pci-hotplug.c
> +++ b/arch/powerpc/kernel/pci-hotplug.c
> @@ -112,8 +112,6 @@ void pci_hp_add_devices(struct pci_bus *bus)
>  	struct pci_controller *phb;
>  	struct device_node *dn = pci_bus_to_OF_node(bus);
>  
> -	eeh_add_device_tree_early(PCI_DN(dn));
> -
>  	phb = pci_bus_to_host(bus);
>  
>  	mode = PCI_PROBE_NORMAL;
> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> index e408e40..9c1e43e 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -494,6 +494,8 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
>  		return retval;
>  
>  	if (state == PRESENT) {
> +		eeh_add_device_tree_early(PCI_DN(slot->dn));
> +
>  		pci_lock_rescan_remove();
>  		pci_hp_add_devices(slot->bus);
>  		pci_unlock_rescan_remove();
> diff --git a/drivers/pci/hotplug/rpaphp_pci.c b/drivers/pci/hotplug/rpaphp_pci.c
> index beca61b..61ebbd8 100644
> --- a/drivers/pci/hotplug/rpaphp_pci.c
> +++ b/drivers/pci/hotplug/rpaphp_pci.c
> @@ -95,8 +95,10 @@ int rpaphp_enable_slot(struct slot *slot)
>  			return -EINVAL;
>  		}
>  
> -		if (list_empty(&bus->devices))
> +		if (list_empty(&bus->devices)) {
> +			eeh_add_device_tree_early(PCI_DN(slot->dn));
>  			pci_hp_add_devices(bus);
> +		}
>  
>  		if (!list_empty(&bus->devices)) {
>  			slot->state = CONFIGURED;
> -- 
> 2.9.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/6] powerpc/eeh: Remove PHB check in probe

[Sam Bobroff]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On Mon, Feb 03, 2020 at 07:35:19PM +1100, Oliver O'Halloran wrote:
> This check for a missing PHB has existing in various forms since the
> initial PPC64 port was upstreamed in 2002. The idea seems to be that we
> need to guard against creating pci-specific data structures for the non-pci
> children of a PCI device tree node (e.g. USB devices). However, we only
> create pci_dn structures for DT nodes that correspond to PCI devices so
> there's not much point in doing this check in the eeh_probe path.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

I always wondered how to test that block... and it's just dead code.
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>

> ---
>  arch/powerpc/kernel/eeh.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 9cb3370..a9e4ca7 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1120,7 +1120,6 @@ core_initcall_sync(eeh_init);
>   */
>  void eeh_add_device_early(struct pci_dn *pdn)
>  {
> -	struct pci_controller *phb = pdn ? pdn->phb : NULL;
>  	struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
>  
>  	if (!edev)
> @@ -1129,11 +1128,6 @@ void eeh_add_device_early(struct pci_dn *pdn)
>  	if (!eeh_has_flag(EEH_PROBE_MODE_DEVTREE))
>  		return;
>  
> -	/* USB Bus children of PCI devices will not have BUID's */
> -	if (NULL == phb ||
> -	    (eeh_has_flag(EEH_PROBE_MODE_DEVTREE) && 0 == phb->buid))
> -		return;
> -
>  	eeh_ops->probe(pdn, NULL);
>  }
>  
> -- 
> 2.9.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific

[Sam Bobroff]

[-- Attachment #1: Type: text/plain, Size: 13378 bytes --]

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@linux.ibm.com>

> Signed-off-by: Oliver O'Halloran <oohall@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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 6/6] powerpc/eeh: Rework eeh_ops->probe()

[Sam Bobroff]

[-- Attachment #1: Type: text/plain, Size: 9210 bytes --]

On Mon, Feb 03, 2020 at 07:35:21PM +1100, Oliver O'Halloran wrote:
> With the EEH early probe now being pseries specific there's no need for
> eeh_ops->probe() to take a pci_dn. Instead, we can make it take a pci_dev
> and use the probe function to map a pci_dev to an eeh_dev. This allows
> the platform to implement it's own method for finding (or creating) an
> eeh_dev for a given pci_dev which also removes a use of pci_dn in
> generic EEH code.
> 
> This patch also renames eeh_device_add_late() to eeh_device_probe(). This
> better reflects what it does does and removes the last vestiges of the
> early/late EEH probe split.

Nice!
Just one nit, below.

Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>


> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
>  arch/powerpc/include/asm/eeh.h               |  6 ++--
>  arch/powerpc/kernel/eeh.c                    | 42 +++++++++++++++-------------
>  arch/powerpc/platforms/powernv/eeh-powernv.c | 30 ++++++++++----------
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 23 ++++++++++++++-
>  4 files changed, 61 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 8580238..964a542 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -215,7 +215,7 @@ enum {
>  struct eeh_ops {
>  	char *name;
>  	int (*init)(void);
> -	void* (*probe)(struct pci_dn *pdn, void *data);
> +	struct eeh_dev *(*probe)(struct pci_dev *pdev);
>  	int (*set_option)(struct eeh_pe *pe, int option);
>  	int (*get_pe_addr)(struct eeh_pe *pe);
>  	int (*get_state)(struct eeh_pe *pe, int *delay);
> @@ -301,7 +301,7 @@ 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_late(struct pci_dev *);
> +void eeh_probe_device(struct pci_dev *pdev);
>  void eeh_remove_device(struct pci_dev *);
>  int eeh_unfreeze_pe(struct eeh_pe *pe);
>  int eeh_pe_reset_and_recover(struct eeh_pe *pe);
> @@ -356,7 +356,7 @@ 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_late(struct pci_dev *dev) { }
> +static inline void eeh_probe_device(struct pci_dev *dev) { }
>  
>  static inline void eeh_remove_device(struct pci_dev *dev) { }
>  
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 55d3ef6..2c5f7a6 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1113,29 +1113,37 @@ core_initcall_sync(eeh_init);
>   * This routine must be used to complete EEH initialization for PCI
>   * devices that were added after system boot (e.g. hotplug, dlpar).
>   */

You can't see it in the patch but up a few lines in the comment block,
there's a leftover "eeh_add_device_late".

> -void eeh_add_device_late(struct pci_dev *dev)
> +void eeh_probe_device(struct pci_dev *dev)
>  {
> -	struct pci_dn *pdn;
>  	struct eeh_dev *edev;
>  
> -	if (!dev)
> +	pr_debug("EEH: Adding device %s\n", pci_name(dev));
> +
> +	/*
> +	 * pci_dev_to_eeh_dev() can only work if eeh_probe_dev() was
> +	 * already called for this device.
> +	 */
> +	if (WARN_ON_ONCE(pci_dev_to_eeh_dev(dev))) {
> +		eeh_edev_dbg(edev, "Already bound to an eeh_dev!\n");
>  		return;
> +	}
>  
> -	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
> -	edev = pdn_to_eeh_dev(pdn);
> -	eeh_edev_dbg(edev, "Adding device\n");
> -	if (edev->pdev == dev) {
> -		eeh_edev_dbg(edev, "Device already referenced!\n");
> +	edev = eeh_ops->probe(dev);
> +	if (!edev) {
> +		pr_debug("EEH: Adding device failed\n");
>  		return;
>  	}
>  
>  	/*
> -	 * The EEH cache might not be removed correctly because of
> -	 * unbalanced kref to the device during unplug time, which
> -	 * relies on pcibios_release_device(). So we have to remove
> -	 * that here explicitly.
> +	 * FIXME: We rely on pcibios_release_device() to remove the
> +	 * existing EEH state. The release function is only called if
> +	 * the pci_dev's refcount drops to zero so if something is
> +	 * keeping a ref to a device (e.g. a filesystem) we need to
> +	 * remove the old EEH state.
> +	 *
> +	 * FIXME: HEY MA, LOOK AT ME, NO LOCKING!
>  	 */
> -	if (edev->pdev) {
> +	if (edev->pdev && edev->pdev != dev) {
>  		eeh_rmv_from_parent_pe(edev);
>  		eeh_addr_cache_rmv_dev(edev->pdev);
>  		eeh_sysfs_remove_device(edev->pdev);
> @@ -1146,17 +1154,11 @@ void eeh_add_device_late(struct pci_dev *dev)
>  		 * into error handler afterwards.
>  		 */
>  		edev->mode |= EEH_DEV_NO_HANDLER;
> -
> -		edev->pdev = NULL;
> -		dev->dev.archdata.edev = NULL;
>  	}
>  
> -	if (eeh_has_flag(EEH_PROBE_MODE_DEV))
> -		eeh_ops->probe(pdn, NULL);
> -
> +	/* bind the pdev and the edev together */
>  	edev->pdev = dev;
>  	dev->dev.archdata.edev = edev;
> -
>  	eeh_addr_cache_insert_dev(dev);
>  	eeh_sysfs_add_device(dev);
>  }
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index eaa8dfe..79409e0 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -41,7 +41,7 @@ static int eeh_event_irq = -EINVAL;
>  void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
>  {
>  	dev_dbg(&pdev->dev, "EEH: Setting up device\n");
> -	eeh_add_device_late(pdev);
> +	eeh_probe_device(pdev);
>  }
>  
>  static int pnv_eeh_init(void)
> @@ -340,23 +340,13 @@ static int pnv_eeh_find_ecap(struct pci_dn *pdn, int cap)
>  
>  /**
>   * pnv_eeh_probe - Do probe on PCI device
> - * @pdn: PCI device node
> - * @data: unused
> + * @pdev: pci_dev to probe
>   *
> - * 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. By default, EEH has been enabled
> - * on all PCI devices. That's to say, we only need do necessary
> - * initialization on the corresponding eeh device and create PE
> - * accordingly.
> - *
> - * It's notable that's unsafe to retrieve the EEH device through
> - * the corresponding PCI device. During the PCI device hotplug, which
> - * was possiblly triggered by EEH core, the binding between EEH device
> - * and the PCI device isn't built yet.
> + * Create, or find the existing, eeh_dev for this pci_dev.
>   */
> -static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
> +static struct eeh_dev *pnv_eeh_probe(struct pci_dev *pdev)
>  {
> +	struct pci_dn *pdn = pci_get_pdn(pdev);
>  	struct pci_controller *hose = pdn->phb;
>  	struct pnv_phb *phb = hose->private_data;
>  	struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
> @@ -373,6 +363,14 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
>  	if (!edev || edev->pe)
>  		return NULL;
>  
> +	/* already configured? */
> +	if (edev->pdev) {
> +		pr_debug("%s: found existing edev for %04x:%02x:%02x.%01x\n",
> +			__func__, hose->global_number, config_addr >> 8,
> +			PCI_SLOT(config_addr), PCI_FUNC(config_addr));
> +		return edev;
> +	}
> +
>  	/* Skip for PCI-ISA bridge */
>  	if ((pdn->class_code >> 8) == PCI_CLASS_BRIDGE_ISA)
>  		return NULL;
> @@ -464,7 +462,7 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
>  
>  	eeh_edev_dbg(edev, "EEH enabled on device\n");
>  
> -	return NULL;
> +	return edev;
>  }
>  
>  /**
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 1ca7cf0..8453428 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -77,7 +77,7 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
>  		eeh_add_to_parent_pe(edev);   /* Add as VF PE type */
>  	}
>  #endif
> -	eeh_add_device_late(pdev);
> +	eeh_probe_device(pdev);
>  }
>  
>  /*
> @@ -335,6 +335,26 @@ void pseries_eeh_init_edev(struct pci_dn *pdn)
>  	eeh_save_bars(edev);
>  }
>  
> +static struct eeh_dev *pseries_eeh_probe(struct pci_dev *pdev)
> +{
> +	struct eeh_dev *edev;
> +	struct pci_dn *pdn;
> +
> +	pdn = pci_get_pdn_by_devfn(pdev->bus, pdev->devfn);
> +	if (!pdn)
> +		return NULL;
> +
> +	/*
> +	 * If the system supports EEH on this device then the eeh_dev was
> +	 * configured and inserted into a PE in pseries_eeh_init_edev()
> +	 */
> +	edev = pdn_to_eeh_dev(pdn);
> +	if (!edev || !edev->pe)
> +		return NULL;
> +
> +	return edev;
> +}
> +
>  /**
>   * pseries_eeh_init_edev_recursive - Enable EEH for the indicated device
>   * @pdn: PCI device node
> @@ -813,6 +833,7 @@ 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,
> -- 
> 2.9.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/6] powerpc/eeh: Add sysfs files in late probe

[Oliver O'Halloran]

On Thu, Feb 6, 2020 at 3:13 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
>
> On Mon, Feb 03, 2020 at 07:35:16PM +1100, Oliver O'Halloran wrote:
> > Move creating the EEH specific sysfs files into eeh_add_device_late()
> > rather than being open-coded all over the place. Calling the function is
> > generally done immediately after calling eeh_add_device_late() anyway. The
> > two cases where it's not done there (OF based PCI probing and the pseries
> > VFs) don't seem to have any issues with the re-ordering.
>
> I haven't tested it explicitly, but I suspect the re-ordering will
> actually improve things: in some error cases it will no longer add sysfs
> files for devices that have failed to init, because bailing out in
> eeh_add_device_late() (or eeh_probve_device()) will now prevent
> eeh_sysfs_add_device() from being called.
>
> Nice cleanup.
>
> Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>

Ah, good point. I'll update the commit message.

Re: [PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific

[Oliver O'Halloran]

On Fri, Feb 7, 2020 at 1:24 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
>
> 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.

The other hotplug drivers seem to be taking the lock manually in their
enable_slot() callback. So I guess we need to be doing it there too.
I'll fix it in another patch since this one is a bit big.

Re: [PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific

[Oliver O'Halloran]

On Fri, Feb 7, 2020 at 2:35 PM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> On Fri, Feb 7, 2020 at 1:24 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
> >
> > 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.
>
> The other hotplug drivers seem to be taking the lock manually in their
> enable_slot() callback. So I guess we need to be doing it there too.
> I'll fix it in another patch since this one is a bit big.

On closer inspection I think we'll need to have a deeper look at this.
This function isn't used for operations on the hotplug slot. Instead
it's used for DLPAR operations including adding / removing whole PHBs.
There doesn't appear to be any code in the DLPAR add / remove paths
which takes the PCI rescan / remove lock so I think we'll need to have
a careful look at what's going on there. Great stuff...