[PATCH V2 0/9] Add RCEC handling to PCI/AER

[PATCH V2 0/9] Add RCEC handling to PCI/AER

[Sean V Kelley]

From: Sean V Kelley <sean.v.kelley@linux.intel.com>

On the use of FLR on RCiEPs for the fatal case, still interested in more
feedback from the earlier discussion here [1]:

[1] https://lore.kernel.org/linux-pci/C21C050B-48B1-4429-B019-C81F3AB8E843@intel.com/

There is also the question of the absence of an FLR for non-fatal error.
If the device driver tells us that it needs "PCI_ERS_RESULT_NEED_RESET" by
the callback report_normal_detected() then we should try FLR on the device
as well.

On the use of variables with RP centric names such as the attributes
dev_attr_aer_rootport_total_err_..., one concern is the ripple effect on code
churn due to renaming. Open to suggestions, but trying to co-habitate so to
speak RCECs with RPs in the same drivers has trade-offs.

Changes since v1 [2]:

- Make PME capability of RCEC discoverable in get_port_device_capability().
- Replace the check on bnr with <= lastbusn in pcie_walk_rcec().
- Fix comment header for pcie_walk_rcec().
- Fix comment header for pci_walk_dev_affected().
- Fix spurious newline.
- Add sanity checks on dev->rcec.
- Use pci_dbg() in place of pci_info() for discovered RCiEPs.
- Remove AER RCEC AP FOUND message (accidently left in previously).
- Remove the check for RC_END from set_device_error_reporting() since
only Ports and RCECs are being passed.
(Jonathan Cameron)
- Fix the return type for flr_on_rciep().
(reported by lkp on DEC Alpha arch.)

[1] https://lore.kernel.org/linux-pci/20200724172223.145608-1-sean.v.kelley@intel.com/

Root Complex Event Collectors (RCEC) provide support for terminating error
and PME messages from Root Complex Integrated Endpoints (RCiEPs).  An RCEC
resides on a Bus in the Root Complex. Multiple RCECs can in fact reside on
a single bus. An RCEC will explicitly declare supported RCiEPs through the
Root Complex Endpoint Association Extended Capability.

(See PCIe 5.0-1, sections 1.3.2.3 (RCiEP), and 7.9.10 (RCEC Ext. Cap.))

The kernel lacks handling for these RCECs and the error messages received
from their respective associated RCiEPs. More recently, a new CPU
interconnect, Compute eXpress Link (CXL) depends on RCEC capabilities for
purposes of error messaging from CXL 1.1 supported RCiEP devices.

DocLink: https://www.computeexpresslink.org/

This use case is not limited to CXL. Existing hardware today includes
support for RCECs, such as the Denverton microserver product
family. Future hardware will be forthcoming.

(See Intel Document, Order number: 33061-003US)

So services such as AER or PME could be associated with an RCEC driver.
In the case of CXL, if an RCiEP (i.e., CXL 1.1 device) is associated with a
platform's RCEC it shall signal PME and AER error conditions through that
RCEC.

Towards the above use cases, add the missing RCEC class and extend the
PCIe Root Port and service drivers to allow association of RCiEPs to their
respective parent RCEC and facilitate handling of terminating error and PME
messages.


AER Test Results:
1) Inject a correctable error to the RCiEP 0000:e9:00.0
    Run ./aer_inject <a parameter file as below>:
    AER
    PCI_ID 0000:e9:00.0
    COR_STATUS BAD_TLP
    HEADER_LOG 0 1 2 3

    Log:
[   76.155963] pcieport 0000:e8:00.4: aer_inject: Injecting errors 00000040/00000000 into device 0000:e9:00.0
[   76.166966] pcieport 0000:e8:00.4: AER: Corrected error received: 0000:e9:00.0
[   76.175253] pci 0000:e9:00.0: PCIe Bus Error: severity=Corrected, type=Data Link Layer, (Receiver ID)
[   76.185633] pci 0000:e9:00.0:   device [8086:4940] error status/mask=00000040/00002000
[   76.194604] pci 0000:e9:00.0:    [ 6] BadTLP

2) Inject a non-fatal error to the RCiEP 0000:e8:01.0
    Run ./aer_inject <a parameter file as below>:
    AER
    PCI_ID 0000:e8:01.0
    UNCOR_STATUS COMP_ABORT
    HEADER_LOG 0 1 2 3

    Log:
[  117.791854] pcieport 0000:e8:00.4: aer_inject: Injecting errors 00000000/00008000 into device 0000:e8:01.0
[  117.804244] pcieport 0000:e8:00.4: AER: Uncorrected (Non-Fatal) error received: 0000:e8:01.0
[  117.814652] igen6_edac 0000:e8:01.0: PCIe Bus Error: severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Completer ID)
[  117.828511] igen6_edac 0000:e8:01.0:   device [8086:0b25] error status/mask=00008000/00100000
[  117.839189] igen6_edac 0000:e8:01.0:    [15] CmpltAbrt
[  117.847365] igen6_edac 0000:e8:01.0: AER:   TLP Header: 00000000 00000001 00000002 00000003
[  117.857775] igen6_edac 0000:e8:01.0: AER: device recovery successful

3) Inject a fatal error to the RCiEP 0000:ed:01.0
    Run ./aer_inject <a parameter file as below>:
    AER
    PCI_ID 0000:ed:01.0
    UNCOR_STATUS MALF_TLP
    HEADER_LOG 0 1 2 3

    Log:
[  131.511623] pcieport 0000:ed:00.4: aer_inject: Injecting errors 00000000/00040000 into device 0000:ed:01.0
[  131.523259] pcieport 0000:ed:00.4: AER: Uncorrected (Fatal) error received: 0000:ed:01.0
[  131.533842] igen6_edac 0000:ed:01.0: AER: PCIe Bus Error: severity=Uncorrected (Fatal), type=Inaccessible, (Unregistered Agent ID)
[  131.655618] igen6_edac 0000:ed:01.0: AER: device recovery successful

Jonathan Cameron (1):
  PCI/AER: Extend AER error handling to RCECs

Qiuxu Zhuo (6):
  pci_ids: Add class code and extended capability for RCEC
  PCI: Extend Root Port Driver to support RCEC
  PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC
  PCI/AER: Apply function level reset to RCiEP on fatal error
  PCI: Add 'rcec' field to pci_dev for associated RCiEPs
  PCI/AER: Add RCEC AER error injection support

Sean V Kelley (2):
  PCI/AER: Add RCEC AER handling
  PCI/PME: Add RCEC PME handling

 drivers/pci/pcie/aer.c          | 36 +++++++++----
 drivers/pci/pcie/aer_inject.c   |  5 +-
 drivers/pci/pcie/err.c          | 90 +++++++++++++++++++++++++++------
 drivers/pci/pcie/pme.c          | 15 ++++--
 drivers/pci/pcie/portdrv.h      |  2 +
 drivers/pci/pcie/portdrv_core.c | 90 +++++++++++++++++++++++++++++++--
 drivers/pci/pcie/portdrv_pci.c  | 20 +++++++-
 include/linux/pci.h             |  3 ++
 include/linux/pci_ids.h         |  1 +
 include/uapi/linux/pci_regs.h   |  7 +++
 10 files changed, 233 insertions(+), 36 deletions(-)

--
2.27.0

[PATCH V2 1/9] pci_ids: Add class code and extended capability for RCEC

[Sean V Kelley]

From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

A PCIe Root Complex Event Collector(RCEC) has the base class 0x08,
sub-class 0x07, and programming interface 0x00. Add the class code
0x0807 to identify RCEC devices and add the defines for the RCEC
Endpoint Association Extended Capability.

See PCI Express Base Specification, version 5.0-1, section "1.3.4
Root Complex Event Collector" and section "7.9.10 Root Complex
Event Collector Endpoint Association Extended Capability"

Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/linux/pci_ids.h       | 1 +
 include/uapi/linux/pci_regs.h | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 5c709a1450b1..bc6d1a4ca02d 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -81,6 +81,7 @@
 #define PCI_CLASS_SYSTEM_RTC		0x0803
 #define PCI_CLASS_SYSTEM_PCI_HOTPLUG	0x0804
 #define PCI_CLASS_SYSTEM_SDHCI		0x0805
+#define PCI_CLASS_SYSTEM_RCEC		0x0807
 #define PCI_CLASS_SYSTEM_OTHER		0x0880
 
 #define PCI_BASE_CLASS_INPUT		0x09
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index f9701410d3b5..f335f65f65d6 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -828,6 +828,13 @@
 #define  PCI_PWR_CAP_BUDGET(x)	((x) & 1)	/* Included in system budget */
 #define PCI_EXT_CAP_PWR_SIZEOF	16
 
+/* Root Complex Event Collector Endpoint Association  */
+#define PCI_RCEC_RCIEP_BITMAP	4	/* Associated Bitmap for RCiEPs */
+#define PCI_RCEC_BUSN		8	/* RCEC Associated Bus Numbers */
+#define  PCI_RCEC_BUSN_REG_VER	0x02	/* Least capability version that BUSN present */
+#define  PCI_RCEC_BUSN_NEXT(x)	(((x) >> 8) & 0xff)
+#define  PCI_RCEC_BUSN_LAST(x)	(((x) >> 16) & 0xff)
+
 /* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */
 #define PCI_VNDR_HEADER		4	/* Vendor-Specific Header */
 #define  PCI_VNDR_HEADER_ID(x)	((x) & 0xffff)
-- 
2.27.0

Re: [PATCH V2 1/9] pci_ids: Add class code and extended capability for RCEC

[Bjorn Helgaas]

On Tue, Aug 04, 2020 at 12:40:44PM -0700, Sean V Kelley wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> A PCIe Root Complex Event Collector(RCEC) has the base class 0x08,
> sub-class 0x07, and programming interface 0x00. Add the class code
> 0x0807 to identify RCEC devices and add the defines for the RCEC
> Endpoint Association Extended Capability.
> 
> See PCI Express Base Specification, version 5.0-1, section "1.3.4
> Root Complex Event Collector" and section "7.9.10 Root Complex
> Event Collector Endpoint Association Extended Capability"
> 
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

1) "git log --oneline include/linux/pci_ids.h".  Match it.  Mention
the most important words, like "RCEC", early in the subject.

2) https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n555

> ---
>  include/linux/pci_ids.h       | 1 +
>  include/uapi/linux/pci_regs.h | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 5c709a1450b1..bc6d1a4ca02d 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -81,6 +81,7 @@
>  #define PCI_CLASS_SYSTEM_RTC		0x0803
>  #define PCI_CLASS_SYSTEM_PCI_HOTPLUG	0x0804
>  #define PCI_CLASS_SYSTEM_SDHCI		0x0805
> +#define PCI_CLASS_SYSTEM_RCEC		0x0807
>  #define PCI_CLASS_SYSTEM_OTHER		0x0880
>  
>  #define PCI_BASE_CLASS_INPUT		0x09
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index f9701410d3b5..f335f65f65d6 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -828,6 +828,13 @@
>  #define  PCI_PWR_CAP_BUDGET(x)	((x) & 1)	/* Included in system budget */
>  #define PCI_EXT_CAP_PWR_SIZEOF	16
>  
> +/* Root Complex Event Collector Endpoint Association  */
> +#define PCI_RCEC_RCIEP_BITMAP	4	/* Associated Bitmap for RCiEPs */
> +#define PCI_RCEC_BUSN		8	/* RCEC Associated Bus Numbers */
> +#define  PCI_RCEC_BUSN_REG_VER	0x02	/* Least capability version that BUSN present */
> +#define  PCI_RCEC_BUSN_NEXT(x)	(((x) >> 8) & 0xff)
> +#define  PCI_RCEC_BUSN_LAST(x)	(((x) >> 16) & 0xff)
> +
>  /* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */
>  #define PCI_VNDR_HEADER		4	/* Vendor-Specific Header */
>  #define  PCI_VNDR_HEADER_ID(x)	((x) & 0xffff)
> -- 
> 2.27.0
> 

[PATCH V2 2/9] PCI: Extend Root Port Driver to support RCEC

[Sean V Kelley]

From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors may
optionally be sent to a corresponding Root Complex Event Collector (RCEC).
Each RCiEP must be associated with no more than one RCEC. Interface errors
are reported to the OS by RCECs.

For an RCEC (technically not a Bridge), error messages "received" from
associated RCiEPs must be enabled for "transmission" in order to cause a
System Error via the Root Control register or (when the Advanced Error
Reporting Capability is present) reporting via the Root Error Command
register and logging in the Root Error Status register and Error Source
Identification register.

Given the commonality with Root Ports and the need to also support AER
and PME services for RCECs, extend the Root Port driver to support RCEC
devices through the addition of the RCEC Class ID to the driver
structure.

Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
 drivers/pci/pcie/portdrv_core.c | 8 ++++----
 drivers/pci/pcie/portdrv_pci.c  | 5 ++++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 50a9522ab07d..5d4a400094fc 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -234,11 +234,11 @@ static int get_port_device_capability(struct pci_dev *dev)
 #endif
 
 	/*
-	 * Root ports are capable of generating PME too.  Root Complex
-	 * Event Collectors can also generate PMEs, but we don't handle
-	 * those yet.
+	 * Root ports and Root Complex Event Collectors are capable
+	 * of generating PME too.
 	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
+	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
 	    (pcie_ports_native || host->native_pme)) {
 		services |= PCIE_PORT_SERVICE_PME;
 
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 3a3ce40ae1ab..4d880679b9b1 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	if (!pci_is_pcie(dev) ||
 	    ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
 	     (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
-	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
+	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
+	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
 		return -ENODEV;
 
 	status = pcie_port_device_register(dev);
@@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[] = {
 	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
 	/* subtractive decode PCI-to-PCI bridge, class type is 060401h */
 	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
+	/* handle any Root Complex Event Collector */
+	{ PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
 	{ },
 };
 
-- 
2.27.0

Re: [PATCH V2 2/9] PCI: Extend Root Port Driver to support RCEC

[Bjorn Helgaas]

"git log --oneline" again.

On Tue, Aug 04, 2020 at 12:40:45PM -0700, Sean V Kelley wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors may
> optionally be sent to a corresponding Root Complex Event Collector (RCEC).
> Each RCiEP must be associated with no more than one RCEC. Interface errors
> are reported to the OS by RCECs.
> 
> For an RCEC (technically not a Bridge), error messages "received" from
> associated RCiEPs must be enabled for "transmission" in order to cause a
> System Error via the Root Control register or (when the Advanced Error
> Reporting Capability is present) reporting via the Root Error Command
> register and logging in the Root Error Status register and Error Source
> Identification register.
> 
> Given the commonality with Root Ports and the need to also support AER
> and PME services for RCECs, extend the Root Port driver to support RCEC
> devices through the addition of the RCEC Class ID to the driver
> structure.
> 
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> ---
>  drivers/pci/pcie/portdrv_core.c | 8 ++++----
>  drivers/pci/pcie/portdrv_pci.c  | 5 ++++-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 50a9522ab07d..5d4a400094fc 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -234,11 +234,11 @@ static int get_port_device_capability(struct pci_dev *dev)
>  #endif
>  
>  	/*
> -	 * Root ports are capable of generating PME too.  Root Complex
> -	 * Event Collectors can also generate PMEs, but we don't handle
> -	 * those yet.
> +	 * Root ports and Root Complex Event Collectors are capable
> +	 * of generating PME too.
>  	 */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
>  	    (pcie_ports_native || host->native_pme)) {
>  		services |= PCIE_PORT_SERVICE_PME;
>  
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 3a3ce40ae1ab..4d880679b9b1 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	if (!pci_is_pcie(dev) ||
>  	    ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
>  	     (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
> -	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>  		return -ENODEV;
>  
>  	status = pcie_port_device_register(dev);
> @@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[] = {
>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
>  	/* subtractive decode PCI-to-PCI bridge, class type is 060401h */
>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
> +	/* handle any Root Complex Event Collector */
> +	{ PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
>  	{ },
>  };
>  
> -- 
> 2.27.0
> 

Re: [PATCH V2 2/9] PCI: Extend Root Port Driver to support RCEC

[Sean V Kelley]

On 5 Aug 2020, at 10:43, Bjorn Helgaas wrote:

> "git log --oneline" again.

Rewording lost track of the first line.  Argh, will fix.
>
> On Tue, Aug 04, 2020 at 12:40:45PM -0700, Sean V Kelley wrote:
>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>
>> If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors 
>> may
>> optionally be sent to a corresponding Root Complex Event Collector 
>> (RCEC).
>> Each RCiEP must be associated with no more than one RCEC. Interface 
>> errors
>> are reported to the OS by RCECs.
>>
>> For an RCEC (technically not a Bridge), error messages "received" 
>> from
>> associated RCiEPs must be enabled for "transmission" in order to 
>> cause a
>> System Error via the Root Control register or (when the Advanced 
>> Error
>> Reporting Capability is present) reporting via the Root Error Command
>> register and logging in the Root Error Status register and Error 
>> Source
>> Identification register.
>>
>> Given the commonality with Root Ports and the need to also support 
>> AER
>> and PME services for RCECs, extend the Root Port driver to support 
>> RCEC
>> devices through the addition of the RCEC Class ID to the driver
>> structure.
>>
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> ---
>>  drivers/pci/pcie/portdrv_core.c | 8 ++++----
>>  drivers/pci/pcie/portdrv_pci.c  | 5 ++++-
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/portdrv_core.c 
>> b/drivers/pci/pcie/portdrv_core.c
>> index 50a9522ab07d..5d4a400094fc 100644
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -234,11 +234,11 @@ static int get_port_device_capability(struct 
>> pci_dev *dev)
>>  #endif
>>
>>  	/*
>> -	 * Root ports are capable of generating PME too.  Root Complex
>> -	 * Event Collectors can also generate PMEs, but we don't handle
>> -	 * those yet.
>> +	 * Root ports and Root Complex Event Collectors are capable
>> +	 * of generating PME too.
>>  	 */
>> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
>> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
>>  	    (pcie_ports_native || host->native_pme)) {
>>  		services |= PCIE_PORT_SERVICE_PME;
>>
>> diff --git a/drivers/pci/pcie/portdrv_pci.c 
>> b/drivers/pci/pcie/portdrv_pci.c
>> index 3a3ce40ae1ab..4d880679b9b1 100644
>> --- a/drivers/pci/pcie/portdrv_pci.c
>> +++ b/drivers/pci/pcie/portdrv_pci.c
>> @@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev 
>> *dev,
>>  	if (!pci_is_pcie(dev) ||
>>  	    ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
>>  	     (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
>> -	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
>> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
>> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>>  		return -ENODEV;
>>
>>  	status = pcie_port_device_register(dev);
>> @@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[] 
>> = {
>>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
>>  	/* subtractive decode PCI-to-PCI bridge, class type is 060401h */
>>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
>> +	/* handle any Root Complex Event Collector */
>> +	{ PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
>>  	{ },
>>  };
>>
>> -- 
>> 2.27.0
>>

Re: [PATCH V2 2/9] PCI: Extend Root Port Driver to support RCEC

[Jonathan Cameron]

On Tue, 4 Aug 2020 12:40:45 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:

> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors may
> optionally be sent to a corresponding Root Complex Event Collector (RCEC).
> Each RCiEP must be associated with no more than one RCEC. Interface errors
> are reported to the OS by RCECs.
> 
> For an RCEC (technically not a Bridge), error messages "received" from
> associated RCiEPs must be enabled for "transmission" in order to cause a
> System Error via the Root Control register or (when the Advanced Error
> Reporting Capability is present) reporting via the Root Error Command
> register and logging in the Root Error Status register and Error Source
> Identification register.
> 
> Given the commonality with Root Ports and the need to also support AER
> and PME services for RCECs, extend the Root Port driver to support RCEC
> devices through the addition of the RCEC Class ID to the driver
> structure.
> 
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
One trivial thing inline.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  drivers/pci/pcie/portdrv_core.c | 8 ++++----
>  drivers/pci/pcie/portdrv_pci.c  | 5 ++++-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 50a9522ab07d..5d4a400094fc 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -234,11 +234,11 @@ static int get_port_device_capability(struct pci_dev *dev)
>  #endif
>  
>  	/*
> -	 * Root ports are capable of generating PME too.  Root Complex
> -	 * Event Collectors can also generate PMEs, but we don't handle
> -	 * those yet.
> +	 * Root ports and Root Complex Event Collectors are capable
> +	 * of generating PME too.

I'm not sure what the 'too' refers to.   I'd just drop it and avoid
any possible confusion!

>  	 */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
>  	    (pcie_ports_native || host->native_pme)) {
>  		services |= PCIE_PORT_SERVICE_PME;
>  
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 3a3ce40ae1ab..4d880679b9b1 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	if (!pci_is_pcie(dev) ||
>  	    ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
>  	     (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
> -	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>  		return -ENODEV;
>  
>  	status = pcie_port_device_register(dev);
> @@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[] = {
>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
>  	/* subtractive decode PCI-to-PCI bridge, class type is 060401h */
>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
> +	/* handle any Root Complex Event Collector */
> +	{ PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
>  	{ },
>  };
>  

Re: [PATCH V2 2/9] PCI: Extend Root Port Driver to support RCEC

[Sean V Kelley]

On 5 Aug 2020, at 10:45, Jonathan Cameron wrote:

> On Tue, 4 Aug 2020 12:40:45 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>
>> If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors 
>> may
>> optionally be sent to a corresponding Root Complex Event Collector 
>> (RCEC).
>> Each RCiEP must be associated with no more than one RCEC. Interface 
>> errors
>> are reported to the OS by RCECs.
>>
>> For an RCEC (technically not a Bridge), error messages "received" 
>> from
>> associated RCiEPs must be enabled for "transmission" in order to 
>> cause a
>> System Error via the Root Control register or (when the Advanced 
>> Error
>> Reporting Capability is present) reporting via the Root Error Command
>> register and logging in the Root Error Status register and Error 
>> Source
>> Identification register.
>>
>> Given the commonality with Root Ports and the need to also support 
>> AER
>> and PME services for RCECs, extend the Root Port driver to support 
>> RCEC
>> devices through the addition of the RCEC Class ID to the driver
>> structure.
>>
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> One trivial thing inline.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>
>> ---
>>  drivers/pci/pcie/portdrv_core.c | 8 ++++----
>>  drivers/pci/pcie/portdrv_pci.c  | 5 ++++-
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/portdrv_core.c 
>> b/drivers/pci/pcie/portdrv_core.c
>> index 50a9522ab07d..5d4a400094fc 100644
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -234,11 +234,11 @@ static int get_port_device_capability(struct 
>> pci_dev *dev)
>>  #endif
>>
>>  	/*
>> -	 * Root ports are capable of generating PME too.  Root Complex
>> -	 * Event Collectors can also generate PMEs, but we don't handle
>> -	 * those yet.
>> +	 * Root ports and Root Complex Event Collectors are capable
>> +	 * of generating PME too.
>
> I'm not sure what the 'too' refers to.   I'd just drop it and avoid
> any possible confusion!

Paste error of text.  Will correct.

Thanks,

Sean

>
>>  	 */
>> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
>> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
>>  	    (pcie_ports_native || host->native_pme)) {
>>  		services |= PCIE_PORT_SERVICE_PME;
>>
>> diff --git a/drivers/pci/pcie/portdrv_pci.c 
>> b/drivers/pci/pcie/portdrv_pci.c
>> index 3a3ce40ae1ab..4d880679b9b1 100644
>> --- a/drivers/pci/pcie/portdrv_pci.c
>> +++ b/drivers/pci/pcie/portdrv_pci.c
>> @@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev 
>> *dev,
>>  	if (!pci_is_pcie(dev) ||
>>  	    ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
>>  	     (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
>> -	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
>> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
>> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>>  		return -ENODEV;
>>
>>  	status = pcie_port_device_register(dev);
>> @@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[] 
>> = {
>>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
>>  	/* subtractive decode PCI-to-PCI bridge, class type is 060401h */
>>  	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
>> +	/* handle any Root Complex Event Collector */
>> +	{ PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
>>  	{ },
>>  };
>>

Re: [PATCH V2 2/9] PCI: Extend Root Port Driver to support RCEC

[Kuppuswamy, Sathyanarayanan]

On 8/4/20 12:40 PM, Sean V Kelley wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> If a Root Complex Integrated Endpoint (RCiEP) is implemented, errors may
> optionally be sent to a corresponding Root Complex Event Collector (RCEC).
> Each RCiEP must be associated with no more than one RCEC. Interface errors
> are reported to the OS by RCECs.
> 
> For an RCEC (technically not a Bridge), error messages "received" from
> associated RCiEPs must be enabled for "transmission" in order to cause a
> System Error via the Root Control register or (when the Advanced Error
> Reporting Capability is present) reporting via the Root Error Command
> register and logging in the Root Error Status register and Error Source
> Identification register.
> 
> Given the commonality with Root Ports and the need to also support AER
> and PME services for RCECs, extend the Root Port driver to support RCEC
> devices through the addition of the RCEC Class ID to the driver
> structure.
> 
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> ---
>   drivers/pci/pcie/portdrv_core.c | 8 ++++----
>   drivers/pci/pcie/portdrv_pci.c  | 5 ++++-
>   2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 50a9522ab07d..5d4a400094fc 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -234,11 +234,11 @@ static int get_port_device_capability(struct pci_dev *dev)
>   #endif
>   
>   	/*
> -	 * Root ports are capable of generating PME too.  Root Complex
> -	 * Event Collectors can also generate PMEs, but we don't handle
> -	 * those yet.
> +	 * Root ports and Root Complex Event Collectors are capable
> +	 * of generating PME too.
>   	 */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
>   	    (pcie_ports_native || host->native_pme)) {
>   		services |= PCIE_PORT_SERVICE_PME;
What about AER service? Don't you need to enable it for RCEC?
>   
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 3a3ce40ae1ab..4d880679b9b1 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>   	if (!pci_is_pcie(dev) ||
>   	    ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
>   	     (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
> -	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
> +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>   		return -ENODEV;
>   
>   	status = pcie_port_device_register(dev);
> @@ -195,6 +196,8 @@ static const struct pci_device_id port_pci_ids[] = {
>   	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
>   	/* subtractive decode PCI-to-PCI bridge, class type is 060401h */
>   	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
> +	/* handle any Root Complex Event Collector */
> +	{ PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0) },
>   	{ },
>   };
>   
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 2/9] PCI: Extend Root Port Driver to support RCEC

[sean.v.kelley]

Hi Sathya,

Thanks for reviewing.

If you haven't see it already there are newer patches under v3 here:

https://lore.kernel.org/linux-pci/20200812164659.1118946-1-sean.v.kelley@intel.com/

Comments below:

On Wed, 2020-08-26 at 09:16 -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> On 8/4/20 12:40 PM, Sean V Kelley wrote:
> > From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > 
> > If a Root Complex Integrated Endpoint (RCiEP) is implemented,
> > errors may
> > optionally be sent to a corresponding Root Complex Event Collector
> > (RCEC).
> > Each RCiEP must be associated with no more than one RCEC. Interface
> > errors
> > are reported to the OS by RCECs.
> > 
> > For an RCEC (technically not a Bridge), error messages "received"
> > from
> > associated RCiEPs must be enabled for "transmission" in order to
> > cause a
> > System Error via the Root Control register or (when the Advanced
> > Error
> > Reporting Capability is present) reporting via the Root Error
> > Command
> > register and logging in the Root Error Status register and Error
> > Source
> > Identification register.
> > 
> > Given the commonality with Root Ports and the need to also support
> > AER
> > and PME services for RCECs, extend the Root Port driver to support
> > RCEC
> > devices through the addition of the RCEC Class ID to the driver
> > structure.
> > 
> > Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> > ---
> >   drivers/pci/pcie/portdrv_core.c | 8 ++++----
> >   drivers/pci/pcie/portdrv_pci.c  | 5 ++++-
> >   2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/portdrv_core.c
> > b/drivers/pci/pcie/portdrv_core.c
> > index 50a9522ab07d..5d4a400094fc 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -234,11 +234,11 @@ static int get_port_device_capability(struct
> > pci_dev *dev)
> >   #endif
> >   
> >   	/*
> > -	 * Root ports are capable of generating PME too.  Root Complex
> > -	 * Event Collectors can also generate PMEs, but we don't handle
> > -	 * those yet.
> > +	 * Root ports and Root Complex Event Collectors are capable
> > +	 * of generating PME too.
> >   	 */
> > -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> > +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > +	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
> >   	    (pcie_ports_native || host->native_pme)) {
> >   		services |= PCIE_PORT_SERVICE_PME;
> What about AER service? Don't you need to enable it for RCEC

It is enabled via set_device_error_reporting() in aer.c and in not seen
in this patch but in the lines above the code section you are
commenting on is:

#ifdef CONFIG_PCIEAER
        if (dev->aer_cap && pci_aer_available() &&
            (pcie_ports_native || host->native_aer)) {
                services |= PCIE_PORT_SERVICE_AER;

                /*
                 * Disable AER on this port in case it's been enabled
by the
                 * BIOS (the AER service driver will enable it when
necessary).
                 */
                pci_disable_pcie_error_reporting(dev);
        }
#endif

Let me know if I'm missing something here.


Thanks!

Sean


> >   
> > diff --git a/drivers/pci/pcie/portdrv_pci.c
> > b/drivers/pci/pcie/portdrv_pci.c
> > index 3a3ce40ae1ab..4d880679b9b1 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -106,7 +106,8 @@ static int pcie_portdrv_probe(struct pci_dev
> > *dev,
> >   	if (!pci_is_pcie(dev) ||
> >   	    ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
> >   	     (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
> > -	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
> > +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM) &&
> > +	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
> >   		return -ENODEV;
> >   
> >   	status = pcie_port_device_register(dev);
> > @@ -195,6 +196,8 @@ static const struct pci_device_id
> > port_pci_ids[] = {
> >   	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x00), ~0) },
> >   	/* subtractive decode PCI-to-PCI bridge, class type is 060401h
> > */
> >   	{ PCI_DEVICE_CLASS(((PCI_CLASS_BRIDGE_PCI << 8) | 0x01), ~0) },
> > +	/* handle any Root Complex Event Collector */
> > +	{ PCI_DEVICE_CLASS(((PCI_CLASS_SYSTEM_RCEC << 8) | 0x00), ~0)
> > },
> >   	{ },
> >   };
> >   
> > 

[PATCH V2 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC

[Sean V Kelley]

From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

When an RCEC device signals error(s) to a CPU core, the CPU core
needs to walk all the RCiEPs associated with that RCEC to check
errors. So add the function pcie_walk_rcec() to walk all RCiEPs
associated with the RCEC device.

Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/pcie/portdrv.h      |  2 +
 drivers/pci/pcie/portdrv_core.c | 82 +++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index af7cf237432a..c11d5ecbad76 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -116,6 +116,8 @@ void pcie_port_service_unregister(struct pcie_port_service_driver *new);
 
 extern struct bus_type pcie_port_bus_type;
 int pcie_port_device_register(struct pci_dev *dev);
+void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
+		    void *userdata);
 #ifdef CONFIG_PM
 int pcie_port_device_suspend(struct device *dev);
 int pcie_port_device_resume_noirq(struct device *dev);
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 5d4a400094fc..daa2dfa83a0b 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -14,6 +14,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/string.h>
 #include <linux/slab.h>
+#include <linux/bitops.h>
 #include <linux/aer.h>
 
 #include "../pci.h"
@@ -365,6 +366,87 @@ int pcie_port_device_register(struct pci_dev *dev)
 	return status;
 }
 
+static int pcie_walk_rciep_devfn(struct pci_bus *pbus, int (*cb)(struct pci_dev *, void *),
+				 void *userdata, unsigned long bitmap)
+{
+	unsigned int dev, fn;
+	struct pci_dev *pdev;
+	int retval;
+
+	for_each_set_bit(dev, &bitmap, 32) {
+		for (fn = 0; fn < 8; fn++) {
+			pdev = pci_get_slot(pbus, PCI_DEVFN(dev, fn));
+
+			if (!pdev || pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
+				continue;
+
+			retval = cb(pdev, userdata);
+			if (retval)
+				return retval;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * pcie_walk_rcec - Walk RCiEP devices associating with RCEC and call callback.
+ * @rcec     RCEC whose RCiEP devices should be walked.
+ * @cb       Callback to be called for each RCiEP device found.
+ * @userdata Arbitrary pointer to be passed to callback.
+ *
+ * Walk the given RCEC. Call the provided callback on each RCiEP device found.
+ *
+ * We check the return of @cb each time. If it returns anything
+ * other than 0, we break out.
+ */
+void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
+		    void *userdata)
+{
+	u32 pos, bitmap, hdr, busn;
+	u8 ver, nextbusn, lastbusn;
+	struct pci_bus *pbus;
+	unsigned int bnr;
+
+	pos = pci_find_ext_capability(rcec, PCI_EXT_CAP_ID_RCEC);
+	if (!pos)
+		return;
+
+	pbus = pci_find_bus(pci_domain_nr(rcec->bus), rcec->bus->number);
+	if (!pbus)
+		return;
+
+	pci_read_config_dword(rcec, pos + PCI_RCEC_RCIEP_BITMAP, &bitmap);
+
+	/* Find RCiEP devices on the same bus as the RCEC */
+	if (pcie_walk_rciep_devfn(pbus, cb, userdata, (unsigned long)bitmap))
+		return;
+
+	/* Check whether RCEC BUSN register is present */
+	pci_read_config_dword(rcec, pos, &hdr);
+	ver = PCI_EXT_CAP_VER(hdr);
+	if (ver < PCI_RCEC_BUSN_REG_VER)
+		return;
+
+	pci_read_config_dword(rcec, pos + PCI_RCEC_BUSN, &busn);
+	nextbusn = PCI_RCEC_BUSN_NEXT(busn);
+	lastbusn = PCI_RCEC_BUSN_LAST(busn);
+
+	/* All RCiEP devices are on the same bus as the RCEC */
+	if (nextbusn == 0xff && lastbusn == 0x00)
+		return;
+
+	for (bnr = nextbusn; bnr <= lastbusn; bnr++) {
+		pbus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
+		if (!pbus)
+			continue;
+
+		/* Find RCiEP devices on the given bus */
+		if (pcie_walk_rciep_devfn(pbus, cb, userdata, 0xffffffff))
+			return;
+	}
+}
+
 #ifdef CONFIG_PM
 typedef int (*pcie_pm_callback_t)(struct pcie_device *);
 
-- 
2.27.0

Re: [PATCH V2 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC

[Bjorn Helgaas]

On Tue, Aug 04, 2020 at 12:40:46PM -0700, Sean V Kelley wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> When an RCEC device signals error(s) to a CPU core, the CPU core
> needs to walk all the RCiEPs associated with that RCEC to check
> errors. So add the function pcie_walk_rcec() to walk all RCiEPs
> associated with the RCEC device.
> 
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/pci/pcie/portdrv.h      |  2 +
>  drivers/pci/pcie/portdrv_core.c | 82 +++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
> index af7cf237432a..c11d5ecbad76 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -116,6 +116,8 @@ void pcie_port_service_unregister(struct pcie_port_service_driver *new);
>  
>  extern struct bus_type pcie_port_bus_type;
>  int pcie_port_device_register(struct pci_dev *dev);
> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
> +		    void *userdata);
>  #ifdef CONFIG_PM
>  int pcie_port_device_suspend(struct device *dev);
>  int pcie_port_device_resume_noirq(struct device *dev);
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 5d4a400094fc..daa2dfa83a0b 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -14,6 +14,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/string.h>
>  #include <linux/slab.h>
> +#include <linux/bitops.h>
>  #include <linux/aer.h>
>  
>  #include "../pci.h"
> @@ -365,6 +366,87 @@ int pcie_port_device_register(struct pci_dev *dev)
>  	return status;
>  }
>  
> +static int pcie_walk_rciep_devfn(struct pci_bus *pbus, int (*cb)(struct pci_dev *, void *),

In drivers/pci, the typical name for a "struct pci_bus *" is "bus",
and for a "struct pci_dev *" is "dev" (below).

> +				 void *userdata, unsigned long bitmap)

Use "u32 bitmap" so the width is explicit.  Looks like this would also
get rid of the cast in the caller.  So maybe you used "unsigned long"
here for some reason?

> +{
> +	unsigned int dev, fn;
> +	struct pci_dev *pdev;
> +	int retval;
> +
> +	for_each_set_bit(dev, &bitmap, 32) {
> +		for (fn = 0; fn < 8; fn++) {
> +			pdev = pci_get_slot(pbus, PCI_DEVFN(dev, fn));

This needs a matching pci_dev_put(), according to the pci_get_slot()
function comment.

> +
> +			if (!pdev || pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
> +				continue;
> +
> +			retval = cb(pdev, userdata);
> +			if (retval)
> +				return retval;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * pcie_walk_rcec - Walk RCiEP devices associating with RCEC and call callback.
> + * @rcec     RCEC whose RCiEP devices should be walked.
> + * @cb       Callback to be called for each RCiEP device found.
> + * @userdata Arbitrary pointer to be passed to callback.
> + *
> + * Walk the given RCEC. Call the provided callback on each RCiEP device found.
> + *
> + * We check the return of @cb each time. If it returns anything
> + * other than 0, we break out.
> + */
> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
> +		    void *userdata)
> +{
> +	u32 pos, bitmap, hdr, busn;
> +	u8 ver, nextbusn, lastbusn;
> +	struct pci_bus *pbus;
> +	unsigned int bnr;
> +
> +	pos = pci_find_ext_capability(rcec, PCI_EXT_CAP_ID_RCEC);
> +	if (!pos)
> +		return;

I think all the registers you care about here are read-only.  If so, we
should find the capability, read the registers (bitmap & bus numbers),
and save them at enumeration-time, e.g., in pci_init_capabilities().
I hope we don't have to dig all this out every time we process an
error.

> +	pbus = pci_find_bus(pci_domain_nr(rcec->bus), rcec->bus->number);
> +	if (!pbus)
> +		return;

This seems like a really complicated way to write:

  pbus = rcec->bus;

"rcec->bus" cannot be NULL, so there's no need to check.

> +	pci_read_config_dword(rcec, pos + PCI_RCEC_RCIEP_BITMAP, &bitmap);
> +
> +	/* Find RCiEP devices on the same bus as the RCEC */
> +	if (pcie_walk_rciep_devfn(pbus, cb, userdata, (unsigned long)bitmap))
> +		return;
> +
> +	/* Check whether RCEC BUSN register is present */
> +	pci_read_config_dword(rcec, pos, &hdr);
> +	ver = PCI_EXT_CAP_VER(hdr);
> +	if (ver < PCI_RCEC_BUSN_REG_VER)
> +		return;
> +
> +	pci_read_config_dword(rcec, pos + PCI_RCEC_BUSN, &busn);
> +	nextbusn = PCI_RCEC_BUSN_NEXT(busn);
> +	lastbusn = PCI_RCEC_BUSN_LAST(busn);
> +
> +	/* All RCiEP devices are on the same bus as the RCEC */
> +	if (nextbusn == 0xff && lastbusn == 0x00)
> +		return;
> +
> +	for (bnr = nextbusn; bnr <= lastbusn; bnr++) {
> +		pbus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
> +		if (!pbus)
> +			continue;
> +
> +		/* Find RCiEP devices on the given bus */
> +		if (pcie_walk_rciep_devfn(pbus, cb, userdata, 0xffffffff))
> +			return;
> +	}
> +}
> +
>  #ifdef CONFIG_PM
>  typedef int (*pcie_pm_callback_t)(struct pcie_device *);
>  
> -- 
> 2.27.0
> 

Re: [PATCH V2 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC

[Sean V Kelley]

On 5 Aug 2020, at 10:43, Bjorn Helgaas wrote:

> On Tue, Aug 04, 2020 at 12:40:46PM -0700, Sean V Kelley wrote:
>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>
>> When an RCEC device signals error(s) to a CPU core, the CPU core
>> needs to walk all the RCiEPs associated with that RCEC to check
>> errors. So add the function pcie_walk_rcec() to walk all RCiEPs
>> associated with the RCEC device.
>>
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> ---
>>  drivers/pci/pcie/portdrv.h      |  2 +
>>  drivers/pci/pcie/portdrv_core.c | 82 
>> +++++++++++++++++++++++++++++++++
>>  2 files changed, 84 insertions(+)
>>
>> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
>> index af7cf237432a..c11d5ecbad76 100644
>> --- a/drivers/pci/pcie/portdrv.h
>> +++ b/drivers/pci/pcie/portdrv.h
>> @@ -116,6 +116,8 @@ void pcie_port_service_unregister(struct 
>> pcie_port_service_driver *new);
>>
>>  extern struct bus_type pcie_port_bus_type;
>>  int pcie_port_device_register(struct pci_dev *dev);
>> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev 
>> *, void *),
>> +		    void *userdata);
>>  #ifdef CONFIG_PM
>>  int pcie_port_device_suspend(struct device *dev);
>>  int pcie_port_device_resume_noirq(struct device *dev);
>> diff --git a/drivers/pci/pcie/portdrv_core.c 
>> b/drivers/pci/pcie/portdrv_core.c
>> index 5d4a400094fc..daa2dfa83a0b 100644
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/pm_runtime.h>
>>  #include <linux/string.h>
>>  #include <linux/slab.h>
>> +#include <linux/bitops.h>
>>  #include <linux/aer.h>
>>
>>  #include "../pci.h"
>> @@ -365,6 +366,87 @@ int pcie_port_device_register(struct pci_dev 
>> *dev)
>>  	return status;
>>  }
>>
>> +static int pcie_walk_rciep_devfn(struct pci_bus *pbus, int 
>> (*cb)(struct pci_dev *, void *),
>
> In drivers/pci, the typical name for a "struct pci_bus *" is "bus",
> and for a "struct pci_dev *" is "dev" (below).
>
>> +				 void *userdata, unsigned long bitmap)

Will correct.

>
> Use "u32 bitmap" so the width is explicit.  Looks like this would also
> get rid of the cast in the caller.  So maybe you used "unsigned long"
> here for some reason?

Will correct. Somehow one cast led to another.  Should have been u32.

>
>> +{
>> +	unsigned int dev, fn;
>> +	struct pci_dev *pdev;
>> +	int retval;
>> +
>> +	for_each_set_bit(dev, &bitmap, 32) {
>> +		for (fn = 0; fn < 8; fn++) {
>> +			pdev = pci_get_slot(pbus, PCI_DEVFN(dev, fn));
>
> This needs a matching pci_dev_put(), according to the pci_get_slot()
> function comment.

Will add.

>
>> +
>> +			if (!pdev || pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
>> +				continue;
>> +
>> +			retval = cb(pdev, userdata);
>> +			if (retval)
>> +				return retval;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * pcie_walk_rcec - Walk RCiEP devices associating with RCEC and 
>> call callback.
>> + * @rcec     RCEC whose RCiEP devices should be walked.
>> + * @cb       Callback to be called for each RCiEP device found.
>> + * @userdata Arbitrary pointer to be passed to callback.
>> + *
>> + * Walk the given RCEC. Call the provided callback on each RCiEP 
>> device found.
>> + *
>> + * We check the return of @cb each time. If it returns anything
>> + * other than 0, we break out.
>> + */
>> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev 
>> *, void *),
>> +		    void *userdata)
>> +{
>> +	u32 pos, bitmap, hdr, busn;
>> +	u8 ver, nextbusn, lastbusn;
>> +	struct pci_bus *pbus;
>> +	unsigned int bnr;
>> +
>> +	pos = pci_find_ext_capability(rcec, PCI_EXT_CAP_ID_RCEC);
>> +	if (!pos)
>> +		return;
>
> I think all the registers you care about here are read-only.  If so, 
> we
> should find the capability, read the registers (bitmap & bus numbers),
> and save them at enumeration-time, e.g., in pci_init_capabilities().
> I hope we don't have to dig all this out every time we process an
> error.

I originally went that way by caching the rcec cap (pos) in pci_dev on 
probe. Will make changes to cache at enumeration time.

>
>> +	pbus = pci_find_bus(pci_domain_nr(rcec->bus), rcec->bus->number);
>> +	if (!pbus)
>> +		return;
>
> This seems like a really complicated way to write:
>
>   pbus = rcec->bus;
>
> "rcec->bus" cannot be NULL, so there's no need to check.

For some reason, at the time it appeared I needed to call pci_find_bus.  
But that makes sense if it is always valid. Will correct.

>
>> +	pci_read_config_dword(rcec, pos + PCI_RCEC_RCIEP_BITMAP, &bitmap);
>> +
>> +	/* Find RCiEP devices on the same bus as the RCEC */
>> +	if (pcie_walk_rciep_devfn(pbus, cb, userdata, (unsigned 
>> long)bitmap))
>> +		return;
>> +
>> +	/* Check whether RCEC BUSN register is present */
>> +	pci_read_config_dword(rcec, pos, &hdr);
>> +	ver = PCI_EXT_CAP_VER(hdr);
>> +	if (ver < PCI_RCEC_BUSN_REG_VER)
>> +		return;
>> +
>> +	pci_read_config_dword(rcec, pos + PCI_RCEC_BUSN, &busn);
>> +	nextbusn = PCI_RCEC_BUSN_NEXT(busn);
>> +	lastbusn = PCI_RCEC_BUSN_LAST(busn);
>> +
>> +	/* All RCiEP devices are on the same bus as the RCEC */
>> +	if (nextbusn == 0xff && lastbusn == 0x00)
>> +		return;
>> +
>> +	for (bnr = nextbusn; bnr <= lastbusn; bnr++) {
>> +		pbus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
>> +		if (!pbus)
>> +			continue;
>> +
>> +		/* Find RCiEP devices on the given bus */
>> +		if (pcie_walk_rciep_devfn(pbus, cb, userdata, 0xffffffff))
>> +			return;
>> +	}
>> +}
>> +
>>  #ifdef CONFIG_PM
>>  typedef int (*pcie_pm_callback_t)(struct pcie_device *);
>>
>> -- 
>> 2.27.0
>>

Re: [PATCH V2 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC

[Kuppuswamy, Sathyanarayanan]

On 8/4/20 12:40 PM, Sean V Kelley wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> When an RCEC device signals error(s) to a CPU core, the CPU core
> needs to walk all the RCiEPs associated with that RCEC to check
> errors. So add the function pcie_walk_rcec() to walk all RCiEPs
> associated with the RCEC device.
I think its better if you merge the usage patch and API
(pcie_walk_rcec) patch together.

Did you not get unused function warning with this patch?
> 
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   drivers/pci/pcie/portdrv.h      |  2 +
>   drivers/pci/pcie/portdrv_core.c | 82 +++++++++++++++++++++++++++++++++
>   2 files changed, 84 insertions(+)
> 
> diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
> index af7cf237432a..c11d5ecbad76 100644
> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -116,6 +116,8 @@ void pcie_port_service_unregister(struct pcie_port_service_driver *new);
>   
>   extern struct bus_type pcie_port_bus_type;
>   int pcie_port_device_register(struct pci_dev *dev);
> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
> +		    void *userdata);
>   #ifdef CONFIG_PM
>   int pcie_port_device_suspend(struct device *dev);
>   int pcie_port_device_resume_noirq(struct device *dev);
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 5d4a400094fc..daa2dfa83a0b 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -14,6 +14,7 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/string.h>
>   #include <linux/slab.h>
> +#include <linux/bitops.h>
>   #include <linux/aer.h>
>   
>   #include "../pci.h"
> @@ -365,6 +366,87 @@ int pcie_port_device_register(struct pci_dev *dev)
>   	return status;
>   }
>   
> +static int pcie_walk_rciep_devfn(struct pci_bus *pbus, int (*cb)(struct pci_dev *, void *),
> +				 void *userdata, unsigned long bitmap)
> +{
> +	unsigned int dev, fn;
> +	struct pci_dev *pdev;
> +	int retval;
> +
> +	for_each_set_bit(dev, &bitmap, 32) {
> +		for (fn = 0; fn < 8; fn++) {
> +			pdev = pci_get_slot(pbus, PCI_DEVFN(dev, fn));
> +
> +			if (!pdev || pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
> +				continue;
> +
> +			retval = cb(pdev, userdata);
> +			if (retval)
> +				return retval;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * pcie_walk_rcec - Walk RCiEP devices associating with RCEC and call callback.
> + * @rcec     RCEC whose RCiEP devices should be walked.
> + * @cb       Callback to be called for each RCiEP device found.
> + * @userdata Arbitrary pointer to be passed to callback.
> + *
> + * Walk the given RCEC. Call the provided callback on each RCiEP device found.
> + *
> + * We check the return of @cb each time. If it returns anything
> + * other than 0, we break out.
> + */
> +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
> +		    void *userdata)
> +{
> +	u32 pos, bitmap, hdr, busn;
> +	u8 ver, nextbusn, lastbusn;
> +	struct pci_bus *pbus;
> +	unsigned int bnr;
> +
> +	pos = pci_find_ext_capability(rcec, PCI_EXT_CAP_ID_RCEC);
> +	if (!pos)
> +		return;
> +
> +	pbus = pci_find_bus(pci_domain_nr(rcec->bus), rcec->bus->number);
> +	if (!pbus)
> +		return;
> +
> +	pci_read_config_dword(rcec, pos + PCI_RCEC_RCIEP_BITMAP, &bitmap);
> +
> +	/* Find RCiEP devices on the same bus as the RCEC */
> +	if (pcie_walk_rciep_devfn(pbus, cb, userdata, (unsigned long)bitmap))
> +		return;
> +
> +	/* Check whether RCEC BUSN register is present */
> +	pci_read_config_dword(rcec, pos, &hdr);
> +	ver = PCI_EXT_CAP_VER(hdr);
> +	if (ver < PCI_RCEC_BUSN_REG_VER)
> +		return;
> +
> +	pci_read_config_dword(rcec, pos + PCI_RCEC_BUSN, &busn);
> +	nextbusn = PCI_RCEC_BUSN_NEXT(busn);
> +	lastbusn = PCI_RCEC_BUSN_LAST(busn);
> +
> +	/* All RCiEP devices are on the same bus as the RCEC */
> +	if (nextbusn == 0xff && lastbusn == 0x00)
> +		return;
> +
> +	for (bnr = nextbusn; bnr <= lastbusn; bnr++) {
> +		pbus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
> +		if (!pbus)
> +			continue;
> +
> +		/* Find RCiEP devices on the given bus */
> +		if (pcie_walk_rciep_devfn(pbus, cb, userdata, 0xffffffff))
> +			return;
> +	}
> +}
> +
>   #ifdef CONFIG_PM
>   typedef int (*pcie_pm_callback_t)(struct pcie_device *);
>   
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH V2 3/9] PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC

[sean.v.kelley]

Hi Sathya,


On Wed, 2020-08-26 at 09:20 -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> On 8/4/20 12:40 PM, Sean V Kelley wrote:
> > From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > 
> > When an RCEC device signals error(s) to a CPU core, the CPU core
> > needs to walk all the RCiEPs associated with that RCEC to check
> > errors. So add the function pcie_walk_rcec() to walk all RCiEPs
> > associated with the RCEC device.
> I think its better if you merge the usage patch and API
> (pcie_walk_rcec) patch together.

Good suggestion, I'll have a look to see if that works well merged.

> 
> Did you not get unused function warning with this patch?

The latest patches are below (v3) and they were built on top of
pci/master (prior to the release of 5.9.0-rc1 btw).  I did not get a
warning with that but will double check...

https://lore.kernel.org/linux-pci/20200812164659.1118946-1-sean.v.kelley@intel.com/

Thanks!

Sean

Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >   drivers/pci/pcie/portdrv.h      |  2 +
> >   drivers/pci/pcie/portdrv_core.c | 82
> > +++++++++++++++++++++++++++++++++
> >   2 files changed, 84 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/portdrv.h
> > b/drivers/pci/pcie/portdrv.h
> > index af7cf237432a..c11d5ecbad76 100644
> > --- a/drivers/pci/pcie/portdrv.h
> > +++ b/drivers/pci/pcie/portdrv.h
> > @@ -116,6 +116,8 @@ void pcie_port_service_unregister(struct
> > pcie_port_service_driver *new);
> >   
> >   extern struct bus_type pcie_port_bus_type;
> >   int pcie_port_device_register(struct pci_dev *dev);
> > +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev
> > *, void *),
> > +		    void *userdata);
> >   #ifdef CONFIG_PM
> >   int pcie_port_device_suspend(struct device *dev);
> >   int pcie_port_device_resume_noirq(struct device *dev);
> > diff --git a/drivers/pci/pcie/portdrv_core.c
> > b/drivers/pci/pcie/portdrv_core.c
> > index 5d4a400094fc..daa2dfa83a0b 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -14,6 +14,7 @@
> >   #include <linux/pm_runtime.h>
> >   #include <linux/string.h>
> >   #include <linux/slab.h>
> > +#include <linux/bitops.h>
> >   #include <linux/aer.h>
> >   
> >   #include "../pci.h"
> > @@ -365,6 +366,87 @@ int pcie_port_device_register(struct pci_dev
> > *dev)
> >   	return status;
> >   }
> >   
> > +static int pcie_walk_rciep_devfn(struct pci_bus *pbus, int
> > (*cb)(struct pci_dev *, void *),
> > +				 void *userdata, unsigned long bitmap)
> > +{
> > +	unsigned int dev, fn;
> > +	struct pci_dev *pdev;
> > +	int retval;
> > +
> > +	for_each_set_bit(dev, &bitmap, 32) {
> > +		for (fn = 0; fn < 8; fn++) {
> > +			pdev = pci_get_slot(pbus, PCI_DEVFN(dev, fn));
> > +
> > +			if (!pdev || pci_pcie_type(pdev) !=
> > PCI_EXP_TYPE_RC_END)
> > +				continue;
> > +
> > +			retval = cb(pdev, userdata);
> > +			if (retval)
> > +				return retval;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * pcie_walk_rcec - Walk RCiEP devices associating with RCEC and
> > call callback.
> > + * @rcec     RCEC whose RCiEP devices should be walked.
> > + * @cb       Callback to be called for each RCiEP device found.
> > + * @userdata Arbitrary pointer to be passed to callback.
> > + *
> > + * Walk the given RCEC. Call the provided callback on each RCiEP
> > device found.
> > + *
> > + * We check the return of @cb each time. If it returns anything
> > + * other than 0, we break out.
> > + */
> > +void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev
> > *, void *),
> > +		    void *userdata)
> > +{
> > +	u32 pos, bitmap, hdr, busn;
> > +	u8 ver, nextbusn, lastbusn;
> > +	struct pci_bus *pbus;
> > +	unsigned int bnr;
> > +
> > +	pos = pci_find_ext_capability(rcec, PCI_EXT_CAP_ID_RCEC);
> > +	if (!pos)
> > +		return;
> > +
> > +	pbus = pci_find_bus(pci_domain_nr(rcec->bus), rcec->bus-
> > >number);
> > +	if (!pbus)
> > +		return;
> > +
> > +	pci_read_config_dword(rcec, pos + PCI_RCEC_RCIEP_BITMAP,
> > &bitmap);
> > +
> > +	/* Find RCiEP devices on the same bus as the RCEC */
> > +	if (pcie_walk_rciep_devfn(pbus, cb, userdata, (unsigned
> > long)bitmap))
> > +		return;
> > +
> > +	/* Check whether RCEC BUSN register is present */
> > +	pci_read_config_dword(rcec, pos, &hdr);
> > +	ver = PCI_EXT_CAP_VER(hdr);
> > +	if (ver < PCI_RCEC_BUSN_REG_VER)
> > +		return;
> > +
> > +	pci_read_config_dword(rcec, pos + PCI_RCEC_BUSN, &busn);
> > +	nextbusn = PCI_RCEC_BUSN_NEXT(busn);
> > +	lastbusn = PCI_RCEC_BUSN_LAST(busn);
> > +
> > +	/* All RCiEP devices are on the same bus as the RCEC */
> > +	if (nextbusn == 0xff && lastbusn == 0x00)
> > +		return;
> > +
> > +	for (bnr = nextbusn; bnr <= lastbusn; bnr++) {
> > +		pbus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
> > +		if (!pbus)
> > +			continue;
> > +
> > +		/* Find RCiEP devices on the given bus */
> > +		if (pcie_walk_rciep_devfn(pbus, cb, userdata,
> > 0xffffffff))
> > +			return;
> > +	}
> > +}
> > +
> >   #ifdef CONFIG_PM
> >   typedef int (*pcie_pm_callback_t)(struct pcie_device *);
> >   
> > 

[PATCH V2 4/9] PCI/AER: Extend AER error handling to RCECs

[Sean V Kelley]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Currently the kernel does not handle AER errors for Root Complex integrated
End Points (RCiEPs)[0]. These devices sit on a root bus within the Root Complex
(RC). AER handling is performed by a Root Complex Event Collector (RCEC) [1]
which is a effectively a type of RCiEP on the same root bus.

For an RCEC (technically not a Bridge), error messages "received" from
associated RCiEPs must be enabled for "transmission" in order to cause a
System Error via the Root Control register or (when the Advanced Error
Reporting Capability is present) reporting via the Root Error Command
register and logging in the Root Error Status register and Error Source
Identification register.

In addition to the defined OS level handling of the reset flow for the
associated RCiEPs of an RCEC, it is possible to also have a firmware first
model. In that case there is no need to take any actions on the RCEC because
the firmware is responsible for them. This is true where APEI [2] is used
to report the AER errors via a GHES[v2] HEST entry [3] and relevant
AER CPER record [4] and Firmware First handling is in use.

We effectively end up with two different types of discovery for
purposes of handling AER errors:

1) Normal bus walk - we pass the downstream port above a bus to which
the device is attached and it walks everything below that point.

2) An RCiEP with no visible association with an RCEC as there is no need to
walk devices. In that case, the flow is to just call the callbacks for the actual
device.

A new walk function, similar to pci_bus_walk is provided that takes a pci_dev
instead of a bus. If that dev corresponds to a downstream port it will walk
the subordinate bus of that downstream port. If the dev does not then it
will call the function on that device alone.

[0] ACPI PCI Express Base Specification 5.0-1 1.3.2.3 Root Complex Integrated
    Endpoint Rules.
[1] ACPI PCI Express Base Specification 5.0-1 6.2 Error Signalling and Logging
[2] ACPI Specification 6.3 Chapter 18 ACPI Platform Error Interface (APEI)
[3] ACPI Specification 6.3 18.2.3.7 Generic Hardware Error Source
[4] UEFI Specification 2.8, N.2.7 PCI Express Error Section

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
 drivers/pci/pcie/err.c | 59 +++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index c543f419d8f9..682302dfb55b 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -146,38 +146,69 @@ static int report_resume(struct pci_dev *dev, void *data)
 	return 0;
 }
 
+/**
+ * pci_walk_dev_affected - walk devices potentially AER affected
+ * @dev      device which may be an RCEC with associated RCiEPs,
+ *           an RCiEP associated with an RCEC, or a Port.
+ * @cb       callback to be called for each device found
+ * @userdata arbitrary pointer to be passed to callback.
+ *
+ * If the device provided is a port, walk the subordinate bus,
+ * including any bridged devices on buses under this bus.
+ * Call the provided callback on each device found.
+ *
+ * If the device provided has no subordinate bus, call the provided
+ * callback on the device itself.
+ */
+static void pci_walk_dev_affected(struct pci_dev *dev, int (*cb)(struct pci_dev *, void *),
+				  void *userdata)
+{
+	if (dev->subordinate) {
+		pci_walk_bus(dev->subordinate, cb, userdata);
+	} else {
+		cb(dev, userdata);
+	}
+}
+
 pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 			pci_channel_state_t state,
 			pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
 {
 	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
-	struct pci_bus *bus;
 
 	/*
 	 * Error recovery runs on all subordinates of the first downstream port.
 	 * If the downstream port detected the error, it is cleared at the end.
+	 * For RCiEPs we should reset just the RCiEP itself.
 	 */
 	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
-	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
+	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
+	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END ||
+	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC))
 		dev = dev->bus->self;
-	bus = dev->subordinate;
 
 	pci_dbg(dev, "broadcast error_detected message\n");
 	if (state == pci_channel_io_frozen) {
-		pci_walk_bus(bus, report_frozen_detected, &status);
+		pci_walk_dev_affected(dev, report_frozen_detected, &status);
+		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
+			pci_warn(dev, "link reset not possible for RCiEP\n");
+			status = PCI_ERS_RESULT_NONE;
+			goto failed;
+		}
+
 		status = reset_link(dev);
 		if (status != PCI_ERS_RESULT_RECOVERED) {
 			pci_warn(dev, "link reset failed\n");
 			goto failed;
 		}
 	} else {
-		pci_walk_bus(bus, report_normal_detected, &status);
+		pci_walk_dev_affected(dev, report_normal_detected, &status);
 	}
 
 	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
 		status = PCI_ERS_RESULT_RECOVERED;
 		pci_dbg(dev, "broadcast mmio_enabled message\n");
-		pci_walk_bus(bus, report_mmio_enabled, &status);
+		pci_walk_dev_affected(dev, report_mmio_enabled, &status);
 	}
 
 	if (status == PCI_ERS_RESULT_NEED_RESET) {
@@ -188,18 +219,22 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 		 */
 		status = PCI_ERS_RESULT_RECOVERED;
 		pci_dbg(dev, "broadcast slot_reset message\n");
-		pci_walk_bus(bus, report_slot_reset, &status);
+		pci_walk_dev_affected(dev, report_slot_reset, &status);
 	}
 
 	if (status != PCI_ERS_RESULT_RECOVERED)
 		goto failed;
 
 	pci_dbg(dev, "broadcast resume message\n");
-	pci_walk_bus(bus, report_resume, &status);
-
-	if (pcie_aer_is_native(dev))
-		pcie_clear_device_status(dev);
-	pci_aer_clear_nonfatal_status(dev);
+	pci_walk_dev_affected(dev, report_resume, &status);
+
+	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
+	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
+		if (pcie_aer_is_native(dev))
+			pcie_clear_device_status(dev);
+		pci_aer_clear_nonfatal_status(dev);
+	}
 	pci_info(dev, "device recovery successful\n");
 	return status;
 
-- 
2.27.0

Re: [PATCH V2 4/9] PCI/AER: Extend AER error handling to RCECs

[Bjorn Helgaas]

On Tue, Aug 04, 2020 at 12:40:47PM -0700, Sean V Kelley wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Currently the kernel does not handle AER errors for Root Complex integrated
> End Points (RCiEPs)[0]. These devices sit on a root bus within the Root Complex
> (RC). AER handling is performed by a Root Complex Event Collector (RCEC) [1]
> which is a effectively a type of RCiEP on the same root bus.
> 
> For an RCEC (technically not a Bridge), error messages "received" from
> associated RCiEPs must be enabled for "transmission" in order to cause a
> System Error via the Root Control register or (when the Advanced Error
> Reporting Capability is present) reporting via the Root Error Command
> register and logging in the Root Error Status register and Error Source
> Identification register.
> 
> In addition to the defined OS level handling of the reset flow for the
> associated RCiEPs of an RCEC, it is possible to also have a firmware first
> model. In that case there is no need to take any actions on the RCEC because
> the firmware is responsible for them. This is true where APEI [2] is used
> to report the AER errors via a GHES[v2] HEST entry [3] and relevant
> AER CPER record [4] and Firmware First handling is in use.

I don't see anything in the patch that mentions "firmware first." Do
we need it in the commit log?  After
https://git.kernel.org/linus/708b20003624 ("PCI/AER: Remove
HEST/FIRMWARE_FIRST parsing for AER ownership"), I think we no longer 
know anything about firmware-first in the kernel.

> We effectively end up with two different types of discovery for
> purposes of handling AER errors:
> 
> 1) Normal bus walk - we pass the downstream port above a bus to which
> the device is attached and it walks everything below that point.
> 
> 2) An RCiEP with no visible association with an RCEC as there is no need to
> walk devices. In that case, the flow is to just call the callbacks for the actual
> device.
> 
> A new walk function, similar to pci_bus_walk is provided that takes a pci_dev
> instead of a bus. If that dev corresponds to a downstream port it will walk
> the subordinate bus of that downstream port. If the dev does not then it
> will call the function on that device alone.

Maybe mention the new function name here?

Add "()" after function names in commit logs and comments so they
don't look like English words.

Wrap commit logs so they fit in 75 columns, so they don't wrap when
"git log" indents them in a default 80 column window.  Yes, I know I
could use wider windows, but I'd still want *some* default so commits
don't just have random widths.

> [0] ACPI PCI Express Base Specification 5.0-1 1.3.2.3 Root Complex Integrated
>     Endpoint Rules.
> [1] ACPI PCI Express Base Specification 5.0-1 6.2 Error Signalling and Logging
> [2] ACPI Specification 6.3 Chapter 18 ACPI Platform Error Interface (APEI)
> [3] ACPI Specification 6.3 18.2.3.7 Generic Hardware Error Source
> [4] UEFI Specification 2.8, N.2.7 PCI Express Error Section
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> ---
>  drivers/pci/pcie/err.c | 59 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index c543f419d8f9..682302dfb55b 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -146,38 +146,69 @@ static int report_resume(struct pci_dev *dev, void *data)
>  	return 0;
>  }
>  
> +/**
> + * pci_walk_dev_affected - walk devices potentially AER affected
> + * @dev      device which may be an RCEC with associated RCiEPs,
> + *           an RCiEP associated with an RCEC, or a Port.

Does this mean that if dev is an RCEC, we call the callback for the
*RCEC* itself?  I would have thought we'd want to do that for the
associated *RCiEPs*?

> + * @cb       callback to be called for each device found
> + * @userdata arbitrary pointer to be passed to callback.
> + *
> + * If the device provided is a port, walk the subordinate bus,

This usage of "port" doesn't seem quite right.  "Port" includes root
ports, switch upstream ports, switch downstream ports, *and* the
upstream ports on endpoints.  The endpoint upstream ports obviously
don't have subordinate buses.  We typically use "bridge" as the
generic term for something with a subordinate bus.

> + * including any bridged devices on buses under this bus.
> + * Call the provided callback on each device found.
> + *
> + * If the device provided has no subordinate bus, call the provided
> + * callback on the device itself.
> + */
> +static void pci_walk_dev_affected(struct pci_dev *dev, int (*cb)(struct pci_dev *, void *),

I don't understand the "affected" reference in the function name.
This doesn't test anything to see whether devices are "affected".
Naming is the hardest part of programming :)

> +				  void *userdata)
> +{
> +	if (dev->subordinate) {
> +		pci_walk_bus(dev->subordinate, cb, userdata);
> +	} else {
> +		cb(dev, userdata);
> +	}

Typical Linux style omits {} for single-line if/else branches.

> +}
> +
>  pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  			pci_channel_state_t state,
>  			pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
>  {
>  	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
> -	struct pci_bus *bus;
>  
>  	/*
>  	 * Error recovery runs on all subordinates of the first downstream port.
>  	 * If the downstream port detected the error, it is cleared at the end.
> +	 * For RCiEPs we should reset just the RCiEP itself.
>  	 */
>  	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> -	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END ||
> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC))
>  		dev = dev->bus->self;
> -	bus = dev->subordinate;
>  
>  	pci_dbg(dev, "broadcast error_detected message\n");
>  	if (state == pci_channel_io_frozen) {
> -		pci_walk_bus(bus, report_frozen_detected, &status);
> +		pci_walk_dev_affected(dev, report_frozen_detected, &status);
> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> +			pci_warn(dev, "link reset not possible for RCiEP\n");
> +			status = PCI_ERS_RESULT_NONE;
> +			goto failed;
> +		}
> +
>  		status = reset_link(dev);

reset_link() might be misnamed.  IIUC "dev" is a bridge, and the point
is really to reset any devices below "dev."  Whether we do that by
resetting link, DPC trigger, secondary bus reset, FLR, etc, is sort of
immaterial.  Some of those methods might be applicable for RCiEPs.

But you didn't add that name; I'm just trying to understand this
better.

>  		if (status != PCI_ERS_RESULT_RECOVERED) {
>  			pci_warn(dev, "link reset failed\n");
>  			goto failed;
>  		}
>  	} else {
> -		pci_walk_bus(bus, report_normal_detected, &status);
> +		pci_walk_dev_affected(dev, report_normal_detected, &status);
>  	}
>  
>  	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
>  		status = PCI_ERS_RESULT_RECOVERED;
>  		pci_dbg(dev, "broadcast mmio_enabled message\n");
> -		pci_walk_bus(bus, report_mmio_enabled, &status);
> +		pci_walk_dev_affected(dev, report_mmio_enabled, &status);
>  	}
>  
>  	if (status == PCI_ERS_RESULT_NEED_RESET) {
> @@ -188,18 +219,22 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  		 */
>  		status = PCI_ERS_RESULT_RECOVERED;
>  		pci_dbg(dev, "broadcast slot_reset message\n");
> -		pci_walk_bus(bus, report_slot_reset, &status);
> +		pci_walk_dev_affected(dev, report_slot_reset, &status);
>  	}
>  
>  	if (status != PCI_ERS_RESULT_RECOVERED)
>  		goto failed;
>  
>  	pci_dbg(dev, "broadcast resume message\n");
> -	pci_walk_bus(bus, report_resume, &status);
> -
> -	if (pcie_aer_is_native(dev))
> -		pcie_clear_device_status(dev);
> -	pci_aer_clear_nonfatal_status(dev);
> +	pci_walk_dev_affected(dev, report_resume, &status);
> +
> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
> +		if (pcie_aer_is_native(dev))
> +			pcie_clear_device_status(dev);
> +		pci_aer_clear_nonfatal_status(dev);

This change (testing pci_pcie_type()) looks like it's not strictly
related to the rest of this patch and maybe should be split out into
its own patch?

> +	}
>  	pci_info(dev, "device recovery successful\n");
>  	return status;
>  
> -- 
> 2.27.0
> 

Re: [PATCH V2 4/9] PCI/AER: Extend AER error handling to RCECs

[Sean V Kelley]

On 7 Aug 2020, at 15:53, Bjorn Helgaas wrote:

> On Tue, Aug 04, 2020 at 12:40:47PM -0700, Sean V Kelley wrote:
>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>
>> Currently the kernel does not handle AER errors for Root Complex 
>> integrated
>> End Points (RCiEPs)[0]. These devices sit on a root bus within the 
>> Root Complex
>> (RC). AER handling is performed by a Root Complex Event Collector 
>> (RCEC) [1]
>> which is a effectively a type of RCiEP on the same root bus.
>>
>> For an RCEC (technically not a Bridge), error messages "received" 
>> from
>> associated RCiEPs must be enabled for "transmission" in order to 
>> cause a
>> System Error via the Root Control register or (when the Advanced 
>> Error
>> Reporting Capability is present) reporting via the Root Error Command
>> register and logging in the Root Error Status register and Error 
>> Source
>> Identification register.
>>
>> In addition to the defined OS level handling of the reset flow for 
>> the
>> associated RCiEPs of an RCEC, it is possible to also have a firmware 
>> first
>> model. In that case there is no need to take any actions on the RCEC 
>> because
>> the firmware is responsible for them. This is true where APEI [2] is 
>> used
>> to report the AER errors via a GHES[v2] HEST entry [3] and relevant
>> AER CPER record [4] and Firmware First handling is in use.
>
> I don't see anything in the patch that mentions "firmware first." Do
> we need it in the commit log?  After
> https://git.kernel.org/linus/708b20003624 ("PCI/AER: Remove
> HEST/FIRMWARE_FIRST parsing for AER ownership"), I think we no longer
> know anything about firmware-first in the kernel.

I’ll let Jonathan reply here.

>
>> We effectively end up with two different types of discovery for
>> purposes of handling AER errors:
>>
>> 1) Normal bus walk - we pass the downstream port above a bus to which
>> the device is attached and it walks everything below that point.
>>
>> 2) An RCiEP with no visible association with an RCEC as there is no 
>> need to
>> walk devices. In that case, the flow is to just call the callbacks 
>> for the actual
>> device.
>>
>> A new walk function, similar to pci_bus_walk is provided that takes a 
>> pci_dev
>> instead of a bus. If that dev corresponds to a downstream port it 
>> will walk
>> the subordinate bus of that downstream port. If the dev does not then 
>> it
>> will call the function on that device alone.
>
> Maybe mention the new function name here?

Agree, I will mention it here.

>
> Add "()" after function names in commit logs and comments so they
> don't look like English words.

Will fix.

>
> Wrap commit logs so they fit in 75 columns, so they don't wrap when
> "git log" indents them in a default 80 column window.  Yes, I know I
> could use wider windows, but I'd still want *some* default so commits
> don't just have random widths.

Will fix.

>
>> [0] ACPI PCI Express Base Specification 5.0-1 1.3.2.3 Root Complex 
>> Integrated
>>     Endpoint Rules.
>> [1] ACPI PCI Express Base Specification 5.0-1 6.2 Error Signalling 
>> and Logging
>> [2] ACPI Specification 6.3 Chapter 18 ACPI Platform Error Interface 
>> (APEI)
>> [3] ACPI Specification 6.3 18.2.3.7 Generic Hardware Error Source
>> [4] UEFI Specification 2.8, N.2.7 PCI Express Error Section
>>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> ---
>>  drivers/pci/pcie/err.c | 59 
>> +++++++++++++++++++++++++++++++++---------
>>  1 file changed, 47 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index c543f419d8f9..682302dfb55b 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -146,38 +146,69 @@ static int report_resume(struct pci_dev *dev, 
>> void *data)
>>  	return 0;
>>  }
>>
>> +/**
>> + * pci_walk_dev_affected - walk devices potentially AER affected
>> + * @dev      device which may be an RCEC with associated RCiEPs,
>> + *           an RCiEP associated with an RCEC, or a Port.
>
> Does this mean that if dev is an RCEC, we call the callback for the
> *RCEC* itself?  I would have thought we'd want to do that for the
> associated *RCiEPs*?

Yes, we do. The errors can come from either an RCEC or its respective 
RCiEPs. Both Root Port and RCEC can report error for themselves. So both 
an error Root Port device and an error RCEC device can be passed here 
for error handling. In fact, the bit corresponding to the device number 
of the RCEC is always set in the RCiEP Bitmap (section 7.9.2). And an 
RCEC must also follow all the rules for an RCiEP (section 1.3.4).

I was wanting to do a test with an AER injection of the RCEC itself. But 
it looks like current aer_inject.c doesn’t support injecting an error 
to Root Ports or RCECs. Will need to take a look at it.

>
>> + * @cb       callback to be called for each device found
>> + * @userdata arbitrary pointer to be passed to callback.
>> + *
>> + * If the device provided is a port, walk the subordinate bus,
>
> This usage of "port" doesn't seem quite right.  "Port" includes root
> ports, switch upstream ports, switch downstream ports, *and* the
> upstream ports on endpoints.  The endpoint upstream ports obviously
> don't have subordinate buses.  We typically use "bridge" as the
> generic term for something with a subordinate bus.

Okay, that makes sense.  Will correct with “bridge”.

>
>> + * including any bridged devices on buses under this bus.
>> + * Call the provided callback on each device found.
>> + *
>> + * If the device provided has no subordinate bus, call the provided
>> + * callback on the device itself.
>> + */
>> +static void pci_walk_dev_affected(struct pci_dev *dev, int 
>> (*cb)(struct pci_dev *, void *),
>
> I don't understand the "affected" reference in the function name.
> This doesn't test anything to see whether devices are "affected".
> Naming is the hardest part of programming :)

In earlier discussion, Cameron had suggested pci_walk_aer_affected(). 
But I thought perhaps that the focus should be on the devices.  Perhaps 
a better description would be pci_walk_aer_devices() or something along 
those lines.  The original incarnation was pci_walk_below_dev().

I’m open to anything, really.

>
>> +				  void *userdata)
>> +{
>> +	if (dev->subordinate) {
>> +		pci_walk_bus(dev->subordinate, cb, userdata);
>> +	} else {
>> +		cb(dev, userdata);
>> +	}
>
> Typical Linux style omits {} for single-line if/else branches.

Will fix.

>
>> +}
>> +
>>  pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>  			pci_channel_state_t state,
>>  			pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
>>  {
>>  	pci_ers_result_t 'status = PCI_ERS_RESULT_CAN_RECOVER;
>> -	struct pci_bus *bus;
>>
>>  	/*
>>  	 * Error recovery runs on all subordinates of the first downstream 
>> port.
>>  	 * If the downstream port detected the error, it is cleared at the 
>> end.
>> +	 * For RCiEPs we should reset just the RCiEP itself.
>>  	 */
>>  	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>> -	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
>> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END ||
>> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC))
>>  		dev = dev->bus->self;
>> -	bus = dev->subordinate;
>>
>>  	pci_dbg(dev, "broadcast error_detected message\n");
>>  	if (state == pci_channel_io_frozen) {
>> -		pci_walk_bus(bus, report_frozen_detected, &status);
>> +		pci_walk_dev_affected(dev, report_frozen_detected, &status);
>> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
>> +			pci_warn(dev, "link reset not possible for RCiEP\n");
>> +			status = PCI_ERS_RESULT_NONE;
>> +			goto failed;
>> +		}
>> +
>>  		status = reset_link(dev);
>
> reset_link() might be misnamed.  IIUC "dev" is a bridge, and the point
> is really to reset any devices below "dev."  Whether we do that by
> resetting link, DPC trigger, secondary bus reset, FLR, etc, is sort of
> immaterial.  Some of those methods might be applicable for RCiEPs.
>
> But you didn't add that name; I'm just trying to understand this
> better.

Yes, that’s a confusing term with the _link attached. It’s difficult 
to relate to the different resets that might be applicable. I was 
thinking about that when looking at the callback path via the 
“reset_link” of the RCiEP to the RCEC for the sole purpose of 
clearing the Root Port Error Status. It would be worth time to spend 
looking at better descriptive naming/methods.

>
>>  		if (status != PCI_ERS_RESULT_RECOVERED) {
>>  			pci_warn(dev, "link reset failed\n");
>>  			goto failed;
>>  		}
>>  	} else {
>> -		pci_walk_bus(bus, report_normal_detected, &status);
>> +		pci_walk_dev_affected(dev, report_normal_detected, &status);
>>  	}
>>
>>  	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
>>  		status = PCI_ERS_RESULT_RECOVERED;
>>  		pci_dbg(dev, "broadcast mmio_enabled message\n");
>> -		pci_walk_bus(bus, report_mmio_enabled, &status);
>> +		pci_walk_dev_affected(dev, report_mmio_enabled, &status);
>>  	}
>>
>>  	if (status == PCI_ERS_RESULT_NEED_RESET) {
>> @@ -188,18 +219,22 @@ pci_ers_result_t pcie_do_recovery(struct 
>> pci_dev *dev,
>>  		 */
>>  		status = PCI_ERS_RESULT_RECOVERED;
>>  		pci_dbg(dev, "broadcast slot_reset message\n");
>> -		pci_walk_bus(bus, report_slot_reset, &status);
>> +		pci_walk_dev_affected(dev, report_slot_reset, &status);
>>  	}
>>
>>  	if (status != PCI_ERS_RESULT_RECOVERED)
>>  		goto failed;
>>
>>  	pci_dbg(dev, "broadcast resume message\n");
>> -	pci_walk_bus(bus, report_resume, &status);
>> -
>> -	if (pcie_aer_is_native(dev))
>> -		pcie_clear_device_status(dev);
>> -	pci_aer_clear_nonfatal_status(dev);
>> +	pci_walk_dev_affected(dev, report_resume, &status);
>> +
>> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
>> +		if (pcie_aer_is_native(dev))
>> +			pcie_clear_device_status(dev);
>> +		pci_aer_clear_nonfatal_status(dev);
>
> This change (testing pci_pcie_type()) looks like it's not strictly
> related to the rest of this patch and maybe should be split out into
> its own patch?

This change was also based on a commit (068c29a24) in the pci/next 
branch. The type testing was brought over from Jonathan’s original V2, 
but actually, it went full circle by adding the RC_EC type, because now 
it was no longer a no-op. There was an original concern about the need 
for those to be called on the RCEC from Jonathan’s RFC.

Thoughts Jonathan?

Thanks,

Sean

>
>> +	}
>>  	pci_info(dev, "device recovery successful\n");
>>  	return status;
>>
>> -- 
>> 2.27.0
>>

Re: [PATCH V2 4/9] PCI/AER: Extend AER error handling to RCECs

[Jonathan Cameron]

On Fri, 7 Aug 2020 17:55:17 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:

> On 7 Aug 2020, at 15:53, Bjorn Helgaas wrote:
> 
> > On Tue, Aug 04, 2020 at 12:40:47PM -0700, Sean V Kelley wrote:  
> >> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>
> >> Currently the kernel does not handle AER errors for Root Complex 
> >> integrated
> >> End Points (RCiEPs)[0]. These devices sit on a root bus within the 
> >> Root Complex
> >> (RC). AER handling is performed by a Root Complex Event Collector 
> >> (RCEC) [1]
> >> which is a effectively a type of RCiEP on the same root bus.
> >>
> >> For an RCEC (technically not a Bridge), error messages "received" 
> >> from
> >> associated RCiEPs must be enabled for "transmission" in order to 
> >> cause a
> >> System Error via the Root Control register or (when the Advanced 
> >> Error
> >> Reporting Capability is present) reporting via the Root Error Command
> >> register and logging in the Root Error Status register and Error 
> >> Source
> >> Identification register.
> >>
> >> In addition to the defined OS level handling of the reset flow for 
> >> the
> >> associated RCiEPs of an RCEC, it is possible to also have a firmware 
> >> first
> >> model. In that case there is no need to take any actions on the RCEC 
> >> because
> >> the firmware is responsible for them. This is true where APEI [2] is 
> >> used
> >> to report the AER errors via a GHES[v2] HEST entry [3] and relevant
> >> AER CPER record [4] and Firmware First handling is in use.  
> >
> > I don't see anything in the patch that mentions "firmware first." Do
> > we need it in the commit log?  After
> > https://git.kernel.org/linus/708b20003624 ("PCI/AER: Remove
> > HEST/FIRMWARE_FIRST parsing for AER ownership"), I think we no longer
> > know anything about firmware-first in the kernel.  
> 
> I’ll let Jonathan reply here.

This is a terminology question rather about what the distinction
between "Firmware First" vs "non native handling" is.

Note that in ARM world, native handling tends to be referred to as
'kernel first' and firmware based handling as 'firmware first' 
but that isn't that relevant here other than perhaps explaining why I used
this terminology. e.g.
http://connect.linaro.org.s3.amazonaws.com/hkg18/presentations/hkg18-116.pdf
The distinction is who receives the notification of the error from hardware
and hence who sees it 'first' - basically where does the interrupt go?

Anyhow, if we want to avoid confusion here, we can just use the phrase
"non native handling" which I think is unambiguous?


...
 
> >  
> >> + * including any bridged devices on buses under this bus.
> >> + * Call the provided callback on each device found.
> >> + *
> >> + * If the device provided has no subordinate bus, call the provided
> >> + * callback on the device itself.
> >> + */
> >> +static void pci_walk_dev_affected(struct pci_dev *dev, int 
> >> (*cb)(struct pci_dev *, void *),  
> >
> > I don't understand the "affected" reference in the function name.
> > This doesn't test anything to see whether devices are "affected".
> > Naming is the hardest part of programming :)  
> 
> In earlier discussion, Cameron had suggested pci_walk_aer_affected(). 
> But I thought perhaps that the focus should be on the devices.  Perhaps 
> a better description would be pci_walk_aer_devices() or something along 
> those lines.  The original incarnation was pci_walk_below_dev().
> 
> I’m open to anything, really.

Agreed. It is really hard to name this function and would be great to 
have a better option.

> >>  pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >>  			pci_channel_state_t state,
> >>  			pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
> >>  {
> >>  	pci_ers_result_t 'status = PCI_ERS_RESULT_CAN_RECOVER;
> >> -	struct pci_bus *bus;
> >>
> >>  	/*
> >>  	 * Error recovery runs on all subordinates of the first downstream 
> >> port.
> >>  	 * If the downstream port detected the error, it is cleared at the 
> >> end.
> >> +	 * For RCiEPs we should reset just the RCiEP itself.
> >>  	 */
> >>  	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> >> -	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
> >> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> >> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END ||
> >> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC))
> >>  		dev = dev->bus->self;
> >> -	bus = dev->subordinate;
> >>
> >>  	pci_dbg(dev, "broadcast error_detected message\n");
> >>  	if (state == pci_channel_io_frozen) {
> >> -		pci_walk_bus(bus, report_frozen_detected, &status);
> >> +		pci_walk_dev_affected(dev, report_frozen_detected, &status);
> >> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> >> +			pci_warn(dev, "link reset not possible for RCiEP\n");
> >> +			status = PCI_ERS_RESULT_NONE;
> >> +			goto failed;
> >> +		}
> >> +
> >>  		status = reset_link(dev);  
> >
> > reset_link() might be misnamed.  IIUC "dev" is a bridge, and the point
> > is really to reset any devices below "dev."  Whether we do that by
> > resetting link, DPC trigger, secondary bus reset, FLR, etc, is sort of
> > immaterial.  Some of those methods might be applicable for RCiEPs.
> >
> > But you didn't add that name; I'm just trying to understand this
> > better.  
> 
> Yes, that’s a confusing term with the _link attached. It’s difficult 
> to relate to the different resets that might be applicable. I was 
> thinking about that when looking at the callback path via the 
> “reset_link” of the RCiEP to the RCEC for the sole purpose of 
> clearing the Root Port Error Status. It would be worth time to spend 
> looking at better descriptive naming/methods.

Agreed, this caused me some some confusion as well so more descriptive
naming would be good.

> 
> >  
> >>  		if (status != PCI_ERS_RESULT_RECOVERED) {
> >>  			pci_warn(dev, "link reset failed\n");
> >>  			goto failed;
> >>  		}
> >>  	} else {
> >> -		pci_walk_bus(bus, report_normal_detected, &status);
> >> +		pci_walk_dev_affected(dev, report_normal_detected, &status);
> >>  	}
> >>
> >>  	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
> >>  		status = PCI_ERS_RESULT_RECOVERED;
> >>  		pci_dbg(dev, "broadcast mmio_enabled message\n");
> >> -		pci_walk_bus(bus, report_mmio_enabled, &status);
> >> +		pci_walk_dev_affected(dev, report_mmio_enabled, &status);
> >>  	}
> >>
> >>  	if (status == PCI_ERS_RESULT_NEED_RESET) {
> >> @@ -188,18 +219,22 @@ pci_ers_result_t pcie_do_recovery(struct 
> >> pci_dev *dev,
> >>  		 */
> >>  		status = PCI_ERS_RESULT_RECOVERED;
> >>  		pci_dbg(dev, "broadcast slot_reset message\n");
> >> -		pci_walk_bus(bus, report_slot_reset, &status);
> >> +		pci_walk_dev_affected(dev, report_slot_reset, &status);
> >>  	}
> >>
> >>  	if (status != PCI_ERS_RESULT_RECOVERED)
> >>  		goto failed;
> >>
> >>  	pci_dbg(dev, "broadcast resume message\n");
> >> -	pci_walk_bus(bus, report_resume, &status);
> >> -
> >> -	if (pcie_aer_is_native(dev))
> >> -		pcie_clear_device_status(dev);
> >> -	pci_aer_clear_nonfatal_status(dev);
> >> +	pci_walk_dev_affected(dev, report_resume, &status);
> >> +
> >> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> >> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> >> +	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)) {
> >> +		if (pcie_aer_is_native(dev))
> >> +			pcie_clear_device_status(dev);
> >> +		pci_aer_clear_nonfatal_status(dev);  
> >
> > This change (testing pci_pcie_type()) looks like it's not strictly
> > related to the rest of this patch and maybe should be split out into
> > its own patch?  
> 
> This change was also based on a commit (068c29a24) in the pci/next 
> branch. The type testing was brought over from Jonathan’s original V2, 
> but actually, it went full circle by adding the RC_EC type, because now 
> it was no longer a no-op. There was an original concern about the need 
> for those to be called on the RCEC from Jonathan’s RFC.

What this is doing is ensuring that we do not call these reset functions
if dev is an RCiEP.  This patch is introducing that possibility for the
first time.  Breaking it out to a precursor patch might be possible but
would seem a bit odd.  Perhaps we could invert the logic to check it
isn't PCI_EXP_TYPE_RC_END?  That seems less intuitive than a positive
check to me.  It might not be obvious to a future reader that we can't get
here with most of the other types.

Thanks,

Jonathan



> 
> Thoughts Jonathan?
> 
> Thanks,
> 
> Sean
> 
> >  
> >> +	}
> >>  	pci_info(dev, "device recovery successful\n");
> >>  	return status;
> >>
> >> -- 
> >> 2.27.0
> >>  

Re: [PATCH V2 4/9] PCI/AER: Extend AER error handling to RCECs

[Bjorn Helgaas]

On Mon, Aug 10, 2020 at 10:32:52AM +0100, Jonathan Cameron wrote:
> On Fri, 7 Aug 2020 17:55:17 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
> > On 7 Aug 2020, at 15:53, Bjorn Helgaas wrote:
> > > On Tue, Aug 04, 2020 at 12:40:47PM -0700, Sean V Kelley wrote:  

> > >>  	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > >> -	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
> > >> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> > >> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END ||
> > >> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC))
> > >>  		dev = dev->bus->self;

I'm not sure I understand this "if" statement.  Previously (with no
RCEC support), the possible ways I see to call pcie_do_recovery() are
with:

  AER native:   Root Port
  AER via APEI: Root Port or other PCIe device (ACPI v6.3, 18.3.2.5)
  DPC:          Root Port or Switch Downstream Port
  EDR:          Root Port or Switch Downstream Port

I *guess* the reason we have this "if" statement is for the AER/APEI
case?  And the effect is that even if AER/APEI gives us an Endpoint,
we back up and handle it as though we got it from the Downstream Port
above it, i.e., we reset the Endpoint along with any other children of
that Downstream Port?

Then, IIUC, your patches add this case:

  AER native:   Root Port or RCEC
  AER via APEI: Root Port, RCEC, or other PCIe device

Just noodling here, but I wonder if this would be more understandable
as something like:

  type = pci_pcie_type(dev);
  if (type == PCI_EXP_TYPE_ROOT_PORT ||
      type == PCI_EXP_TYPE_DOWNSTREAM ||
      type == PCI_EXP_TYPE_RC_EC)
    bridge = dev;
  else if (type == PCI_EXP_TYPE_RC_END)
    bridge = dev->rcec;
  else
    bridge = pci_upstream_bridge(dev);

and then we could do:

  if (type == PCI_EXP_TYPE_RC_END)
    flr_on_rciep(dev);
  else
    reset_link(bridge);

It's still awkward to have to deal with being supplied either
endpoints or bridges.  But I guess in the AER/APEI case, we aren't
allowed to touch the error registers so maybe we can't avoid the
awkwardness.

> > >>  		status = reset_link(dev);  
> > >
> > > reset_link() might be misnamed.  IIUC "dev" is a bridge, and the point
> > > is really to reset any devices below "dev."  Whether we do that by
> > > resetting link, DPC trigger, secondary bus reset, FLR, etc, is sort of
> > > immaterial.  Some of those methods might be applicable for RCiEPs.
> > >
> > > But you didn't add that name; I'm just trying to understand this
> > > better.  
> > 
> > Yes, that’s a confusing term with the _link attached. It’s difficult 
> > to relate to the different resets that might be applicable. I was 
> > thinking about that when looking at the callback path via the 
> > “reset_link” of the RCiEP to the RCEC for the sole purpose of 
> > clearing the Root Port Error Status. It would be worth time to spend 
> > looking at better descriptive naming/methods.
> 
> Agreed, this caused me some some confusion as well so more descriptive
> naming would be good.

Maybe something like reset_subordinate_devices()?  Then it's clear
that we pass a bridge and reset the devices *below* it.  It's not
quite as obvious for RCECs, since they aren't bridges and the RCiEPs
aren't actually *subordinates*, but maybe it's still suggestive of the
logical relationship?

Bjorn

Re: [PATCH V2 4/9] PCI/AER: Extend AER error handling to RCECs

[Jonathan Cameron]

On Mon, 17 Aug 2020 17:24:33 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Mon, Aug 10, 2020 at 10:32:52AM +0100, Jonathan Cameron wrote:
> > On Fri, 7 Aug 2020 17:55:17 -0700
> > Sean V Kelley <sean.v.kelley@intel.com> wrote:  
> > > On 7 Aug 2020, at 15:53, Bjorn Helgaas wrote:  
> > > > On Tue, Aug 04, 2020 at 12:40:47PM -0700, Sean V Kelley wrote:    
> 
> > > >>  	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > > >> -	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
> > > >> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> > > >> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END ||
> > > >> +	      pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC))
> > > >>  		dev = dev->bus->self;  
> 
> I'm not sure I understand this "if" statement.  Previously (with no
> RCEC support), the possible ways I see to call pcie_do_recovery() are
> with:
> 
>   AER native:   Root Port
>   AER via APEI: Root Port or other PCIe device (ACPI v6.3, 18.3.2.5)
>   DPC:          Root Port or Switch Downstream Port
>   EDR:          Root Port or Switch Downstream Port
> 
> I *guess* the reason we have this "if" statement is for the AER/APEI
> case?  And the effect is that even if AER/APEI gives us an Endpoint,
> we back up and handle it as though we got it from the Downstream Port
> above it, i.e., we reset the Endpoint along with any other children of
> that Downstream Port?
> 
> Then, IIUC, your patches add this case:
> 
>   AER native:   Root Port or RCEC
>   AER via APEI: Root Port, RCEC, or other PCIe device
> 
> Just noodling here, but I wonder if this would be more understandable
> as something like:
> 
>   type = pci_pcie_type(dev);
>   if (type == PCI_EXP_TYPE_ROOT_PORT ||
>       type == PCI_EXP_TYPE_DOWNSTREAM ||
>       type == PCI_EXP_TYPE_RC_EC)
>     bridge = dev;
>   else if (type == PCI_EXP_TYPE_RC_END)
>     bridge = dev->rcec;
>   else
>     bridge = pci_upstream_bridge(dev);
> 
> and then we could do:
> 
>   if (type == PCI_EXP_TYPE_RC_END)
>     flr_on_rciep(dev);
>   else
>     reset_link(bridge);
> 
> It's still awkward to have to deal with being supplied either
> endpoints or bridges.  But I guess in the AER/APEI case, we aren't
> allowed to touch the error registers so maybe we can't avoid the
> awkwardness.

Agreed with your analysis with one exception. It isn't just that we
aren't allowed to touch the error registers, but also that they may
not even exist (i.e. there is no RCEC).

There are quite a lot of places where we have to then handle the
cases separately.  For an RC_END in the APEI case we don't
have to have an RCEC as we should never be touching it or
any of its registers.  We have platforms that do it this way
(obviously there is a hardware entity doing RCEC like stuff, but it is
not visible to the OS).

In these cases (bridge == NULL) and we can't call the bus_walk on it
to call the various desired resets on the RCiEP.  We could
do something like

pci_walk_affected(bridge, dev, report_frozen, &status);

and if bridge is NULL, perform the reset just on dev.

Would that be clearer?

Thanks,

Jonathan

> 
> > > >>  		status = reset_link(dev);    
> > > >
> > > > reset_link() might be misnamed.  IIUC "dev" is a bridge, and the point
> > > > is really to reset any devices below "dev."  Whether we do that by
> > > > resetting link, DPC trigger, secondary bus reset, FLR, etc, is sort of
> > > > immaterial.  Some of those methods might be applicable for RCiEPs.
> > > >
> > > > But you didn't add that name; I'm just trying to understand this
> > > > better.    
> > > 
> > > Yes, that’s a confusing term with the _link attached. It’s difficult 
> > > to relate to the different resets that might be applicable. I was 
> > > thinking about that when looking at the callback path via the 
> > > “reset_link” of the RCiEP to the RCEC for the sole purpose of 
> > > clearing the Root Port Error Status. It would be worth time to spend 
> > > looking at better descriptive naming/methods.  
> > 
> > Agreed, this caused me some some confusion as well so more descriptive
> > naming would be good.  
> 
> Maybe something like reset_subordinate_devices()?  Then it's clear
> that we pass a bridge and reset the devices *below* it.  It's not
> quite as obvious for RCECs, since they aren't bridges and the RCiEPs
> aren't actually *subordinates*, but maybe it's still suggestive of the
> logical relationship?
> 
> Bjorn

[PATCH V2 5/9] PCI/AER: Apply function level reset to RCiEP on fatal error

[Sean V Kelley]

From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Attempt to do function level reset for an RCiEP associated with an
RCEC device on fatal error.

Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
 drivers/pci/pcie/err.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 682302dfb55b..4812aa678eff 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -170,6 +170,17 @@ static void pci_walk_dev_affected(struct pci_dev *dev, int (*cb)(struct pci_dev
 	}
 }
 
+static enum pci_ers_result flr_on_rciep(struct pci_dev *dev)
+{
+	if (!pcie_has_flr(dev))
+		return PCI_ERS_RESULT_NONE;
+
+	if (pcie_flr(dev))
+		return PCI_ERS_RESULT_DISCONNECT;
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
 pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 			pci_channel_state_t state,
 			pci_ers_result_t (*reset_link)(struct pci_dev *pdev))
@@ -191,15 +202,17 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	if (state == pci_channel_io_frozen) {
 		pci_walk_dev_affected(dev, report_frozen_detected, &status);
 		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
-			pci_warn(dev, "link reset not possible for RCiEP\n");
-			status = PCI_ERS_RESULT_NONE;
-			goto failed;
-		}
-
-		status = reset_link(dev);
-		if (status != PCI_ERS_RESULT_RECOVERED) {
-			pci_warn(dev, "link reset failed\n");
-			goto failed;
+			status = flr_on_rciep(dev);
+			if (status != PCI_ERS_RESULT_RECOVERED) {
+				pci_warn(dev, "function level reset failed\n");
+				goto failed;
+			}
+		} else {
+			status = reset_link(dev);
+			if (status != PCI_ERS_RESULT_RECOVERED) {
+				pci_warn(dev, "link reset failed\n");
+				goto failed;
+			}
 		}
 	} else {
 		pci_walk_dev_affected(dev, report_normal_detected, &status);
-- 
2.27.0

[PATCH V2 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs

[Sean V Kelley]

From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

When attempting error recovery for an RCiEP associated with an RCEC device,
there needs to be a way to update the Root Error Status, the Uncorrectable
Error Status and the Uncorrectable Error Severity of the parent RCEC.
So add the 'rcec' field to the pci_dev structure and provide a hook for the
Root Port Driver to associate RCiEPs with their respective parent RCEC.

Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
 drivers/pci/pcie/aer.c         |  9 +++++----
 drivers/pci/pcie/err.c         | 12 ++++++++++++
 drivers/pci/pcie/portdrv_pci.c | 15 +++++++++++++++
 include/linux/pci.h            |  3 +++
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 87283cda3990..f658607e8e00 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1358,17 +1358,18 @@ static int aer_probe(struct pcie_device *dev)
 static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 {
 	int aer = dev->aer_cap;
+	int rc = 0;
 	u32 reg32;
-	int rc;
-
 
 	/* Disable Root's interrupt in response to error messages */
 	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
 	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
 	pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
 
-	rc = pci_bus_error_reset(dev);
-	pci_info(dev, "Root Port link has been reset\n");
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC) {
+		rc = pci_bus_error_reset(dev);
+		pci_info(dev, "Root Port link has been reset\n");
+	}
 
 	/* Clear Root Error Status */
 	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 4812aa678eff..43f1c55c76db 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -203,6 +203,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 		pci_walk_dev_affected(dev, report_frozen_detected, &status);
 		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
 			status = flr_on_rciep(dev);
+			/*
+			 * The callback only clears the Root Error Status
+			 * of the RCEC (see aer.c).
+			 */
+			if (pcie_aer_is_native(dev) && dev->rcec)
+				reset_link(dev->rcec);
 			if (status != PCI_ERS_RESULT_RECOVERED) {
 				pci_warn(dev, "function level reset failed\n");
 				goto failed;
@@ -247,7 +253,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 		if (pcie_aer_is_native(dev))
 			pcie_clear_device_status(dev);
 		pci_aer_clear_nonfatal_status(dev);
+	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
+		if (pcie_aer_is_native(dev) && dev->rcec)
+			pcie_clear_device_status(dev->rcec);
+		if (dev->rcec)
+			pci_aer_clear_nonfatal_status(dev->rcec);
 	}
+
 	pci_info(dev, "device recovery successful\n");
 	return status;
 
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 4d880679b9b1..dff5c9e13412 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -90,6 +90,18 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 #define PCIE_PORTDRV_PM_OPS	NULL
 #endif /* !PM */
 
+static int pcie_hook_rcec(struct pci_dev *pdev, void *data)
+{
+	struct pci_dev *rcec = (struct pci_dev *)data;
+
+	pdev->rcec = rcec;
+	pci_dbg(rcec, "RCiEP(under an RCEC) %04x:%02x:%02x.%d\n",
+		pci_domain_nr(pdev->bus), pdev->bus->number,
+		PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+
+	return 0;
+}
+
 /*
  * pcie_portdrv_probe - Probe PCI-Express port devices
  * @dev: PCI-Express port device being probed
@@ -110,6 +122,9 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
 		return -ENODEV;
 
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
+		pcie_walk_rcec(dev, pcie_hook_rcec, dev);
+
 	status = pcie_port_device_register(dev);
 	if (status)
 		return status;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ee49469bd2b5..d5f7dbbf5e2f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -326,6 +326,9 @@ struct pci_dev {
 #ifdef CONFIG_PCIEAER
 	u16		aer_cap;	/* AER capability offset */
 	struct aer_stats *aer_stats;	/* AER stats for this device */
+#endif
+#ifdef CONFIG_PCIEPORTBUS
+	struct pci_dev	*rcec;		/* Associated RCEC device */
 #endif
 	u8		pcie_cap;	/* PCIe capability offset */
 	u8		msi_cap;	/* MSI capability offset */
-- 
2.27.0

Re: [PATCH V2 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs

[Jonathan Cameron]

On Tue, 4 Aug 2020 12:40:49 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:

> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> When attempting error recovery for an RCiEP associated with an RCEC device,
> there needs to be a way to update the Root Error Status, the Uncorrectable
> Error Status and the Uncorrectable Error Severity of the parent RCEC.
> So add the 'rcec' field to the pci_dev structure and provide a hook for the
> Root Port Driver to associate RCiEPs with their respective parent RCEC.
> 
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Hi,

One question in line.

> ---
>  drivers/pci/pcie/aer.c         |  9 +++++----
>  drivers/pci/pcie/err.c         | 12 ++++++++++++
>  drivers/pci/pcie/portdrv_pci.c | 15 +++++++++++++++
>  include/linux/pci.h            |  3 +++
>  4 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 87283cda3990..f658607e8e00 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1358,17 +1358,18 @@ static int aer_probe(struct pcie_device *dev)
>  static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>  {
>  	int aer = dev->aer_cap;
> +	int rc = 0;
>  	u32 reg32;
> -	int rc;
> -
>  
>  	/* Disable Root's interrupt in response to error messages */
>  	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>  	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
>  	pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>  
> -	rc = pci_bus_error_reset(dev);
> -	pci_info(dev, "Root Port link has been reset\n");
> +	if (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC) {
> +		rc = pci_bus_error_reset(dev);
> +		pci_info(dev, "Root Port link has been reset\n");
> +	}
>  
>  	/* Clear Root Error Status */
>  	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 4812aa678eff..43f1c55c76db 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -203,6 +203,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  		pci_walk_dev_affected(dev, report_frozen_detected, &status);
>  		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
>  			status = flr_on_rciep(dev);
> +			/*
> +			 * The callback only clears the Root Error Status
> +			 * of the RCEC (see aer.c).
> +			 */
> +			if (pcie_aer_is_native(dev) && dev->rcec)
> +				reset_link(dev->rcec);

I'm not sure about this pcie_aer_is_native check.
We don't check in the normal EP path.  Perhaps we should be checking there
as well?

I can contrive a CPER record that hits the reset_link for the normal EP on my qemu
test setup.  Just for fun it causes a synchronous external abort that I need
to track down but not related this patch or indeed reset_link and may just reflect
an impossible to hit path in the e1000e driver.

It needs a very contrived combination of blocks that say the error is fatal
and others that say it isn't so I'm not that worried about that.


>  			if (status != PCI_ERS_RESULT_RECOVERED) {
>  				pci_warn(dev, "function level reset failed\n");
>  				goto failed;
> @@ -247,7 +253,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  		if (pcie_aer_is_native(dev))
>  			pcie_clear_device_status(dev);
>  		pci_aer_clear_nonfatal_status(dev);
> +	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> +		if (pcie_aer_is_native(dev) && dev->rcec)
> +			pcie_clear_device_status(dev->rcec);
> +		if (dev->rcec)
> +			pci_aer_clear_nonfatal_status(dev->rcec);
>  	}
> +
>  	pci_info(dev, "device recovery successful\n");
>  	return status;
>  
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 4d880679b9b1..dff5c9e13412 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -90,6 +90,18 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  #define PCIE_PORTDRV_PM_OPS	NULL
>  #endif /* !PM */
>  
> +static int pcie_hook_rcec(struct pci_dev *pdev, void *data)
> +{
> +	struct pci_dev *rcec = (struct pci_dev *)data;
> +
> +	pdev->rcec = rcec;
> +	pci_dbg(rcec, "RCiEP(under an RCEC) %04x:%02x:%02x.%d\n",
> +		pci_domain_nr(pdev->bus), pdev->bus->number,
> +		PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +
> +	return 0;
> +}
> +
>  /*
>   * pcie_portdrv_probe - Probe PCI-Express port devices
>   * @dev: PCI-Express port device being probed
> @@ -110,6 +122,9 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>  		return -ENODEV;
>  
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> +		pcie_walk_rcec(dev, pcie_hook_rcec, dev);
> +
>  	status = pcie_port_device_register(dev);
>  	if (status)
>  		return status;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index ee49469bd2b5..d5f7dbbf5e2f 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -326,6 +326,9 @@ struct pci_dev {
>  #ifdef CONFIG_PCIEAER
>  	u16		aer_cap;	/* AER capability offset */
>  	struct aer_stats *aer_stats;	/* AER stats for this device */
> +#endif
> +#ifdef CONFIG_PCIEPORTBUS
> +	struct pci_dev	*rcec;		/* Associated RCEC device */
>  #endif
>  	u8		pcie_cap;	/* PCIe capability offset */
>  	u8		msi_cap;	/* MSI capability offset */

Re: [PATCH V2 6/9] PCI: Add 'rcec' field to pci_dev for associated RCiEPs

[Sean V Kelley]

On 5 Aug 2020, at 10:40, Jonathan Cameron wrote:

> On Tue, 4 Aug 2020 12:40:49 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>
>> When attempting error recovery for an RCiEP associated with an RCEC 
>> device,
>> there needs to be a way to update the Root Error Status, the 
>> Uncorrectable
>> Error Status and the Uncorrectable Error Severity of the parent RCEC.
>> So add the 'rcec' field to the pci_dev structure and provide a hook 
>> for the
>> Root Port Driver to associate RCiEPs with their respective parent 
>> RCEC.
>>
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Hi,
>
> One question in line.
>
>> ---
>>  drivers/pci/pcie/aer.c         |  9 +++++----
>>  drivers/pci/pcie/err.c         | 12 ++++++++++++
>>  drivers/pci/pcie/portdrv_pci.c | 15 +++++++++++++++
>>  include/linux/pci.h            |  3 +++
>>  4 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index 87283cda3990..f658607e8e00 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -1358,17 +1358,18 @@ static int aer_probe(struct pcie_device *dev)
>>  static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>>  {
>>  	int aer = dev->aer_cap;
>> +	int rc = 0;
>>  	u32 reg32;
>> -	int rc;
>> -
>>
>>  	/* Disable Root's interrupt in response to error messages */
>>  	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>>  	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
>>  	pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>>
>> -	rc = pci_bus_error_reset(dev);
>> -	pci_info(dev, "Root Port link has been reset\n");
>> +	if (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC) {
>> +		rc = pci_bus_error_reset(dev);
>> +		pci_info(dev, "Root Port link has been reset\n");
>> +	}
>>
>>  	/* Clear Root Error Status */
>>  	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index 4812aa678eff..43f1c55c76db 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -203,6 +203,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev 
>> *dev,
>>  		pci_walk_dev_affected(dev, report_frozen_detected, &status);
>>  		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
>>  			status = flr_on_rciep(dev);
>> +			/*
>> +			 * The callback only clears the Root Error Status
>> +			 * of the RCEC (see aer.c).
>> +			 */
>> +			if (pcie_aer_is_native(dev) && dev->rcec)
>> +				reset_link(dev->rcec);
>
> I'm not sure about this pcie_aer_is_native check.
> We don't check in the normal EP path.  Perhaps we should be checking 
> there
> as well?

Looks like we should.  Will make adjustments and test.

>
> I can contrive a CPER record that hits the reset_link for the normal 
> EP on my qemu
> test setup.  Just for fun it causes a synchronous external abort that 
> I need
> to track down but not related this patch or indeed reset_link and may 
> just reflect
> an impossible to hit path in the e1000e driver.
>
> It needs a very contrived combination of blocks that say the error is 
> fatal
> and others that say it isn't so I'm not that worried about that.

Okay, will also test on my end.

Thanks,

Sean

>
>
>>  			if (status != PCI_ERS_RESULT_RECOVERED) {
>>  				pci_warn(dev, "function level reset failed\n");
>>  				goto failed;
>> @@ -247,7 +253,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev 
>> *dev,
>>  		if (pcie_aer_is_native(dev))
>>  			pcie_clear_device_status(dev);
>>  		pci_aer_clear_nonfatal_status(dev);
>> +	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
>> +		if (pcie_aer_is_native(dev) && dev->rcec)
>> +			pcie_clear_device_status(dev->rcec);
>> +		if (dev->rcec)
>> +			pci_aer_clear_nonfatal_status(dev->rcec);
>>  	}
>> +
>>  	pci_info(dev, "device recovery successful\n");
>>  	return status;
>>
>> diff --git a/drivers/pci/pcie/portdrv_pci.c 
>> b/drivers/pci/pcie/portdrv_pci.c
>> index 4d880679b9b1..dff5c9e13412 100644
>> --- a/drivers/pci/pcie/portdrv_pci.c
>> +++ b/drivers/pci/pcie/portdrv_pci.c
>> @@ -90,6 +90,18 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops 
>> = {
>>  #define PCIE_PORTDRV_PM_OPS	NULL
>>  #endif /* !PM */
>>
>> +static int pcie_hook_rcec(struct pci_dev *pdev, void *data)
>> +{
>> +	struct pci_dev *rcec = (struct pci_dev *)data;
>> +
>> +	pdev->rcec = rcec;
>> +	pci_dbg(rcec, "RCiEP(under an RCEC) %04x:%02x:%02x.%d\n",
>> +		pci_domain_nr(pdev->bus), pdev->bus->number,
>> +		PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> +
>> +	return 0;
>> +}
>> +
>>  /*
>>   * pcie_portdrv_probe - Probe PCI-Express port devices
>>   * @dev: PCI-Express port device being probed
>> @@ -110,6 +122,9 @@ static int pcie_portdrv_probe(struct pci_dev 
>> *dev,
>>  	     (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
>>  		return -ENODEV;
>>
>> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
>> +		pcie_walk_rcec(dev, pcie_hook_rcec, dev);
>> +
>>  	status = pcie_port_device_register(dev);
>>  	if (status)
>>  		return status;
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index ee49469bd2b5..d5f7dbbf5e2f 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -326,6 +326,9 @@ struct pci_dev {
>>  #ifdef CONFIG_PCIEAER
>>  	u16		aer_cap;	/* AER capability offset */
>>  	struct aer_stats *aer_stats;	/* AER stats for this device */
>> +#endif
>> +#ifdef CONFIG_PCIEPORTBUS
>> +	struct pci_dev	*rcec;		/* Associated RCEC device */
>>  #endif
>>  	u8		pcie_cap;	/* PCIe capability offset */
>>  	u8		msi_cap;	/* MSI capability offset */

[PATCH V2 7/9] PCI/AER: Add RCEC AER handling

[Sean V Kelley]

The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
and also have the AER capability. So add RCEC support to the current AER
service driver and attach the AER service driver to the RCEC device.

Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
 drivers/pci/pcie/aer.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f658607e8e00..55ee9518368f 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -300,7 +300,7 @@ int pci_aer_raw_clear_status(struct pci_dev *dev)
 		return -EIO;
 
 	port_type = pci_pcie_type(dev);
-	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
+	if (port_type == PCI_EXP_TYPE_ROOT_PORT || port_type == PCI_EXP_TYPE_RC_EC) {
 		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &status);
 		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, status);
 	}
@@ -595,7 +595,8 @@ static umode_t aer_stats_attrs_are_visible(struct kobject *kobj,
 	if ((a == &dev_attr_aer_rootport_total_err_cor.attr ||
 	     a == &dev_attr_aer_rootport_total_err_fatal.attr ||
 	     a == &dev_attr_aer_rootport_total_err_nonfatal.attr) &&
-	    pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT)
+	    ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) &&
+	    (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_EC)))
 		return 0;
 
 	return a->mode;
@@ -916,7 +917,10 @@ static bool find_source_device(struct pci_dev *parent,
 	if (result)
 		return true;
 
-	pci_walk_bus(parent->subordinate, find_device_iter, e_info);
+	if (pci_pcie_type(parent) == PCI_EXP_TYPE_RC_EC)
+		pcie_walk_rcec(parent, find_device_iter, e_info);
+	else
+		pci_walk_bus(parent->subordinate, find_device_iter, e_info);
 
 	if (!e_info->error_dev_num) {
 		pci_info(parent, "can't find device of ID%04x\n", e_info->id);
@@ -1053,6 +1057,7 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 		if (!(info->status & ~info->mask))
 			return 0;
 	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+		   pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC ||
 	           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
 		   info->severity == AER_NONFATAL) {
 
@@ -1205,6 +1210,7 @@ static int set_device_error_reporting(struct pci_dev *dev, void *data)
 	int type = pci_pcie_type(dev);
 
 	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
+	    (type == PCI_EXP_TYPE_RC_EC) ||
 	    (type == PCI_EXP_TYPE_UPSTREAM) ||
 	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
 		if (enable)
@@ -1229,9 +1235,11 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev,
 {
 	set_device_error_reporting(dev, &enable);
 
-	if (!dev->subordinate)
-		return;
-	pci_walk_bus(dev->subordinate, set_device_error_reporting, &enable);
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
+		pcie_walk_rcec(dev, set_device_error_reporting, &enable);
+	else if (dev->subordinate)
+		pci_walk_bus(dev->subordinate, set_device_error_reporting, &enable);
+
 }
 
 /**
@@ -1329,6 +1337,11 @@ static int aer_probe(struct pcie_device *dev)
 	struct device *device = &dev->device;
 	struct pci_dev *port = dev->port;
 
+	/* Limit to Root Ports or Root Complex Event Collectors */
+	if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
+	    (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
+		return -ENODEV;
+
 	rpc = devm_kzalloc(device, sizeof(struct aer_rpc), GFP_KERNEL);
 	if (!rpc)
 		return -ENOMEM;
@@ -1385,7 +1398,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 
 static struct pcie_port_service_driver aerdriver = {
 	.name		= "aer",
-	.port_type	= PCI_EXP_TYPE_ROOT_PORT,
+	.port_type	= PCIE_ANY_PORT,
 	.service	= PCIE_PORT_SERVICE_AER,
 
 	.probe		= aer_probe,
-- 
2.27.0

Re: [PATCH V2 7/9] PCI/AER: Add RCEC AER handling

[Jonathan Cameron]

On Tue, 4 Aug 2020 12:40:50 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:

> The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
> and also have the AER capability. So add RCEC support to the current AER
> service driver and attach the AER service driver to the RCEC device.
> 
> Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pci/pcie/aer.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f658607e8e00..55ee9518368f 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -300,7 +300,7 @@ int pci_aer_raw_clear_status(struct pci_dev *dev)
>  		return -EIO;
>  
>  	port_type = pci_pcie_type(dev);
> -	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
> +	if (port_type == PCI_EXP_TYPE_ROOT_PORT || port_type == PCI_EXP_TYPE_RC_EC) {
>  		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &status);
>  		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, status);
>  	}
> @@ -595,7 +595,8 @@ static umode_t aer_stats_attrs_are_visible(struct kobject *kobj,
>  	if ((a == &dev_attr_aer_rootport_total_err_cor.attr ||
>  	     a == &dev_attr_aer_rootport_total_err_fatal.attr ||
>  	     a == &dev_attr_aer_rootport_total_err_nonfatal.attr) &&
> -	    pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT)
> +	    ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) &&
> +	    (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_EC)))
>  		return 0;
>  
>  	return a->mode;
> @@ -916,7 +917,10 @@ static bool find_source_device(struct pci_dev *parent,
>  	if (result)
>  		return true;
>  
> -	pci_walk_bus(parent->subordinate, find_device_iter, e_info);
> +	if (pci_pcie_type(parent) == PCI_EXP_TYPE_RC_EC)
> +		pcie_walk_rcec(parent, find_device_iter, e_info);
> +	else
> +		pci_walk_bus(parent->subordinate, find_device_iter, e_info);
>  
>  	if (!e_info->error_dev_num) {
>  		pci_info(parent, "can't find device of ID%04x\n", e_info->id);
> @@ -1053,6 +1057,7 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
>  		if (!(info->status & ~info->mask))
>  			return 0;
>  	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +		   pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC ||
>  	           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>  		   info->severity == AER_NONFATAL) {
>  
> @@ -1205,6 +1210,7 @@ static int set_device_error_reporting(struct pci_dev *dev, void *data)
>  	int type = pci_pcie_type(dev);
>  
>  	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
> +	    (type == PCI_EXP_TYPE_RC_EC) ||
>  	    (type == PCI_EXP_TYPE_UPSTREAM) ||
>  	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
>  		if (enable)
> @@ -1229,9 +1235,11 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev,
>  {
>  	set_device_error_reporting(dev, &enable);
>  
> -	if (!dev->subordinate)
> -		return;
> -	pci_walk_bus(dev->subordinate, set_device_error_reporting, &enable);
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> +		pcie_walk_rcec(dev, set_device_error_reporting, &enable);
> +	else if (dev->subordinate)
> +		pci_walk_bus(dev->subordinate, set_device_error_reporting, &enable);
> +
>  }
>  
>  /**
> @@ -1329,6 +1337,11 @@ static int aer_probe(struct pcie_device *dev)
>  	struct device *device = &dev->device;
>  	struct pci_dev *port = dev->port;
>  
> +	/* Limit to Root Ports or Root Complex Event Collectors */
> +	if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
> +	    (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
> +		return -ENODEV;
> +
>  	rpc = devm_kzalloc(device, sizeof(struct aer_rpc), GFP_KERNEL);
>  	if (!rpc)
>  		return -ENOMEM;
> @@ -1385,7 +1398,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>  
>  static struct pcie_port_service_driver aerdriver = {
>  	.name		= "aer",
> -	.port_type	= PCI_EXP_TYPE_ROOT_PORT,
> +	.port_type	= PCIE_ANY_PORT,
>  	.service	= PCIE_PORT_SERVICE_AER,
>  
>  	.probe		= aer_probe,

[PATCH V2 8/9] PCI/PME: Add RCEC PME handling

[Sean V Kelley]

The Root Complex Event Collectors(RCEC) appear as peers of Root Ports
and also have the PME capability. So add RCEC support to the current PME
service driver and attach the PME service driver to the RCEC device.

Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
 drivers/pci/pcie/pme.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 6a32970bb731..87799166c96a 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -310,7 +310,10 @@ static int pcie_pme_can_wakeup(struct pci_dev *dev, void *ign)
 static void pcie_pme_mark_devices(struct pci_dev *port)
 {
 	pcie_pme_can_wakeup(port, NULL);
-	if (port->subordinate)
+
+	if (pci_pcie_type(port) == PCI_EXP_TYPE_RC_EC)
+		pcie_walk_rcec(port, pcie_pme_can_wakeup, NULL);
+	else if (port->subordinate)
 		pci_walk_bus(port->subordinate, pcie_pme_can_wakeup, NULL);
 }
 
@@ -320,10 +323,15 @@ static void pcie_pme_mark_devices(struct pci_dev *port)
  */
 static int pcie_pme_probe(struct pcie_device *srv)
 {
-	struct pci_dev *port;
+	struct pci_dev *port = srv->port;
 	struct pcie_pme_service_data *data;
 	int ret;
 
+	/* Limit to Root Ports or Root Complex Event Collectors */
+	if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
+	    (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
+		return -ENODEV;
+
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -333,7 +341,6 @@ static int pcie_pme_probe(struct pcie_device *srv)
 	data->srv = srv;
 	set_service_data(srv, data);
 
-	port = srv->port;
 	pcie_pme_interrupt_enable(port, false);
 	pcie_clear_root_pme_status(port);
 
@@ -445,7 +452,7 @@ static void pcie_pme_remove(struct pcie_device *srv)
 
 static struct pcie_port_service_driver pcie_pme_driver = {
 	.name		= "pcie_pme",
-	.port_type	= PCI_EXP_TYPE_ROOT_PORT,
+	.port_type	= PCIE_ANY_PORT,
 	.service	= PCIE_PORT_SERVICE_PME,
 
 	.probe		= pcie_pme_probe,
-- 
2.27.0

Re: [PATCH V2 8/9] PCI/PME: Add RCEC PME handling

[Jonathan Cameron]

On Tue, 4 Aug 2020 12:40:51 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:

> The Root Complex Event Collectors(RCEC) appear as peers of Root Ports
> and also have the PME capability. So add RCEC support to the current PME
> service driver and attach the PME service driver to the RCEC device.
> 
> Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

I'm not particularly familiar with the PME side of things, but from the
stand point of it does what I'd expect to see...

FWIW:
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pci/pcie/pme.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> index 6a32970bb731..87799166c96a 100644
> --- a/drivers/pci/pcie/pme.c
> +++ b/drivers/pci/pcie/pme.c
> @@ -310,7 +310,10 @@ static int pcie_pme_can_wakeup(struct pci_dev *dev, void *ign)
>  static void pcie_pme_mark_devices(struct pci_dev *port)
>  {
>  	pcie_pme_can_wakeup(port, NULL);
> -	if (port->subordinate)
> +
> +	if (pci_pcie_type(port) == PCI_EXP_TYPE_RC_EC)
> +		pcie_walk_rcec(port, pcie_pme_can_wakeup, NULL);
> +	else if (port->subordinate)
>  		pci_walk_bus(port->subordinate, pcie_pme_can_wakeup, NULL);
>  }
>  
> @@ -320,10 +323,15 @@ static void pcie_pme_mark_devices(struct pci_dev *port)
>   */
>  static int pcie_pme_probe(struct pcie_device *srv)
>  {
> -	struct pci_dev *port;
> +	struct pci_dev *port = srv->port;
>  	struct pcie_pme_service_data *data;
>  	int ret;
>  
> +	/* Limit to Root Ports or Root Complex Event Collectors */
> +	if ((pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC) &&
> +	    (pci_pcie_type(port) != PCI_EXP_TYPE_ROOT_PORT))
> +		return -ENODEV;
> +
>  	data = kzalloc(sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
> @@ -333,7 +341,6 @@ static int pcie_pme_probe(struct pcie_device *srv)
>  	data->srv = srv;
>  	set_service_data(srv, data);
>  
> -	port = srv->port;
>  	pcie_pme_interrupt_enable(port, false);
>  	pcie_clear_root_pme_status(port);
>  
> @@ -445,7 +452,7 @@ static void pcie_pme_remove(struct pcie_device *srv)
>  
>  static struct pcie_port_service_driver pcie_pme_driver = {
>  	.name		= "pcie_pme",
> -	.port_type	= PCI_EXP_TYPE_ROOT_PORT,
> +	.port_type	= PCIE_ANY_PORT,
>  	.service	= PCIE_PORT_SERVICE_PME,
>  
>  	.probe		= pcie_pme_probe,

[PATCH V2 9/9] PCI/AER: Add RCEC AER error injection support

[Sean V Kelley]

From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
and also have the AER capability. So add RCEC support to the current AER
error injection driver.

Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
 drivers/pci/pcie/aer_inject.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
index c2cbf425afc5..2077dc826fdf 100644
--- a/drivers/pci/pcie/aer_inject.c
+++ b/drivers/pci/pcie/aer_inject.c
@@ -333,8 +333,11 @@ static int aer_inject(struct aer_error_inj *einj)
 	if (!dev)
 		return -ENODEV;
 	rpdev = pcie_find_root_port(dev);
+	/* If Root port not found, try to find an RCEC */
+	if (!rpdev)
+		rpdev = dev->rcec;
 	if (!rpdev) {
-		pci_err(dev, "Root port not found\n");
+		pci_err(dev, "Root port or RCEC not found\n");
 		ret = -ENODEV;
 		goto out_put;
 	}
-- 
2.27.0

Re: [PATCH V2 9/9] PCI/AER: Add RCEC AER error injection support

[Jonathan Cameron]

On Tue, 4 Aug 2020 12:40:52 -0700
Sean V Kelley <sean.v.kelley@intel.com> wrote:

> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
> and also have the AER capability. So add RCEC support to the current AER
> error injection driver.
> 
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>

Silly English subtlety inline.

> ---
>  drivers/pci/pcie/aer_inject.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
> index c2cbf425afc5..2077dc826fdf 100644
> --- a/drivers/pci/pcie/aer_inject.c
> +++ b/drivers/pci/pcie/aer_inject.c
> @@ -333,8 +333,11 @@ static int aer_inject(struct aer_error_inj *einj)
>  	if (!dev)
>  		return -ENODEV;
>  	rpdev = pcie_find_root_port(dev);
> +	/* If Root port not found, try to find an RCEC */
> +	if (!rpdev)
> +		rpdev = dev->rcec;
>  	if (!rpdev) {
> -		pci_err(dev, "Root port not found\n");
> +		pci_err(dev, "Root port or RCEC not found\n");

That is a bit confusing, could be

RP | !RCEC

"Neither root port nor RCEC found\n" perhaps?


>  		ret = -ENODEV;
>  		goto out_put;
>  	}

Re: [PATCH V2 9/9] PCI/AER: Add RCEC AER error injection support

[Sean V Kelley]

On 5 Aug 2020, at 10:54, Jonathan Cameron wrote:

> On Tue, 4 Aug 2020 12:40:52 -0700
> Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
>> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>
>> The Root Complex Event Collectors(RCEC) appear as peers to Root Ports
>> and also have the AER capability. So add RCEC support to the current AER
>> error injection driver.
>>
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>
> Silly English subtlety inline.
>
>> ---
>>  drivers/pci/pcie/aer_inject.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
>> index c2cbf425afc5..2077dc826fdf 100644
>> --- a/drivers/pci/pcie/aer_inject.c
>> +++ b/drivers/pci/pcie/aer_inject.c
>> @@ -333,8 +333,11 @@ static int aer_inject(struct aer_error_inj *einj)
>>  	if (!dev)
>>  		return -ENODEV;
>>  	rpdev = pcie_find_root_port(dev);
>> +	/* If Root port not found, try to find an RCEC */
>> +	if (!rpdev)
>> +		rpdev = dev->rcec;
>>  	if (!rpdev) {
>> -		pci_err(dev, "Root port not found\n");
>> +		pci_err(dev, "Root port or RCEC not found\n");
>
> That is a bit confusing, could be
>
> RP | !RCEC
>
> "Neither root port nor RCEC found\n" perhaps?

Sounds good to me. Will correct.

Thanks,

Sean

>
>
>>  		ret = -ENODEV;
>>  		goto out_put;
>>  	}

Re: [PATCH V2 0/9] Add RCEC handling to PCI/AER

[Bjorn Helgaas]

On Tue, Aug 04, 2020 at 12:40:43PM -0700, Sean V Kelley wrote:
> From: Sean V Kelley <sean.v.kelley@linux.intel.com>
> 
> On the use of FLR on RCiEPs for the fatal case, still interested in more
> feedback from the earlier discussion here [1]:
> 
> [1] https://lore.kernel.org/linux-pci/C21C050B-48B1-4429-B019-C81F3AB8E843@intel.com/
> 
> There is also the question of the absence of an FLR for non-fatal error.
> If the device driver tells us that it needs "PCI_ERS_RESULT_NEED_RESET" by
> the callback report_normal_detected() then we should try FLR on the device
> as well.
> 
> On the use of variables with RP centric names such as the attributes
> dev_attr_aer_rootport_total_err_..., one concern is the ripple effect on code
> churn due to renaming. Open to suggestions, but trying to co-habitate so to
> speak RCECs with RPs in the same drivers has trade-offs.
> 
> Changes since v1 [2]:
> 
> - Make PME capability of RCEC discoverable in get_port_device_capability().
> - Replace the check on bnr with <= lastbusn in pcie_walk_rcec().
> - Fix comment header for pcie_walk_rcec().
> - Fix comment header for pci_walk_dev_affected().
> - Fix spurious newline.
> - Add sanity checks on dev->rcec.
> - Use pci_dbg() in place of pci_info() for discovered RCiEPs.
> - Remove AER RCEC AP FOUND message (accidently left in previously).
> - Remove the check for RC_END from set_device_error_reporting() since
> only Ports and RCECs are being passed.
> (Jonathan Cameron)
> - Fix the return type for flr_on_rciep().
> (reported by lkp on DEC Alpha arch.)
> 
> [1] https://lore.kernel.org/linux-pci/20200724172223.145608-1-sean.v.kelley@intel.com/
> 
> Root Complex Event Collectors (RCEC) provide support for terminating error
> and PME messages from Root Complex Integrated Endpoints (RCiEPs).  An RCEC
> resides on a Bus in the Root Complex. Multiple RCECs can in fact reside on
> a single bus. An RCEC will explicitly declare supported RCiEPs through the
> Root Complex Endpoint Association Extended Capability.
> 
> (See PCIe 5.0-1, sections 1.3.2.3 (RCiEP), and 7.9.10 (RCEC Ext. Cap.))
> 
> The kernel lacks handling for these RCECs and the error messages received
> from their respective associated RCiEPs. More recently, a new CPU
> interconnect, Compute eXpress Link (CXL) depends on RCEC capabilities for
> purposes of error messaging from CXL 1.1 supported RCiEP devices.
> 
> DocLink: https://www.computeexpresslink.org/
> 
> This use case is not limited to CXL. Existing hardware today includes
> support for RCECs, such as the Denverton microserver product
> family. Future hardware will be forthcoming.
> 
> (See Intel Document, Order number: 33061-003US)
> 
> So services such as AER or PME could be associated with an RCEC driver.
> In the case of CXL, if an RCiEP (i.e., CXL 1.1 device) is associated with a
> platform's RCEC it shall signal PME and AER error conditions through that
> RCEC.
> 
> Towards the above use cases, add the missing RCEC class and extend the
> PCIe Root Port and service drivers to allow association of RCiEPs to their
> respective parent RCEC and facilitate handling of terminating error and PME
> messages.
> 
> 
> AER Test Results:
> 1) Inject a correctable error to the RCiEP 0000:e9:00.0
>     Run ./aer_inject <a parameter file as below>:
>     AER
>     PCI_ID 0000:e9:00.0
>     COR_STATUS BAD_TLP
>     HEADER_LOG 0 1 2 3
> 
>     Log:
> [   76.155963] pcieport 0000:e8:00.4: aer_inject: Injecting errors 00000040/00000000 into device 0000:e9:00.0
> [   76.166966] pcieport 0000:e8:00.4: AER: Corrected error received: 0000:e9:00.0
> [   76.175253] pci 0000:e9:00.0: PCIe Bus Error: severity=Corrected, type=Data Link Layer, (Receiver ID)
> [   76.185633] pci 0000:e9:00.0:   device [8086:4940] error status/mask=00000040/00002000
> [   76.194604] pci 0000:e9:00.0:    [ 6] BadTLP

If you remove the timestamps, there will be less distraction here.  As
I'm sure you know, the 0/n cover letter text doesn't really go
anywhere except the email archives.  If this is potentially useful in
the future, it should be in the actual patch commit logs.

> 2) Inject a non-fatal error to the RCiEP 0000:e8:01.0
>     Run ./aer_inject <a parameter file as below>:
>     AER
>     PCI_ID 0000:e8:01.0
>     UNCOR_STATUS COMP_ABORT
>     HEADER_LOG 0 1 2 3

I think maybe this could be written in a way that could be cut and
pasted?

>     Log:
> [  117.791854] pcieport 0000:e8:00.4: aer_inject: Injecting errors 00000000/00008000 into device 0000:e8:01.0
> [  117.804244] pcieport 0000:e8:00.4: AER: Uncorrected (Non-Fatal) error received: 0000:e8:01.0
> [  117.814652] igen6_edac 0000:e8:01.0: PCIe Bus Error: severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Completer ID)
> [  117.828511] igen6_edac 0000:e8:01.0:   device [8086:0b25] error status/mask=00008000/00100000
> [  117.839189] igen6_edac 0000:e8:01.0:    [15] CmpltAbrt
> [  117.847365] igen6_edac 0000:e8:01.0: AER:   TLP Header: 00000000 00000001 00000002 00000003
> [  117.857775] igen6_edac 0000:e8:01.0: AER: device recovery successful
> 
> 3) Inject a fatal error to the RCiEP 0000:ed:01.0
>     Run ./aer_inject <a parameter file as below>:
>     AER
>     PCI_ID 0000:ed:01.0
>     UNCOR_STATUS MALF_TLP
>     HEADER_LOG 0 1 2 3
> 
>     Log:
> [  131.511623] pcieport 0000:ed:00.4: aer_inject: Injecting errors 00000000/00040000 into device 0000:ed:01.0
> [  131.523259] pcieport 0000:ed:00.4: AER: Uncorrected (Fatal) error received: 0000:ed:01.0
> [  131.533842] igen6_edac 0000:ed:01.0: AER: PCIe Bus Error: severity=Uncorrected (Fatal), type=Inaccessible, (Unregistered Agent ID)
> [  131.655618] igen6_edac 0000:ed:01.0: AER: device recovery successful
> 
> Jonathan Cameron (1):
>   PCI/AER: Extend AER error handling to RCECs
> 
> Qiuxu Zhuo (6):
>   pci_ids: Add class code and extended capability for RCEC
>   PCI: Extend Root Port Driver to support RCEC
>   PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with RCEC
>   PCI/AER: Apply function level reset to RCiEP on fatal error
>   PCI: Add 'rcec' field to pci_dev for associated RCiEPs
>   PCI/AER: Add RCEC AER error injection support
> 
> Sean V Kelley (2):
>   PCI/AER: Add RCEC AER handling
>   PCI/PME: Add RCEC PME handling
> 

>  drivers/pci/pcie/aer.c          | 36 +++++++++----
>  drivers/pci/pcie/aer_inject.c   |  5 +-
>  drivers/pci/pcie/err.c          | 90 +++++++++++++++++++++++++++------
>  drivers/pci/pcie/pme.c          | 15 ++++--
>  drivers/pci/pcie/portdrv.h      |  2 +
>  drivers/pci/pcie/portdrv_core.c | 90 +++++++++++++++++++++++++++++++--
>  drivers/pci/pcie/portdrv_pci.c  | 20 +++++++-
>  include/linux/pci.h             |  3 ++
>  include/linux/pci_ids.h         |  1 +
>  include/uapi/linux/pci_regs.h   |  7 +++
>  10 files changed, 233 insertions(+), 36 deletions(-)

I always apply patches to topic branches based at my "master" branch
(typically -rc1, so currently v5.8-rc1, but will soon be v5.9-rc1).

If your series doesn't apply there (as this one doesn't), it saves me
time if you tell me where it does apply.  I figured out that this
applies cleanly on top of my pci/error branch, which does make sense.

I'd actually *rather* have patches based on "master", even if I have
to resolve conflicts, because that gives me the flexibility to squash
in fixes and re-merge the topic branches.

Bjorn

Re: [PATCH V2 0/9] Add RCEC handling to PCI/AER

[Sean V Kelley]

On 5 Aug 2020, at 11:00, Bjorn Helgaas wrote:

> On Tue, Aug 04, 2020 at 12:40:43PM -0700, Sean V Kelley wrote:
>> From: Sean V Kelley <sean.v.kelley@linux.intel.com>
>>
>> On the use of FLR on RCiEPs for the fatal case, still interested in 
>> more
>> feedback from the earlier discussion here [1]:
>>
>> [1] 
>> https://lore.kernel.org/linux-pci/C21C050B-48B1-4429-B019-C81F3AB8E843@intel.com/
>>
>> There is also the question of the absence of an FLR for non-fatal 
>> error.
>> If the device driver tells us that it needs 
>> "PCI_ERS_RESULT_NEED_RESET" by
>> the callback report_normal_detected() then we should try FLR on the 
>> device
>> as well.
>>
>> On the use of variables with RP centric names such as the attributes
>> dev_attr_aer_rootport_total_err_..., one concern is the ripple effect 
>> on code
>> churn due to renaming. Open to suggestions, but trying to co-habitate 
>> so to
>> speak RCECs with RPs in the same drivers has trade-offs.
>>
>> Changes since v1 [2]:
>>
>> - Make PME capability of RCEC discoverable in 
>> get_port_device_capability().
>> - Replace the check on bnr with <= lastbusn in pcie_walk_rcec().
>> - Fix comment header for pcie_walk_rcec().
>> - Fix comment header for pci_walk_dev_affected().
>> - Fix spurious newline.
>> - Add sanity checks on dev->rcec.
>> - Use pci_dbg() in place of pci_info() for discovered RCiEPs.
>> - Remove AER RCEC AP FOUND message (accidently left in previously).
>> - Remove the check for RC_END from set_device_error_reporting() since
>> only Ports and RCECs are being passed.
>> (Jonathan Cameron)
>> - Fix the return type for flr_on_rciep().
>> (reported by lkp on DEC Alpha arch.)
>>
>> [1] 
>> https://lore.kernel.org/linux-pci/20200724172223.145608-1-sean.v.kelley@intel.com/
>>
>> Root Complex Event Collectors (RCEC) provide support for terminating 
>> error
>> and PME messages from Root Complex Integrated Endpoints (RCiEPs).  An 
>> RCEC
>> resides on a Bus in the Root Complex. Multiple RCECs can in fact 
>> reside on
>> a single bus. An RCEC will explicitly declare supported RCiEPs 
>> through the
>> Root Complex Endpoint Association Extended Capability.
>>
>> (See PCIe 5.0-1, sections 1.3.2.3 (RCiEP), and 7.9.10 (RCEC Ext. 
>> Cap.))
>>
>> The kernel lacks handling for these RCECs and the error messages 
>> received
>> from their respective associated RCiEPs. More recently, a new CPU
>> interconnect, Compute eXpress Link (CXL) depends on RCEC capabilities 
>> for
>> purposes of error messaging from CXL 1.1 supported RCiEP devices.
>>
>> DocLink: https://www.computeexpresslink.org/
>>
>> This use case is not limited to CXL. Existing hardware today includes
>> support for RCECs, such as the Denverton microserver product
>> family. Future hardware will be forthcoming.
>>
>> (See Intel Document, Order number: 33061-003US)
>>
>> So services such as AER or PME could be associated with an RCEC 
>> driver.
>> In the case of CXL, if an RCiEP (i.e., CXL 1.1 device) is associated 
>> with a
>> platform's RCEC it shall signal PME and AER error conditions through 
>> that
>> RCEC.
>>
>> Towards the above use cases, add the missing RCEC class and extend 
>> the
>> PCIe Root Port and service drivers to allow association of RCiEPs to 
>> their
>> respective parent RCEC and facilitate handling of terminating error 
>> and PME
>> messages.
>>
>>
>> AER Test Results:
>> 1) Inject a correctable error to the RCiEP 0000:e9:00.0
>>     Run ./aer_inject <a parameter file as below>:
>>     AER
>>     PCI_ID 0000:e9:00.0
>>     COR_STATUS BAD_TLP
>>     HEADER_LOG 0 1 2 3
>>
>>     Log:
>> [   76.155963] pcieport 0000:e8:00.4: aer_inject: Injecting errors 
>> 00000040/00000000 into device 0000:e9:00.0
>> [   76.166966] pcieport 0000:e8:00.4: AER: Corrected error received: 
>> 0000:e9:00.0
>> [   76.175253] pci 0000:e9:00.0: PCIe Bus Error: severity=Corrected, 
>> type=Data Link Layer, (Receiver ID)
>> [   76.185633] pci 0000:e9:00.0:   device [8086:4940] error 
>> status/mask=00000040/00002000
>> [   76.194604] pci 0000:e9:00.0:    [ 6] BadTLP
>
> If you remove the timestamps, there will be less distraction here.  As
> I'm sure you know, the 0/n cover letter text doesn't really go
> anywhere except the email archives.  If this is potentially useful in
> the future, it should be in the actual patch commit logs.

Right, the intent is to show how it was tested.  Understood.

>
>> 2) Inject a non-fatal error to the RCiEP 0000:e8:01.0
>>     Run ./aer_inject <a parameter file as below>:
>>     AER
>>     PCI_ID 0000:e8:01.0
>>     UNCOR_STATUS COMP_ABORT
>>     HEADER_LOG 0 1 2 3
>
> I think maybe this could be written in a way that could be cut and
> pasted?

Good point.  Will do.

>
>>     Log:
>> [  117.791854] pcieport 0000:e8:00.4: aer_inject: Injecting errors 
>> 00000000/00008000 into device 0000:e8:01.0
>> [  117.804244] pcieport 0000:e8:00.4: AER: Uncorrected (Non-Fatal) 
>> error received: 0000:e8:01.0
>> [  117.814652] igen6_edac 0000:e8:01.0: PCIe Bus Error: 
>> severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Completer 
>> ID)
>> [  117.828511] igen6_edac 0000:e8:01.0:   device [8086:0b25] error 
>> status/mask=00008000/00100000
>> [  117.839189] igen6_edac 0000:e8:01.0:    [15] CmpltAbrt
>> [  117.847365] igen6_edac 0000:e8:01.0: AER:   TLP Header: 00000000 
>> 00000001 00000002 00000003
>> [  117.857775] igen6_edac 0000:e8:01.0: AER: device recovery 
>> successful
>>
>> 3) Inject a fatal error to the RCiEP 0000:ed:01.0
>>     Run ./aer_inject <a parameter file as below>:
>>     AER
>>     PCI_ID 0000:ed:01.0
>>     UNCOR_STATUS MALF_TLP
>>     HEADER_LOG 0 1 2 3
>>
>>     Log:
>> [  131.511623] pcieport 0000:ed:00.4: aer_inject: Injecting errors 
>> 00000000/00040000 into device 0000:ed:01.0
>> [  131.523259] pcieport 0000:ed:00.4: AER: Uncorrected (Fatal) error 
>> received: 0000:ed:01.0
>> [  131.533842] igen6_edac 0000:ed:01.0: AER: PCIe Bus Error: 
>> severity=Uncorrected (Fatal), type=Inaccessible, (Unregistered Agent 
>> ID)
>> [  131.655618] igen6_edac 0000:ed:01.0: AER: device recovery 
>> successful
>>
>> Jonathan Cameron (1):
>>   PCI/AER: Extend AER error handling to RCECs
>>
>> Qiuxu Zhuo (6):
>>   pci_ids: Add class code and extended capability for RCEC
>>   PCI: Extend Root Port Driver to support RCEC
>>   PCI/portdrv: Add pcie_walk_rcec() to walk RCiEPs associated with 
>> RCEC
>>   PCI/AER: Apply function level reset to RCiEP on fatal error
>>   PCI: Add 'rcec' field to pci_dev for associated RCiEPs
>>   PCI/AER: Add RCEC AER error injection support
>>
>> Sean V Kelley (2):
>>   PCI/AER: Add RCEC AER handling
>>   PCI/PME: Add RCEC PME handling
>>
>
>>  drivers/pci/pcie/aer.c          | 36 +++++++++----
>>  drivers/pci/pcie/aer_inject.c   |  5 +-
>>  drivers/pci/pcie/err.c          | 90 
>> +++++++++++++++++++++++++++------
>>  drivers/pci/pcie/pme.c          | 15 ++++--
>>  drivers/pci/pcie/portdrv.h      |  2 +
>>  drivers/pci/pcie/portdrv_core.c | 90 
>> +++++++++++++++++++++++++++++++--
>>  drivers/pci/pcie/portdrv_pci.c  | 20 +++++++-
>>  include/linux/pci.h             |  3 ++
>>  include/linux/pci_ids.h         |  1 +
>>  include/uapi/linux/pci_regs.h   |  7 +++
>>  10 files changed, 233 insertions(+), 36 deletions(-)
>
> I always apply patches to topic branches based at my "master" branch
> (typically -rc1, so currently v5.8-rc1, but will soon be v5.9-rc1).
>
> If your series doesn't apply there (as this one doesn't), it saves me
> time if you tell me where it does apply.  I figured out that this
> applies cleanly on top of my pci/error branch, which does make sense.
>
> I'd actually *rather* have patches based on "master", even if I have
> to resolve conflicts, because that gives me the flexibility to squash
> in fixes and re-merge the topic branches.

Okay. Good to know as this time it was put on your pci branch. Will 
switch back to master.

Thanks,

Sean

>
> Bjorn