[PATCH v2 0/5] powerpc/powernv/pci: Discover surprise-hotplugged PCIe devices during rescan

[PATCH v2 0/5] powerpc/powernv/pci: Discover surprise-hotplugged PCIe devices during rescan

[Sergey Miroshnichenko]

This patchset allows hotplugged PCIe devices to be enumerated during a bus
rescan being issued via sysfs on PowerNV platforms, when the "Presence
Detect Changed" interrupt is not available.

As a first part of our work on adding support for hotplugging PCIe bridges
full of devices (without special requirement such as Hot-Plug Controller,
reservation of bus numbers and memory regions by firmware, etc.), this
serie is intended to solve the first of two problems from the list below:

I   PowerNV doesn't discover new hotplugged PCIe devices
II  EEH is falsely triggered when poking empty slots during the PCIe rescan
III The PCI subsystem is not prepared to runtime changes of BAR addresses
IV  Device drivers don't track changes of their BAR addresses
V   Allow the moving BARs of working devices to make space for new ones
VI  Add support for PCIe bridge hotplug

Tested on:
 - POWER8 PowerNV+OPAL ppc64le (our Vesnin server) with pci=realloc;
 - POWER8 IBM 8247-42L (pSeries);
 - POWER8 IBM 8247-42L (PowerNV+OPAL) with pci=realloc.

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.

Sergey Miroshnichenko (5):
  powerpc/pci: Access PCI config space directly w/o pci_dn
  powerpc/pci: Create pci_dn on demand
  powerpc/pci: Use DT to create pci_dn for root bridges only
  powerpc/powernv/pci: Enable reassigning the bus numbers
  PCI/powerpc/eeh: Add pcibios hooks for preparing to rescan

 arch/powerpc/include/asm/eeh.h               |  2 +
 arch/powerpc/kernel/eeh.c                    | 12 +++
 arch/powerpc/kernel/pci_dn.c                 | 82 +++++++++++++----
 arch/powerpc/kernel/rtas_pci.c               | 97 +++++++++++++-------
 arch/powerpc/platforms/powernv/eeh-powernv.c | 22 +++++
 arch/powerpc/platforms/powernv/pci.c         | 65 ++++++++-----
 drivers/pci/probe.c                          | 14 +++
 include/linux/pci.h                          |  2 +
 8 files changed, 225 insertions(+), 71 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] powerpc/pci: Access PCI config space directly w/o pci_dn

[Sergey Miroshnichenko]

The pci_dn structures are retrieved from a DT, but hot-plugged PCIe
devices don't have them. Don't stop PCIe I/O in absence of pci_dn, so
it is now possible to discover new devices.

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..0611b46d9b5f 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 && eeh_enabled()) {
+		/* 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 && eeh_enabled()) {
+		/* 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 13aef2323bbc..3f87a2dc6578 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -654,30 +654,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)
+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;
 	}
@@ -686,27 +685,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)
+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;
@@ -715,6 +715,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)
 {
@@ -750,13 +768,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;
@@ -779,12 +799,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.17.1

Re: [PATCH v2 1/5] powerpc/pci: Access PCI config space directly w/o pci_dn

[Sam Bobroff]

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

Hi Sergey,

On Thu, Sep 06, 2018 at 02:57:48PM +0300, Sergey Miroshnichenko wrote:
> The pci_dn structures are retrieved from a DT, but hot-plugged PCIe
> devices don't have them. Don't stop PCIe I/O in absence of pci_dn, so
> it is now possible to discover new devices.
> 
> 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..0611b46d9b5f 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 && eeh_enabled()) {
> +		/* 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);
> +	}

In the above block, if pdn is valid but EEH isn't enabled,
rtas_read_raw_config() will be used instead of rtas_read_config(), so
config_access_valid() won't be tested. Is that correct?

>  
>  	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 && eeh_enabled()) {
> +		/* 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);

Same comment as rtas_pci_read_config() above.

>  
> -	/* 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 13aef2323bbc..3f87a2dc6578 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -654,30 +654,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)
> +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;
>  	}
> @@ -686,27 +685,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)
> +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;
> @@ -715,6 +715,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)
>  {
> @@ -750,13 +768,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;
> @@ -779,12 +799,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.17.1
> 

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

Re: [PATCH v2 1/5] powerpc/pci: Access PCI config space directly w/o pci_dn

[Sergey Miroshnichenko]

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

Hello Sam,

On 9/10/18 7:23 AM, Sam Bobroff wrote:
> Hi Sergey,
> 
> On Thu, Sep 06, 2018 at 02:57:48PM +0300, Sergey Miroshnichenko wrote:
>> The pci_dn structures are retrieved from a DT, but hot-plugged PCIe
>> devices don't have them. Don't stop PCIe I/O in absence of pci_dn, so
>> it is now possible to discover new devices.
>>
>> 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..0611b46d9b5f 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 && eeh_enabled()) {
>> +		/* 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);
>> +	}
> 
> In the above block, if pdn is valid but EEH isn't enabled,
> rtas_read_raw_config() will be used instead of rtas_read_config(), so
> config_access_valid() won't be tested. Is that correct?
> 

Thank you for the review!

This was the original intention, but now I can see that if a pdn is
valid, the EEH-branch should be taken even if EEH is disabled, as it was
before this patch; and functions there have checks for eeh_enabled()
inside. I'll fix that in v3 as follows:

-	if (pdn && eeh_enabled()) {
+	if (pdn) {

>>  
>>  	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 && eeh_enabled()) {
>> +		/* 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);
> 
> Same comment as rtas_pci_read_config() above.
> 

I'll fix that symmetrically.

>>  
>> -	/* 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 13aef2323bbc..3f87a2dc6578 100644
>> --- a/arch/powerpc/platforms/powernv/pci.c
>> +++ b/arch/powerpc/platforms/powernv/pci.c
>> @@ -654,30 +654,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)
>> +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;
>>  	}
>> @@ -686,27 +685,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)
>> +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;
>> @@ -715,6 +715,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)
>>  {
>> @@ -750,13 +768,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;
>> @@ -779,12 +799,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.17.1
>>

Best regards,
Serge


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

[PATCH v2 2/5] powerpc/pci: Create pci_dn on demand

[Sergey Miroshnichenko]

The pci_dn structures can be created not only from DT, but also
directly from newly discovered PCIe devices, so allocate them
dynamically.

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

diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index ab147a1909c8..48ec16407835 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
@@ -58,6 +60,9 @@ static struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
 		pbus = pbus->parent;
 	}
 
+	if (!pbus->self && !pci_is_root_bus(pbus))
+		return NULL;
+
 	/*
 	 * Except virtual bus, all PCI buses should
 	 * have device nodes.
@@ -65,13 +70,15 @@ static 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;
 }
 
 struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
 				    int devfn)
 {
-	struct device_node *dn = NULL;
 	struct pci_dn *parent, *pdn;
 	struct pci_dev *pdev = NULL;
 
@@ -80,17 +87,10 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
 		if (pdev->devfn == devfn) {
 			if (pdev->dev.archdata.pci_data)
 				return pdev->dev.archdata.pci_data;
-
-			dn = pci_device_to_OF_node(pdev);
 			break;
 		}
 	}
 
-	/* Fast path: fetch from device node */
-	pdn = dn ? PCI_DN(dn) : NULL;
-	if (pdn)
-		return pdn;
-
 	/* Slow path: fetch from firmware data hierarchy */
 	parent = pci_bus_to_pdn(bus);
 	if (!parent)
@@ -128,16 +128,9 @@ struct pci_dn *pci_get_pdn(struct pci_dev *pdev)
 	if (!parent)
 		return NULL;
 
-	list_for_each_entry(pdn, &parent->child_list, list) {
-		if (pdn->busno == pdev->bus->number &&
-		    pdn->devfn == pdev->devfn)
-			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 +149,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 +159,54 @@ 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;
+
+	pdn = add_one_dev_pci_data(parent, 0, pdev->bus->number, 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 */
+
+		pdn->busno = pdev->bus->busn_res.start;
+
+		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.17.1

Re: [PATCH v2 2/5] powerpc/pci: Create pci_dn on demand

[Sam Bobroff]

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

Hi Sergey,

On Thu, Sep 06, 2018 at 02:57:49PM +0300, Sergey Miroshnichenko wrote:
> The pci_dn structures can be created not only from DT, but also
> directly from newly discovered PCIe devices, so allocate them
> dynamically.
> 
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
>  arch/powerpc/kernel/pci_dn.c | 76 ++++++++++++++++++++++++++++--------
>  1 file changed, 59 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
> index ab147a1909c8..48ec16407835 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
> @@ -58,6 +60,9 @@ static struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
>  		pbus = pbus->parent;
>  	}
>  
> +	if (!pbus->self && !pci_is_root_bus(pbus))
> +		return NULL;
> +
>  	/*
>  	 * Except virtual bus, all PCI buses should
>  	 * have device nodes.
> @@ -65,13 +70,15 @@ static 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;
>  }
>  
>  struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>  				    int devfn)
>  {
> -	struct device_node *dn = NULL;
>  	struct pci_dn *parent, *pdn;
>  	struct pci_dev *pdev = NULL;
>  
> @@ -80,17 +87,10 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>  		if (pdev->devfn == devfn) {
>  			if (pdev->dev.archdata.pci_data)
>  				return pdev->dev.archdata.pci_data;
> -
> -			dn = pci_device_to_OF_node(pdev);
>  			break;
>  		}
>  	}
>  
> -	/* Fast path: fetch from device node */
> -	pdn = dn ? PCI_DN(dn) : NULL;
> -	if (pdn)
> -		return pdn;
> -

Why is it necessary to remove the above fast-path?

>  	/* Slow path: fetch from firmware data hierarchy */
>  	parent = pci_bus_to_pdn(bus);
>  	if (!parent)
> @@ -128,16 +128,9 @@ struct pci_dn *pci_get_pdn(struct pci_dev *pdev)
>  	if (!parent)
>  		return NULL;
>  
> -	list_for_each_entry(pdn, &parent->child_list, list) {
> -		if (pdn->busno == pdev->bus->number &&
> -		    pdn->devfn == pdev->devfn)
> -			return pdn;
> -	}

Could you explain why the above block was removed? Is it now impossible
for it to find a 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 +149,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);

I can see that this change allows you to re-use this to set up a pdn in
create_pdn(). Perhaps you should refactor pci_add_device_node_info() to
use it as well, now that it's possible?

> @@ -164,7 +159,54 @@ 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;
> +
> +	pdn = add_one_dev_pci_data(parent, 0, pdev->bus->number, 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 */
> +
> +		pdn->busno = pdev->bus->busn_res.start;

It seems strange that pdn->busno is set by the call to
add_one_dev_pci_data() above (to pdev->bus->number) and then overwritten
here with a different value. Should add_one_dev_pci_data() use
pdev->bus->busn_res.start and this line be removed?

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

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

Re: [PATCH v2 2/5] powerpc/pci: Create pci_dn on demand

[Sergey Miroshnichenko]

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

Hello Sam,

On 9/10/18 7:47 AM, Sam Bobroff wrote:
> Hi Sergey,
> 
> On Thu, Sep 06, 2018 at 02:57:49PM +0300, Sergey Miroshnichenko wrote:
>> The pci_dn structures can be created not only from DT, but also
>> directly from newly discovered PCIe devices, so allocate them
>> dynamically.
>>
>> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
>> ---
>>  arch/powerpc/kernel/pci_dn.c | 76 ++++++++++++++++++++++++++++--------
>>  1 file changed, 59 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
>> index ab147a1909c8..48ec16407835 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
>> @@ -58,6 +60,9 @@ static struct pci_dn *pci_bus_to_pdn(struct pci_bus *bus)
>>  		pbus = pbus->parent;
>>  	}
>>  
>> +	if (!pbus->self && !pci_is_root_bus(pbus))
>> +		return NULL;
>> +
>>  	/*
>>  	 * Except virtual bus, all PCI buses should
>>  	 * have device nodes.
>> @@ -65,13 +70,15 @@ static 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;
>>  }
>>  
>>  struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>>  				    int devfn)
>>  {
>> -	struct device_node *dn = NULL;
>>  	struct pci_dn *parent, *pdn;
>>  	struct pci_dev *pdev = NULL;
>>  
>> @@ -80,17 +87,10 @@ struct pci_dn *pci_get_pdn_by_devfn(struct pci_bus *bus,
>>  		if (pdev->devfn == devfn) {
>>  			if (pdev->dev.archdata.pci_data)
>>  				return pdev->dev.archdata.pci_data;
>> -
>> -			dn = pci_device_to_OF_node(pdev);
>>  			break;
>>  		}
>>  	}
>>  
>> -	/* Fast path: fetch from device node */
>> -	pdn = dn ? PCI_DN(dn) : NULL;
>> -	if (pdn)
>> -		return pdn;
>> -
> 
> Why is it necessary to remove the above fast-path?
> 

It is not, actually - this had leaked from early stages of debugging,
when I've found that after hotplug+rescan or hotplug+kexec the kernel
took pdns from device nodes that do not represent the actual PCIe
topology anymore. But after patches 3 and 4 the PCI_DN() is NULL for
every PCIe device except root on PowerNV, so this block is safe.

It will remain in version 3 of this patchset.

>>  	/* Slow path: fetch from firmware data hierarchy */
>>  	parent = pci_bus_to_pdn(bus);
>>  	if (!parent)
>> @@ -128,16 +128,9 @@ struct pci_dn *pci_get_pdn(struct pci_dev *pdev)
>>  	if (!parent)
>>  		return NULL;
>>  
>> -	list_for_each_entry(pdn, &parent->child_list, list) {
>> -		if (pdn->busno == pdev->bus->number &&
>> -		    pdn->devfn == pdev->devfn)
>> -			return pdn;
>> -	}
> 
> Could you explain why the above block was removed? Is it now impossible
> for it to find a pdn?
> 

I see now that this block was also removed too early: on PowerNV with
this patchset it is impossible to have pdn being present in
parent->child_list but not in pdev->dev.archdata.pci_data; but this may
be not the case for pSeries.

>> -
>> -	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 +149,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);
> 
> I can see that this change allows you to re-use this to set up a pdn in
> create_pdn(). Perhaps you should refactor pci_add_device_node_info() to
> use it as well, now that it's possible?
> 

Sure, will do.

>> @@ -164,7 +159,54 @@ 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;
>> +
>> +	pdn = add_one_dev_pci_data(parent, 0, pdev->bus->number, 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 */
>> +
>> +		pdn->busno = pdev->bus->busn_res.start;
> 
> It seems strange that pdn->busno is set by the call to
> add_one_dev_pci_data() above (to pdev->bus->number) and then overwritten
> here with a different value. Should add_one_dev_pci_data() use
> pdev->bus->busn_res.start and this line be removed?
> 

Thanks for capturing that! I'll prepare v3 with the fixes.

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

Best regards,
Serge


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

[PATCH v2 3/5] powerpc/pci: Use DT to create pci_dn for root bridges only

[Sergey Miroshnichenko]

Endpoint's pci_dn can be created dynamically.

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

diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index 48ec16407835..3b4368f07c82 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -537,8 +537,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);
+	}
 }
 
 /** 
-- 
2.17.1

[PATCH v2 4/5] powerpc/powernv/pci: Enable reassigning the bus numbers

[Sergey Miroshnichenko]

PowerNV doesn't depend on PCIe topology info from DT anymore, and now
it is able to enumerate the fabric and assign the bus numbers.

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

diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index 3f87a2dc6578..ece3f3e2808e 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -1112,6 +1112,7 @@ void __init pnv_pci_init(void)
 	struct device_node *np;
 
 	pci_add_flags(PCI_CAN_SKIP_ISA_ALIGN);
+	pci_add_flags(PCI_REASSIGN_ALL_BUS);
 
 	/* If we don't have OPAL, eg. in sim, just skip PCI probe */
 	if (!firmware_has_feature(FW_FEATURE_OPAL))
-- 
2.17.1

[PATCH v2 5/5] PCI/powerpc/eeh: Add pcibios hooks for preparing to rescan

[Sergey Miroshnichenko]

Reading an empty slot returns all ones, which triggers a false
EEH error event on PowerNV.

New callbacks pcibios_rescan_prepare/done are introduced to
pause/resume the EEH during rescan.

Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
---
 arch/powerpc/include/asm/eeh.h               |  2 ++
 arch/powerpc/kernel/eeh.c                    | 12 +++++++++++
 arch/powerpc/platforms/powernv/eeh-powernv.c | 22 ++++++++++++++++++++
 drivers/pci/probe.c                          | 14 +++++++++++++
 include/linux/pci.h                          |  2 ++
 5 files changed, 52 insertions(+)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 219637ea69a1..926c3e31df99 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -219,6 +219,8 @@ struct eeh_ops {
 	int (*next_error)(struct eeh_pe **pe);
 	int (*restore_config)(struct pci_dn *pdn);
 	int (*notify_resume)(struct pci_dn *pdn);
+	int (*pause)(struct pci_bus *bus);
+	int (*resume)(struct pci_bus *bus);
 };
 
 extern int eeh_subsystem_flags;
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 6ebba3e48b01..9fb5012f389d 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1831,3 +1831,15 @@ static int __init eeh_init_proc(void)
 	return 0;
 }
 __initcall(eeh_init_proc);
+
+void pcibios_rescan_prepare(struct pci_bus *bus)
+{
+	if (eeh_ops && eeh_ops->pause)
+		eeh_ops->pause(bus);
+}
+
+void pcibios_rescan_done(struct pci_bus *bus)
+{
+	if (eeh_ops && eeh_ops->resume)
+		eeh_ops->resume(bus);
+}
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index 3c1beae29f2d..9724a58afcd2 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -59,6 +59,26 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
 	eeh_sysfs_add_device(pdev);
 }
 
+static int pnv_eeh_pause(struct pci_bus *bus)
+{
+	struct pci_controller *hose = pci_bus_to_host(bus);
+	struct pnv_phb *phb = hose->private_data;
+
+	phb->flags &= ~PNV_PHB_FLAG_EEH;
+	disable_irq(eeh_event_irq);
+	return 0;
+}
+
+static int pnv_eeh_resume(struct pci_bus *bus)
+{
+	struct pci_controller *hose = pci_bus_to_host(bus);
+	struct pnv_phb *phb = hose->private_data;
+
+	enable_irq(eeh_event_irq);
+	phb->flags |= PNV_PHB_FLAG_EEH;
+	return 0;
+}
+
 static int pnv_eeh_init(void)
 {
 	struct pci_controller *hose;
@@ -1710,6 +1730,8 @@ static struct eeh_ops pnv_eeh_ops = {
 	.write_config           = pnv_eeh_write_config,
 	.next_error		= pnv_eeh_next_error,
 	.restore_config		= pnv_eeh_restore_config,
+	.pause			= pnv_eeh_pause,
+	.resume			= pnv_eeh_resume,
 	.notify_resume		= NULL
 };
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac876e32de4b..4a9045364809 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2801,6 +2801,14 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
 {
 }
 
+void __weak pcibios_rescan_prepare(struct pci_bus *bus)
+{
+}
+
+void __weak pcibios_rescan_done(struct pci_bus *bus)
+{
+}
+
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 		struct pci_ops *ops, void *sysdata, struct list_head *resources)
 {
@@ -3055,9 +3063,15 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
 unsigned int pci_rescan_bus(struct pci_bus *bus)
 {
 	unsigned int max;
+	struct pci_bus *root = bus;
+
+	while (!pci_is_root_bus(root))
+		root = root->parent;
 
+	pcibios_rescan_prepare(root);
 	max = pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
+	pcibios_rescan_done(root);
 	pci_bus_add_devices(bus);
 
 	return max;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 340029b2fb38..42930731c5a7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1929,6 +1929,8 @@ void pcibios_penalize_isa_irq(int irq, int active);
 int pcibios_alloc_irq(struct pci_dev *dev);
 void pcibios_free_irq(struct pci_dev *dev);
 resource_size_t pcibios_default_alignment(void);
+void pcibios_rescan_prepare(struct pci_bus *bus);
+void pcibios_rescan_done(struct pci_bus *bus);
 
 #ifdef CONFIG_HIBERNATE_CALLBACKS
 extern struct dev_pm_ops pcibios_pm_ops;
-- 
2.17.1

Re: [PATCH v2 5/5] PCI/powerpc/eeh: Add pcibios hooks for preparing to rescan

[Sam Bobroff]

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

Hi Sergey,

On Thu, Sep 06, 2018 at 02:57:52PM +0300, Sergey Miroshnichenko wrote:
> Reading an empty slot returns all ones, which triggers a false
> EEH error event on PowerNV.
> 
> New callbacks pcibios_rescan_prepare/done are introduced to
> pause/resume the EEH during rescan.

If I understand it correctly, this temporarily disables EEH for config space
accesses on the whole PHB while the rescan runs. Is it possible that a
real EEH event could be missed if it occurred during the rescan?

Even if it's not possible, I think it would be good to mention that in a
comment.

> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
>  arch/powerpc/include/asm/eeh.h               |  2 ++
>  arch/powerpc/kernel/eeh.c                    | 12 +++++++++++
>  arch/powerpc/platforms/powernv/eeh-powernv.c | 22 ++++++++++++++++++++
>  drivers/pci/probe.c                          | 14 +++++++++++++
>  include/linux/pci.h                          |  2 ++
>  5 files changed, 52 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 219637ea69a1..926c3e31df99 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -219,6 +219,8 @@ struct eeh_ops {
>  	int (*next_error)(struct eeh_pe **pe);
>  	int (*restore_config)(struct pci_dn *pdn);
>  	int (*notify_resume)(struct pci_dn *pdn);
> +	int (*pause)(struct pci_bus *bus);
> +	int (*resume)(struct pci_bus *bus);

I think these names are a bit too generic, what about naming them
pause_bus()/resume_bus() or even prepare_rescan()/rescan_done()?

>  };
>  
>  extern int eeh_subsystem_flags;
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 6ebba3e48b01..9fb5012f389d 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1831,3 +1831,15 @@ static int __init eeh_init_proc(void)
>  	return 0;
>  }
>  __initcall(eeh_init_proc);
> +
> +void pcibios_rescan_prepare(struct pci_bus *bus)
> +{
> +	if (eeh_ops && eeh_ops->pause)
> +		eeh_ops->pause(bus);
> +}
> +
> +void pcibios_rescan_done(struct pci_bus *bus)
> +{
> +	if (eeh_ops && eeh_ops->resume)
> +		eeh_ops->resume(bus);
> +}
> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
> index 3c1beae29f2d..9724a58afcd2 100644
> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> @@ -59,6 +59,26 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
>  	eeh_sysfs_add_device(pdev);
>  }
>  
> +static int pnv_eeh_pause(struct pci_bus *bus)
> +{
> +	struct pci_controller *hose = pci_bus_to_host(bus);
> +	struct pnv_phb *phb = hose->private_data;
> +
> +	phb->flags &= ~PNV_PHB_FLAG_EEH;
> +	disable_irq(eeh_event_irq);
> +	return 0;
> +}
> +
> +static int pnv_eeh_resume(struct pci_bus *bus)
> +{
> +	struct pci_controller *hose = pci_bus_to_host(bus);
> +	struct pnv_phb *phb = hose->private_data;
> +
> +	enable_irq(eeh_event_irq);
> +	phb->flags |= PNV_PHB_FLAG_EEH;
> +	return 0;
> +}
> +
>  static int pnv_eeh_init(void)
>  {
>  	struct pci_controller *hose;
> @@ -1710,6 +1730,8 @@ static struct eeh_ops pnv_eeh_ops = {
>  	.write_config           = pnv_eeh_write_config,
>  	.next_error		= pnv_eeh_next_error,
>  	.restore_config		= pnv_eeh_restore_config,
> +	.pause			= pnv_eeh_pause,
> +	.resume			= pnv_eeh_resume,
>  	.notify_resume		= NULL
>  };
>  
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac876e32de4b..4a9045364809 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2801,6 +2801,14 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
>  {
>  }
>  
> +void __weak pcibios_rescan_prepare(struct pci_bus *bus)
> +{
> +}
> +
> +void __weak pcibios_rescan_done(struct pci_bus *bus)
> +{
> +}
> +
>  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  		struct pci_ops *ops, void *sysdata, struct list_head *resources)
>  {
> @@ -3055,9 +3063,15 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
>  unsigned int pci_rescan_bus(struct pci_bus *bus)
>  {
>  	unsigned int max;
> +	struct pci_bus *root = bus;
> +
> +	while (!pci_is_root_bus(root))
> +		root = root->parent;
>  
> +	pcibios_rescan_prepare(root);
>  	max = pci_scan_child_bus(bus);
>  	pci_assign_unassigned_bus_resources(bus);
> +	pcibios_rescan_done(root);
>  	pci_bus_add_devices(bus);
>  
>  	return max;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 340029b2fb38..42930731c5a7 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1929,6 +1929,8 @@ void pcibios_penalize_isa_irq(int irq, int active);
>  int pcibios_alloc_irq(struct pci_dev *dev);
>  void pcibios_free_irq(struct pci_dev *dev);
>  resource_size_t pcibios_default_alignment(void);
> +void pcibios_rescan_prepare(struct pci_bus *bus);
> +void pcibios_rescan_done(struct pci_bus *bus);
>  
>  #ifdef CONFIG_HIBERNATE_CALLBACKS
>  extern struct dev_pm_ops pcibios_pm_ops;
> -- 
> 2.17.1
> 

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

Re: [PATCH v2 5/5] PCI/powerpc/eeh: Add pcibios hooks for preparing to rescan

[Sergey Miroshnichenko]

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

Hello Sam,

On 9/10/18 8:03 AM, Sam Bobroff wrote:
> Hi Sergey,
> 
> On Thu, Sep 06, 2018 at 02:57:52PM +0300, Sergey Miroshnichenko wrote:
>> Reading an empty slot returns all ones, which triggers a false
>> EEH error event on PowerNV.
>>
>> New callbacks pcibios_rescan_prepare/done are introduced to
>> pause/resume the EEH during rescan.
> 
> If I understand it correctly, this temporarily disables EEH for config space
> accesses on the whole PHB while the rescan runs. Is it possible that a
> real EEH event could be missed if it occurred during the rescan?
> 
> Even if it's not possible, I think it would be good to mention that in a
> comment.

Yes, missing a real EEH event is possible, unfortunately, and it is
indeed worth mentioning.

To reduce this probability the next patchset I'll post in a few days
among other things puts all the affected device drivers to pause during
rescan, mainly because of moving BARs and bridge windows, but it will
also help here a bit.

> 
>> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
>> ---
>>  arch/powerpc/include/asm/eeh.h               |  2 ++
>>  arch/powerpc/kernel/eeh.c                    | 12 +++++++++++
>>  arch/powerpc/platforms/powernv/eeh-powernv.c | 22 ++++++++++++++++++++
>>  drivers/pci/probe.c                          | 14 +++++++++++++
>>  include/linux/pci.h                          |  2 ++
>>  5 files changed, 52 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
>> index 219637ea69a1..926c3e31df99 100644
>> --- a/arch/powerpc/include/asm/eeh.h
>> +++ b/arch/powerpc/include/asm/eeh.h
>> @@ -219,6 +219,8 @@ struct eeh_ops {
>>  	int (*next_error)(struct eeh_pe **pe);
>>  	int (*restore_config)(struct pci_dn *pdn);
>>  	int (*notify_resume)(struct pci_dn *pdn);
>> +	int (*pause)(struct pci_bus *bus);
>> +	int (*resume)(struct pci_bus *bus);
> 
> I think these names are a bit too generic, what about naming them
> pause_bus()/resume_bus() or even prepare_rescan()/rescan_done()?
> 

Thanks! I will rename them to rescan_prepare/rescan_done to make friends
with reset_prepare/reset_done from struct pci_error_handlers.

>>  };
>>  
>>  extern int eeh_subsystem_flags;
>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>> index 6ebba3e48b01..9fb5012f389d 100644
>> --- a/arch/powerpc/kernel/eeh.c
>> +++ b/arch/powerpc/kernel/eeh.c
>> @@ -1831,3 +1831,15 @@ static int __init eeh_init_proc(void)
>>  	return 0;
>>  }
>>  __initcall(eeh_init_proc);
>> +
>> +void pcibios_rescan_prepare(struct pci_bus *bus)
>> +{
>> +	if (eeh_ops && eeh_ops->pause)
>> +		eeh_ops->pause(bus);
>> +}
>> +
>> +void pcibios_rescan_done(struct pci_bus *bus)
>> +{
>> +	if (eeh_ops && eeh_ops->resume)
>> +		eeh_ops->resume(bus);
>> +}
>> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
>> index 3c1beae29f2d..9724a58afcd2 100644
>> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
>> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
>> @@ -59,6 +59,26 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
>>  	eeh_sysfs_add_device(pdev);
>>  }
>>  
>> +static int pnv_eeh_pause(struct pci_bus *bus)
>> +{
>> +	struct pci_controller *hose = pci_bus_to_host(bus);
>> +	struct pnv_phb *phb = hose->private_data;
>> +
>> +	phb->flags &= ~PNV_PHB_FLAG_EEH;
>> +	disable_irq(eeh_event_irq);
>> +	return 0;
>> +}
>> +
>> +static int pnv_eeh_resume(struct pci_bus *bus)
>> +{
>> +	struct pci_controller *hose = pci_bus_to_host(bus);
>> +	struct pnv_phb *phb = hose->private_data;
>> +
>> +	enable_irq(eeh_event_irq);
>> +	phb->flags |= PNV_PHB_FLAG_EEH;
>> +	return 0;
>> +}
>> +
>>  static int pnv_eeh_init(void)
>>  {
>>  	struct pci_controller *hose;
>> @@ -1710,6 +1730,8 @@ static struct eeh_ops pnv_eeh_ops = {
>>  	.write_config           = pnv_eeh_write_config,
>>  	.next_error		= pnv_eeh_next_error,
>>  	.restore_config		= pnv_eeh_restore_config,
>> +	.pause			= pnv_eeh_pause,
>> +	.resume			= pnv_eeh_resume,
>>  	.notify_resume		= NULL
>>  };
>>  
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index ac876e32de4b..4a9045364809 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2801,6 +2801,14 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
>>  {
>>  }
>>  
>> +void __weak pcibios_rescan_prepare(struct pci_bus *bus)
>> +{
>> +}
>> +
>> +void __weak pcibios_rescan_done(struct pci_bus *bus)
>> +{
>> +}
>> +
>>  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>>  		struct pci_ops *ops, void *sysdata, struct list_head *resources)
>>  {
>> @@ -3055,9 +3063,15 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
>>  unsigned int pci_rescan_bus(struct pci_bus *bus)
>>  {
>>  	unsigned int max;
>> +	struct pci_bus *root = bus;
>> +
>> +	while (!pci_is_root_bus(root))
>> +		root = root->parent;
>>  
>> +	pcibios_rescan_prepare(root);
>>  	max = pci_scan_child_bus(bus);
>>  	pci_assign_unassigned_bus_resources(bus);
>> +	pcibios_rescan_done(root);
>>  	pci_bus_add_devices(bus);
>>  
>>  	return max;
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 340029b2fb38..42930731c5a7 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1929,6 +1929,8 @@ void pcibios_penalize_isa_irq(int irq, int active);
>>  int pcibios_alloc_irq(struct pci_dev *dev);
>>  void pcibios_free_irq(struct pci_dev *dev);
>>  resource_size_t pcibios_default_alignment(void);
>> +void pcibios_rescan_prepare(struct pci_bus *bus);
>> +void pcibios_rescan_done(struct pci_bus *bus);
>>  
>>  #ifdef CONFIG_HIBERNATE_CALLBACKS
>>  extern struct dev_pm_ops pcibios_pm_ops;
>> -- 
>> 2.17.1
>>

Best regards,
Serge


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

Re: [PATCH v2 5/5] PCI/powerpc/eeh: Add pcibios hooks for preparing to rescan

[Benjamin Herrenschmidt]

On Mon, 2018-09-10 at 19:00 +0300, Sergey Miroshnichenko wrote:
> 
> Yes, missing a real EEH event is possible, unfortunately, and it is
> indeed worth mentioning.
> 
> To reduce this probability the next patchset I'll post in a few days
> among other things puts all the affected device drivers to pause during
> rescan, mainly because of moving BARs and bridge windows, but it will
> also help here a bit.

How do you deal with moving BARs etc... within the segmenting
restrictions of EEH ?

It's a horrible mess right now and I don't know if the current code can
even work properly to be honest.

Cheers,
Ben.

Re: [PATCH v2 5/5] PCI/powerpc/eeh: Add pcibios hooks for preparing to rescan

[Sergey Miroshnichenko]

On 9/12/18 1:39 PM,  wrote:
> On Mon, 2018-09-10 at 19:00 +0300, Sergey Miroshnichenko wrote:
>>
>> Yes, missing a real EEH event is possible, unfortunately, and it is
>> indeed worth mentioning.
>>
>> To reduce this probability the next patchset I'll post in a few days
>> among other things puts all the affected device drivers to pause during
>> rescan, mainly because of moving BARs and bridge windows, but it will
>> also help here a bit.
> 
> How do you deal with moving BARs etc... within the segmenting
> restrictions of EEH ?
> 

Actually, [un]fortunately, we haven't encountered any segmenting issues
yet, but to move BARs we are using the same existing mechanism in Linux
kernel that re-enumerated the PCIe topology during startup with
"pci=realloc" and PCI_REASSIGN_ALL_BUS. What restrictions must be broken
to provoke a segmenting event?

Are there any other limitations on segmenting besides keeping all the
BARs of the PHB within its huge M32+M64 segments which are 2GiB+4GiB on
our setup?

> It's a horrible mess right now and I don't know if the current code can
> even work properly to be honest.
> 
> Cheers,
> Ben.
> 
> 

Best regards,
Serge