[PATCH RFC v4 0/9] powerpc/powernv/pci: Make hotplug self-sufficient, independent of FW and DT

[PATCH RFC v4 0/9] powerpc/powernv/pci: Make hotplug self-sufficient, independent of FW and DT

[Sergey Miroshnichenko]

This patchset allows switching from the pnv_php module to the standard
pciehp driver for PCIe hotplug functionality, if the platform supports it:
PowerNV working on on top of the skiboot with the "core/pci: Sync VFs and
the changes of bdfns between the firmware and the OS" patch serie applied.

The feature is activated by the "pci=realloc" command line argument.

The goal is ability to hotplug bridges full of devices in the future. The
"Movable BARs" [1] is a platform-independent part of our work in this. The
final part will be movable bus numbers to support inserting a bridge in the
middle of an existing PCIe tree.

Tested on POWER8 PowerNV+PHB3 ppc64le (our Vesnin server) with:
 - the pciehp driver active;
 - the pnv_php driver disabled;
 - The "pci=realloc" argument is passed;
 - surprise hotplug of an NVME disk works;
 - controlled hotplug of a network card with SR-IOV works;
 - activating of SR-IOV on a network card works;
 - [with extra patches] manually initiated (via sysfs) rescan has found
   and turned on a hotplugged bridge;
 - Without "pci=realloc" works just as before.

Changes since v3 [2]:
 - Subject changed;
 - Don't disable EEH during rescan anymore - instead just unfreeze the
   target buses deliberately;
 - Add synchronization with the firmware when changing the PCIe topology;
 - Fixed for VFs;
 - Code cleanup.

Changes since v2:
 - Don't reassign bus numbers on PowerNV by default (to retain the default
   behavior), but only when pci=realloc is passed;
 - Less code affected;
 - pci_add_device_node_info is refactored with add_one_dev_pci_data;
 - Minor code cleanup.

Changes since v1:
 - Fixed build for ppc64le and ppc64be when CONFIG_PCI_IOV is disabled;
 - Fixed build for ppc64e when CONFIG_EEH is disabled;
 - Fixed code style warnings.

[1] https://www.spinics.net/lists/linux-pci/msg79995.html
[2] https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-September/178053.html

Sergey Miroshnichenko (9):
  powerpc/pci: Access PCI config space directly w/o pci_dn
  powerpc/powernv/pci: Suppress an EEH error when reading an empty slot
  powerpc/pci: Create pci_dn on demand
  powerpc/pci: Reduce code duplication in pci_add_device_node_info
  powerpc/powernv/ioda: Fix using uninitialized IOMMU group
  powerpc/pci/IOV: Add support for runtime enabling the VFs
  powerpc/pci: Don't rely on DT is the PCI_REASSIGN_ALL_BUS is set
  powerpc/powernv/pci: Hook up the writes to PCI_SECONDARY_BUS register
  powerpc/powernv/pci: Enable reassigning the bus numbers

 arch/powerpc/include/asm/pci-bridge.h        |   2 +-
 arch/powerpc/include/asm/ppc-pci.h           |   1 +
 arch/powerpc/kernel/pci_dn.c                 | 164 +++++++++++----
 arch/powerpc/kernel/rtas_pci.c               |  97 ++++++---
 arch/powerpc/platforms/powernv/eeh-powernv.c |   2 +-
 arch/powerpc/platforms/powernv/pci-ioda.c    |   5 +-
 arch/powerpc/platforms/powernv/pci.c         | 208 +++++++++++++++++--
 arch/powerpc/platforms/pseries/pci.c         |   2 +-
 8 files changed, 381 insertions(+), 100 deletions(-)

-- 
2.20.1

[PATCH RFC v4 1/9] powerpc/pci: Access PCI config space directly w/o pci_dn

[Sergey Miroshnichenko]

To fetch an updated DT for the newly hotplugged device, OS must explicitly
request it from the firmware via the pnv_php driver.

If pnv_php wasn't triggered/loaded, it is still possible to discover new
devices if PCIe I/O will not stop in absence of the pci_dn structure.

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/kernel/rtas_pci.c       | 97 +++++++++++++++++++---------
 arch/powerpc/platforms/powernv/pci.c | 64 ++++++++++++------
 2 files changed, 109 insertions(+), 52 deletions(-)

diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
index c2b148b1634a..f675b5ecb5bc 100644
--- a/arch/powerpc/kernel/rtas_pci.c
+++ b/arch/powerpc/kernel/rtas_pci.c
@@ -55,10 +55,26 @@ static inline int config_access_valid(struct pci_dn *dn, int where)
 	return 0;
 }
 
-int rtas_read_config(struct pci_dn *pdn, int where, int size, u32 *val)
+static int rtas_read_raw_config(unsigned long buid, int busno, unsigned int devfn,
+				int where, int size, u32 *val)
 {
 	int returnval = -1;
-	unsigned long buid, addr;
+	unsigned long addr = rtas_config_addr(busno, devfn, where);
+	int ret;
+
+	if (buid) {
+		ret = rtas_call(ibm_read_pci_config, 4, 2, &returnval,
+				addr, BUID_HI(buid), BUID_LO(buid), size);
+	} else {
+		ret = rtas_call(read_pci_config, 2, 2, &returnval, addr, size);
+	}
+	*val = returnval;
+
+	return ret;
+}
+
+int rtas_read_config(struct pci_dn *pdn, int where, int size, u32 *val)
+{
 	int ret;
 
 	if (!pdn)
@@ -71,16 +87,8 @@ int rtas_read_config(struct pci_dn *pdn, int where, int size, u32 *val)
 		return PCIBIOS_SET_FAILED;
 #endif
 
-	addr = rtas_config_addr(pdn->busno, pdn->devfn, where);
-	buid = pdn->phb->buid;
-	if (buid) {
-		ret = rtas_call(ibm_read_pci_config, 4, 2, &returnval,
-				addr, BUID_HI(buid), BUID_LO(buid), size);
-	} else {
-		ret = rtas_call(read_pci_config, 2, 2, &returnval, addr, size);
-	}
-	*val = returnval;
-
+	ret = rtas_read_raw_config(pdn->phb->buid, pdn->busno, pdn->devfn,
+				   where, size, val);
 	if (ret)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
@@ -98,18 +106,44 @@ static int rtas_pci_read_config(struct pci_bus *bus,
 
 	pdn = pci_get_pdn_by_devfn(bus, devfn);
 
-	/* Validity of pdn is checked in here */
-	ret = rtas_read_config(pdn, where, size, val);
-	if (*val == EEH_IO_ERROR_VALUE(size) &&
-	    eeh_dev_check_failure(pdn_to_eeh_dev(pdn)))
-		return PCIBIOS_DEVICE_NOT_FOUND;
+	if (pdn) {
+		/* Validity of pdn is checked in here */
+		ret = rtas_read_config(pdn, where, size, val);
+
+		if (*val == EEH_IO_ERROR_VALUE(size) &&
+		    eeh_dev_check_failure(pdn_to_eeh_dev(pdn)))
+			ret = PCIBIOS_DEVICE_NOT_FOUND;
+	} else {
+		struct pci_controller *phb = pci_bus_to_host(bus);
+
+		ret = rtas_read_raw_config(phb->buid, bus->number, devfn,
+					   where, size, val);
+	}
 
 	return ret;
 }
 
+static int rtas_write_raw_config(unsigned long buid, int busno, unsigned int devfn,
+				 int where, int size, u32 val)
+{
+	unsigned long addr = rtas_config_addr(busno, devfn, where);
+	int ret;
+
+	if (buid) {
+		ret = rtas_call(ibm_write_pci_config, 5, 1, NULL, addr,
+				BUID_HI(buid), BUID_LO(buid), size, (ulong)val);
+	} else {
+		ret = rtas_call(write_pci_config, 3, 1, NULL, addr, size, (ulong)val);
+	}
+
+	if (ret)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 int rtas_write_config(struct pci_dn *pdn, int where, int size, u32 val)
 {
-	unsigned long buid, addr;
 	int ret;
 
 	if (!pdn)
@@ -122,15 +156,8 @@ int rtas_write_config(struct pci_dn *pdn, int where, int size, u32 val)
 		return PCIBIOS_SET_FAILED;
 #endif
 
-	addr = rtas_config_addr(pdn->busno, pdn->devfn, where);
-	buid = pdn->phb->buid;
-	if (buid) {
-		ret = rtas_call(ibm_write_pci_config, 5, 1, NULL, addr,
-			BUID_HI(buid), BUID_LO(buid), size, (ulong) val);
-	} else {
-		ret = rtas_call(write_pci_config, 3, 1, NULL, addr, size, (ulong)val);
-	}
-
+	ret = rtas_write_raw_config(pdn->phb->buid, pdn->busno, pdn->devfn,
+				    where, size, val);
 	if (ret)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
@@ -141,12 +168,20 @@ static int rtas_pci_write_config(struct pci_bus *bus,
 				 unsigned int devfn,
 				 int where, int size, u32 val)
 {
-	struct pci_dn *pdn;
+	struct pci_dn *pdn = pci_get_pdn_by_devfn(bus, devfn);
+	int ret;
 
-	pdn = pci_get_pdn_by_devfn(bus, devfn);
+	if (pdn) {
+		/* Validity of pdn is checked in here. */
+		ret = rtas_write_config(pdn, where, size, val);
+	} else {
+		struct pci_controller *phb = pci_bus_to_host(bus);
 
-	/* Validity of pdn is checked in here. */
-	return rtas_write_config(pdn, where, size, val);
+		ret = rtas_write_raw_config(phb->buid, bus->number, devfn,
+					    where, size, val);
+	}
+
+	return ret;
 }
 
 static struct pci_ops rtas_pci_ops = {
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index 45fb70b4bfa7..3260250d2029 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -652,30 +652,29 @@ static void pnv_pci_config_check_eeh(struct pci_dn *pdn)
 	}
 }
 
-int pnv_pci_cfg_read(struct pci_dn *pdn,
-		     int where, int size, u32 *val)
+static int pnv_pci_cfg_read_raw(u64 phb_id, int busno, unsigned int devfn,
+				int where, int size, u32 *val)
 {
-	struct pnv_phb *phb = pdn->phb->private_data;
-	u32 bdfn = (pdn->busno << 8) | pdn->devfn;
+	u32 bdfn = (busno << 8) | devfn;
 	s64 rc;
 
 	switch (size) {
 	case 1: {
 		u8 v8;
-		rc = opal_pci_config_read_byte(phb->opal_id, bdfn, where, &v8);
+		rc = opal_pci_config_read_byte(phb_id, bdfn, where, &v8);
 		*val = (rc == OPAL_SUCCESS) ? v8 : 0xff;
 		break;
 	}
 	case 2: {
 		__be16 v16;
-		rc = opal_pci_config_read_half_word(phb->opal_id, bdfn, where,
-						   &v16);
+		rc = opal_pci_config_read_half_word(phb_id, bdfn, where,
+						    &v16);
 		*val = (rc == OPAL_SUCCESS) ? be16_to_cpu(v16) : 0xffff;
 		break;
 	}
 	case 4: {
 		__be32 v32;
-		rc = opal_pci_config_read_word(phb->opal_id, bdfn, where, &v32);
+		rc = opal_pci_config_read_word(phb_id, bdfn, where, &v32);
 		*val = (rc == OPAL_SUCCESS) ? be32_to_cpu(v32) : 0xffffffff;
 		break;
 	}
@@ -684,27 +683,28 @@ int pnv_pci_cfg_read(struct pci_dn *pdn,
 	}
 
 	pr_devel("%s: bus: %x devfn: %x +%x/%x -> %08x\n",
-		 __func__, pdn->busno, pdn->devfn, where, size, *val);
+		 __func__, busno, devfn, where, size, *val);
+
 	return PCIBIOS_SUCCESSFUL;
 }
 
-int pnv_pci_cfg_write(struct pci_dn *pdn,
-		      int where, int size, u32 val)
+static int pnv_pci_cfg_write_raw(u64 phb_id, int busno, unsigned int devfn,
+				 int where, int size, u32 val)
 {
-	struct pnv_phb *phb = pdn->phb->private_data;
-	u32 bdfn = (pdn->busno << 8) | pdn->devfn;
+	u32 bdfn = (busno << 8) | devfn;
 
 	pr_devel("%s: bus: %x devfn: %x +%x/%x -> %08x\n",
-		 __func__, pdn->busno, pdn->devfn, where, size, val);
+		 __func__, busno, devfn, where, size, val);
+
 	switch (size) {
 	case 1:
-		opal_pci_config_write_byte(phb->opal_id, bdfn, where, val);
+		opal_pci_config_write_byte(phb_id, bdfn, where, val);
 		break;
 	case 2:
-		opal_pci_config_write_half_word(phb->opal_id, bdfn, where, val);
+		opal_pci_config_write_half_word(phb_id, bdfn, where, val);
 		break;
 	case 4:
-		opal_pci_config_write_word(phb->opal_id, bdfn, where, val);
+		opal_pci_config_write_word(phb_id, bdfn, where, val);
 		break;
 	default:
 		return PCIBIOS_FUNC_NOT_SUPPORTED;
@@ -713,6 +713,24 @@ int pnv_pci_cfg_write(struct pci_dn *pdn,
 	return PCIBIOS_SUCCESSFUL;
 }
 
+int pnv_pci_cfg_read(struct pci_dn *pdn,
+		     int where, int size, u32 *val)
+{
+	struct pnv_phb *phb = pdn->phb->private_data;
+
+	return pnv_pci_cfg_read_raw(phb->opal_id, pdn->busno, pdn->devfn,
+				    where, size, val);
+}
+
+int pnv_pci_cfg_write(struct pci_dn *pdn,
+		      int where, int size, u32 val)
+{
+	struct pnv_phb *phb = pdn->phb->private_data;
+
+	return pnv_pci_cfg_write_raw(phb->opal_id, pdn->busno, pdn->devfn,
+				     where, size, val);
+}
+
 #if CONFIG_EEH
 static bool pnv_pci_cfg_check(struct pci_dn *pdn)
 {
@@ -748,13 +766,15 @@ static int pnv_pci_read_config(struct pci_bus *bus,
 			       int where, int size, u32 *val)
 {
 	struct pci_dn *pdn;
-	struct pnv_phb *phb;
+	struct pci_controller *hose = pci_bus_to_host(bus);
+	struct pnv_phb *phb = hose->private_data;
 	int ret;
 
 	*val = 0xFFFFFFFF;
 	pdn = pci_get_pdn_by_devfn(bus, devfn);
 	if (!pdn)
-		return PCIBIOS_DEVICE_NOT_FOUND;
+		return pnv_pci_cfg_read_raw(phb->opal_id, bus->number, devfn,
+					    where, size, val);
 
 	if (!pnv_pci_cfg_check(pdn))
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -777,12 +797,14 @@ static int pnv_pci_write_config(struct pci_bus *bus,
 				int where, int size, u32 val)
 {
 	struct pci_dn *pdn;
-	struct pnv_phb *phb;
+	struct pci_controller *hose = pci_bus_to_host(bus);
+	struct pnv_phb *phb = hose->private_data;
 	int ret;
 
 	pdn = pci_get_pdn_by_devfn(bus, devfn);
 	if (!pdn)
-		return PCIBIOS_DEVICE_NOT_FOUND;
+		return pnv_pci_cfg_write_raw(phb->opal_id, bus->number, devfn,
+					     where, size, val);
 
 	if (!pnv_pci_cfg_check(pdn))
 		return PCIBIOS_DEVICE_NOT_FOUND;
-- 
2.20.1

[PATCH RFC v4 2/9] powerpc/powernv/pci: Suppress an EEH error when reading an empty slot

[Sergey Miroshnichenko]

Reading an empty slot returns all ones, which triggers a false
EEH error event on PowerNV. This patch unfreezes the bus where
it has happened.

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/include/asm/ppc-pci.h   |  1 +
 arch/powerpc/kernel/pci_dn.c         |  2 +-
 arch/powerpc/platforms/powernv/pci.c | 34 ++++++++++++++++++++++++----
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h
index f67da277d652..737393c54f58 100644
--- a/arch/powerpc/include/asm/ppc-pci.h
+++ b/arch/powerpc/include/asm/ppc-pci.h
@@ -40,6 +40,7 @@ void *traverse_pci_dn(struct pci_dn *root,
 		      void *(*fn)(struct pci_dn *, void *),
 		      void *data);
 extern void pci_devs_phb_init_dynamic(struct pci_controller *phb);
+struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus);
 
 /* From rtas_pci.h */
 extern void init_pci_config_tokens (void);
diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index ab147a1909c8..341ed71250f1 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -40,7 +40,7 @@
  * one of PF's bridge. For other devices, their firmware
  * data is linked to that of their bridge.
  */
-static struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
+struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
 {
 	struct pci_bus *pbus;
 	struct device_node *dn;
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index 3260250d2029..73c2d0aed996 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -761,6 +761,21 @@ static inline pnv_pci_cfg_check(struct pci_dn *pdn)
 }
 #endif /* CONFIG_EEH */
 
+static int get_bus_pe_number(struct pci_bus *bus)
+{
+	struct pci_dn *pdn = pci_bus_to_pdn(bus);
+	struct pci_dn *child;
+
+	if (!pdn)
+		return IODA_INVALID_PE;
+
+	list_for_each_entry(child, &pdn->child_list, list)
+		if (child->pe_number != IODA_INVALID_PE)
+			return child->pe_number;
+
+	return IODA_INVALID_PE;
+}
+
 static int pnv_pci_read_config(struct pci_bus *bus,
 			       unsigned int devfn,
 			       int where, int size, u32 *val)
@@ -769,12 +784,23 @@ static int pnv_pci_read_config(struct pci_bus *bus,
 	struct pci_controller *hose = pci_bus_to_host(bus);
 	struct pnv_phb *phb = hose->private_data;
 	int ret;
+	u32 empty_val = 0xFFFFFFFF;
 
-	*val = 0xFFFFFFFF;
+	*val = empty_val;
 	pdn = pci_get_pdn_by_devfn(bus, devfn);
-	if (!pdn)
-		return pnv_pci_cfg_read_raw(phb->opal_id, bus->number, devfn,
-					    where, size, val);
+	if (!pdn) {
+		int pe_number = get_bus_pe_number(bus);
+
+		ret = pnv_pci_cfg_read_raw(phb->opal_id, bus->number, devfn,
+					   where, size, val);
+
+		if (!ret && (*val == empty_val) && phb->unfreeze_pe)
+			phb->unfreeze_pe(phb, (pe_number == IODA_INVALID_PE) ?
+					 0xff : pe_number,
+					 OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
+
+		return ret;
+	}
 
 	if (!pnv_pci_cfg_check(pdn))
 		return PCIBIOS_DEVICE_NOT_FOUND;
-- 
2.20.1

[PATCH RFC v4 3/9] powerpc/pci: Create pci_dn on demand

[Sergey Miroshnichenko]

If a struct pci_dn hasn't yet been created for the PCIe device (there was
no DT node for it), allocate this structure and fill with info read from
the device directly.

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/kernel/pci_dn.c | 79 ++++++++++++++++++++++++++++++++----
 1 file changed, 72 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index 341ed71250f1..67ccd7be8344 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -33,6 +33,8 @@
 #include <asm/firmware.h>
 #include <asm/eeh.h>
 
+static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent);
+
 /*
  * The function is used to find the firmware data of one
  * specific PCI device, which is attached to the indicated
@@ -65,6 +67,9 @@ struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
 	dn = pci_bus_to_OF_node(pbus);
 	pdn = dn ? PCI_DN(dn) : NULL;
 
+	if (!pdn && pbus->self)
+		pdn = pbus->self->dev.archdata.pci_data;
+
 	return pdn;
 }
 
@@ -74,10 +79,13 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
 	struct device_node *dn = NULL;
 	struct pci_dn *parent, *pdn;
 	struct pci_dev *pdev = NULL;
+	bool pdev_found = false;
 
 	/* Fast path: fetch from PCI device */
 	list_for_each_entry(pdev, &bus->devices, bus_list) {
 		if (pdev->devfn == devfn) {
+			pdev_found = true;
+
 			if (pdev->dev.archdata.pci_data)
 				return pdev->dev.archdata.pci_data;
 
@@ -86,6 +94,9 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
 		}
 	}
 
+	if (!pdev_found)
+		pdev = NULL;
+
 	/* Fast path: fetch from device node */
 	pdn = dn ? PCI_DN(dn) : NULL;
 	if (pdn)
@@ -98,9 +109,12 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
 
 	list_for_each_entry(pdn, &parent->child_list, list) {
 		if (pdn->busno == bus->number &&
-                    pdn->devfn == devfn)
-                        return pdn;
-        }
+		    pdn->devfn == devfn) {
+			if (pdev)
+				pdev->dev.archdata.pci_data = pdn;
+			return pdn;
+		}
+	}
 
 	return NULL;
 }
@@ -130,14 +144,15 @@ struct pci_dn *pci_get_pdn(struct pci_dev *pdev)
 
 	list_for_each_entry(pdn, &parent->child_list, list) {
 		if (pdn->busno == pdev->bus->number &&
-		    pdn->devfn == pdev->devfn)
+		    pdn->devfn == pdev->devfn) {
+			pdev->dev.archdata.pci_data = pdn;
 			return pdn;
+		}
 	}
 
-	return NULL;
+	return create_pdn(pdev, parent);
 }
 
-#ifdef CONFIG_PCI_IOV
 static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
 					   int vf_index,
 					   int busno, int devfn)
@@ -156,7 +171,9 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
 	pdn->parent = parent;
 	pdn->busno = busno;
 	pdn->devfn = devfn;
+	#ifdef CONFIG_PCI_IOV
 	pdn->vf_index = vf_index;
+	#endif /* CONFIG_PCI_IOV */
 	pdn->pe_number = IODA_INVALID_PE;
 	INIT_LIST_HEAD(&pdn->child_list);
 	INIT_LIST_HEAD(&pdn->list);
@@ -164,7 +181,55 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
 
 	return pdn;
 }
-#endif
+
+static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent)
+{
+	struct pci_dn *pdn = NULL;
+
+	if (!parent)
+		return NULL;
+
+	pdn = add_one_dev_pci_data(parent, 0, pdev->bus->busn_res.start, pdev->devfn);
+	dev_info(&pdev->dev, "Create a new pdn for devfn %2x\n", pdev->devfn / 8);
+
+	if (pdn) {
+		#ifdef CONFIG_EEH
+		struct eeh_dev *edev;
+		#endif /* CONFIG_EEH */
+		u32 class_code;
+		u16 device_id;
+		u16 vendor_id;
+
+		#ifdef CONFIG_EEH
+		edev = eeh_dev_init(pdn);
+		if (!edev) {
+			kfree(pdn);
+			dev_err(&pdev->dev, "%s: Failed to allocate edev\n", __func__);
+			return NULL;
+		}
+		#endif /* CONFIG_EEH */
+
+		pci_bus_read_config_word(pdev->bus, pdev->devfn,
+					 PCI_VENDOR_ID, &vendor_id);
+		pdn->vendor_id = vendor_id;
+
+		pci_bus_read_config_word(pdev->bus, pdev->devfn,
+					 PCI_DEVICE_ID, &device_id);
+		pdn->device_id = device_id;
+
+		pci_bus_read_config_dword(pdev->bus, pdev->devfn,
+					  PCI_CLASS_REVISION, &class_code);
+		class_code >>= 8;
+		pdn->class_code = class_code;
+
+		pdn->pci_ext_config_space = 0;
+		pdev->dev.archdata.pci_data = pdn;
+	} else {
+		dev_err(&pdev->dev, "%s: Failed to allocate pdn\n", __func__);
+	}
+
+	return pdn;
+}
 
 struct pci_dn *add_dev_pci_data(struct pci_dev *pdev)
 {
-- 
2.20.1

[PATCH RFC v4 4/9] powerpc/pci: Reduce code duplication in pci_add_device_node_info

[Sergey Miroshnichenko]

It is possible now to allocate and fill a new pdn with add_one_dev_pci_data

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/kernel/pci_dn.c | 38 +++++++++++++++---------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index 67ccd7be8344..ed1aab424e91 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -159,15 +159,10 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
 {
 	struct pci_dn *pdn;
 
-	/* Except PHB, we always have the parent */
-	if (!parent)
-		return NULL;
-
 	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
 	if (!pdn)
 		return NULL;
 
-	pdn->phb = parent->phb;
 	pdn->parent = parent;
 	pdn->busno = busno;
 	pdn->devfn = devfn;
@@ -177,7 +172,10 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
 	pdn->pe_number = IODA_INVALID_PE;
 	INIT_LIST_HEAD(&pdn->child_list);
 	INIT_LIST_HEAD(&pdn->list);
-	list_add_tail(&pdn->list, &parent->child_list);
+	if (parent) {
+		pdn->phb = parent->phb;
+		list_add_tail(&pdn->list, &parent->child_list);
+	}
 
 	return pdn;
 }
@@ -346,25 +344,29 @@ struct pci_dn *pci_add_device_node_info(struct pci_controller *hose,
 	const __be32 *regs;
 	struct device_node *parent;
 	struct pci_dn *pdn;
+	int busno = 0, devfn = 0;
 #ifdef CONFIG_EEH
 	struct eeh_dev *edev;
 #endif
 
-	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
-	if (pdn == NULL)
-		return NULL;
-	dn->data = pdn;
-	pdn->phb = hose;
-	pdn->pe_number = IODA_INVALID_PE;
 	regs = of_get_property(dn, "reg", NULL);
 	if (regs) {
 		u32 addr = of_read_number(regs, 1);
 
 		/* First register entry is addr (00BBSS00)  */
-		pdn->busno = (addr >> 16) & 0xff;
-		pdn->devfn = (addr >> 8) & 0xff;
+		busno = (addr >> 16) & 0xff;
+		devfn = (addr >> 8) & 0xff;
 	}
 
+	parent = of_get_parent(dn);
+	pdn = add_one_dev_pci_data(parent ? PCI_DN(parent) : NULL,
+				   0, busno, devfn);
+	if (!pdn)
+		return NULL;
+
+	dn->data = pdn;
+	pdn->phb = hose;
+
 	/* vendor/device IDs and class code */
 	regs = of_get_property(dn, "vendor-id", NULL);
 	pdn->vendor_id = regs ? of_read_number(regs, 1) : 0;
@@ -385,14 +387,6 @@ struct pci_dn *pci_add_device_node_info(struct pci_controller *hose,
 	}
 #endif
 
-	/* Attach to parent node */
-	INIT_LIST_HEAD(&pdn->child_list);
-	INIT_LIST_HEAD(&pdn->list);
-	parent = of_get_parent(dn);
-	pdn->parent = parent ? PCI_DN(parent) : NULL;
-	if (pdn->parent)
-		list_add_tail(&pdn->list, &pdn->parent->child_list);
-
 	return pdn;
 }
 EXPORT_SYMBOL_GPL(pci_add_device_node_info);
-- 
2.20.1

[PATCH RFC v4 5/9] powerpc/powernv/ioda: Fix using uninitialized IOMMU group

[Sergey Miroshnichenko]

Otherwise there will be a NULL pointer dereferencing in iommu_add_device()
later during activating a newly created VF.

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 3b8b21a70196..0631e8196dad 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1593,6 +1593,9 @@ static void pnv_ioda_setup_vf_PE(struct pci_dev *pdev, u16 num_vfs)
 
 		pnv_pci_ioda2_setup_dma_pe(phb, pe);
 #ifdef CONFIG_IOMMU_API
+		iommu_register_group(&pe->table_group,
+				     pe->phb->hose->global_number,
+				     pe->pe_number);
 		pnv_ioda_setup_bus_iommu_group(pe, &pe->table_group, NULL);
 #endif
 	}
-- 
2.20.1

[PATCH RFC v4 6/9] powerpc/pci/IOV: Add support for runtime enabling the VFs

[Sergey Miroshnichenko]

When called within pcibios_sriov_enable(), the pci_sriov_get_totalvfs(pdev)
returns zero, because the device is yet preparing to enable the VFs.

With this patch it becomes possible to enable VFs via sysfs "sriov_numvfs"
on PowerNV.

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/include/asm/pci-bridge.h     |  2 +-
 arch/powerpc/kernel/pci_dn.c              | 27 ++++++++++++++---------
 arch/powerpc/platforms/powernv/pci-ioda.c |  2 +-
 arch/powerpc/platforms/pseries/pci.c      |  2 +-
 4 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
index aee4fcc24990..8a30c48caa3e 100644
--- a/arch/powerpc/include/asm/pci-bridge.h
+++ b/arch/powerpc/include/asm/pci-bridge.h
@@ -225,7 +225,7 @@ struct pci_dn {
 extern struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
 					   int devfn);
 extern struct pci_dn *pci_get_pdn(struct pci_dev *pdev);
-extern struct pci_dn *add_dev_pci_data(struct pci_dev *pdev);
+extern struct pci_dn *add_dev_pci_data(struct pci_dev *pdev, int num_vfs);
 extern void remove_dev_pci_data(struct pci_dev *pdev);
 extern struct pci_dn *pci_add_device_node_info(struct pci_controller *hose,
 					       struct device_node *dn);
diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index ed1aab424e91..1b1a6198eb28 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -229,18 +229,19 @@ static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent)
 	return pdn;
 }
 
-struct pci_dn *add_dev_pci_data(struct pci_dev *pdev)
+struct pci_dn *add_dev_pci_data(struct pci_dev *pdev, int num_vfs)
 {
 #ifdef CONFIG_PCI_IOV
 	struct pci_dn *parent, *pdn;
 	int i;
 
+	pdn = pci_get_pdn(pdev);
+
 	/* Only support IOV for now */
 	if (!pdev->is_physfn)
-		return pci_get_pdn(pdev);
+		return pdn;
 
 	/* Check if VFs have been populated */
-	pdn = pci_get_pdn(pdev);
 	if (!pdn || (pdn->flags & PCI_DN_FLAG_IOV_VF))
 		return NULL;
 
@@ -249,28 +250,34 @@ struct pci_dn *add_dev_pci_data(struct pci_dev *pdev)
 	if (!parent)
 		return NULL;
 
-	for (i = 0; i < pci_sriov_get_totalvfs(pdev); i++) {
+	for (i = 0; i < num_vfs; i++) {
 		struct eeh_dev *edev __maybe_unused;
+		struct pci_dn *vpdn;
 
-		pdn = add_one_dev_pci_data(parent, i,
-					   pci_iov_virtfn_bus(pdev, i),
-					   pci_iov_virtfn_devfn(pdev, i));
-		if (!pdn) {
+		vpdn = add_one_dev_pci_data(parent, i,
+					    pci_iov_virtfn_bus(pdev, i),
+					    pci_iov_virtfn_devfn(pdev, i));
+		if (!vpdn) {
 			dev_warn(&pdev->dev, "%s: Cannot create firmware data for VF#%d\n",
 				 __func__, i);
 			return NULL;
 		}
 
+		vpdn->vendor_id = pdn->vendor_id;
+		vpdn->device_id = pdn->device_id;
+		vpdn->class_code = pdn->class_code;
+		vpdn->pci_ext_config_space = 0;
+
 #ifdef CONFIG_EEH
 		/* Create the EEH device for the VF */
-		edev = eeh_dev_init(pdn);
+		edev = eeh_dev_init(vpdn);
 		BUG_ON(!edev);
 		edev->physfn = pdev;
 #endif /* CONFIG_EEH */
 	}
 #endif /* CONFIG_PCI_IOV */
 
-	return pci_get_pdn(pdev);
+	return pdn;
 }
 
 void remove_dev_pci_data(struct pci_dev *pdev)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 0631e8196dad..371e60c824d7 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1728,7 +1728,7 @@ int pnv_pcibios_sriov_disable(struct pci_dev *pdev)
 int pnv_pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 {
 	/* Allocate PCI data */
-	add_dev_pci_data(pdev);
+	add_dev_pci_data(pdev, num_vfs);
 
 	return pnv_pci_sriov_enable(pdev, num_vfs);
 }
diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
index 37a77e57893e..054e8bf5d5d3 100644
--- a/arch/powerpc/platforms/pseries/pci.c
+++ b/arch/powerpc/platforms/pseries/pci.c
@@ -205,7 +205,7 @@ int pseries_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 int pseries_pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 {
 	/* Allocate PCI data */
-	add_dev_pci_data(pdev);
+	add_dev_pci_data(pdev, num_vfs);
 	return pseries_pci_sriov_enable(pdev, num_vfs);
 }
 
-- 
2.20.1

[PATCH RFC v4 7/9] powerpc/pci: Don't rely on DT is the PCI_REASSIGN_ALL_BUS is set

[Sergey Miroshnichenko]

If supported by the platform, endpoint's pci_dn can be created dynamically,
without need to wait for DT updates from the firmware.

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/kernel/pci_dn.c                 | 6 ++++--
 arch/powerpc/platforms/powernv/eeh-powernv.c | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index 1b1a6198eb28..b1dc788865b9 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -561,8 +561,10 @@ void pci_devs_phb_init_dynamic(struct pci_controller *phb)
 		phb->pci_data = pdn;
 	}
 
-	/* Update dn->phb ptrs for new phb and children devices */
-	pci_traverse_device_nodes(dn, add_pdn, phb);
+	if (!pci_has_flag(PCI_REASSIGN_ALL_BUS)) {
+		/* Update dn->phb ptrs for new phb and children devices */
+		pci_traverse_device_nodes(dn, add_pdn, phb);
+	}
 }
 
 /** 
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index f38078976c5d..40feff2653a0 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -47,7 +47,7 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
 {
 	struct pci_dn *pdn = pci_get_pdn(pdev);
 
-	if (!pdev->is_virtfn)
+	if (!pci_has_flag(PCI_REASSIGN_ALL_BUS) && !pdev->is_virtfn)
 		return;
 
 	/*
-- 
2.20.1

[PATCH RFC v4 8/9] powerpc/powernv/pci: Hook up the writes to PCI_SECONDARY_BUS register

[Sergey Miroshnichenko]

Writing a new value to the PCI_SECONDARY_BUS register of the bridge means
that its children will become addressable on another address (new B in BDF)
or even un-addressable if the secondary bus is set to zero.

On PowerNV, device PEs are heavily BDF-dependent, so they must be updated
on every such change of its address.

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/platforms/powernv/pci.c | 118 ++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index 73c2d0aed996..52ce717a7905 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -722,13 +722,127 @@ int pnv_pci_cfg_read(struct pci_dn *pdn,
 				    where, size, val);
 }
 
+static void invalidate_children_pes(struct pci_dn *pdn)
+{
+	struct pnv_phb *phb = pdn->phb->private_data;
+	struct pci_dn *child;
+	bool found_pe = false;
+	int pe_num;
+	int pe_bus;
+
+	list_for_each_entry(child, &pdn->child_list, list) {
+		struct pnv_ioda_pe *pe = (child->pe_number != IODA_INVALID_PE) ?
+			&phb->ioda.pe_array[child->pe_number] :
+			NULL;
+
+		if (!child->busno)
+			continue;
+
+		if ((child->class_code >> 8) == PCI_CLASS_BRIDGE_PCI)
+			invalidate_children_pes(child);
+
+		if (pe) {
+			u8 rid_bus = (pe->rid >> 8) & 0xff;
+
+			if (rid_bus) {
+				pe_num = child->pe_number;
+				pe_bus = rid_bus;
+				found_pe = true;
+			}
+
+			pe->rid &= 0xff;
+		}
+
+		child->busno = 0;
+	}
+
+	if (found_pe) {
+		u16 rid = pe_bus << 8;
+
+		opal_pci_set_pe(phb->opal_id, pe_num, rid, 7, 0, 0, OPAL_UNMAP_PE);
+	}
+}
+
+static u8 pre_hook_new_sec_bus(struct pci_dn *pdn, u8 new_secondary_bus)
+{
+	u32 old_secondary_bus = 0;
+
+	if ((pdn->class_code >> 8) != PCI_CLASS_BRIDGE_PCI)
+		return 0;
+
+	pnv_pci_cfg_read(pdn, PCI_SECONDARY_BUS, 1, &old_secondary_bus);
+	old_secondary_bus &= 0xff;
+
+	if (old_secondary_bus != new_secondary_bus)
+		invalidate_children_pes(pdn);
+
+	return old_secondary_bus;
+}
+
+static void update_children_pes(struct pci_dn *pdn, u8 new_secondary_bus)
+{
+	struct pnv_phb *phb = pdn->phb->private_data;
+	struct pci_dn *child;
+	bool found_pe = false;
+	int pe_num;
+
+	if (!new_secondary_bus)
+		return;
+
+	list_for_each_entry(child, &pdn->child_list, list) {
+		struct pnv_ioda_pe *pe = (child->pe_number != IODA_INVALID_PE) ?
+			&phb->ioda.pe_array[child->pe_number] :
+			NULL;
+
+		if (child->busno)
+			continue;
+
+		child->busno = new_secondary_bus;
+
+		if (pe) {
+			pe->rid |= (child->busno << 8);
+			pe_num = child->pe_number;
+			found_pe = true;
+		}
+	}
+
+	if (found_pe) {
+		u16 rid = new_secondary_bus << 8;
+
+		opal_pci_set_pe(phb->opal_id, pe_num, rid, 7, 0, 0, OPAL_MAP_PE);
+	}
+}
+
+static void post_hook_new_sec_bus(struct pci_dn *pdn, u8 new_secondary_bus)
+{
+	if ((pdn->class_code >> 8) != PCI_CLASS_BRIDGE_PCI)
+		return;
+
+	update_children_pes(pdn, new_secondary_bus);
+}
+
 int pnv_pci_cfg_write(struct pci_dn *pdn,
 		      int where, int size, u32 val)
 {
 	struct pnv_phb *phb = pdn->phb->private_data;
+	u8 old_secondary_bus = 0, new_secondary_bus = 0;
+	int rc;
+
+	if (where == PCI_SECONDARY_BUS) {
+		new_secondary_bus = val & 0xff;
+		old_secondary_bus = pre_hook_new_sec_bus(pdn, new_secondary_bus);
+	} else if (where == PCI_PRIMARY_BUS && size > 1) {
+		new_secondary_bus = (val >> 8) & 0xff;
+		old_secondary_bus = pre_hook_new_sec_bus(pdn, new_secondary_bus);
+	}
 
-	return pnv_pci_cfg_write_raw(phb->opal_id, pdn->busno, pdn->devfn,
-				     where, size, val);
+	rc = pnv_pci_cfg_write_raw(phb->opal_id, pdn->busno, pdn->devfn,
+				   where, size, val);
+
+	if (new_secondary_bus && old_secondary_bus != new_secondary_bus)
+		post_hook_new_sec_bus(pdn, new_secondary_bus);
+
+	return rc;
 }
 
 #if CONFIG_EEH
-- 
2.20.1

[PATCH RFC v4 9/9] powerpc/powernv/pci: Enable reassigning the bus numbers

[Sergey Miroshnichenko]

When the pci=realloc command line switch is enabled (which should only be
set when working on on top of the skiboot with the "core/pci: Sync VFs and
the changes of bdfns between the firmware and the OS" patch serie applied),
PowerNV will not depend on PCIe topology info from DT anymore.

This makes possible to re-enumerate the fabric, assign the new bus numbers
and switch from the pnv_php module to the standard pciehp driver for PCIe
hotplug functionality.

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/kernel/pci_dn.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index b1dc788865b9..b823c4177184 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -601,3 +601,15 @@ static void pci_dev_pdn_setup(struct pci_dev *pdev)
 	pdev->dev.archdata.pci_data = pdn;
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_ANY_ID, PCI_ANY_ID, pci_dev_pdn_setup);
+
+char * __init pcibios_setup(char *str)
+{
+	if (!strncmp(str, "realloc=", 8)) {
+		if (!strncmp(str + 8, "on", 2))
+			pci_add_flags(PCI_REASSIGN_ALL_BUS);
+	} else if (!strncmp(str, "realloc", 7)) {
+		pci_add_flags(PCI_REASSIGN_ALL_BUS);
+	}
+
+	return str;
+}
-- 
2.20.1

Re: [PATCH RFC v4 0/9] powerpc/powernv/pci: Make hotplug self-sufficient, independent of FW and DT

[Oliver]

On Sat, Mar 2, 2019 at 3:04 AM Sergey Miroshnichenko
<s.miroshnichenko@yadro.com> wrote:
>
> This patchset allows switching from the pnv_php module to the standard
> pciehp driver for PCIe hotplug functionality, if the platform supports it:
> PowerNV working on on top of the skiboot with the "core/pci: Sync VFs and
> the changes of bdfns between the firmware and the OS" patch serie applied.
>
> The feature is activated by the "pci=realloc" command line argument.
>
> The goal is ability to hotplug bridges full of devices in the future. The
> "Movable BARs" [1] is a platform-independent part of our work in this. The
> final part will be movable bus numbers to support inserting a bridge in the
> middle of an existing PCIe tree.
>
> Tested on POWER8 PowerNV+PHB3 ppc64le (our Vesnin server) with:
>  - the pciehp driver active;
>  - the pnv_php driver disabled;

Cool :)

I've had a brief look at this and it the approach seems reasonable.
I'll have a more detailed look and see how it fares on a P9 system
tomorrow.

>  - The "pci=realloc" argument is passed;
>  - surprise hotplug of an NVME disk works;
>  - controlled hotplug of a network card with SR-IOV works;
>  - activating of SR-IOV on a network card works;
>  - [with extra patches] manually initiated (via sysfs) rescan has found
>    and turned on a hotplugged bridge;
>  - Without "pci=realloc" works just as before.
>
> Changes since v3 [2]:
>  - Subject changed;
>  - Don't disable EEH during rescan anymore - instead just unfreeze the
>    target buses deliberately;
>  - Add synchronization with the firmware when changing the PCIe topology;
>  - Fixed for VFs;
>  - Code cleanup.
>
> Changes since v2:
>  - Don't reassign bus numbers on PowerNV by default (to retain the default
>    behavior), but only when pci=realloc is passed;
>  - Less code affected;
>  - pci_add_device_node_info is refactored with add_one_dev_pci_data;
>  - Minor code cleanup.
>
> Changes since v1:
>  - Fixed build for ppc64le and ppc64be when CONFIG_PCI_IOV is disabled;
>  - Fixed build for ppc64e when CONFIG_EEH is disabled;
>  - Fixed code style warnings.
>
> [1] https://www.spinics.net/lists/linux-pci/msg79995.html
> [2] https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-September/178053.html
>
> Sergey Miroshnichenko (9):
>   powerpc/pci: Access PCI config space directly w/o pci_dn
>   powerpc/powernv/pci: Suppress an EEH error when reading an empty slot
>   powerpc/pci: Create pci_dn on demand
>   powerpc/pci: Reduce code duplication in pci_add_device_node_info
>   powerpc/powernv/ioda: Fix using uninitialized IOMMU group
>   powerpc/pci/IOV: Add support for runtime enabling the VFs
>   powerpc/pci: Don't rely on DT is the PCI_REASSIGN_ALL_BUS is set
>   powerpc/powernv/pci: Hook up the writes to PCI_SECONDARY_BUS register
>   powerpc/powernv/pci: Enable reassigning the bus numbers
>
>  arch/powerpc/include/asm/pci-bridge.h        |   2 +-
>  arch/powerpc/include/asm/ppc-pci.h           |   1 +
>  arch/powerpc/kernel/pci_dn.c                 | 164 +++++++++++----
>  arch/powerpc/kernel/rtas_pci.c               |  97 ++++++---
>  arch/powerpc/platforms/powernv/eeh-powernv.c |   2 +-
>  arch/powerpc/platforms/powernv/pci-ioda.c    |   5 +-
>  arch/powerpc/platforms/powernv/pci.c         | 208 +++++++++++++++++--
>  arch/powerpc/platforms/pseries/pci.c         |   2 +-
>  8 files changed, 381 insertions(+), 100 deletions(-)
>
> --
> 2.20.1
>

Re: [PATCH RFC v4 2/9] powerpc/powernv/pci: Suppress an EEH error when reading an empty slot

[Oliver]

On Sat, Mar 2, 2019 at 3:04 AM Sergey Miroshnichenko
<s.miroshnichenko@yadro.com> wrote:
>
> Reading an empty slot returns all ones, which triggers a false
> EEH error event on PowerNV. This patch unfreezes the bus where
> it has happened.
>
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
>  arch/powerpc/include/asm/ppc-pci.h   |  1 +
>  arch/powerpc/kernel/pci_dn.c         |  2 +-
>  arch/powerpc/platforms/powernv/pci.c | 34 ++++++++++++++++++++++++----
>  3 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h
> index f67da277d652..737393c54f58 100644
> --- a/arch/powerpc/include/asm/ppc-pci.h
> +++ b/arch/powerpc/include/asm/ppc-pci.h
> @@ -40,6 +40,7 @@ void *traverse_pci_dn(struct pci_dn *root,
>                       void *(*fn)(struct pci_dn *, void *),
>                       void *data);
>  extern void pci_devs_phb_init_dynamic(struct pci_controller *phb);
> +struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus);
>
>  /* From rtas_pci.h */
>  extern void init_pci_config_tokens (void);
> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
> index ab147a1909c8..341ed71250f1 100644
> --- a/arch/powerpc/kernel/pci_dn.c
> +++ b/arch/powerpc/kernel/pci_dn.c
> @@ -40,7 +40,7 @@
>   * one of PF's bridge. For other devices, their firmware
>   * data is linked to that of their bridge.
>   */
> -static struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
> +struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
>  {
>         struct pci_bus *pbus;
>         struct device_node *dn;
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index 3260250d2029..73c2d0aed996 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -761,6 +761,21 @@ static inline pnv_pci_cfg_check(struct pci_dn *pdn)
>  }
>  #endif /* CONFIG_EEH */
>
> +static int get_bus_pe_number(struct pci_bus *bus)
> +{
> +       struct pci_dn *pdn = pci_bus_to_pdn(bus);
> +       struct pci_dn *child;
> +
> +       if (!pdn)
> +               return IODA_INVALID_PE;
> +
> +       list_for_each_entry(child, &pdn->child_list, list)
> +               if (child->pe_number != IODA_INVALID_PE)
> +                       return child->pe_number;
> +
> +       return IODA_INVALID_PE;
> +}
> +
>  static int pnv_pci_read_config(struct pci_bus *bus,
>                                unsigned int devfn,
>                                int where, int size, u32 *val)
> @@ -769,12 +784,23 @@ static int pnv_pci_read_config(struct pci_bus *bus,
>         struct pci_controller *hose = pci_bus_to_host(bus);
>         struct pnv_phb *phb = hose->private_data;
>         int ret;
> +       u32 empty_val = 0xFFFFFFFF;
>
> -       *val = 0xFFFFFFFF;
> +       *val = empty_val;
>         pdn = pci_get_pdn_by_devfn(bus, devfn);
> -       if (!pdn)
> -               return pnv_pci_cfg_read_raw(phb->opal_id, bus->number, devfn,
> -                                           where, size, val);
> +       if (!pdn) {
> +               int pe_number = get_bus_pe_number(bus);
> +
> +               ret = pnv_pci_cfg_read_raw(phb->opal_id, bus->number, devfn,
> +                                          where, size, val);
> +
> +               if (!ret && (*val == empty_val) && phb->unfreeze_pe)

Do this empty val check work when using 1 or 2 byte cfg accesses?

> +                       phb->unfreeze_pe(phb, (pe_number == IODA_INVALID_PE) ?
> +                                        0xff : pe_number,

Use phb->ioda.reserved_pe_idx rather than guessing that 0xff is safe
to use. On P9 we have PHBs with 512 PEs and some older P8 firmware
releases used 0 as the reserved PE rather than 0xff.

> +                                        OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
> +
> +               return ret;
> +       }
>
>         if (!pnv_pci_cfg_check(pdn))
>                 return PCIBIOS_DEVICE_NOT_FOUND;
> --
> 2.20.1
>

Re: [PATCH RFC v4 3/9] powerpc/pci: Create pci_dn on demand

[Oliver]

On Sat, Mar 2, 2019 at 3:04 AM Sergey Miroshnichenko
<s.miroshnichenko@yadro.com> wrote:
>
> If a struct pci_dn hasn't yet been created for the PCIe device (there was
> no DT node for it), allocate this structure and fill with info read from
> the device directly.
>
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
>  arch/powerpc/kernel/pci_dn.c | 79 ++++++++++++++++++++++++++++++++----
>  1 file changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
> index 341ed71250f1..67ccd7be8344 100644
> --- a/arch/powerpc/kernel/pci_dn.c
> +++ b/arch/powerpc/kernel/pci_dn.c
> @@ -33,6 +33,8 @@
>  #include <asm/firmware.h>
>  #include <asm/eeh.h>
>
> +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent);
> +
>  /*
>   * The function is used to find the firmware data of one
>   * specific PCI device, which is attached to the indicated
> @@ -65,6 +67,9 @@ struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
>         dn = pci_bus_to_OF_node(pbus);
>         pdn = dn ? PCI_DN(dn) : NULL;
>
> +       if (!pdn && pbus->self)
> +               pdn = pbus->self->dev.archdata.pci_data;
> +
>         return pdn;
>  }
>
> @@ -74,10 +79,13 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>         struct device_node *dn = NULL;
>         struct pci_dn *parent, *pdn;
>         struct pci_dev *pdev = NULL;
> +       bool pdev_found = false;
>
>         /* Fast path: fetch from PCI device */
>         list_for_each_entry(pdev, &bus->devices, bus_list) {
>                 if (pdev->devfn == devfn) {
> +                       pdev_found = true;
> +
>                         if (pdev->dev.archdata.pci_data)
>                                 return pdev->dev.archdata.pci_data;
>
> @@ -86,6 +94,9 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>                 }
>         }
>
> +       if (!pdev_found)
> +               pdev = NULL;
> +
>         /* Fast path: fetch from device node */
>         pdn = dn ? PCI_DN(dn) : NULL;
>         if (pdn)
> @@ -98,9 +109,12 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>
>         list_for_each_entry(pdn, &parent->child_list, list) {
>                 if (pdn->busno == bus->number &&
> -                    pdn->devfn == devfn)
> -                        return pdn;
> -        }
> +                   pdn->devfn == devfn) {
> +                       if (pdev)
> +                               pdev->dev.archdata.pci_data = pdn;
> +                       return pdn;
> +               }
> +       }
>
>         return NULL;
>  }
> @@ -130,14 +144,15 @@ struct pci_dn *pci_get_pdn(struct pci_dev *pdev)
>
>         list_for_each_entry(pdn, &parent->child_list, list) {
>                 if (pdn->busno == pdev->bus->number &&
> -                   pdn->devfn == pdev->devfn)
> +                   pdn->devfn == pdev->devfn) {
> +                       pdev->dev.archdata.pci_data = pdn;
>                         return pdn;
> +               }
>         }
>
> -       return NULL;
> +       return create_pdn(pdev, parent);
>  }

Eh... If it works I guess it's fine? Killing pdn entirely (or at the
very least greatly restricting it's role) is something we should work
towards though since it doesn't make much sense on PNV.

> -#ifdef CONFIG_PCI_IOV
>  static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
>                                            int vf_index,
>                                            int busno, int devfn)
> @@ -156,7 +171,9 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
>         pdn->parent = parent;
>         pdn->busno = busno;
>         pdn->devfn = devfn;
> +       #ifdef CONFIG_PCI_IOV
>         pdn->vf_index = vf_index;
> +       #endif /* CONFIG_PCI_IOV */

This function is currently only used when adding VFs, so I'd move the
VF specific params from this and put them in the call site at
add_dev_pci_data(). Then you can drop the #ifdef hacks. It might be
worth renaming add_one_dev_pci_data() and add_one_dev_pci_data() since
their actual usage is limited to the VF init path, but their name seem
generic.

>         pdn->pe_number = IODA_INVALID_PE;
>         INIT_LIST_HEAD(&pdn->child_list);
>         INIT_LIST_HEAD(&pdn->list);
> @@ -164,7 +181,55 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
>
>         return pdn;
>  }
> -#endif
> +
> +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent)
> +{
> +       struct pci_dn *pdn = NULL;
> +
> +       if (!parent)
> +               return NULL;
> +
> +       pdn = add_one_dev_pci_data(parent, 0, pdev->bus->busn_res.start, pdev->devfn);
> +       dev_info(&pdev->dev, "Create a new pdn for devfn %2x\n", pdev->devfn / 8);
> +

> +       if (pdn) {

Check for !pdn and exit early. Stacking indentation levels just gets
kind of ugly.

> +               #ifdef CONFIG_EEH
> +               struct eeh_dev *edev;
> +               #endif /* CONFIG_EEH */
You don't use the variable for anything. You can either switch this to
a void pointer or just check the return value of eeh_dev_init()
directly and drop this.

> +               u32 class_code;
> +               u16 device_id;
> +               u16 vendor_id;
> +

> +               #ifdef CONFIG_EEH
> +               edev = eeh_dev_init(pdn);
> +               if (!edev) {
> +                       kfree(pdn);
> +                       dev_err(&pdev->dev, "%s: Failed to allocate edev\n", __func__);
> +                       return NULL;
> +               }
> +               #endif /* CONFIG_EEH */
> +
> +               pci_bus_read_config_word(pdev->bus, pdev->devfn,
> +                                        PCI_VENDOR_ID, &vendor_id);
> +               pdn->vendor_id = vendor_id;
> +
> +               pci_bus_read_config_word(pdev->bus, pdev->devfn,
> +                                        PCI_DEVICE_ID, &device_id);
> +               pdn->device_id = device_id;
> +
> +               pci_bus_read_config_dword(pdev->bus, pdev->devfn,
> +                                         PCI_CLASS_REVISION, &class_code);
> +               class_code >>= 8;
> +               pdn->class_code = class_code;
> +
> +               pdn->pci_ext_config_space = 0;
Does this need to be explicitly initialised or can we rely on it being
allocated with kzalloc() or similar?

> +               pdev->dev.archdata.pci_data = pdn;
> +       } else {
> +               dev_err(&pdev->dev, "%s: Failed to allocate pdn\n", __func__);
> +       }
> +
> +       return pdn;
> +}
>
>  struct pci_dn *add_dev_pci_data(struct pci_dev *pdev)
>  {
> --
> 2.20.1
>

Re: [PATCH RFC v4 2/9] powerpc/powernv/pci: Suppress an EEH error when reading an empty slot

[Sergey Miroshnichenko]

[-- Attachment #1.1: Type: text/plain, Size: 4788 bytes --]



On 3/5/19 9:14 AM, Oliver wrote:
> On Sat, Mar 2, 2019 at 3:04 AM Sergey Miroshnichenko
> <s.miroshnichenko@yadro.com> wrote:
>>
>> Reading an empty slot returns all ones, which triggers a false
>> EEH error event on PowerNV. This patch unfreezes the bus where
>> it has happened.
>>
>> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
>> ---
>>  arch/powerpc/include/asm/ppc-pci.h   |  1 +
>>  arch/powerpc/kernel/pci_dn.c         |  2 +-
>>  arch/powerpc/platforms/powernv/pci.c | 34 ++++++++++++++++++++++++----
>>  3 files changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h
>> index f67da277d652..737393c54f58 100644
>> --- a/arch/powerpc/include/asm/ppc-pci.h
>> +++ b/arch/powerpc/include/asm/ppc-pci.h
>> @@ -40,6 +40,7 @@ void *traverse_pci_dn(struct pci_dn *root,
>>                       void *(*fn)(struct pci_dn *, void *),
>>                       void *data);
>>  extern void pci_devs_phb_init_dynamic(struct pci_controller *phb);
>> +struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus);
>>
>>  /* From rtas_pci.h */
>>  extern void init_pci_config_tokens (void);
>> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
>> index ab147a1909c8..341ed71250f1 100644
>> --- a/arch/powerpc/kernel/pci_dn.c
>> +++ b/arch/powerpc/kernel/pci_dn.c
>> @@ -40,7 +40,7 @@
>>   * one of PF's bridge. For other devices, their firmware
>>   * data is linked to that of their bridge.
>>   */
>> -static struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
>> +struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
>>  {
>>         struct pci_bus *pbus;
>>         struct device_node *dn;
>> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
>> index 3260250d2029..73c2d0aed996 100644
>> --- a/arch/powerpc/platforms/powernv/pci.c
>> +++ b/arch/powerpc/platforms/powernv/pci.c
>> @@ -761,6 +761,21 @@ static inline pnv_pci_cfg_check(struct pci_dn *pdn)
>>  }
>>  #endif /* CONFIG_EEH */
>>
>> +static int get_bus_pe_number(struct pci_bus *bus)
>> +{
>> +       struct pci_dn *pdn = pci_bus_to_pdn(bus);
>> +       struct pci_dn *child;
>> +
>> +       if (!pdn)
>> +               return IODA_INVALID_PE;
>> +
>> +       list_for_each_entry(child, &pdn->child_list, list)
>> +               if (child->pe_number != IODA_INVALID_PE)
>> +                       return child->pe_number;
>> +
>> +       return IODA_INVALID_PE;
>> +}
>> +
>>  static int pnv_pci_read_config(struct pci_bus *bus,
>>                                unsigned int devfn,
>>                                int where, int size, u32 *val)
>> @@ -769,12 +784,23 @@ static int pnv_pci_read_config(struct pci_bus *bus,
>>         struct pci_controller *hose = pci_bus_to_host(bus);
>>         struct pnv_phb *phb = hose->private_data;
>>         int ret;
>> +       u32 empty_val = 0xFFFFFFFF;
>>
>> -       *val = 0xFFFFFFFF;
>> +       *val = empty_val;
>>         pdn = pci_get_pdn_by_devfn(bus, devfn);
>> -       if (!pdn)
>> -               return pnv_pci_cfg_read_raw(phb->opal_id, bus->number, devfn,
>> -                                           where, size, val);
>> +       if (!pdn) {
>> +               int pe_number = get_bus_pe_number(bus);
>> +
>> +               ret = pnv_pci_cfg_read_raw(phb->opal_id, bus->number, devfn,
>> +                                          where, size, val);
>> +
>> +               if (!ret && (*val == empty_val) && phb->unfreeze_pe)
> 
> Do this empty val check work when using 1 or 2 byte cfg accesses?
> 

That was intentional because 0xff and 0xffff are valid values, but the
0xffffffff is the only reliable sign of an empty slot. And the kernel
pokes a slot by the pci_bus_generic_read_dev_vendor_id() function, which
in turn tries to pci_bus_read_config_dword(PCI_VENDOR_ID).

But I haven't tried actually to read 1-2 bytes from an empty slot to
test if that triggers an EEH. If it does, I'll change that to
EEH_IO_ERROR_VALUE(size).

>> +                       phb->unfreeze_pe(phb, (pe_number == IODA_INVALID_PE) ?
>> +                                        0xff : pe_number,
> 
> Use phb->ioda.reserved_pe_idx rather than guessing that 0xff is safe
> to use. On P9 we have PHBs with 512 PEs and some older P8 firmware
> releases used 0 as the reserved PE rather than 0xff.
> 

Thanks for the catch! I'll fix that in v5.

Best regards,
Serge

>> +                                        OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
>> +
>> +               return ret;
>> +       }
>>
>>         if (!pnv_pci_cfg_check(pdn))
>>                 return PCIBIOS_DEVICE_NOT_FOUND;
>> --
>> 2.20.1
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC v4 3/9] powerpc/pci: Create pci_dn on demand

[Sergey Miroshnichenko]

[-- Attachment #1.1: Type: text/plain, Size: 7784 bytes --]

On 3/5/19 11:04 AM, Oliver wrote:
> On Sat, Mar 2, 2019 at 3:04 AM Sergey Miroshnichenko
> <s.miroshnichenko@yadro.com> wrote:
>>
>> If a struct pci_dn hasn't yet been created for the PCIe device (there was
>> no DT node for it), allocate this structure and fill with info read from
>> the device directly.
>>
>> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
>> ---
>>  arch/powerpc/kernel/pci_dn.c | 79 ++++++++++++++++++++++++++++++++----
>>  1 file changed, 72 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
>> index 341ed71250f1..67ccd7be8344 100644
>> --- a/arch/powerpc/kernel/pci_dn.c
>> +++ b/arch/powerpc/kernel/pci_dn.c
>> @@ -33,6 +33,8 @@
>>  #include <asm/firmware.h>
>>  #include <asm/eeh.h>
>>
>> +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent);
>> +
>>  /*
>>   * The function is used to find the firmware data of one
>>   * specific PCI device, which is attached to the indicated
>> @@ -65,6 +67,9 @@ struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
>>         dn = pci_bus_to_OF_node(pbus);
>>         pdn = dn ? PCI_DN(dn) : NULL;
>>
>> +       if (!pdn && pbus->self)
>> +               pdn = pbus->self->dev.archdata.pci_data;
>> +
>>         return pdn;
>>  }
>>
>> @@ -74,10 +79,13 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>>         struct device_node *dn = NULL;
>>         struct pci_dn *parent, *pdn;
>>         struct pci_dev *pdev = NULL;
>> +       bool pdev_found = false;
>>
>>         /* Fast path: fetch from PCI device */
>>         list_for_each_entry(pdev, &bus->devices, bus_list) {
>>                 if (pdev->devfn == devfn) {
>> +                       pdev_found = true;
>> +
>>                         if (pdev->dev.archdata.pci_data)
>>                                 return pdev->dev.archdata.pci_data;
>>
>> @@ -86,6 +94,9 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>>                 }
>>         }
>>
>> +       if (!pdev_found)
>> +               pdev = NULL;
>> +
>>         /* Fast path: fetch from device node */
>>         pdn = dn ? PCI_DN(dn) : NULL;
>>         if (pdn)
>> @@ -98,9 +109,12 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>>
>>         list_for_each_entry(pdn, &parent->child_list, list) {
>>                 if (pdn->busno == bus->number &&
>> -                    pdn->devfn == devfn)
>> -                        return pdn;
>> -        }
>> +                   pdn->devfn == devfn) {
>> +                       if (pdev)
>> +                               pdev->dev.archdata.pci_data = pdn;
>> +                       return pdn;
>> +               }
>> +       }
>>
>>         return NULL;
>>  }
>> @@ -130,14 +144,15 @@ struct pci_dn *pci_get_pdn(struct pci_dev *pdev)
>>
>>         list_for_each_entry(pdn, &parent->child_list, list) {
>>                 if (pdn->busno == pdev->bus->number &&
>> -                   pdn->devfn == pdev->devfn)
>> +                   pdn->devfn == pdev->devfn) {
>> +                       pdev->dev.archdata.pci_data = pdn;
>>                         return pdn;
>> +               }
>>         }
>>
>> -       return NULL;
>> +       return create_pdn(pdev, parent);
>>  }
> 
> Eh... If it works I guess it's fine? Killing pdn entirely (or at the
> very least greatly restricting it's role) is something we should work
> towards though since it doesn't make much sense on PNV.
> 

As I can see, pdns are mostly used on PNV for the following:
 - storing a pe_number;
 - storing struct resource holes - to reserve memory for SR-IOV;
 - IOMMU table_group;
 - storing m64_map;
 - storing struct eeh_dev *edev;

Other fields (busno, devfn, etc.) are available in struct pci_dev.

The pe_numbers are probably can be somehow moved to struct pnv_phb, and
they can be fetched from there by a bdfn. The same for other used
fields. Or there could some new structure - shorter version of struct
pci_dn, without .child_list etc.

Thanks for the review, all the issues you've pointed below will be fixed
in v5!

Best regards,
Serge

>> -#ifdef CONFIG_PCI_IOV
>>  static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
>>                                            int vf_index,
>>                                            int busno, int devfn)
>> @@ -156,7 +171,9 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
>>         pdn->parent = parent;
>>         pdn->busno = busno;
>>         pdn->devfn = devfn;
>> +       #ifdef CONFIG_PCI_IOV
>>         pdn->vf_index = vf_index;
>> +       #endif /* CONFIG_PCI_IOV */
> 
> This function is currently only used when adding VFs, so I'd move the
> VF specific params from this and put them in the call site at
> add_dev_pci_data(). Then you can drop the #ifdef hacks. It might be
> worth renaming add_one_dev_pci_data() and add_one_dev_pci_data() since
> their actual usage is limited to the VF init path, but their name seem
> generic.
> 
>>         pdn->pe_number = IODA_INVALID_PE;
>>         INIT_LIST_HEAD(&pdn->child_list);
>>         INIT_LIST_HEAD(&pdn->list);
>> @@ -164,7 +181,55 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
>>
>>         return pdn;
>>  }
>> -#endif
>> +
>> +static struct pci_dn *create_pdn(struct pci_dev *pdev, struct pci_dn *parent)
>> +{
>> +       struct pci_dn *pdn = NULL;
>> +
>> +       if (!parent)
>> +               return NULL;
>> +
>> +       pdn = add_one_dev_pci_data(parent, 0, pdev->bus->busn_res.start, pdev->devfn);
>> +       dev_info(&pdev->dev, "Create a new pdn for devfn %2x\n", pdev->devfn / 8);
>> +
> 
>> +       if (pdn) {
> 
> Check for !pdn and exit early. Stacking indentation levels just gets
> kind of ugly.
> 
>> +               #ifdef CONFIG_EEH
>> +               struct eeh_dev *edev;
>> +               #endif /* CONFIG_EEH */
> You don't use the variable for anything. You can either switch this to
> a void pointer or just check the return value of eeh_dev_init()
> directly and drop this.
> 
>> +               u32 class_code;
>> +               u16 device_id;
>> +               u16 vendor_id;
>> +
> 
>> +               #ifdef CONFIG_EEH
>> +               edev = eeh_dev_init(pdn);
>> +               if (!edev) {
>> +                       kfree(pdn);
>> +                       dev_err(&pdev->dev, "%s: Failed to allocate edev\n", __func__);
>> +                       return NULL;
>> +               }
>> +               #endif /* CONFIG_EEH */
>> +
>> +               pci_bus_read_config_word(pdev->bus, pdev->devfn,
>> +                                        PCI_VENDOR_ID, &vendor_id);
>> +               pdn->vendor_id = vendor_id;
>> +
>> +               pci_bus_read_config_word(pdev->bus, pdev->devfn,
>> +                                        PCI_DEVICE_ID, &device_id);
>> +               pdn->device_id = device_id;
>> +
>> +               pci_bus_read_config_dword(pdev->bus, pdev->devfn,
>> +                                         PCI_CLASS_REVISION, &class_code);
>> +               class_code >>= 8;
>> +               pdn->class_code = class_code;
>> +
>> +               pdn->pci_ext_config_space = 0;
> Does this need to be explicitly initialised or can we rely on it being
> allocated with kzalloc() or similar?
> 
>> +               pdev->dev.archdata.pci_data = pdn;
>> +       } else {
>> +               dev_err(&pdev->dev, "%s: Failed to allocate pdn\n", __func__);
>> +       }
>> +
>> +       return pdn;
>> +}
>>
>>  struct pci_dn *add_dev_pci_data(struct pci_dev *pdev)
>>  {
>> --
>> 2.20.1
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]