[PATCH v12 00/15] Add RCEC handling to PCI/AER

[PATCH v12 00/15] Add RCEC handling to PCI/AER

[Sean V Kelley]

Changes since v11 [1] and based on pci/master tree [2]:

- No functional changes. Tested with aer injection.

- Merge RCEC class code and extended capability patch with usage.
- Apply same optimization for pci_pcie_type(dev) calls in
drivers/pci/pcie/portdrv_pci.c and drivers/pci/pcie/aer.c.
(Kuppuswamy Sathyanarayanan)

[1] https://lore.kernel.org/linux-pci/20201117191954.1322844-1-sean.v.kelley@intel.com/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/


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.

Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> #non-native/no RCEC


Qiuxu Zhuo (3):
  PCI/RCEC: Bind RCEC devices to the Root Port driver
  PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR
  PCI/AER: Add RCEC AER error injection support

Sean V Kelley (12):
  AER: aer_root_reset() non-native handling
  PCI/RCEC: Cache RCEC capabilities in pci_init_capabilities()
  PCI/ERR: Rename reset_link() to reset_subordinates()
  PCI/ERR: Simplify by using pci_upstream_bridge()
  PCI/ERR: Simplify by computing pci_pcie_type() once
  PCI/ERR: Use "bridge" for clarity in pcie_do_recovery()
  PCI/ERR: Avoid negated conditional for clarity
  PCI/ERR: Add pci_walk_bridge() to pcie_do_recovery()
  PCI/ERR: Limit AER resets in pcie_do_recovery()
  PCI/RCEC: Add pcie_link_rcec() to associate RCiEPs
  PCI/AER: Add pcie_walk_rcec() to RCEC AER handling
  PCI/PME: Add pcie_walk_rcec() to RCEC PME handling

 drivers/pci/pci.h               |  29 ++++-
 drivers/pci/pcie/Makefile       |   2 +-
 drivers/pci/pcie/aer.c          |  89 +++++++++++----
 drivers/pci/pcie/aer_inject.c   |   5 +-
 drivers/pci/pcie/err.c          |  93 +++++++++++-----
 drivers/pci/pcie/pme.c          |  16 ++-
 drivers/pci/pcie/portdrv_core.c |   9 +-
 drivers/pci/pcie/portdrv_pci.c  |  13 ++-
 drivers/pci/pcie/rcec.c         | 190 ++++++++++++++++++++++++++++++++
 drivers/pci/probe.c             |   2 +
 include/linux/pci.h             |   5 +
 include/linux/pci_ids.h         |   1 +
 include/uapi/linux/pci_regs.h   |   7 ++
 13 files changed, 393 insertions(+), 68 deletions(-)
 create mode 100644 drivers/pci/pcie/rcec.c

--
2.29.2

[PATCH v12 01/15] AER: aer_root_reset() non-native handling

[Sean V Kelley]

If an OS has not been granted AER control via _OSC, then
the OS should not make changes to PCI_ERR_ROOT_COMMAND and
PCI_ERR_ROOT_STATUS related registers. Per section 4.5.1 of
the System Firmware Intermediary (SFI) _OSC and DPC Updates
ECN [1], this bit also covers these aspects of the PCI
Express Advanced Error Reporting. Based on the above and
earlier discussion [2], make the following changes:

Add a check for the native case (i.e., AER control via _OSC)

Note that the current "clear, reset, enable" order suggests that the
reset might cause errors that we should ignore. Lacking historical
context, these will be retained.

[1] System Firmware Intermediary (SFI) _OSC and DPC Updates ECN, Feb 24,
    2020, affecting PCI Firmware Specification, Rev. 3.2
    https://members.pcisig.com/wg/PCI-SIG/document/14076
[2] https://lore.kernel.org/linux-pci/20201020162820.GA370938@bjorn-Precision-5520/

Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
 drivers/pci/pcie/aer.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 65dff5f3457a..6fe2d4ef0635 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1361,23 +1361,26 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 	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);
+	if (pcie_aer_is_native(dev)) {
+		/* 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");
+	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
 
-	/* Clear Root Error Status */
-	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
-	pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
+	if (pcie_aer_is_native(dev)) {
+		/* Clear Root Error Status */
+		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
+		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
 
-	/* Enable Root Port'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);
+		/* Enable Root Port'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);
+	}
 
 	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
 }
-- 
2.29.2

[PATCH v12 02/15] PCI/RCEC: Bind RCEC devices to the Root Port driver

[Sean V Kelley]

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

If a Root Complex Integrated Endpoint (RCiEP) is implemented, it may signal
errors through a Root Complex Event Collector (RCEC).  Each RCiEP must be
associated with no more than one RCEC.

For an RCEC (which is 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
by adding the RCEC Class ID to the driver structure.

Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Link: https://lore.kernel.org/r/20201002184735.1229220-3-seanvk.dev@oregontracks.org
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/portdrv_pci.c | 5 ++++-
 include/linux/pci_ids.h        | 1 +
 include/uapi/linux/pci_regs.h  | 7 +++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

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) },
 	{ },
 };
 
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 1ab1e24bcbce..d8156a5dbee8 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 a95d55f9f257..bccd3e35cb65 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -831,6 +831,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 version with 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.29.2

[PATCH v12 03/15] PCI/RCEC: Cache RCEC capabilities in pci_init_capabilities()

[Sean V Kelley]

Extend support for Root Complex Event Collectors by decoding and caching
the RCEC Endpoint Association Extended Capabilities when enumerating. Use
that cached information for later error source reporting. See PCIe r5.0,
sec 7.9.10.

[bhelgaas: make pci_rcec_init() void, set dev->rcec_ea after filling it]
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Link: https://lore.kernel.org/r/20201002184735.1229220-4-seanvk.dev@oregontracks.org
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/pci.h         | 17 +++++++++++
 drivers/pci/pcie/Makefile |  2 +-
 drivers/pci/pcie/rcec.c   | 59 +++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c       |  2 ++
 include/linux/pci.h       |  4 +++
 5 files changed, 83 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/pcie/rcec.c

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f86cae9aa1f4..12dcad8f3635 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -448,6 +448,15 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
 #endif	/* CONFIG_PCIEAER */
 
+#ifdef CONFIG_PCIEPORTBUS
+/* Cached RCEC Endpoint Association */
+struct rcec_ea {
+	u8		nextbusn;
+	u8		lastbusn;
+	u32		bitmap;
+};
+#endif
+
 #ifdef CONFIG_PCIE_DPC
 void pci_save_dpc_state(struct pci_dev *dev);
 void pci_restore_dpc_state(struct pci_dev *dev);
@@ -460,6 +469,14 @@ static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
 static inline void pci_dpc_init(struct pci_dev *pdev) {}
 #endif
 
+#ifdef CONFIG_PCIEPORTBUS
+void pci_rcec_init(struct pci_dev *dev);
+void pci_rcec_exit(struct pci_dev *dev);
+#else
+static inline void pci_rcec_init(struct pci_dev *dev) {}
+static inline void pci_rcec_exit(struct pci_dev *dev) {}
+#endif
+
 #ifdef CONFIG_PCI_ATS
 /* Address Translation Service */
 void pci_ats_init(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 68da9280ff11..d9697892fa3e 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -2,7 +2,7 @@
 #
 # Makefile for PCI Express features and port driver
 
-pcieportdrv-y			:= portdrv_core.o portdrv_pci.o err.o
+pcieportdrv-y			:= portdrv_core.o portdrv_pci.o err.o rcec.o
 
 obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o
 
diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
new file mode 100644
index 000000000000..038e9d706d5f
--- /dev/null
+++ b/drivers/pci/pcie/rcec.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Root Complex Event Collector Support
+ *
+ * Authors:
+ *  Sean V Kelley <sean.v.kelley@intel.com>
+ *  Qiuxu Zhuo <qiuxu.zhuo@intel.com>
+ *
+ * Copyright (C) 2020 Intel Corp.
+ */
+
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci_regs.h>
+
+#include "../pci.h"
+
+void pci_rcec_init(struct pci_dev *dev)
+{
+	struct rcec_ea *rcec_ea;
+	u32 rcec, hdr, busn;
+	u8 ver;
+
+	/* Only for Root Complex Event Collectors */
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)
+		return;
+
+	rcec = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_RCEC);
+	if (!rcec)
+		return;
+
+	rcec_ea = kzalloc(sizeof(*rcec_ea), GFP_KERNEL);
+	if (!rcec_ea)
+		return;
+
+	pci_read_config_dword(dev, rcec + PCI_RCEC_RCIEP_BITMAP,
+			      &rcec_ea->bitmap);
+
+	/* Check whether RCEC BUSN register is present */
+	pci_read_config_dword(dev, rcec, &hdr);
+	ver = PCI_EXT_CAP_VER(hdr);
+	if (ver >= PCI_RCEC_BUSN_REG_VER) {
+		pci_read_config_dword(dev, rcec + PCI_RCEC_BUSN, &busn);
+		rcec_ea->nextbusn = PCI_RCEC_BUSN_NEXT(busn);
+		rcec_ea->lastbusn = PCI_RCEC_BUSN_LAST(busn);
+	} else {
+		/* Avoid later ver check by setting nextbusn */
+		rcec_ea->nextbusn = 0xff;
+		rcec_ea->lastbusn = 0x00;
+	}
+
+	dev->rcec_ea = rcec_ea;
+}
+
+void pci_rcec_exit(struct pci_dev *dev)
+{
+	kfree(dev->rcec_ea);
+	dev->rcec_ea = NULL;
+}
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4289030b0fff..f22e1451d65d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2216,6 +2216,7 @@ static void pci_configure_device(struct pci_dev *dev)
 static void pci_release_capabilities(struct pci_dev *dev)
 {
 	pci_aer_exit(dev);
+	pci_rcec_exit(dev);
 	pci_vpd_release(dev);
 	pci_iov_release(dev);
 	pci_free_cap_save_buffers(dev);
@@ -2415,6 +2416,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_ptm_init(dev);		/* Precision Time Measurement */
 	pci_aer_init(dev);		/* Advanced Error Reporting */
 	pci_dpc_init(dev);		/* Downstream Port Containment */
+	pci_rcec_init(dev);		/* Root Complex Event Collector */
 
 	pcie_report_downtraining(dev);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 22207a79762c..f8c927fd0602 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -304,6 +304,7 @@ struct pcie_link_state;
 struct pci_vpd;
 struct pci_sriov;
 struct pci_p2pdma;
+struct rcec_ea;
 
 /* The pci_dev structure describes PCI devices */
 struct pci_dev {
@@ -326,6 +327,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 rcec_ea	*rcec_ea;	/* RCEC cached endpoint association */
 #endif
 	u8		pcie_cap;	/* PCIe capability offset */
 	u8		msi_cap;	/* MSI capability offset */
-- 
2.29.2

[PATCH v12 04/15] PCI/ERR: Rename reset_link() to reset_subordinates()

[Sean V Kelley]

reset_link() appears to be misnamed.  The point is to reset any devices
below a given bridge, so rename it to reset_subordinates() to make it clear
that we are passing a bridge with the intent to reset the devices below it.

[bhelgaas: fix reset_subordinate_device() typo, shorten name]
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Link: https://lore.kernel.org/r/20201002184735.1229220-5-seanvk.dev@oregontracks.org
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pci.h      | 4 ++--
 drivers/pci/pcie/err.c | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 12dcad8f3635..3c4570a3058f 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -572,8 +572,8 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
 
 /* PCI error reporting and recovery */
 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_channel_state_t state,
+		pci_ers_result_t (*reset_subordinates)(struct pci_dev *pdev));
 
 bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
 #ifdef CONFIG_PCIEASPM
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index c543f419d8f9..db149c6ce4fb 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -147,8 +147,8 @@ static int report_resume(struct pci_dev *dev, void *data)
 }
 
 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_channel_state_t state,
+		pci_ers_result_t (*reset_subordinates)(struct pci_dev *pdev))
 {
 	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
 	struct pci_bus *bus;
@@ -165,9 +165,9 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	pci_dbg(dev, "broadcast error_detected message\n");
 	if (state == pci_channel_io_frozen) {
 		pci_walk_bus(bus, report_frozen_detected, &status);
-		status = reset_link(dev);
+		status = reset_subordinates(dev);
 		if (status != PCI_ERS_RESULT_RECOVERED) {
-			pci_warn(dev, "link reset failed\n");
+			pci_warn(dev, "subordinate device reset failed\n");
 			goto failed;
 		}
 	} else {
-- 
2.29.2

[PATCH v12 05/15] PCI/ERR: Simplify by using pci_upstream_bridge()

[Sean V Kelley]

Use pci_upstream_bridge() in place of dev->bus->self.  No functional change
intended.

[bhelgaas: split to separate patch]
Link: https://lore.kernel.org/r/20201002184735.1229220-6-seanvk.dev@oregontracks.org
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/err.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index db149c6ce4fb..05f61da5ed9d 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -159,7 +159,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	 */
 	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
 	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
-		dev = dev->bus->self;
+		dev = pci_upstream_bridge(dev);
 	bus = dev->subordinate;
 
 	pci_dbg(dev, "broadcast error_detected message\n");
-- 
2.29.2

[PATCH v12 06/15] PCI/ERR: Simplify by computing pci_pcie_type() once

[Sean V Kelley]

Instead of calling pci_pcie_type(dev) twice, call it once and save the
result.  No functional change intended.

[bhelgaas: split to separate patch]
Link: https://lore.kernel.org/r/20201002184735.1229220-6-seanvk.dev@oregontracks.org
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/pcie/aer.c         | 5 +++--
 drivers/pci/pcie/err.c         | 5 +++--
 drivers/pci/pcie/portdrv_pci.c | 9 +++++----
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 6fe2d4ef0635..0ba0b47ae751 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1034,6 +1034,7 @@ EXPORT_SYMBOL_GPL(aer_recover_queue);
  */
 int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 {
+	int type = pci_pcie_type(dev);
 	int aer = dev->aer_cap;
 	int temp;
 
@@ -1052,8 +1053,8 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 			&info->mask);
 		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_DOWNSTREAM ||
+	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
+		   type == PCI_EXP_TYPE_DOWNSTREAM ||
 		   info->severity == AER_NONFATAL) {
 
 		/* Link is still healthy for IO reads */
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 05f61da5ed9d..7a5af873d8bc 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -150,6 +150,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 		pci_channel_state_t state,
 		pci_ers_result_t (*reset_subordinates)(struct pci_dev *pdev))
 {
+	int type = pci_pcie_type(dev);
 	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
 	struct pci_bus *bus;
 
@@ -157,8 +158,8 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	 * Error recovery runs on all subordinates of the first downstream port.
 	 * If the downstream port detected the error, it is cleared at the end.
 	 */
-	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
-	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
+	if (!(type == PCI_EXP_TYPE_ROOT_PORT ||
+	      type == PCI_EXP_TYPE_DOWNSTREAM))
 		dev = pci_upstream_bridge(dev);
 	bus = dev->subordinate;
 
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 4d880679b9b1..ff9517ee92b3 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -101,13 +101,14 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 static int pcie_portdrv_probe(struct pci_dev *dev,
 					const struct pci_device_id *id)
 {
+	int type = pci_pcie_type(dev);
 	int status;
 
 	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_RC_EC)))
+	    ((type != PCI_EXP_TYPE_ROOT_PORT) &&
+	     (type != PCI_EXP_TYPE_UPSTREAM) &&
+	     (type != PCI_EXP_TYPE_DOWNSTREAM) &&
+	     (type != PCI_EXP_TYPE_RC_EC)))
 		return -ENODEV;
 
 	status = pcie_port_device_register(dev);
-- 
2.29.2

[PATCH v12 07/15] PCI/ERR: Use "bridge" for clarity in pcie_do_recovery()

[Sean V Kelley]

pcie_do_recovery() may be called with "dev" being either a bridge (Root
Port or Switch Downstream Port) or an Endpoint.  The bulk of the function
deals with the bridge, so if we start with an Endpoint, we reset "dev" to
be the bridge leading to it.

For clarity, replace "dev" in the body of the function with "bridge".  No
functional change intended.

[bhelgaas: commit log, split pieces out so this is pure rename, also
replace "dev" with "bridge" in messages and pci_uevent_ers()]
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Link: https://lore.kernel.org/r/20201002184735.1229220-6-seanvk.dev@oregontracks.org
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/err.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 7a5af873d8bc..46a5b84f8842 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -151,24 +151,27 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 		pci_ers_result_t (*reset_subordinates)(struct pci_dev *pdev))
 {
 	int type = pci_pcie_type(dev);
-	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
+	struct pci_dev *bridge;
 	struct pci_bus *bus;
+	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
 
 	/*
-	 * Error recovery runs on all subordinates of the first downstream port.
-	 * If the downstream port detected the error, it is cleared at the end.
+	 * Error recovery runs on all subordinates of the bridge.  If the
+	 * bridge detected the error, it is cleared at the end.
 	 */
 	if (!(type == PCI_EXP_TYPE_ROOT_PORT ||
 	      type == PCI_EXP_TYPE_DOWNSTREAM))
-		dev = pci_upstream_bridge(dev);
-	bus = dev->subordinate;
+		bridge = pci_upstream_bridge(dev);
+	else
+		bridge = dev;
 
-	pci_dbg(dev, "broadcast error_detected message\n");
+	bus = bridge->subordinate;
+	pci_dbg(bridge, "broadcast error_detected message\n");
 	if (state == pci_channel_io_frozen) {
 		pci_walk_bus(bus, report_frozen_detected, &status);
-		status = reset_subordinates(dev);
+		status = reset_subordinates(bridge);
 		if (status != PCI_ERS_RESULT_RECOVERED) {
-			pci_warn(dev, "subordinate device reset failed\n");
+			pci_warn(bridge, "subordinate device reset failed\n");
 			goto failed;
 		}
 	} else {
@@ -177,7 +180,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 
 	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
 		status = PCI_ERS_RESULT_RECOVERED;
-		pci_dbg(dev, "broadcast mmio_enabled message\n");
+		pci_dbg(bridge, "broadcast mmio_enabled message\n");
 		pci_walk_bus(bus, report_mmio_enabled, &status);
 	}
 
@@ -188,27 +191,27 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 		 * drivers' slot_reset callbacks?
 		 */
 		status = PCI_ERS_RESULT_RECOVERED;
-		pci_dbg(dev, "broadcast slot_reset message\n");
+		pci_dbg(bridge, "broadcast slot_reset message\n");
 		pci_walk_bus(bus, report_slot_reset, &status);
 	}
 
 	if (status != PCI_ERS_RESULT_RECOVERED)
 		goto failed;
 
-	pci_dbg(dev, "broadcast resume message\n");
+	pci_dbg(bridge, "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_info(dev, "device recovery successful\n");
+	if (pcie_aer_is_native(bridge))
+		pcie_clear_device_status(bridge);
+	pci_aer_clear_nonfatal_status(bridge);
+	pci_info(bridge, "device recovery successful\n");
 	return status;
 
 failed:
-	pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
+	pci_uevent_ers(bridge, PCI_ERS_RESULT_DISCONNECT);
 
 	/* TODO: Should kernel panic here? */
-	pci_info(dev, "device recovery failed\n");
+	pci_info(bridge, "device recovery failed\n");
 
 	return status;
 }
-- 
2.29.2

[PATCH v12 08/15] PCI/ERR: Avoid negated conditional for clarity

[Sean V Kelley]

Reverse the sense of the Root Port/Downstream Port conditional for clarity.
No functional change intended.

[bhelgaas: split to separate patch]
Link: https://lore.kernel.org/r/20201002184735.1229220-6-seanvk.dev@oregontracks.org
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/err.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 46a5b84f8842..931e75f2549d 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -159,11 +159,11 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	 * Error recovery runs on all subordinates of the bridge.  If the
 	 * bridge detected the error, it is cleared at the end.
 	 */
-	if (!(type == PCI_EXP_TYPE_ROOT_PORT ||
-	      type == PCI_EXP_TYPE_DOWNSTREAM))
-		bridge = pci_upstream_bridge(dev);
-	else
+	if (type == PCI_EXP_TYPE_ROOT_PORT ||
+	    type == PCI_EXP_TYPE_DOWNSTREAM)
 		bridge = dev;
+	else
+		bridge = pci_upstream_bridge(dev);
 
 	bus = bridge->subordinate;
 	pci_dbg(bridge, "broadcast error_detected message\n");
-- 
2.29.2

[PATCH v12 09/15] PCI/ERR: Add pci_walk_bridge() to pcie_do_recovery()

[Sean V Kelley]

Consolidate subordinate bus checks with pci_walk_bus() into
pci_walk_bridge() for walking below potentially AER affected bridges.

[bhelgaas: fix kerneldoc]
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Link: https://lore.kernel.org/r/20201002184735.1229220-7-seanvk.dev@oregontracks.org
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/err.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 931e75f2549d..8b53aecdb43d 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -146,13 +146,30 @@ static int report_resume(struct pci_dev *dev, void *data)
 	return 0;
 }
 
+/**
+ * pci_walk_bridge - walk bridges potentially AER affected
+ * @bridge:	bridge which may be 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 bridge, walk the subordinate bus, including
+ * any bridged devices on buses under this bus.  Call the provided callback
+ * on each device found.
+ */
+static void pci_walk_bridge(struct pci_dev *bridge,
+			    int (*cb)(struct pci_dev *, void *),
+			    void *userdata)
+{
+	if (bridge->subordinate)
+		pci_walk_bus(bridge->subordinate, cb, userdata);
+}
+
 pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 		pci_channel_state_t state,
 		pci_ers_result_t (*reset_subordinates)(struct pci_dev *pdev))
 {
 	int type = pci_pcie_type(dev);
 	struct pci_dev *bridge;
-	struct pci_bus *bus;
 	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
 
 	/*
@@ -165,23 +182,22 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	else
 		bridge = pci_upstream_bridge(dev);
 
-	bus = bridge->subordinate;
 	pci_dbg(bridge, "broadcast error_detected message\n");
 	if (state == pci_channel_io_frozen) {
-		pci_walk_bus(bus, report_frozen_detected, &status);
+		pci_walk_bridge(bridge, report_frozen_detected, &status);
 		status = reset_subordinates(bridge);
 		if (status != PCI_ERS_RESULT_RECOVERED) {
 			pci_warn(bridge, "subordinate device reset failed\n");
 			goto failed;
 		}
 	} else {
-		pci_walk_bus(bus, report_normal_detected, &status);
+		pci_walk_bridge(bridge, report_normal_detected, &status);
 	}
 
 	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
 		status = PCI_ERS_RESULT_RECOVERED;
 		pci_dbg(bridge, "broadcast mmio_enabled message\n");
-		pci_walk_bus(bus, report_mmio_enabled, &status);
+		pci_walk_bridge(bridge, report_mmio_enabled, &status);
 	}
 
 	if (status == PCI_ERS_RESULT_NEED_RESET) {
@@ -192,14 +208,14 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 		 */
 		status = PCI_ERS_RESULT_RECOVERED;
 		pci_dbg(bridge, "broadcast slot_reset message\n");
-		pci_walk_bus(bus, report_slot_reset, &status);
+		pci_walk_bridge(bridge, report_slot_reset, &status);
 	}
 
 	if (status != PCI_ERS_RESULT_RECOVERED)
 		goto failed;
 
 	pci_dbg(bridge, "broadcast resume message\n");
-	pci_walk_bus(bus, report_resume, &status);
+	pci_walk_bridge(bridge, report_resume, &status);
 
 	if (pcie_aer_is_native(bridge))
 		pcie_clear_device_status(bridge);
-- 
2.29.2

[PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Sean V Kelley]

In some cases a bridge may not exist as the hardware controlling may be
handled only by firmware and so is not visible to the OS. This scenario is
also possible in future use cases involving non-native use of RCECs by
firmware.

Explicitly apply conditional logic around these resets by limiting them to
Root Ports and Downstream Ports.

Link: https://lore.kernel.org/r/20201002184735.1229220-8-seanvk.dev@oregontracks.org
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/pcie/err.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 8b53aecdb43d..7883c9791562 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -148,13 +148,17 @@ static int report_resume(struct pci_dev *dev, void *data)
 
 /**
  * pci_walk_bridge - walk bridges potentially AER affected
- * @bridge:	bridge which may be a Port
+ * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
+ *		or an RCiEP associated with an RCEC
  * @cb:		callback to be called for each device found
  * @userdata:	arbitrary pointer to be passed to callback
  *
  * If the device provided is a bridge, 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 callback on the
+ * device itself.
  */
 static void pci_walk_bridge(struct pci_dev *bridge,
 			    int (*cb)(struct pci_dev *, void *),
@@ -162,6 +166,8 @@ static void pci_walk_bridge(struct pci_dev *bridge,
 {
 	if (bridge->subordinate)
 		pci_walk_bus(bridge->subordinate, cb, userdata);
+	else
+		cb(bridge, userdata);
 }
 
 pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
@@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 
 	/*
 	 * Error recovery runs on all subordinates of the bridge.  If the
-	 * bridge detected the error, it is cleared at the end.
+	 * bridge detected the error, it is cleared at the end.  For RCiEPs
+	 * we should reset just the RCiEP itself.
 	 */
 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
-	    type == PCI_EXP_TYPE_DOWNSTREAM)
+	    type == PCI_EXP_TYPE_DOWNSTREAM ||
+	    type == PCI_EXP_TYPE_RC_EC ||
+	    type == PCI_EXP_TYPE_RC_END)
 		bridge = dev;
 	else
 		bridge = pci_upstream_bridge(dev);
@@ -185,6 +194,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	pci_dbg(bridge, "broadcast error_detected message\n");
 	if (state == pci_channel_io_frozen) {
 		pci_walk_bridge(bridge, report_frozen_detected, &status);
+		if (type == PCI_EXP_TYPE_RC_END) {
+			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
+			status = PCI_ERS_RESULT_NONE;
+			goto failed;
+		}
+
 		status = reset_subordinates(bridge);
 		if (status != PCI_ERS_RESULT_RECOVERED) {
 			pci_warn(bridge, "subordinate device reset failed\n");
@@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	pci_dbg(bridge, "broadcast resume message\n");
 	pci_walk_bridge(bridge, report_resume, &status);
 
-	if (pcie_aer_is_native(bridge))
-		pcie_clear_device_status(bridge);
-	pci_aer_clear_nonfatal_status(bridge);
+	if (type == PCI_EXP_TYPE_ROOT_PORT ||
+	    type == PCI_EXP_TYPE_DOWNSTREAM ||
+	    type == PCI_EXP_TYPE_RC_EC) {
+		if (pcie_aer_is_native(bridge))
+			pcie_clear_device_status(bridge);
+		pci_aer_clear_nonfatal_status(bridge);
+	}
 	pci_info(bridge, "device recovery successful\n");
 	return status;
 
-- 
2.29.2

[PATCH v12 11/15] PCI/RCEC: Add pcie_link_rcec() to associate RCiEPs

[Sean V Kelley]

A Root Complex Event Collector terminates error and PME messages from
associated RCiEPs.

Use the RCEC Endpoint Association Extended Capability to identify
associated RCiEPs. Link the associated RCiEPs as the RCECs are enumerated.

Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Link: https://lore.kernel.org/r/20201002184735.1229220-11-seanvk.dev@oregontracks.org
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/pci.h              |  2 +
 drivers/pci/pcie/portdrv_pci.c |  3 ++
 drivers/pci/pcie/rcec.c        | 94 ++++++++++++++++++++++++++++++++++
 include/linux/pci.h            |  1 +
 4 files changed, 100 insertions(+)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3c4570a3058f..ae2ee4df1cff 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -472,9 +472,11 @@ static inline void pci_dpc_init(struct pci_dev *pdev) {}
 #ifdef CONFIG_PCIEPORTBUS
 void pci_rcec_init(struct pci_dev *dev);
 void pci_rcec_exit(struct pci_dev *dev);
+void pcie_link_rcec(struct pci_dev *rcec);
 #else
 static inline void pci_rcec_init(struct pci_dev *dev) {}
 static inline void pci_rcec_exit(struct pci_dev *dev) {}
+static inline void pcie_link_rcec(struct pci_dev *rcec) {}
 #endif
 
 #ifdef CONFIG_PCI_ATS
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index ff9517ee92b3..0b250bc5f405 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -111,6 +111,9 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	     (type != PCI_EXP_TYPE_RC_EC)))
 		return -ENODEV;
 
+	if (type == PCI_EXP_TYPE_RC_EC)
+		pcie_link_rcec(dev);
+
 	status = pcie_port_device_register(dev);
 	if (status)
 		return status;
diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
index 038e9d706d5f..cdec277cbd62 100644
--- a/drivers/pci/pcie/rcec.c
+++ b/drivers/pci/pcie/rcec.c
@@ -15,6 +15,100 @@
 
 #include "../pci.h"
 
+struct walk_rcec_data {
+	struct pci_dev *rcec;
+	int (*user_callback)(struct pci_dev *dev, void *data);
+	void *user_data;
+};
+
+static bool rcec_assoc_rciep(struct pci_dev *rcec, struct pci_dev *rciep)
+{
+	unsigned long bitmap = rcec->rcec_ea->bitmap;
+	unsigned int devn;
+
+	/* An RCiEP found on a different bus in range */
+	if (rcec->bus->number != rciep->bus->number)
+		return true;
+
+	/* Same bus, so check bitmap */
+	for_each_set_bit(devn, &bitmap, 32)
+		if (devn == rciep->devfn)
+			return true;
+
+	return false;
+}
+
+static int link_rcec_helper(struct pci_dev *dev, void *data)
+{
+	struct walk_rcec_data *rcec_data = data;
+	struct pci_dev *rcec = rcec_data->rcec;
+
+	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) &&
+	    rcec_assoc_rciep(rcec, dev)) {
+		dev->rcec = rcec;
+		pci_dbg(dev, "PME & error events signaled via %s\n",
+			pci_name(rcec));
+	}
+
+	return 0;
+}
+
+static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
+		      void *userdata)
+{
+	struct walk_rcec_data *rcec_data = userdata;
+	struct pci_dev *rcec = rcec_data->rcec;
+	u8 nextbusn, lastbusn;
+	struct pci_bus *bus;
+	unsigned int bnr;
+
+	if (!rcec->rcec_ea)
+		return;
+
+	/* Walk own bus for bitmap based association */
+	pci_walk_bus(rcec->bus, cb, rcec_data);
+
+	nextbusn = rcec->rcec_ea->nextbusn;
+	lastbusn = rcec->rcec_ea->lastbusn;
+
+	/* All RCiEP devices are on the same bus as the RCEC */
+	if (nextbusn == 0xff && lastbusn == 0x00)
+		return;
+
+	for (bnr = nextbusn; bnr <= lastbusn; bnr++) {
+		/* No association indicated (PCIe 5.0-1, 7.9.10.3) */
+		if (bnr == rcec->bus->number)
+			continue;
+
+		bus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
+		if (!bus)
+			continue;
+
+		/* Find RCiEP devices on the given bus ranges */
+		pci_walk_bus(bus, cb, rcec_data);
+	}
+}
+
+/**
+ * pcie_link_rcec - Link RCiEP devices associated with RCEC.
+ * @rcec: RCEC whose RCiEP devices should be linked.
+ *
+ * Link the given RCEC to each RCiEP device found.
+ */
+void pcie_link_rcec(struct pci_dev *rcec)
+{
+	struct walk_rcec_data rcec_data;
+
+	if (!rcec->rcec_ea)
+		return;
+
+	rcec_data.rcec = rcec;
+	rcec_data.user_callback = NULL;
+	rcec_data.user_data = NULL;
+
+	walk_rcec(link_rcec_helper, &rcec_data);
+}
+
 void pci_rcec_init(struct pci_dev *dev)
 {
 	struct rcec_ea *rcec_ea;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f8c927fd0602..7c7d2d23e8a3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -330,6 +330,7 @@ struct pci_dev {
 #endif
 #ifdef CONFIG_PCIEPORTBUS
 	struct rcec_ea	*rcec_ea;	/* RCEC cached endpoint association */
+	struct pci_dev  *rcec;          /* Associated RCEC device */
 #endif
 	u8		pcie_cap;	/* PCIe capability offset */
 	u8		msi_cap;	/* MSI capability offset */
-- 
2.29.2

[PATCH v12 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

[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.  In
some non-native cases in which there is no OS-visible device associated
with the RCiEP, there is nothing to act upon as the firmware is acting
before the OS.

Add handling for the linked RCEC in AER/ERR while taking into account
non-native cases.

Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Link: https://lore.kernel.org/r/20201002184735.1229220-12-seanvk.dev@oregontracks.org
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/pci/pcie/aer.c | 46 +++++++++++++++++++++++++++++++-----------
 drivers/pci/pcie/err.c | 20 +++++++++---------
 2 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 0ba0b47ae751..51389a6ee4ca 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1358,29 +1358,51 @@ 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 type = pci_pcie_type(dev);
+	struct pci_dev *root;
+	int aer = 0;
+	int rc = 0;
 	u32 reg32;
-	int rc;
 
-	if (pcie_aer_is_native(dev)) {
+	if (type == PCI_EXP_TYPE_RC_END)
+		/*
+		 * The reset should only clear the Root Error Status
+		 * of the RCEC. Only perform this for the
+		 * native case, i.e., an RCEC is present.
+		 */
+		root = dev->rcec;
+	else
+		root = dev;
+
+	if (root)
+		aer = dev->aer_cap;
+
+	if ((aer) && pcie_aer_is_native(dev)) {
 		/* Disable Root's interrupt in response to error messages */
-		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
+		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
 		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
-		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
 	}
 
-	rc = pci_bus_error_reset(dev);
-	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
+	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
+		if (pcie_has_flr(dev)) {
+			rc = pcie_flr(dev);
+			pci_info(dev, "has been reset (%d)\n", rc);
+		}
+	} else {
+		rc = pci_bus_error_reset(dev);
+		pci_info(dev, "Root Port link has been reset (%d)\n", rc);
+	}
 
-	if (pcie_aer_is_native(dev)) {
+	if ((aer) && pcie_aer_is_native(dev)) {
 		/* Clear Root Error Status */
-		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
-		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
+		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
+		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
 
 		/* Enable Root Port's interrupt in response to error messages */
-		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
+		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
 		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
-		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
 	}
 
 	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 7883c9791562..cbc5abfe767b 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -148,10 +148,10 @@ static int report_resume(struct pci_dev *dev, void *data)
 
 /**
  * pci_walk_bridge - walk bridges potentially AER affected
- * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
- *		or an RCiEP associated with an RCEC
- * @cb:		callback to be called for each device found
- * @userdata:	arbitrary pointer to be passed to callback
+ * @bridge   bridge which may be an RCEC with associated RCiEPs,
+ *           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 bridge, walk the subordinate bus, including
  * any bridged devices on buses under this bus.  Call the provided callback
@@ -164,8 +164,14 @@ static void pci_walk_bridge(struct pci_dev *bridge,
 			    int (*cb)(struct pci_dev *, void *),
 			    void *userdata)
 {
+	/*
+	 * In a non-native case where there is no OS-visible reporting
+	 * device the bridge will be NULL, i.e., no RCEC, no Downstream Port.
+	 */
 	if (bridge->subordinate)
 		pci_walk_bus(bridge->subordinate, cb, userdata);
+	else if (bridge->rcec)
+		cb(bridge->rcec, userdata);
 	else
 		cb(bridge, userdata);
 }
@@ -194,12 +200,6 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	pci_dbg(bridge, "broadcast error_detected message\n");
 	if (state == pci_channel_io_frozen) {
 		pci_walk_bridge(bridge, report_frozen_detected, &status);
-		if (type == PCI_EXP_TYPE_RC_END) {
-			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
-			status = PCI_ERS_RESULT_NONE;
-			goto failed;
-		}
-
 		status = reset_subordinates(bridge);
 		if (status != PCI_ERS_RESULT_RECOVERED) {
 			pci_warn(bridge, "subordinate device reset failed\n");
-- 
2.29.2

[PATCH v12 13/15] PCI/AER: Add pcie_walk_rcec() to RCEC AER handling

[Sean V Kelley]

Root Complex Event Collectors (RCEC) appear as peers to Root Ports and also
have the AER capability. In addition, actions need to be taken for
associated RCiEPs. In such cases the RCECs will need to be walked in order
to find and act upon their respective RCiEPs.

Extend the existing ability to link the RCECs with a walking function
pcie_walk_rcec(). Add RCEC support to the current AER service driver and
attach the AER service driver to the RCEC device.

[bhelgaas: kernel doc, whitespace cleanup]
Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Link: https://lore.kernel.org/r/20201002184735.1229220-13-seanvk.dev@oregontracks.org
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pci.h       |  6 ++++++
 drivers/pci/pcie/aer.c  | 29 ++++++++++++++++++++++-------
 drivers/pci/pcie/rcec.c | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ae2ee4df1cff..e988cc930d0b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -473,10 +473,16 @@ static inline void pci_dpc_init(struct pci_dev *pdev) {}
 void pci_rcec_init(struct pci_dev *dev);
 void pci_rcec_exit(struct pci_dev *dev);
 void pcie_link_rcec(struct pci_dev *rcec);
+void pcie_walk_rcec(struct pci_dev *rcec,
+		    int (*cb)(struct pci_dev *, void *),
+		    void *userdata);
 #else
 static inline void pci_rcec_init(struct pci_dev *dev) {}
 static inline void pci_rcec_exit(struct pci_dev *dev) {}
 static inline void pcie_link_rcec(struct pci_dev *rcec) {}
+static inline void pcie_walk_rcec(struct pci_dev *rcec,
+				  int (*cb)(struct pci_dev *, void *),
+				  void *userdata) {}
 #endif
 
 #ifdef CONFIG_PCI_ATS
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 51389a6ee4ca..b86a92494345 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -300,7 +300,8 @@ 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 +596,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 +918,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);
@@ -1054,6 +1059,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 (type == PCI_EXP_TYPE_ROOT_PORT ||
+		   type == PCI_EXP_TYPE_RC_EC ||
 		   type == PCI_EXP_TYPE_DOWNSTREAM ||
 		   info->severity == AER_NONFATAL) {
 
@@ -1206,6 +1212,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)
@@ -1230,9 +1237,12 @@ 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);
+
 }
 
 /**
@@ -1330,6 +1340,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;
@@ -1410,7 +1425,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,
diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
index cdec277cbd62..2c5c552994e4 100644
--- a/drivers/pci/pcie/rcec.c
+++ b/drivers/pci/pcie/rcec.c
@@ -53,6 +53,18 @@ static int link_rcec_helper(struct pci_dev *dev, void *data)
 	return 0;
 }
 
+static int walk_rcec_helper(struct pci_dev *dev, void *data)
+{
+	struct walk_rcec_data *rcec_data = data;
+	struct pci_dev *rcec = rcec_data->rcec;
+
+	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) &&
+	    rcec_assoc_rciep(rcec, dev))
+		rcec_data->user_callback(dev, rcec_data->user_data);
+
+	return 0;
+}
+
 static void walk_rcec(int (*cb)(struct pci_dev *dev, void *data),
 		      void *userdata)
 {
@@ -109,6 +121,31 @@ void pcie_link_rcec(struct pci_dev *rcec)
 	walk_rcec(link_rcec_helper, &rcec_data);
 }
 
+/**
+ * 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 callback on each RCiEP found.
+ *
+ * If @cb returns anything other than 0, break out.
+ */
+void pcie_walk_rcec(struct pci_dev *rcec, int (*cb)(struct pci_dev *, void *),
+		    void *userdata)
+{
+	struct walk_rcec_data rcec_data;
+
+	if (!rcec->rcec_ea)
+		return;
+
+	rcec_data.rcec = rcec;
+	rcec_data.user_callback = cb;
+	rcec_data.user_data = userdata;
+
+	walk_rcec(walk_rcec_helper, &rcec_data);
+}
+
 void pci_rcec_init(struct pci_dev *dev)
 {
 	struct rcec_ea *rcec_ea;
-- 
2.29.2

[PATCH v12 14/15] PCI/PME: Add pcie_walk_rcec() to RCEC PME handling

[Sean V Kelley]

Root Complex Event Collectors (RCEC) appear as peers of Root Ports and also
have the PME capability. As with AER, there is a need to be able to walk
the RCiEPs associated with their RCEC for purposes of acting upon them with
callbacks.

Add RCEC support through the use of pcie_walk_rcec() 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>
Link: https://lore.kernel.org/r/20201002184735.1229220-14-seanvk.dev@oregontracks.org
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/pme.c          | 16 ++++++++++++----
 drivers/pci/pcie/portdrv_core.c |  9 +++------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 6a32970bb731..3fc08488d65f 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,16 @@ 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 type = pci_pcie_type(port);
 	int ret;
 
+	/* Limit to Root Ports or Root Complex Event Collectors */
+	if (type != PCI_EXP_TYPE_RC_EC &&
+	    type != PCI_EXP_TYPE_ROOT_PORT)
+		return -ENODEV;
+
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -333,7 +342,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 +453,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,
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 50a9522ab07d..e1fed6649c41 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -233,12 +233,9 @@ 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.
-	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
+	/* Root Ports and Root Complex Event Collectors may generate PMEs */
+	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;
 
-- 
2.29.2

[PATCH v12 15/15] PCI/AER: Add RCEC AER error injection support

[Sean V Kelley]

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

Root Complex Event Collectors (RCEC) appear as peers to Root Ports and may
also have the AER capability.

Add RCEC support to the AER error injection driver.

Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
Link: https://lore.kernel.org/r/20201002184735.1229220-15-seanvk.dev@oregontracks.org
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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..767f8859b99b 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, "Neither Root Port nor RCEC found\n");
 		ret = -ENODEV;
 		goto out_put;
 	}
-- 
2.29.2

Re: [PATCH v12 00/15] Add RCEC handling to PCI/AER

[Bjorn Helgaas]

On Fri, Nov 20, 2020 at 04:10:21PM -0800, Sean V Kelley wrote:
> Changes since v11 [1] and based on pci/master tree [2]:
> 
> - No functional changes. Tested with aer injection.
> 
> - Merge RCEC class code and extended capability patch with usage.
> - Apply same optimization for pci_pcie_type(dev) calls in
> drivers/pci/pcie/portdrv_pci.c and drivers/pci/pcie/aer.c.
> (Kuppuswamy Sathyanarayanan)
> 
> [1] https://lore.kernel.org/linux-pci/20201117191954.1322844-1-sean.v.kelley@intel.com/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/
> 
> 
> 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.
> 
> Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> #non-native/no RCEC
> 
> 
> Qiuxu Zhuo (3):
>   PCI/RCEC: Bind RCEC devices to the Root Port driver
>   PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR
>   PCI/AER: Add RCEC AER error injection support
> 
> Sean V Kelley (12):
>   AER: aer_root_reset() non-native handling
>   PCI/RCEC: Cache RCEC capabilities in pci_init_capabilities()
>   PCI/ERR: Rename reset_link() to reset_subordinates()
>   PCI/ERR: Simplify by using pci_upstream_bridge()
>   PCI/ERR: Simplify by computing pci_pcie_type() once
>   PCI/ERR: Use "bridge" for clarity in pcie_do_recovery()
>   PCI/ERR: Avoid negated conditional for clarity
>   PCI/ERR: Add pci_walk_bridge() to pcie_do_recovery()
>   PCI/ERR: Limit AER resets in pcie_do_recovery()
>   PCI/RCEC: Add pcie_link_rcec() to associate RCiEPs
>   PCI/AER: Add pcie_walk_rcec() to RCEC AER handling
>   PCI/PME: Add pcie_walk_rcec() to RCEC PME handling
> 
>  drivers/pci/pci.h               |  29 ++++-
>  drivers/pci/pcie/Makefile       |   2 +-
>  drivers/pci/pcie/aer.c          |  89 +++++++++++----
>  drivers/pci/pcie/aer_inject.c   |   5 +-
>  drivers/pci/pcie/err.c          |  93 +++++++++++-----
>  drivers/pci/pcie/pme.c          |  16 ++-
>  drivers/pci/pcie/portdrv_core.c |   9 +-
>  drivers/pci/pcie/portdrv_pci.c  |  13 ++-
>  drivers/pci/pcie/rcec.c         | 190 ++++++++++++++++++++++++++++++++
>  drivers/pci/probe.c             |   2 +
>  include/linux/pci.h             |   5 +
>  include/linux/pci_ids.h         |   1 +
>  include/uapi/linux/pci_regs.h   |   7 ++
>  13 files changed, 393 insertions(+), 68 deletions(-)
>  create mode 100644 drivers/pci/pcie/rcec.c

Good timing, I was just tidying up v11 :)

Anyway, I applied this to pci/err for v5.11, thanks!

Now I see a Tested-by from Jonathan above; this cover letter doesn't
become part of the git history, so probably I should add that to each
individual patch, or maybe just the relevant ones if there are some
that it wouldn't apply to.  I'll tidy that up next week.

Minor procedural things I fixed up already because I think they're a
consequence of you building on a previous branch I published: patches
you post shouldn't include Signed-off-by; you should add your own when
you write the patch or are part of the delivery path, but you
shouldn't add *mine*.  I add that when I apply them.

I also removed the first Link: tags since they also look like they're
from an older version.  You don't need to add those; I add those
automatically so they point to the mailing list message where the
patch was posted.

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Bjorn Helgaas]

On Fri, Nov 20, 2020 at 04:10:31PM -0800, Sean V Kelley wrote:
> In some cases a bridge may not exist as the hardware controlling may be
> handled only by firmware and so is not visible to the OS. This scenario is
> also possible in future use cases involving non-native use of RCECs by
> firmware.
> 
> Explicitly apply conditional logic around these resets by limiting them to
> Root Ports and Downstream Ports.

Can you help me understand this?  The subject says "Limit AER resets"
and here you say "limit them to RPs and DPs", but it's not completely
obvious how the resets are being limited, i.e., the patch doesn't add
anything like:

 +  if (type == PCI_EXP_TYPE_ROOT_PORT ||
 +      type == PCI_EXP_TYPE_DOWNSTREAM)
      reset_subordinates(bridge);

It *does* add checks around pcie_clear_device_status(), but that also
includes RC_EC.  And that's not a reset, so I don't think that's
explicitly mentioned in the commit log.

Also see the question below.

> Link: https://lore.kernel.org/r/20201002184735.1229220-8-seanvk.dev@oregontracks.org
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/pci/pcie/err.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 8b53aecdb43d..7883c9791562 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -148,13 +148,17 @@ static int report_resume(struct pci_dev *dev, void *data)
>  
>  /**
>   * pci_walk_bridge - walk bridges potentially AER affected
> - * @bridge:	bridge which may be a Port
> + * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
> + *		or an RCiEP associated with an RCEC
>   * @cb:		callback to be called for each device found
>   * @userdata:	arbitrary pointer to be passed to callback
>   *
>   * If the device provided is a bridge, 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 callback on the
> + * device itself.
>   */
>  static void pci_walk_bridge(struct pci_dev *bridge,
>  			    int (*cb)(struct pci_dev *, void *),
> @@ -162,6 +166,8 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>  {
>  	if (bridge->subordinate)
>  		pci_walk_bus(bridge->subordinate, cb, userdata);
> +	else
> +		cb(bridge, userdata);
>  }
>  
>  pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> @@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  
>  	/*
>  	 * Error recovery runs on all subordinates of the bridge.  If the
> -	 * bridge detected the error, it is cleared at the end.
> +	 * bridge detected the error, it is cleared at the end.  For RCiEPs
> +	 * we should reset just the RCiEP itself.
>  	 */
>  	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> +	    type == PCI_EXP_TYPE_RC_EC ||
> +	    type == PCI_EXP_TYPE_RC_END)
>  		bridge = dev;
>  	else
>  		bridge = pci_upstream_bridge(dev);
> @@ -185,6 +194,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  	pci_dbg(bridge, "broadcast error_detected message\n");
>  	if (state == pci_channel_io_frozen) {
>  		pci_walk_bridge(bridge, report_frozen_detected, &status);
> +		if (type == PCI_EXP_TYPE_RC_END) {
> +			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
> +			status = PCI_ERS_RESULT_NONE;
> +			goto failed;
> +		}
> +
>  		status = reset_subordinates(bridge);
>  		if (status != PCI_ERS_RESULT_RECOVERED) {
>  			pci_warn(bridge, "subordinate device reset failed\n");
> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  	pci_dbg(bridge, "broadcast resume message\n");
>  	pci_walk_bridge(bridge, report_resume, &status);
>  
> -	if (pcie_aer_is_native(bridge))
> -		pcie_clear_device_status(bridge);
> -	pci_aer_clear_nonfatal_status(bridge);
> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> +	    type == PCI_EXP_TYPE_RC_EC) {
> +		if (pcie_aer_is_native(bridge))
> +			pcie_clear_device_status(bridge);
> +		pci_aer_clear_nonfatal_status(bridge);

This is hard to understand because "type" is from "dev", but "bridge"
is not necessarily the same device.  Should it be this?

  type = pci_pcie_type(bridge);
  if (type == PCI_EXP_TYPE_ROOT_PORT ||
      ...)

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

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Kelley, Sean V]

Hi Bjorn,

> On Nov 23, 2020, at 3:28 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Fri, Nov 20, 2020 at 04:10:31PM -0800, Sean V Kelley wrote:
>> In some cases a bridge may not exist as the hardware controlling may be
>> handled only by firmware and so is not visible to the OS. This scenario is
>> also possible in future use cases involving non-native use of RCECs by
>> firmware.
>> 
>> Explicitly apply conditional logic around these resets by limiting them to
>> Root Ports and Downstream Ports.
> 
> Can you help me understand this?  The subject says "Limit AER resets"
> and here you say "limit them to RPs and DPs", but it's not completely
> obvious how the resets are being limited, i.e., the patch doesn't add
> anything like:
> 
> +  if (type == PCI_EXP_TYPE_ROOT_PORT ||
> +      type == PCI_EXP_TYPE_DOWNSTREAM)
>      reset_subordinates(bridge);
> 
> It *does* add checks around pcie_clear_device_status(), but that also
> includes RC_EC.  And that's not a reset, so I don't think that's
> explicitly mentioned in the commit log.

The subject should have referred to the clearing of the device status rather than resets.
It originally came from this simpler patch in which I made use of reset instead of clear:

https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/

So a rephrase of clearing in place of resets would be more appropriate.

Then we added the notion of bridges…below

> 
> Also see the question below.
> 
>> Link: https://lore.kernel.org/r/20201002184735.1229220-8-seanvk.dev@oregontracks.org
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> ---
>> drivers/pci/pcie/err.c | 31 +++++++++++++++++++++++++------
>> 1 file changed, 25 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index 8b53aecdb43d..7883c9791562 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -148,13 +148,17 @@ static int report_resume(struct pci_dev *dev, void *data)
>> 
>> /**
>>  * pci_walk_bridge - walk bridges potentially AER affected
>> - * @bridge:	bridge which may be a Port
>> + * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
>> + *		or an RCiEP associated with an RCEC
>>  * @cb:		callback to be called for each device found
>>  * @userdata:	arbitrary pointer to be passed to callback
>>  *
>>  * If the device provided is a bridge, 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 callback on the
>> + * device itself.
>>  */
>> static void pci_walk_bridge(struct pci_dev *bridge,
>> 			    int (*cb)(struct pci_dev *, void *),
>> @@ -162,6 +166,8 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>> {
>> 	if (bridge->subordinate)
>> 		pci_walk_bus(bridge->subordinate, cb, userdata);
>> +	else
>> +		cb(bridge, userdata);
>> }
>> 
>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> @@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> 
>> 	/*
>> 	 * Error recovery runs on all subordinates of the bridge.  If the
>> -	 * bridge detected the error, it is cleared at the end.
>> +	 * bridge detected the error, it is cleared at the end.  For RCiEPs
>> +	 * we should reset just the RCiEP itself.
>> 	 */
>> 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>> +	    type == PCI_EXP_TYPE_RC_EC ||
>> +	    type == PCI_EXP_TYPE_RC_END)
>> 		bridge = dev;
>> 	else
>> 		bridge = pci_upstream_bridge(dev);
>> @@ -185,6 +194,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> 	pci_dbg(bridge, "broadcast error_detected message\n");
>> 	if (state == pci_channel_io_frozen) {
>> 		pci_walk_bridge(bridge, report_frozen_detected, &status);
>> +		if (type == PCI_EXP_TYPE_RC_END) {
>> +			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
>> +			status = PCI_ERS_RESULT_NONE;
>> +			goto failed;
>> +		}
>> +
>> 		status = reset_subordinates(bridge);
>> 		if (status != PCI_ERS_RESULT_RECOVERED) {
>> 			pci_warn(bridge, "subordinate device reset failed\n");
>> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> 	pci_dbg(bridge, "broadcast resume message\n");
>> 	pci_walk_bridge(bridge, report_resume, &status);
>> 
>> -	if (pcie_aer_is_native(bridge))
>> -		pcie_clear_device_status(bridge);
>> -	pci_aer_clear_nonfatal_status(bridge);
>> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>> +	    type == PCI_EXP_TYPE_RC_EC) {
>> +		if (pcie_aer_is_native(bridge))
>> +			pcie_clear_device_status(bridge);
>> +		pci_aer_clear_nonfatal_status(bridge);
> 
> This is hard to understand because "type" is from "dev", but "bridge"
> is not necessarily the same device.  Should it be this?
> 
>  type = pci_pcie_type(bridge);
>  if (type == PCI_EXP_TYPE_ROOT_PORT ||
>      ...)

Correct, it would be better if the type was based on the ‘bridge’.

Thanks,

Sean

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

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Bjorn Helgaas]

On Mon, Nov 23, 2020 at 11:57:35PM +0000, Kelley, Sean V wrote:
> > On Nov 23, 2020, at 3:28 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Nov 20, 2020 at 04:10:31PM -0800, Sean V Kelley wrote:
> >> In some cases a bridge may not exist as the hardware controlling may be
> >> handled only by firmware and so is not visible to the OS. This scenario is
> >> also possible in future use cases involving non-native use of RCECs by
> >> firmware.
> >> 
> >> Explicitly apply conditional logic around these resets by limiting them to
> >> Root Ports and Downstream Ports.
> > 
> > Can you help me understand this?  The subject says "Limit AER resets"
> > and here you say "limit them to RPs and DPs", but it's not completely
> > obvious how the resets are being limited, i.e., the patch doesn't add
> > anything like:
> > 
> > +  if (type == PCI_EXP_TYPE_ROOT_PORT ||
> > +      type == PCI_EXP_TYPE_DOWNSTREAM)
> >      reset_subordinates(bridge);
> > 
> > It *does* add checks around pcie_clear_device_status(), but that also
> > includes RC_EC.  And that's not a reset, so I don't think that's
> > explicitly mentioned in the commit log.
> 
> The subject should have referred to the clearing of the device status rather than resets.
> It originally came from this simpler patch in which I made use of reset instead of clear:
> 
> https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/
> 
> So a rephrase of clearing in place of resets would be more appropriate.
> 
> Then we added the notion of bridges…below
> 
> > 
> > Also see the question below.
> > 
> >> Link: https://lore.kernel.org/r/20201002184735.1229220-8-seanvk.dev@oregontracks.org
> >> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >> ---
> >> drivers/pci/pcie/err.c | 31 +++++++++++++++++++++++++------
> >> 1 file changed, 25 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> >> index 8b53aecdb43d..7883c9791562 100644
> >> --- a/drivers/pci/pcie/err.c
> >> +++ b/drivers/pci/pcie/err.c
> >> @@ -148,13 +148,17 @@ static int report_resume(struct pci_dev *dev, void *data)
> >> 
> >> /**
> >>  * pci_walk_bridge - walk bridges potentially AER affected
> >> - * @bridge:	bridge which may be a Port
> >> + * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
> >> + *		or an RCiEP associated with an RCEC
> >>  * @cb:		callback to be called for each device found
> >>  * @userdata:	arbitrary pointer to be passed to callback
> >>  *
> >>  * If the device provided is a bridge, 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 callback on the
> >> + * device itself.
> >>  */
> >> static void pci_walk_bridge(struct pci_dev *bridge,
> >> 			    int (*cb)(struct pci_dev *, void *),
> >> @@ -162,6 +166,8 @@ static void pci_walk_bridge(struct pci_dev *bridge,
> >> {
> >> 	if (bridge->subordinate)
> >> 		pci_walk_bus(bridge->subordinate, cb, userdata);
> >> +	else
> >> +		cb(bridge, userdata);
> >> }
> >> 
> >> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >> @@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >> 
> >> 	/*
> >> 	 * Error recovery runs on all subordinates of the bridge.  If the
> >> -	 * bridge detected the error, it is cleared at the end.
> >> +	 * bridge detected the error, it is cleared at the end.  For RCiEPs
> >> +	 * we should reset just the RCiEP itself.
> >> 	 */
> >> 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
> >> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> >> +	    type == PCI_EXP_TYPE_RC_EC ||
> >> +	    type == PCI_EXP_TYPE_RC_END)
> >> 		bridge = dev;
> >> 	else
> >> 		bridge = pci_upstream_bridge(dev);
> >> @@ -185,6 +194,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >> 	pci_dbg(bridge, "broadcast error_detected message\n");
> >> 	if (state == pci_channel_io_frozen) {
> >> 		pci_walk_bridge(bridge, report_frozen_detected, &status);
> >> +		if (type == PCI_EXP_TYPE_RC_END) {
> >> +			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
> >> +			status = PCI_ERS_RESULT_NONE;
> >> +			goto failed;
> >> +		}
> >> +
> >> 		status = reset_subordinates(bridge);
> >> 		if (status != PCI_ERS_RESULT_RECOVERED) {
> >> 			pci_warn(bridge, "subordinate device reset failed\n");
> >> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >> 	pci_dbg(bridge, "broadcast resume message\n");
> >> 	pci_walk_bridge(bridge, report_resume, &status);
> >> 
> >> -	if (pcie_aer_is_native(bridge))
> >> -		pcie_clear_device_status(bridge);
> >> -	pci_aer_clear_nonfatal_status(bridge);
> >> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> >> +	    type == PCI_EXP_TYPE_RC_EC) {
> >> +		if (pcie_aer_is_native(bridge))
> >> +			pcie_clear_device_status(bridge);
> >> +		pci_aer_clear_nonfatal_status(bridge);
> > 
> > This is hard to understand because "type" is from "dev", but "bridge"
> > is not necessarily the same device.  Should it be this?
> > 
> >  type = pci_pcie_type(bridge);
> >  if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >      ...)
> 
> Correct, it would be better if the type was based on the ‘bridge’.

OK.  This is similar to
https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/,
which you cited above except for the bridge/dev question and the
addition here of RC_EC.

I tried to split that back into its own patch and started with the
commit message from that patch.  But I got stuck on the commit
message.  I got as far as:

  In some cases an error may be reported by a device not visible to
  the OS, e.g., if firmware manages the device and passes error
  information to the OS via ACPI APEI.

But I still can't quite connect that to the patch.  "bridge" is
clearly a device visible to Linux.

I guess we're trying to assert that if "bridge" is not a Root Port,
Downstream Port, or RCEC, we shouldn't clear the error status because 
the error came from a device Linux doesn't know about.  But I think
"bridge" is *always* either a Root Port or a Downstream Port:

  if (type == PCI_EXP_TYPE_ROOT_PORT ||
      type == PCI_EXP_TYPE_DOWNSTREAM)
	  bridge = dev;
  else
	  bridge = pci_upstream_bridge(dev);

pci_upstream_bridge() returns either NULL (in which case previous uses
dereference a NULL pointer), or dev->bus->self, which is always a Root
Port, Switch Downstream Port, or Switch Upstream Port (or NULL for the
special case of VFs).

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

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Kelley, Sean V]

Hi Bjorn,

Was away briefly for the holidays, comments below:

> On Nov 24, 2020, at 9:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Mon, Nov 23, 2020 at 11:57:35PM +0000, Kelley, Sean V wrote:
>>> On Nov 23, 2020, at 3:28 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Fri, Nov 20, 2020 at 04:10:31PM -0800, Sean V Kelley wrote:
>>>> In some cases a bridge may not exist as the hardware controlling may be
>>>> handled only by firmware and so is not visible to the OS. This scenario is
>>>> also possible in future use cases involving non-native use of RCECs by
>>>> firmware.
>>>> 
>>>> Explicitly apply conditional logic around these resets by limiting them to
>>>> Root Ports and Downstream Ports.
>>> 
>>> Can you help me understand this?  The subject says "Limit AER resets"
>>> and here you say "limit them to RPs and DPs", but it's not completely
>>> obvious how the resets are being limited, i.e., the patch doesn't add
>>> anything like:
>>> 
>>> +  if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>> +      type == PCI_EXP_TYPE_DOWNSTREAM)
>>>     reset_subordinates(bridge);
>>> 
>>> It *does* add checks around pcie_clear_device_status(), but that also
>>> includes RC_EC.  And that's not a reset, so I don't think that's
>>> explicitly mentioned in the commit log.
>> 
>> The subject should have referred to the clearing of the device status rather than resets.
>> It originally came from this simpler patch in which I made use of reset instead of clear:
>> 
>> https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/
>> 
>> So a rephrase of clearing in place of resets would be more appropriate.
>> 
>> Then we added the notion of bridges…below
>> 
>>> 
>>> Also see the question below.
>>> 
>>>> Link: https://lore.kernel.org/r/20201002184735.1229220-8-seanvk.dev@oregontracks.org
>>>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> ---
>>>> drivers/pci/pcie/err.c | 31 +++++++++++++++++++++++++------
>>>> 1 file changed, 25 insertions(+), 6 deletions(-)
>>>> 
>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>> index 8b53aecdb43d..7883c9791562 100644
>>>> --- a/drivers/pci/pcie/err.c
>>>> +++ b/drivers/pci/pcie/err.c
>>>> @@ -148,13 +148,17 @@ static int report_resume(struct pci_dev *dev, void *data)
>>>> 
>>>> /**
>>>> * pci_walk_bridge - walk bridges potentially AER affected
>>>> - * @bridge:	bridge which may be a Port
>>>> + * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
>>>> + *		or an RCiEP associated with an RCEC
>>>> * @cb:		callback to be called for each device found
>>>> * @userdata:	arbitrary pointer to be passed to callback
>>>> *
>>>> * If the device provided is a bridge, 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 callback on the
>>>> + * device itself.
>>>> */
>>>> static void pci_walk_bridge(struct pci_dev *bridge,
>>>> 			    int (*cb)(struct pci_dev *, void *),
>>>> @@ -162,6 +166,8 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>>>> {
>>>> 	if (bridge->subordinate)
>>>> 		pci_walk_bus(bridge->subordinate, cb, userdata);
>>>> +	else
>>>> +		cb(bridge, userdata);
>>>> }
>>>> 
>>>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>> @@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>> 
>>>> 	/*
>>>> 	 * Error recovery runs on all subordinates of the bridge.  If the
>>>> -	 * bridge detected the error, it is cleared at the end.
>>>> +	 * bridge detected the error, it is cleared at the end.  For RCiEPs
>>>> +	 * we should reset just the RCiEP itself.
>>>> 	 */
>>>> 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
>>>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>>>> +	    type == PCI_EXP_TYPE_RC_EC ||
>>>> +	    type == PCI_EXP_TYPE_RC_END)
>>>> 		bridge = dev;
>>>> 	else
>>>> 		bridge = pci_upstream_bridge(dev);
>>>> @@ -185,6 +194,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>> 	pci_dbg(bridge, "broadcast error_detected message\n");
>>>> 	if (state == pci_channel_io_frozen) {
>>>> 		pci_walk_bridge(bridge, report_frozen_detected, &status);
>>>> +		if (type == PCI_EXP_TYPE_RC_END) {
>>>> +			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
>>>> +			status = PCI_ERS_RESULT_NONE;
>>>> +			goto failed;
>>>> +		}
>>>> +
>>>> 		status = reset_subordinates(bridge);
>>>> 		if (status != PCI_ERS_RESULT_RECOVERED) {
>>>> 			pci_warn(bridge, "subordinate device reset failed\n");
>>>> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>> 	pci_dbg(bridge, "broadcast resume message\n");
>>>> 	pci_walk_bridge(bridge, report_resume, &status);
>>>> 
>>>> -	if (pcie_aer_is_native(bridge))
>>>> -		pcie_clear_device_status(bridge);
>>>> -	pci_aer_clear_nonfatal_status(bridge);
>>>> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>>>> +	    type == PCI_EXP_TYPE_RC_EC) {
>>>> +		if (pcie_aer_is_native(bridge))
>>>> +			pcie_clear_device_status(bridge);
>>>> +		pci_aer_clear_nonfatal_status(bridge);
>>> 
>>> This is hard to understand because "type" is from "dev", but "bridge"
>>> is not necessarily the same device.  Should it be this?
>>> 
>>> type = pci_pcie_type(bridge);
>>> if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>     ...)
>> 
>> Correct, it would be better if the type was based on the ‘bridge’.
> 
> OK.  This is similar to
> https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/,
> which you cited above except for the bridge/dev question and the
> addition here of RC_EC.
> 
> I tried to split that back into its own patch and started with the
> commit message from that patch.  But I got stuck on the commit
> message.  I got as far as:
> 
>  In some cases an error may be reported by a device not visible to
>  the OS, e.g., if firmware manages the device and passes error
>  information to the OS via ACPI APEI.
> 
> But I still can't quite connect that to the patch.  "bridge" is
> clearly a device visible to Linux.

> 
> I guess we're trying to assert that if "bridge" is not a Root Port,
> Downstream Port, or RCEC, we shouldn't clear the error status because 
> the error came from a device Linux doesn't know about.  But I think
> "bridge" is *always* either a Root Port or a Downstream Port:

That’s ultimately what we are trying to do.

> 
>  if (type == PCI_EXP_TYPE_ROOT_PORT ||
>      type == PCI_EXP_TYPE_DOWNSTREAM)
> 	  bridge = dev;
>  else
> 	  bridge = pci_upstream_bridge(dev);
> 
> pci_upstream_bridge() returns either NULL (in which case previous uses
> dereference a NULL pointer), or dev->bus->self, which is always a Root
> Port, Switch Downstream Port, or Switch Upstream Port (or NULL for the
> special case of VFs).

In the past recall we were augmenting it with bridge = dev->rcec for RC_END.
But we were able to relocate the handling in aer_root_reset().

So in this patch - we add the conditionals because RC_END is being passed in addition to RC_EC.
 
 	if (type == PCI_EXP_TYPE_ROOT_PORT ||

-	    type == PCI_EXP_TYPE_DOWNSTREAM)

+	    type == PCI_EXP_TYPE_DOWNSTREAM ||
+	    type == PCI_EXP_TYPE_RC_EC ||
+	    type == PCI_EXP_TYPE_RC_END)

 		bridge = dev;
 	else
 		bridge = pci_upstream_bridge(dev);

So we need to check for RP, DS, and RC_EC

@@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,

 	pci_dbg(bridge, "broadcast resume message\n");
 	pci_walk_bridge(bridge, report_resume, &status);
 

-	if (pcie_aer_is_native(bridge))
-		pcie_clear_device_status(bridge);
-	pci_aer_clear_nonfatal_status(bridge);

+	if (type == PCI_EXP_TYPE_ROOT_PORT ||
+	    type == PCI_EXP_TYPE_DOWNSTREAM ||
+	    type == PCI_EXP_TYPE_RC_EC) {
+		if (pcie_aer_is_native(bridge))
+			pcie_clear_device_status(bridge);
+		pci_aer_clear_nonfatal_status(bridge);
+	}


Breaking out a separate patch would be unnecessary as you correctly point out that it’s only going to be an RP or DS before this patch.

Thanks,

Sean


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

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Bjorn Helgaas]

On Mon, Nov 30, 2020 at 07:54:37PM +0000, Kelley, Sean V wrote:
> > On Nov 24, 2020, at 9:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Nov 23, 2020 at 11:57:35PM +0000, Kelley, Sean V wrote:
> >>> On Nov 23, 2020, at 3:28 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>> On Fri, Nov 20, 2020 at 04:10:31PM -0800, Sean V Kelley wrote:
> >>>> In some cases a bridge may not exist as the hardware controlling may be
> >>>> handled only by firmware and so is not visible to the OS. This scenario is
> >>>> also possible in future use cases involving non-native use of RCECs by
> >>>> firmware.
> >>>> 
> >>>> Explicitly apply conditional logic around these resets by limiting them to
> >>>> Root Ports and Downstream Ports.
> >>> 
> >>> Can you help me understand this?  The subject says "Limit AER resets"
> >>> and here you say "limit them to RPs and DPs", but it's not completely
> >>> obvious how the resets are being limited, i.e., the patch doesn't add
> >>> anything like:
> >>> 
> >>> +  if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >>> +      type == PCI_EXP_TYPE_DOWNSTREAM)
> >>>     reset_subordinates(bridge);
> >>> 
> >>> It *does* add checks around pcie_clear_device_status(), but that also
> >>> includes RC_EC.  And that's not a reset, so I don't think that's
> >>> explicitly mentioned in the commit log.
> >> 
> >> The subject should have referred to the clearing of the device status rather than resets.
> >> It originally came from this simpler patch in which I made use of reset instead of clear:
> >> 
> >> https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/
> >> 
> >> So a rephrase of clearing in place of resets would be more appropriate.
> >> 
> >> Then we added the notion of bridges…below
> >> 
> >>> 
> >>> Also see the question below.
> >>> 
> >>>> Link: https://lore.kernel.org/r/20201002184735.1229220-8-seanvk.dev@oregontracks.org
> >>>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> >>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >>>> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>>> ---
> >>>> drivers/pci/pcie/err.c | 31 +++++++++++++++++++++++++------
> >>>> 1 file changed, 25 insertions(+), 6 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> >>>> index 8b53aecdb43d..7883c9791562 100644
> >>>> --- a/drivers/pci/pcie/err.c
> >>>> +++ b/drivers/pci/pcie/err.c
> >>>> @@ -148,13 +148,17 @@ static int report_resume(struct pci_dev *dev, void *data)
> >>>> 
> >>>> /**
> >>>> * pci_walk_bridge - walk bridges potentially AER affected
> >>>> - * @bridge:	bridge which may be a Port
> >>>> + * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
> >>>> + *		or an RCiEP associated with an RCEC
> >>>> * @cb:		callback to be called for each device found
> >>>> * @userdata:	arbitrary pointer to be passed to callback
> >>>> *
> >>>> * If the device provided is a bridge, 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 callback on the
> >>>> + * device itself.
> >>>> */
> >>>> static void pci_walk_bridge(struct pci_dev *bridge,
> >>>> 			    int (*cb)(struct pci_dev *, void *),
> >>>> @@ -162,6 +166,8 @@ static void pci_walk_bridge(struct pci_dev *bridge,
> >>>> {
> >>>> 	if (bridge->subordinate)
> >>>> 		pci_walk_bus(bridge->subordinate, cb, userdata);
> >>>> +	else
> >>>> +		cb(bridge, userdata);
> >>>> }
> >>>> 
> >>>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >>>> @@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >>>> 
> >>>> 	/*
> >>>> 	 * Error recovery runs on all subordinates of the bridge.  If the
> >>>> -	 * bridge detected the error, it is cleared at the end.
> >>>> +	 * bridge detected the error, it is cleared at the end.  For RCiEPs
> >>>> +	 * we should reset just the RCiEP itself.
> >>>> 	 */
> >>>> 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >>>> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
> >>>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> >>>> +	    type == PCI_EXP_TYPE_RC_EC ||
> >>>> +	    type == PCI_EXP_TYPE_RC_END)
> >>>> 		bridge = dev;
> >>>> 	else
> >>>> 		bridge = pci_upstream_bridge(dev);
> >>>> @@ -185,6 +194,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >>>> 	pci_dbg(bridge, "broadcast error_detected message\n");
> >>>> 	if (state == pci_channel_io_frozen) {
> >>>> 		pci_walk_bridge(bridge, report_frozen_detected, &status);
> >>>> +		if (type == PCI_EXP_TYPE_RC_END) {
> >>>> +			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
> >>>> +			status = PCI_ERS_RESULT_NONE;
> >>>> +			goto failed;
> >>>> +		}
> >>>> +
> >>>> 		status = reset_subordinates(bridge);
> >>>> 		if (status != PCI_ERS_RESULT_RECOVERED) {
> >>>> 			pci_warn(bridge, "subordinate device reset failed\n");
> >>>> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> >>>> 	pci_dbg(bridge, "broadcast resume message\n");
> >>>> 	pci_walk_bridge(bridge, report_resume, &status);
> >>>> 
> >>>> -	if (pcie_aer_is_native(bridge))
> >>>> -		pcie_clear_device_status(bridge);
> >>>> -	pci_aer_clear_nonfatal_status(bridge);
> >>>> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >>>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> >>>> +	    type == PCI_EXP_TYPE_RC_EC) {
> >>>> +		if (pcie_aer_is_native(bridge))
> >>>> +			pcie_clear_device_status(bridge);
> >>>> +		pci_aer_clear_nonfatal_status(bridge);
> >>> 
> >>> This is hard to understand because "type" is from "dev", but "bridge"
> >>> is not necessarily the same device.  Should it be this?
> >>> 
> >>> type = pci_pcie_type(bridge);
> >>> if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >>>     ...)
> >> 
> >> Correct, it would be better if the type was based on the ‘bridge’.
> > 
> > OK.  This is similar to
> > https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/,
> > which you cited above except for the bridge/dev question and the
> > addition here of RC_EC.
> > 
> > I tried to split that back into its own patch and started with the
> > commit message from that patch.  But I got stuck on the commit
> > message.  I got as far as:
> > 
> >  In some cases an error may be reported by a device not visible to
> >  the OS, e.g., if firmware manages the device and passes error
> >  information to the OS via ACPI APEI.
> > 
> > But I still can't quite connect that to the patch.  "bridge" is
> > clearly a device visible to Linux.
> > 
> > I guess we're trying to assert that if "bridge" is not a Root Port,
> > Downstream Port, or RCEC, we shouldn't clear the error status because 
> > the error came from a device Linux doesn't know about.  But I think
> > "bridge" is *always* either a Root Port or a Downstream Port:
> 
> That’s ultimately what we are trying to do.
> 
> >  if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >      type == PCI_EXP_TYPE_DOWNSTREAM)
> > 	  bridge = dev;
> >  else
> > 	  bridge = pci_upstream_bridge(dev);
> > 
> > pci_upstream_bridge() returns either NULL (in which case previous uses
> > dereference a NULL pointer), or dev->bus->self, which is always a Root
> > Port, Switch Downstream Port, or Switch Upstream Port (or NULL for the
> > special case of VFs).
> 
> In the past recall we were augmenting it with bridge = dev->rcec for
> RC_END.  But we were able to relocate the handling in
> aer_root_reset().
> 
> So in this patch - we add the conditionals because RC_END is being
> passed in addition to RC_EC.
>  
>  	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> 
> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
> 
> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> +	    type == PCI_EXP_TYPE_RC_EC ||
> +	    type == PCI_EXP_TYPE_RC_END)
> 
>  		bridge = dev;
>  	else
>  		bridge = pci_upstream_bridge(dev);
> 
> So we need to check for RP, DS, and RC_EC
> 
> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> 
>  	pci_dbg(bridge, "broadcast resume message\n");
>  	pci_walk_bridge(bridge, report_resume, &status);
>  
> 
> -	if (pcie_aer_is_native(bridge))
> -		pcie_clear_device_status(bridge);
> -	pci_aer_clear_nonfatal_status(bridge);
> 
> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> +	    type == PCI_EXP_TYPE_RC_EC) {
> +		if (pcie_aer_is_native(bridge))
> +			pcie_clear_device_status(bridge);
> +		pci_aer_clear_nonfatal_status(bridge);
> +	}
> 
> Breaking out a separate patch would be unnecessary as you correctly
> point out that it’s only going to be an RP or DS before this patch.

Still trying to sort this out in my head, so half-baked questions
before I quit for the day: We call pcie_do_recovery() in four paths:
AER, APEI, DPC, and EDR, and I'm trying to understand what "dev" we
pass in all those cases.

For DPC, I think "dev" must be a downstream port that triggered DPC,
and its secondary link is disabled.  The device and any siblings have
already been reset by the link going down.  We want to clear AER
status in downstream device(s) after they come back up (the AER status
bits are sticky, so they're not cleared by the reset), and the AER
status in the downstream port.

I think EDR is the same as DPC?

For AER, I think "dev" will typically (maybe always?) be the device
that detected the error and logged it in its AER Capability, not the
Root Port or RCEC that generated the interrupt.  We want to reset
"dev" and any siblings, clear AER status in "dev", and clear AER
status in the Root Port or RCEC.

For APEI, I assume "dev" is typically the device that detected the
error, and we want to reset it and any siblings.  Firmware has already
read the AER status for "dev", and I assume firmware also clears it.
I assume firmware is also responsible for clearing AER status in the
Root Port, RCEC, or non-architected device that generated the
interrupt.

It seems like there are basically two devices of interest in
pcie_do_recovery(): the endpoint where we have to call the driver
error recovery, and the port that generated the interrupt.  I wonder
if this would make more sense if the caller passed both of them in
explicitly instead of having pcie_do_recovery() check the type of
"dev" and try to figure things out after the fact.

Bjorn

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Kuppuswamy, Sathyanarayanan]

On 11/30/20 4:25 PM, Bjorn Helgaas wrote:
> I think EDR is the same as DPC?
Yes, EDR is same as DPC.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Kelley, Sean V]

> On Nov 30, 2020, at 4:25 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Mon, Nov 30, 2020 at 07:54:37PM +0000, Kelley, Sean V wrote:
>>> On Nov 24, 2020, at 9:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Mon, Nov 23, 2020 at 11:57:35PM +0000, Kelley, Sean V wrote:
>>>>> On Nov 23, 2020, at 3:28 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>> On Fri, Nov 20, 2020 at 04:10:31PM -0800, Sean V Kelley wrote:
>>>>>> In some cases a bridge may not exist as the hardware controlling may be
>>>>>> handled only by firmware and so is not visible to the OS. This scenario is
>>>>>> also possible in future use cases involving non-native use of RCECs by
>>>>>> firmware.
>>>>>> 
>>>>>> Explicitly apply conditional logic around these resets by limiting them to
>>>>>> Root Ports and Downstream Ports.
>>>>> 
>>>>> Can you help me understand this?  The subject says "Limit AER resets"
>>>>> and here you say "limit them to RPs and DPs", but it's not completely
>>>>> obvious how the resets are being limited, i.e., the patch doesn't add
>>>>> anything like:
>>>>> 
>>>>> +  if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>>> +      type == PCI_EXP_TYPE_DOWNSTREAM)
>>>>>    reset_subordinates(bridge);
>>>>> 
>>>>> It *does* add checks around pcie_clear_device_status(), but that also
>>>>> includes RC_EC.  And that's not a reset, so I don't think that's
>>>>> explicitly mentioned in the commit log.
>>>> 
>>>> The subject should have referred to the clearing of the device status rather than resets.
>>>> It originally came from this simpler patch in which I made use of reset instead of clear:
>>>> 
>>>> https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/
>>>> 
>>>> So a rephrase of clearing in place of resets would be more appropriate.
>>>> 
>>>> Then we added the notion of bridges…below
>>>> 
>>>>> 
>>>>> Also see the question below.
>>>>> 
>>>>>> Link: https://lore.kernel.org/r/20201002184735.1229220-8-seanvk.dev@oregontracks.org
>>>>>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>>>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>>> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>>> ---
>>>>>> drivers/pci/pcie/err.c | 31 +++++++++++++++++++++++++------
>>>>>> 1 file changed, 25 insertions(+), 6 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>>>> index 8b53aecdb43d..7883c9791562 100644
>>>>>> --- a/drivers/pci/pcie/err.c
>>>>>> +++ b/drivers/pci/pcie/err.c
>>>>>> @@ -148,13 +148,17 @@ static int report_resume(struct pci_dev *dev, void *data)
>>>>>> 
>>>>>> /**
>>>>>> * pci_walk_bridge - walk bridges potentially AER affected
>>>>>> - * @bridge:	bridge which may be a Port
>>>>>> + * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
>>>>>> + *		or an RCiEP associated with an RCEC
>>>>>> * @cb:		callback to be called for each device found
>>>>>> * @userdata:	arbitrary pointer to be passed to callback
>>>>>> *
>>>>>> * If the device provided is a bridge, 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 callback on the
>>>>>> + * device itself.
>>>>>> */
>>>>>> static void pci_walk_bridge(struct pci_dev *bridge,
>>>>>> 			    int (*cb)(struct pci_dev *, void *),
>>>>>> @@ -162,6 +166,8 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>>>>>> {
>>>>>> 	if (bridge->subordinate)
>>>>>> 		pci_walk_bus(bridge->subordinate, cb, userdata);
>>>>>> +	else
>>>>>> +		cb(bridge, userdata);
>>>>>> }
>>>>>> 
>>>>>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>>> @@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>>> 
>>>>>> 	/*
>>>>>> 	 * Error recovery runs on all subordinates of the bridge.  If the
>>>>>> -	 * bridge detected the error, it is cleared at the end.
>>>>>> +	 * bridge detected the error, it is cleared at the end.  For RCiEPs
>>>>>> +	 * we should reset just the RCiEP itself.
>>>>>> 	 */
>>>>>> 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>>>> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
>>>>>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>>>>>> +	    type == PCI_EXP_TYPE_RC_EC ||
>>>>>> +	    type == PCI_EXP_TYPE_RC_END)
>>>>>> 		bridge = dev;
>>>>>> 	else
>>>>>> 		bridge = pci_upstream_bridge(dev);
>>>>>> @@ -185,6 +194,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>>> 	pci_dbg(bridge, "broadcast error_detected message\n");
>>>>>> 	if (state == pci_channel_io_frozen) {
>>>>>> 		pci_walk_bridge(bridge, report_frozen_detected, &status);
>>>>>> +		if (type == PCI_EXP_TYPE_RC_END) {
>>>>>> +			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
>>>>>> +			status = PCI_ERS_RESULT_NONE;
>>>>>> +			goto failed;
>>>>>> +		}
>>>>>> +
>>>>>> 		status = reset_subordinates(bridge);
>>>>>> 		if (status != PCI_ERS_RESULT_RECOVERED) {
>>>>>> 			pci_warn(bridge, "subordinate device reset failed\n");
>>>>>> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>>> 	pci_dbg(bridge, "broadcast resume message\n");
>>>>>> 	pci_walk_bridge(bridge, report_resume, &status);
>>>>>> 
>>>>>> -	if (pcie_aer_is_native(bridge))
>>>>>> -		pcie_clear_device_status(bridge);
>>>>>> -	pci_aer_clear_nonfatal_status(bridge);
>>>>>> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>>>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>>>>>> +	    type == PCI_EXP_TYPE_RC_EC) {
>>>>>> +		if (pcie_aer_is_native(bridge))
>>>>>> +			pcie_clear_device_status(bridge);
>>>>>> +		pci_aer_clear_nonfatal_status(bridge);
>>>>> 
>>>>> This is hard to understand because "type" is from "dev", but "bridge"
>>>>> is not necessarily the same device.  Should it be this?
>>>>> 
>>>>> type = pci_pcie_type(bridge);
>>>>> if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>>>    ...)
>>>> 
>>>> Correct, it would be better if the type was based on the ‘bridge’.
>>> 
>>> OK.  This is similar to
>>> https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/,
>>> which you cited above except for the bridge/dev question and the
>>> addition here of RC_EC.
>>> 
>>> I tried to split that back into its own patch and started with the
>>> commit message from that patch.  But I got stuck on the commit
>>> message.  I got as far as:
>>> 
>>> In some cases an error may be reported by a device not visible to
>>> the OS, e.g., if firmware manages the device and passes error
>>> information to the OS via ACPI APEI.
>>> 
>>> But I still can't quite connect that to the patch.  "bridge" is
>>> clearly a device visible to Linux.
>>> 
>>> I guess we're trying to assert that if "bridge" is not a Root Port,
>>> Downstream Port, or RCEC, we shouldn't clear the error status because 
>>> the error came from a device Linux doesn't know about.  But I think
>>> "bridge" is *always* either a Root Port or a Downstream Port:
>> 
>> That’s ultimately what we are trying to do.
>> 
>>> if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>     type == PCI_EXP_TYPE_DOWNSTREAM)
>>> 	  bridge = dev;
>>> else
>>> 	  bridge = pci_upstream_bridge(dev);
>>> 
>>> pci_upstream_bridge() returns either NULL (in which case previous uses
>>> dereference a NULL pointer), or dev->bus->self, which is always a Root
>>> Port, Switch Downstream Port, or Switch Upstream Port (or NULL for the
>>> special case of VFs).
>> 
>> In the past recall we were augmenting it with bridge = dev->rcec for
>> RC_END.  But we were able to relocate the handling in
>> aer_root_reset().
>> 
>> So in this patch - we add the conditionals because RC_END is being
>> passed in addition to RC_EC.
>> 
>> 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>> 
>> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
>> 
>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>> +	    type == PCI_EXP_TYPE_RC_EC ||
>> +	    type == PCI_EXP_TYPE_RC_END)
>> 
>> 		bridge = dev;
>> 	else
>> 		bridge = pci_upstream_bridge(dev);
>> 
>> So we need to check for RP, DS, and RC_EC
>> 
>> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> 
>> 	pci_dbg(bridge, "broadcast resume message\n");
>> 	pci_walk_bridge(bridge, report_resume, &status);
>> 
>> 
>> -	if (pcie_aer_is_native(bridge))
>> -		pcie_clear_device_status(bridge);
>> -	pci_aer_clear_nonfatal_status(bridge);
>> 
>> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>> +	    type == PCI_EXP_TYPE_RC_EC) {
>> +		if (pcie_aer_is_native(bridge))
>> +			pcie_clear_device_status(bridge);
>> +		pci_aer_clear_nonfatal_status(bridge);
>> +	}
>> 
>> Breaking out a separate patch would be unnecessary as you correctly
>> point out that it’s only going to be an RP or DS before this patch.
> 
> Still trying to sort this out in my head, so half-baked questions
> before I quit for the day: We call pcie_do_recovery() in four paths:
> AER, APEI, DPC, and EDR, and I'm trying to understand what "dev" we
> pass in all those cases.
> 
> For DPC, I think "dev" must be a downstream port that triggered DPC,
> and its secondary link is disabled.  The device and any siblings have
> already been reset by the link going down.  We want to clear AER
> status in downstream device(s) after they come back up (the AER status
> bits are sticky, so they're not cleared by the reset), and the AER
> status in the downstream port.
> 
> I think EDR is the same as DPC?
> 
> For AER, I think "dev" will typically (maybe always?) be the device
> that detected the error and logged it in its AER Capability, not the
> Root Port or RCEC that generated the interrupt.  We want to reset
> "dev" and any siblings, clear AER status in "dev", and clear AER
> status in the Root Port or RCEC.


It’s also possible for RCEC’s to have errors themselves without a corresponding end point device.


Sean


> 
> For APEI, I assume "dev" is typically the device that detected the
> error, and we want to reset it and any siblings.  Firmware has already
> read the AER status for "dev", and I assume firmware also clears it.
> I assume firmware is also responsible for clearing AER status in the
> Root Port, RCEC, or non-architected device that generated the
> interrupt.
> 
> It seems like there are basically two devices of interest in
> pcie_do_recovery(): the endpoint where we have to call the driver
> error recovery, and the port that generated the interrupt.  I wonder
> if this would make more sense if the caller passed both of them in
> explicitly instead of having pcie_do_recovery() check the type of
> "dev" and try to figure things out after the fact.
> 
> Bjorn

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Kelley, Sean V]

Hi Bjorn,


> On Nov 30, 2020, at 4:25 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Mon, Nov 30, 2020 at 07:54:37PM +0000, Kelley, Sean V wrote:
>>> On Nov 24, 2020, at 9:17 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Mon, Nov 23, 2020 at 11:57:35PM +0000, Kelley, Sean V wrote:
>>>>> On Nov 23, 2020, at 3:28 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>> On Fri, Nov 20, 2020 at 04:10:31PM -0800, Sean V Kelley wrote:
>>>>>> In some cases a bridge may not exist as the hardware controlling may be
>>>>>> handled only by firmware and so is not visible to the OS. This scenario is
>>>>>> also possible in future use cases involving non-native use of RCECs by
>>>>>> firmware.
>>>>>> 
>>>>>> Explicitly apply conditional logic around these resets by limiting them to
>>>>>> Root Ports and Downstream Ports.
>>>>> 
>>>>> Can you help me understand this?  The subject says "Limit AER resets"
>>>>> and here you say "limit them to RPs and DPs", but it's not completely
>>>>> obvious how the resets are being limited, i.e., the patch doesn't add
>>>>> anything like:
>>>>> 
>>>>> +  if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>>> +      type == PCI_EXP_TYPE_DOWNSTREAM)
>>>>>    reset_subordinates(bridge);
>>>>> 
>>>>> It *does* add checks around pcie_clear_device_status(), but that also
>>>>> includes RC_EC.  And that's not a reset, so I don't think that's
>>>>> explicitly mentioned in the commit log.
>>>> 
>>>> The subject should have referred to the clearing of the device status rather than resets.
>>>> It originally came from this simpler patch in which I made use of reset instead of clear:
>>>> 
>>>> https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/
>>>> 
>>>> So a rephrase of clearing in place of resets would be more appropriate.
>>>> 
>>>> Then we added the notion of bridges…below
>>>> 
>>>>> 
>>>>> Also see the question below.
>>>>> 
>>>>>> Link: https://lore.kernel.org/r/20201002184735.1229220-8-seanvk.dev@oregontracks.org
>>>>>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>>>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>>> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>>> ---
>>>>>> drivers/pci/pcie/err.c | 31 +++++++++++++++++++++++++------
>>>>>> 1 file changed, 25 insertions(+), 6 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>>>> index 8b53aecdb43d..7883c9791562 100644
>>>>>> --- a/drivers/pci/pcie/err.c
>>>>>> +++ b/drivers/pci/pcie/err.c
>>>>>> @@ -148,13 +148,17 @@ static int report_resume(struct pci_dev *dev, void *data)
>>>>>> 
>>>>>> /**
>>>>>> * pci_walk_bridge - walk bridges potentially AER affected
>>>>>> - * @bridge:	bridge which may be a Port
>>>>>> + * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
>>>>>> + *		or an RCiEP associated with an RCEC
>>>>>> * @cb:		callback to be called for each device found
>>>>>> * @userdata:	arbitrary pointer to be passed to callback
>>>>>> *
>>>>>> * If the device provided is a bridge, 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 callback on the
>>>>>> + * device itself.
>>>>>> */
>>>>>> static void pci_walk_bridge(struct pci_dev *bridge,
>>>>>> 			    int (*cb)(struct pci_dev *, void *),
>>>>>> @@ -162,6 +166,8 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>>>>>> {
>>>>>> 	if (bridge->subordinate)
>>>>>> 		pci_walk_bus(bridge->subordinate, cb, userdata);
>>>>>> +	else
>>>>>> +		cb(bridge, userdata);
>>>>>> }
>>>>>> 
>>>>>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>>> @@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>>> 
>>>>>> 	/*
>>>>>> 	 * Error recovery runs on all subordinates of the bridge.  If the
>>>>>> -	 * bridge detected the error, it is cleared at the end.
>>>>>> +	 * bridge detected the error, it is cleared at the end.  For RCiEPs
>>>>>> +	 * we should reset just the RCiEP itself.
>>>>>> 	 */
>>>>>> 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>>>> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
>>>>>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>>>>>> +	    type == PCI_EXP_TYPE_RC_EC ||
>>>>>> +	    type == PCI_EXP_TYPE_RC_END)
>>>>>> 		bridge = dev;
>>>>>> 	else
>>>>>> 		bridge = pci_upstream_bridge(dev);
>>>>>> @@ -185,6 +194,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>>> 	pci_dbg(bridge, "broadcast error_detected message\n");
>>>>>> 	if (state == pci_channel_io_frozen) {
>>>>>> 		pci_walk_bridge(bridge, report_frozen_detected, &status);
>>>>>> +		if (type == PCI_EXP_TYPE_RC_END) {
>>>>>> +			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
>>>>>> +			status = PCI_ERS_RESULT_NONE;
>>>>>> +			goto failed;
>>>>>> +		}
>>>>>> +
>>>>>> 		status = reset_subordinates(bridge);
>>>>>> 		if (status != PCI_ERS_RESULT_RECOVERED) {
>>>>>> 			pci_warn(bridge, "subordinate device reset failed\n");
>>>>>> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>>> 	pci_dbg(bridge, "broadcast resume message\n");
>>>>>> 	pci_walk_bridge(bridge, report_resume, &status);
>>>>>> 
>>>>>> -	if (pcie_aer_is_native(bridge))
>>>>>> -		pcie_clear_device_status(bridge);
>>>>>> -	pci_aer_clear_nonfatal_status(bridge);
>>>>>> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>>>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>>>>>> +	    type == PCI_EXP_TYPE_RC_EC) {
>>>>>> +		if (pcie_aer_is_native(bridge))
>>>>>> +			pcie_clear_device_status(bridge);
>>>>>> +		pci_aer_clear_nonfatal_status(bridge);
>>>>> 
>>>>> This is hard to understand because "type" is from "dev", but "bridge"
>>>>> is not necessarily the same device.  Should it be this?
>>>>> 
>>>>> type = pci_pcie_type(bridge);
>>>>> if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>>>    ...)
>>>> 
>>>> Correct, it would be better if the type was based on the ‘bridge’.
>>> 
>>> OK.  This is similar to
>>> https://lore.kernel.org/linux-pci/20201002184735.1229220-8-seanvk.dev@oregontracks.org/,
>>> which you cited above except for the bridge/dev question and the
>>> addition here of RC_EC.
>>> 
>>> I tried to split that back into its own patch and started with the
>>> commit message from that patch.  But I got stuck on the commit
>>> message.  I got as far as:
>>> 
>>> In some cases an error may be reported by a device not visible to
>>> the OS, e.g., if firmware manages the device and passes error
>>> information to the OS via ACPI APEI.
>>> 
>>> But I still can't quite connect that to the patch.  "bridge" is
>>> clearly a device visible to Linux.
>>> 
>>> I guess we're trying to assert that if "bridge" is not a Root Port,
>>> Downstream Port, or RCEC, we shouldn't clear the error status because 
>>> the error came from a device Linux doesn't know about.  But I think
>>> "bridge" is *always* either a Root Port or a Downstream Port:
>> 
>> That’s ultimately what we are trying to do.
>> 
>>> if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>     type == PCI_EXP_TYPE_DOWNSTREAM)
>>> 	  bridge = dev;
>>> else
>>> 	  bridge = pci_upstream_bridge(dev);
>>> 
>>> pci_upstream_bridge() returns either NULL (in which case previous uses
>>> dereference a NULL pointer), or dev->bus->self, which is always a Root
>>> Port, Switch Downstream Port, or Switch Upstream Port (or NULL for the
>>> special case of VFs).
>> 
>> In the past recall we were augmenting it with bridge = dev->rcec for
>> RC_END.  But we were able to relocate the handling in
>> aer_root_reset().
>> 
>> So in this patch - we add the conditionals because RC_END is being
>> passed in addition to RC_EC.
>> 
>> 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>> 
>> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
>> 
>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>> +	    type == PCI_EXP_TYPE_RC_EC ||
>> +	    type == PCI_EXP_TYPE_RC_END)
>> 
>> 		bridge = dev;
>> 	else
>> 		bridge = pci_upstream_bridge(dev);
>> 
>> So we need to check for RP, DS, and RC_EC
>> 
>> @@ -217,9 +232,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> 
>> 	pci_dbg(bridge, "broadcast resume message\n");
>> 	pci_walk_bridge(bridge, report_resume, &status);
>> 
>> 
>> -	if (pcie_aer_is_native(bridge))
>> -		pcie_clear_device_status(bridge);
>> -	pci_aer_clear_nonfatal_status(bridge);
>> 
>> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>> +	    type == PCI_EXP_TYPE_RC_EC) {
>> +		if (pcie_aer_is_native(bridge))
>> +			pcie_clear_device_status(bridge);
>> +		pci_aer_clear_nonfatal_status(bridge);
>> +	}
>> 
>> Breaking out a separate patch would be unnecessary as you correctly
>> point out that it’s only going to be an RP or DS before this patch.
> 
> Still trying to sort this out in my head, so half-baked questions
> before I quit for the day: We call pcie_do_recovery() in four paths:
> AER, APEI, DPC, and EDR, and I'm trying to understand what "dev" we
> pass in all those cases.
> 
> For DPC, I think "dev" must be a downstream port that triggered DPC,
> and its secondary link is disabled.  The device and any siblings have
> already been reset by the link going down.  We want to clear AER
> status in downstream device(s) after they come back up (the AER status
> bits are sticky, so they're not cleared by the reset), and the AER
> status in the downstream port.
> 
> I think EDR is the same as DPC?
> 
> For AER, I think "dev" will typically (maybe always?) be the device
> that detected the error and logged it in its AER Capability, not the
> Root Port or RCEC that generated the interrupt.  We want to reset
> "dev" and any siblings, clear AER status in "dev", and clear AER
> status in the Root Port or RCEC.
> 
> For APEI, I assume "dev" is typically the device that detected the
> error, and we want to reset it and any siblings.  Firmware has already
> read the AER status for "dev", and I assume firmware also clears it.
> I assume firmware is also responsible for clearing AER status in the
> Root Port, RCEC, or non-architected device that generated the
> interrupt.
> 
> It seems like there are basically two devices of interest in
> pcie_do_recovery(): the endpoint where we have to call the driver
> error recovery, and the port that generated the interrupt.  I wonder
> if this would make more sense if the caller passed both of them in
> explicitly instead of having pcie_do_recovery() check the type of
> "dev" and try to figure things out after the fact.

On this last point I wanted to add that this is a possibility that could provide a clearer distinction,
especially where actions need to be taken or not taken as a part of pcie_do_recovery(), i.e., bridge versus dev.
In this patch series we have taken steps to minimize the need for the distinction by pushing the
awareness into the driver’s error recovery routine, i.e., dev->rcec.  A future evolution after this series
could lead to both devices of interest being passed explicitly for the larger scope EDR/DPC/AER/etc.

Thanks,

Sean



> 
> Bjorn

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Bjorn Helgaas]

On Wed, Dec 02, 2020 at 08:53:54PM +0000, Kelley, Sean V wrote:
> > On Nov 30, 2020, at 4:25 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Nov 30, 2020 at 07:54:37PM +0000, Kelley, Sean V wrote:

> >> -	if (pcie_aer_is_native(bridge))
> >> -		pcie_clear_device_status(bridge);
> >> -	pci_aer_clear_nonfatal_status(bridge);
> >> 
> >> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> >> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> >> +	    type == PCI_EXP_TYPE_RC_EC) {
> >> +		if (pcie_aer_is_native(bridge))
> >> +			pcie_clear_device_status(bridge);
> >> +		pci_aer_clear_nonfatal_status(bridge);
> >> +	}

Back to this specific hunk, what if we made it this?

  struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);

  if (host->native_aer || pcie_ports_native) {
    pcie_clear_device_status(bridge);
    pci_aer_clear_nonfatal_status(bridge);
  }

Previously, if "bridge" didn't have an AER Capability, we didn't
pcie_clear_device_status().  In the case of a DPC bridge without AER,
I think we *should* call pcie_clear_device_status().

Otherwise, I think this should work the same and would be a little
simpler.

> > It seems like there are basically two devices of interest in
> > pcie_do_recovery(): the endpoint where we have to call the driver
> > error recovery, and the port that generated the interrupt.  I wonder
> > if this would make more sense if the caller passed both of them in
> > explicitly instead of having pcie_do_recovery() check the type of
> > "dev" and try to figure things out after the fact.
> 
> On this last point I wanted to add that this is a possibility that
> could provide a clearer distinction, especially where actions need
> to be taken or not taken as a part of pcie_do_recovery(), i.e.,
> bridge versus dev.  In this patch series we have taken steps to
> minimize the need for the distinction by pushing the awareness into
> the driver’s error recovery routine, i.e., dev->rcec.  A future
> evolution after this series could lead to both devices of interest
> being passed explicitly for the larger scope EDR/DPC/AER/etc.

Yeah, not worth doing in *this* series.

Bjorn

Re: [PATCH v12 10/15] PCI/ERR: Limit AER resets in pcie_do_recovery()

[Kelley, Sean V]

Hi Bjorn,


> On Dec 2, 2020, at 1:27 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Wed, Dec 02, 2020 at 08:53:54PM +0000, Kelley, Sean V wrote:
>>> On Nov 30, 2020, at 4:25 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Mon, Nov 30, 2020 at 07:54:37PM +0000, Kelley, Sean V wrote:
> 
>>>> -	if (pcie_aer_is_native(bridge))
>>>> -		pcie_clear_device_status(bridge);
>>>> -	pci_aer_clear_nonfatal_status(bridge);
>>>> 
>>>> +	if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>>> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
>>>> +	    type == PCI_EXP_TYPE_RC_EC) {
>>>> +		if (pcie_aer_is_native(bridge))
>>>> +			pcie_clear_device_status(bridge);
>>>> +		pci_aer_clear_nonfatal_status(bridge);
>>>> +	}
> 
> Back to this specific hunk, what if we made it this?
> 
>  struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> 
>  if (host->native_aer || pcie_ports_native) {
>    pcie_clear_device_status(bridge);
>    pci_aer_clear_nonfatal_status(bridge);
>  }
> 
> Previously, if "bridge" didn't have an AER Capability, we didn't
> pcie_clear_device_status().  In the case of a DPC bridge without AER,
> I think we *should* call pcie_clear_device_status().

Agree, I was overlooking DPC here with the AER check.

> 
> Otherwise, I think this should work the same and would be a little
> simpler.

Looks fine to me.  It simplifies it a bit.

> 
>>> It seems like there are basically two devices of interest in
>>> pcie_do_recovery(): the endpoint where we have to call the driver
>>> error recovery, and the port that generated the interrupt.  I wonder
>>> if this would make more sense if the caller passed both of them in
>>> explicitly instead of having pcie_do_recovery() check the type of
>>> "dev" and try to figure things out after the fact.
>> 
>> On this last point I wanted to add that this is a possibility that
>> could provide a clearer distinction, especially where actions need
>> to be taken or not taken as a part of pcie_do_recovery(), i.e.,
>> bridge versus dev.  In this patch series we have taken steps to
>> minimize the need for the distinction by pushing the awareness into
>> the driver’s error recovery routine, i.e., dev->rcec.  A future
>> evolution after this series could lead to both devices of interest
>> being passed explicitly for the larger scope EDR/DPC/AER/etc.
> 
> Yeah, not worth doing in *this* series.
> 
> Bjorn

Thanks,

Sean

Re: [PATCH v12 07/15] PCI/ERR: Use "bridge" for clarity in pcie_do_recovery()

[Bjorn Helgaas]

On Fri, Nov 20, 2020 at 04:10:28PM -0800, Sean V Kelley wrote:
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 7a5af873d8bc..46a5b84f8842 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -151,24 +151,27 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  		pci_ers_result_t (*reset_subordinates)(struct pci_dev *pdev))
>  {
>  	int type = pci_pcie_type(dev);
> -	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
> +	struct pci_dev *bridge;
>  	struct pci_bus *bus;
> +	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
>  
>  	/*
> -	 * Error recovery runs on all subordinates of the first downstream port.
> -	 * If the downstream port detected the error, it is cleared at the end.
> +	 * Error recovery runs on all subordinates of the bridge.  If the
> +	 * bridge detected the error, it is cleared at the end.
>  	 */
>  	if (!(type == PCI_EXP_TYPE_ROOT_PORT ||
>  	      type == PCI_EXP_TYPE_DOWNSTREAM))
> -		dev = pci_upstream_bridge(dev);
> -	bus = dev->subordinate;
> +		bridge = pci_upstream_bridge(dev);
> +	else
> +		bridge = dev;

I think there's a bug here even before your series.  We started with:

  pcie_do_recovery(struct pci_dev *dev, ..., pci_ers_result_t (*reset_link)())
  {
    if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
	  pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
      dev = dev->bus->self;
    ...
    reset_link(dev);

so if we called pcie_do_recovery() with an Endpoint, we set "dev" to
the upstream bridge, either a Root Port or a Switch Downstream Port,
which we then pass on to reset_link().  For native AER and APEI,
that's aer_root_reset(), which assumes it gets a Root Port.

If we pass a Switch Downstream Port, aer_root_reset() writes to the
*switch port's* PCI_ERR_ROOT_COMMAND and PCI_ERR_ROOT_STATUS, which
are reserved since it's not a Root Port or an RCEC.

The writes probably don't *break* anything since those registers are
reserved, but they also don't disable the interrupt or clear the Root
Error Status.

Bjorn

Re: [PATCH v12 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

[Bjorn Helgaas]

On Fri, Nov 20, 2020 at 04:10:33PM -0800, Sean V Kelley 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.  In
> some non-native cases in which there is no OS-visible device associated
> with the RCiEP, there is nothing to act upon as the firmware is acting
> before the OS.
> 
> Add handling for the linked RCEC in AER/ERR while taking into account
> non-native cases.
> 
> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> Link: https://lore.kernel.org/r/20201002184735.1229220-12-seanvk.dev@oregontracks.org
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/pci/pcie/aer.c | 46 +++++++++++++++++++++++++++++++-----------
>  drivers/pci/pcie/err.c | 20 +++++++++---------
>  2 files changed, 44 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 0ba0b47ae751..51389a6ee4ca 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1358,29 +1358,51 @@ 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 type = pci_pcie_type(dev);
> +	struct pci_dev *root;
> +	int aer = 0;
> +	int rc = 0;
>  	u32 reg32;
> -	int rc;
>  
> -	if (pcie_aer_is_native(dev)) {
> +	if (type == PCI_EXP_TYPE_RC_END)
> +		/*
> +		 * The reset should only clear the Root Error Status
> +		 * of the RCEC. Only perform this for the
> +		 * native case, i.e., an RCEC is present.
> +		 */
> +		root = dev->rcec;
> +	else
> +		root = dev;
> +
> +	if (root)
> +		aer = dev->aer_cap;
> +
> +	if ((aer) && pcie_aer_is_native(dev)) {
>  		/* Disable Root's interrupt in response to error messages */
> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>  		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
>  	}
>  
> -	rc = pci_bus_error_reset(dev);
> -	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
> +	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
> +		if (pcie_has_flr(dev)) {
> +			rc = pcie_flr(dev);
> +			pci_info(dev, "has been reset (%d)\n", rc);

Maybe:

  +             } else {
  +                     rc = -ENOTTY;
  +                     pci_info(dev, "not reset (no FLR support)\n");

Or do we want to pretend the device was reset and return
PCI_ERS_RESULT_RECOVERED?

> +	} else {
> +		rc = pci_bus_error_reset(dev);
> +		pci_info(dev, "Root Port link has been reset (%d)\n", rc);
> +	}
>  
> -	if (pcie_aer_is_native(dev)) {
> +	if ((aer) && pcie_aer_is_native(dev)) {
>  		/* Clear Root Error Status */
> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
>  
>  		/* Enable Root Port's interrupt in response to error messages */
> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>  		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
>  	}
>  
>  	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 7883c9791562..cbc5abfe767b 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -148,10 +148,10 @@ static int report_resume(struct pci_dev *dev, void *data)
>  
>  /**
>   * pci_walk_bridge - walk bridges potentially AER affected
> - * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
> - *		or an RCiEP associated with an RCEC
> - * @cb:		callback to be called for each device found
> - * @userdata:	arbitrary pointer to be passed to callback
> + * @bridge   bridge which may be an RCEC with associated RCiEPs,
> + *           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 bridge, walk the subordinate bus, including
>   * any bridged devices on buses under this bus.  Call the provided callback
> @@ -164,8 +164,14 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>  			    int (*cb)(struct pci_dev *, void *),
>  			    void *userdata)
>  {
> +	/*
> +	 * In a non-native case where there is no OS-visible reporting
> +	 * device the bridge will be NULL, i.e., no RCEC, no Downstream Port.

I don't quite understand this comment.  I see that in the non-native
case, the reporting device may not be OS-visible.  But I don't
understand why the comment is *here*.

If "bridge" can be NULL here, we should test that before dereferencing
"bridge->subordinate".

>  	if (bridge->subordinate)
>  		pci_walk_bus(bridge->subordinate, cb, userdata);
> +	else if (bridge->rcec)
> +		cb(bridge->rcec, userdata);

And I don't understand what's going on here.  In this case, I *think*
"bridge" is an RCiEP and "bridge->rcec" is the related RCEC, so it
looks like we'll call report_frozen_detected(), report_mmio_enabled(),
etc for the RCEC driver.  I would think we'd want the RCiEP driver.

Sorry if I'm missing the obvious.

>  	else
>  		cb(bridge, userdata);
>  }
> @@ -194,12 +200,6 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  	pci_dbg(bridge, "broadcast error_detected message\n");
>  	if (state == pci_channel_io_frozen) {
>  		pci_walk_bridge(bridge, report_frozen_detected, &status);
> -		if (type == PCI_EXP_TYPE_RC_END) {
> -			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
> -			status = PCI_ERS_RESULT_NONE;
> -			goto failed;
> -		}
> -
>  		status = reset_subordinates(bridge);
>  		if (status != PCI_ERS_RESULT_RECOVERED) {
>  			pci_warn(bridge, "subordinate device reset failed\n");
> -- 
> 2.29.2
> 

Re: [PATCH v12 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

[Kelley, Sean V]

> On Dec 2, 2020, at 3:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Fri, Nov 20, 2020 at 04:10:33PM -0800, Sean V Kelley 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.  In
>> some non-native cases in which there is no OS-visible device associated
>> with the RCiEP, there is nothing to act upon as the firmware is acting
>> before the OS.
>> 
>> Add handling for the linked RCEC in AER/ERR while taking into account
>> non-native cases.
>> 
>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Link: https://lore.kernel.org/r/20201002184735.1229220-12-seanvk.dev@oregontracks.org
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> ---
>> drivers/pci/pcie/aer.c | 46 +++++++++++++++++++++++++++++++-----------
>> drivers/pci/pcie/err.c | 20 +++++++++---------
>> 2 files changed, 44 insertions(+), 22 deletions(-)
>> 
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index 0ba0b47ae751..51389a6ee4ca 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -1358,29 +1358,51 @@ 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 type = pci_pcie_type(dev);
>> +	struct pci_dev *root;
>> +	int aer = 0;
>> +	int rc = 0;
>> 	u32 reg32;
>> -	int rc;
>> 
>> -	if (pcie_aer_is_native(dev)) {
>> +	if (type == PCI_EXP_TYPE_RC_END)
>> +		/*
>> +		 * The reset should only clear the Root Error Status
>> +		 * of the RCEC. Only perform this for the
>> +		 * native case, i.e., an RCEC is present.
>> +		 */
>> +		root = dev->rcec;
>> +	else
>> +		root = dev;
>> +
>> +	if (root)
>> +		aer = dev->aer_cap;
>> +
>> +	if ((aer) && pcie_aer_is_native(dev)) {
>> 		/* Disable Root's interrupt in response to error messages */
>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>> 		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> 	}
>> 
>> -	rc = pci_bus_error_reset(dev);
>> -	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>> +	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
>> +		if (pcie_has_flr(dev)) {
>> +			rc = pcie_flr(dev);
>> +			pci_info(dev, "has been reset (%d)\n", rc);
> 
> Maybe:
> 
>  +             } else {
>  +                     rc = -ENOTTY;
>  +                     pci_info(dev, "not reset (no FLR support)\n");
> 
> Or do we want to pretend the device was reset and return
> PCI_ERS_RESULT_RECOVERED?

We are currently doing the latter now with the default of rc = 0 above and so  I’m not sure the extra detail here on the absence of FLR support is of value.


> 
>> +	} else {
>> +		rc = pci_bus_error_reset(dev);
>> +		pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>> +	}
>> 
>> -	if (pcie_aer_is_native(dev)) {
>> +	if ((aer) && pcie_aer_is_native(dev)) {
>> 		/* Clear Root Error Status */
>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
>> 
>> 		/* Enable Root Port's interrupt in response to error messages */
>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>> 		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> 	}
>> 
>> 	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index 7883c9791562..cbc5abfe767b 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -148,10 +148,10 @@ static int report_resume(struct pci_dev *dev, void *data)
>> 
>> /**
>>  * pci_walk_bridge - walk bridges potentially AER affected
>> - * @bridge:	bridge which may be a Port, an RCEC with associated RCiEPs,
>> - *		or an RCiEP associated with an RCEC
>> - * @cb:		callback to be called for each device found
>> - * @userdata:	arbitrary pointer to be passed to callback
>> + * @bridge   bridge which may be an RCEC with associated RCiEPs,
>> + *           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 bridge, walk the subordinate bus, including
>>  * any bridged devices on buses under this bus.  Call the provided callback
>> @@ -164,8 +164,14 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>> 			    int (*cb)(struct pci_dev *, void *),
>> 			    void *userdata)
>> {
>> +	/*
>> +	 * In a non-native case where there is no OS-visible reporting
>> +	 * device the bridge will be NULL, i.e., no RCEC, no Downstream Port.
> 
> I don't quite understand this comment.  I see that in the non-native
> case, the reporting device may not be OS-visible.  But I don't
> understand why the comment is *here*.
> 
> If "bridge" can be NULL here, we should test that before dereferencing
> "bridge->subordinate".

Wrongly worded.  The subordinate may be NULL or the associated RCEC may be NULL, not the “bridge”.
However, per below, we should not be trying to call report_frozen_detected(), report_mmio_enabled() via
the associated RCEC’s driver, but rather the CB for the RCiEP itself.

Going back to this conversation,

https://lore.kernel.org/linux-pci/20201016172210.GA86168@bjorn-Precision-5520/

"Looks like *this* is the patch where the "no subordinate bus" case
becomes possible?  If you agree, I can just move the test here, no
need to repost.”

It is actually the case we are only dealing with the absence of a subordinate bus.

> 
>> 	if (bridge->subordinate)
>> 		pci_walk_bus(bridge->subordinate, cb, userdata);
>> +	else if (bridge->rcec)
>> +		cb(bridge->rcec, userdata);
> 
> And I don't understand what's going on here.  In this case, I *think*
> "bridge" is an RCiEP and "bridge->rcec" is the related RCEC, so it
> looks like we'll call report_frozen_detected(), report_mmio_enabled(),
> etc for the RCEC driver.  I would think we'd want the RCiEP driver.

Indeed, the bridge->rcec here is the dev->rcec in which the dev is the RCiEP.

And we don’t need that conditional here, it should just hit the device driver’s routines.

This is an unfortunate side effect of the RCiEP being subordinate to the RCEC but for
 purposes of linking, it gives the impression of the other way around.


> 
> Sorry if I'm missing the obvious.

Actually your observations are on point.

Thanks,

Sean

> 
>> 	else
>> 		cb(bridge, userdata);
>> }
>> @@ -194,12 +200,6 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> 	pci_dbg(bridge, "broadcast error_detected message\n");
>> 	if (state == pci_channel_io_frozen) {
>> 		pci_walk_bridge(bridge, report_frozen_detected, &status);
>> -		if (type == PCI_EXP_TYPE_RC_END) {
>> -			pci_warn(dev, "subordinate device reset not possible for RCiEP\n");
>> -			status = PCI_ERS_RESULT_NONE;
>> -			goto failed;
>> -		}
>> -
>> 		status = reset_subordinates(bridge);
>> 		if (status != PCI_ERS_RESULT_RECOVERED) {
>> 			pci_warn(bridge, "subordinate device reset failed\n");
>> -- 
>> 2.29.2
>> 

Re: [PATCH v12 05/15] PCI/ERR: Simplify by using pci_upstream_bridge()

[Kelley, Sean V]

Hi Bjorn,

Just confirming that when switched from dev->bus->self to pci_upstream_bridge() we’re okay with the NULL case:

> On Nov 20, 2020, at 4:10 PM, Sean V Kelley <sean.v.kelley@intel.com> wrote:
> 
> Use pci_upstream_bridge() in place of dev->bus->self.  No functional change
> intended.
> 
> [bhelgaas: split to separate patch]
> Link: https://lore.kernel.org/r/20201002184735.1229220-6-seanvk.dev@oregontracks.org
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> drivers/pci/pcie/err.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index db149c6ce4fb..05f61da5ed9d 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -159,7 +159,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> 	 */
> 	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> 	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
> -		dev = dev->bus->self;
> +		dev = pci_upstream_bridge(dev);


The only case where pci_upstream_bridge(dev) could be NULL is if dev->bus is the root bus and we are being
selective based on the type.

Even later in this series when we actually add in RC_EC/RC_END we maintain the conditional checks:

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

 pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,

@@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,

 
 	/*
 	 * Error recovery runs on all subordinates of the bridge.  If the
-	 * bridge detected the error, it is cleared at the end.
+	 * bridge detected the error, it is cleared at the end.  For RCiEPs
+	 * we should reset just the RCiEP itself.

 	 */
 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
-	    type == PCI_EXP_TYPE_DOWNSTREAM)
+	    type == PCI_EXP_TYPE_DOWNSTREAM ||
+	    type == PCI_EXP_TYPE_RC_EC ||
+	    type == PCI_EXP_TYPE_RC_END)
 		bridge = dev;
 	else
 		bridge = pci_upstream_bridge(dev);

I believe we are okay here.

Sean


> 	bus = dev->subordinate;
> 
> 	pci_dbg(dev, "broadcast error_detected message\n");
> -- 
> 2.29.2
> 
> 

Re: [PATCH v12 05/15] PCI/ERR: Simplify by using pci_upstream_bridge()

[Bjorn Helgaas]

On Thu, Dec 03, 2020 at 06:45:00PM +0000, Kelley, Sean V wrote:
> Just confirming that when switched from dev->bus->self to
> pci_upstream_bridge() we’re okay with the NULL case:
> 
> > On Nov 20, 2020, at 4:10 PM, Sean V Kelley <sean.v.kelley@intel.com> wrote:
> > 
> > Use pci_upstream_bridge() in place of dev->bus->self.  No functional change
> > intended.
> > 
> > [bhelgaas: split to separate patch]
> > Link: https://lore.kernel.org/r/20201002184735.1229220-6-seanvk.dev@oregontracks.org
> > Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > ---
> > drivers/pci/pcie/err.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > index db149c6ce4fb..05f61da5ed9d 100644
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -159,7 +159,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> > 	 */
> > 	if (!(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > 	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
> > -		dev = dev->bus->self;
> > +		dev = pci_upstream_bridge(dev);
> 
> The only case where pci_upstream_bridge(dev) could be NULL is if
> dev->bus is the root bus and we are being selective based on the
> type.

Yes, pci_upstream_bridge(dev) returns NULL if dev is on a root bus.
I think bus->self is NULL for root buses anyway, so I think this patch
is OK at least at this point in the series.

> Even later in this series when we actually add in RC_EC/RC_END we
> maintain the conditional checks:
> 
> https://lore.kernel.org/linux-pci/20201121001036.8560-11-sean.v.kelley@intel.com/
> 
>  pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> 
> @@ -174,10 +180,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> 
>  
>  	/*
>  	 * Error recovery runs on all subordinates of the bridge.  If the
> -	 * bridge detected the error, it is cleared at the end.
> +	 * bridge detected the error, it is cleared at the end.  For RCiEPs
> +	 * we should reset just the RCiEP itself.
> 
>  	 */
>  	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> -	    type == PCI_EXP_TYPE_DOWNSTREAM)
> +	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> +	    type == PCI_EXP_TYPE_RC_EC ||
> +	    type == PCI_EXP_TYPE_RC_END)
>  		bridge = dev;
>  	else
>  		bridge = pci_upstream_bridge(dev);
> 
> I believe we are okay here.

Well, I think we're at least no worse than today.  Today we keep
"bridge == dev" for Root Ports and Downstream Ports, and use
"bridge = dev->bus->self" for everything else.

After this series we keep "bridge == dev" for RCEC and RCiEPs in
addition.

I'm not sure about PCI_EXP_TYPE_PCI_BRIDGE, which is a PCIe-to-PCI
bridge.  According to the ancient "PCI Express to PCI/PCI-X Bridge
Specification" r1.0 of July 14, 2003, sec 5.23, these bridges can have
an AER Capability (with different format that I don't see mentioned
elsewhere).

But this AER Capability doesn't have the Root registers, so I assume
it would have to be below a Root Port in order to actually signal the
errors, which would mean it could not be on a root bus.

So I don't really *love* the fact that we use pci_upstream_bridge()
and rely on these assumptions that the result will not be NULL, but I
guess it's OK for now.

> > 	bus = dev->subordinate;
> > 
> > 	pci_dbg(dev, "broadcast error_detected message\n");
> > -- 
> > 2.29.2
> > 
> > 
> 

Re: [PATCH v12 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

[Bjorn Helgaas]

On Thu, Dec 03, 2020 at 12:51:40AM +0000, Kelley, Sean V wrote:
> > On Dec 2, 2020, at 3:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Nov 20, 2020 at 04:10:33PM -0800, Sean V Kelley 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.  In
> >> some non-native cases in which there is no OS-visible device associated
> >> with the RCiEP, there is nothing to act upon as the firmware is acting
> >> before the OS.
> >> 
> >> Add handling for the linked RCEC in AER/ERR while taking into account
> >> non-native cases.
> >> 
> >> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> >> Link: https://lore.kernel.org/r/20201002184735.1229220-12-seanvk.dev@oregontracks.org
> >> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> >> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >> ---
> >> drivers/pci/pcie/aer.c | 46 +++++++++++++++++++++++++++++++-----------
> >> drivers/pci/pcie/err.c | 20 +++++++++---------
> >> 2 files changed, 44 insertions(+), 22 deletions(-)
> >> 
> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> >> index 0ba0b47ae751..51389a6ee4ca 100644
> >> --- a/drivers/pci/pcie/aer.c
> >> +++ b/drivers/pci/pcie/aer.c
> >> @@ -1358,29 +1358,51 @@ 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 type = pci_pcie_type(dev);
> >> +	struct pci_dev *root;
> >> +	int aer = 0;
> >> +	int rc = 0;
> >> 	u32 reg32;
> >> -	int rc;
> >> 
> >> -	if (pcie_aer_is_native(dev)) {
> >> +	if (type == PCI_EXP_TYPE_RC_END)
> >> +		/*
> >> +		 * The reset should only clear the Root Error Status
> >> +		 * of the RCEC. Only perform this for the
> >> +		 * native case, i.e., an RCEC is present.
> >> +		 */
> >> +		root = dev->rcec;
> >> +	else
> >> +		root = dev;
> >> +
> >> +	if (root)
> >> +		aer = dev->aer_cap;
> >> +
> >> +	if ((aer) && pcie_aer_is_native(dev)) {
> >> 		/* Disable Root's interrupt in response to error messages */
> >> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> >> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> >> 		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> >> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> >> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> >> 	}
> >> 
> >> -	rc = pci_bus_error_reset(dev);
> >> -	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
> >> +	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
> >> +		if (pcie_has_flr(dev)) {
> >> +			rc = pcie_flr(dev);
> >> +			pci_info(dev, "has been reset (%d)\n", rc);
> > 
> > Maybe:
> > 
> >  +             } else {
> >  +                     rc = -ENOTTY;
> >  +                     pci_info(dev, "not reset (no FLR support)\n");
> > 
> > Or do we want to pretend the device was reset and return
> > PCI_ERS_RESULT_RECOVERED?
> 
> We are currently doing the latter now with the default of rc = 0
> above and so  I’m not sure the extra detail here on the absence of
> FLR support is of value.

So to make sure I understand the proposal here, for RCECs and RCiEPs
that don't support FLR, you're saying you want to continue silently
and return PCI_ERS_RESULT_RECOVERED and let the drivers assume their
device was reset when it was not?

> >> +	} else {
> >> +		rc = pci_bus_error_reset(dev);
> >> +		pci_info(dev, "Root Port link has been reset (%d)\n", rc);
> >> +	}
> >> 
> >> -	if (pcie_aer_is_native(dev)) {
> >> +	if ((aer) && pcie_aer_is_native(dev)) {
> >> 		/* Clear Root Error Status */
> >> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
> >> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
> >> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
> >> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
> >> 
> >> 		/* Enable Root Port's interrupt in response to error messages */
> >> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> >> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> >> 		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> >> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> >> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> >> 	}
> >> 
> >> 	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;

> >> @@ -164,8 +164,14 @@ static void pci_walk_bridge(struct pci_dev *bridge,
> >> 			    int (*cb)(struct pci_dev *, void *),
> >> 			    void *userdata)
> >> {
> >> +	/*
> >> +	 * In a non-native case where there is no OS-visible reporting
> >> +	 * device the bridge will be NULL, i.e., no RCEC, no Downstream Port.
> > 
> > I don't quite understand this comment.  I see that in the non-native
> > case, the reporting device may not be OS-visible.  But I don't
> > understand why the comment is *here*.
> > 
> > If "bridge" can be NULL here, we should test that before dereferencing
> > "bridge->subordinate".
> 
> Wrongly worded.  The subordinate may be NULL or the associated RCEC
> may be NULL, not the “bridge”.  However, per below, we should not be
> trying to call report_frozen_detected(), report_mmio_enabled() via
> the associated RCEC’s driver, but rather the CB for the RCiEP
> itself.

OK, so if we want a comment here, I assume it would be along the lines
of:

  If "bridge" has no subordinate bus, it's an RCEC or an RCiEP.  In
  either of those cases, we want to call the callback on "bridge"
  itself.

> Going back to this conversation,
> 
> https://lore.kernel.org/linux-pci/20201016172210.GA86168@bjorn-Precision-5520/
> 
> "Looks like *this* is the patch where the "no subordinate bus" case
> becomes possible?  If you agree, I can just move the test here, no
> need to repost.”
> 
> It is actually the case we are only dealing with the absence of a
> subordinate bus.
> 
> >> 	if (bridge->subordinate)
> >> 		pci_walk_bus(bridge->subordinate, cb, userdata);
> >> +	else if (bridge->rcec)
> >> +		cb(bridge->rcec, userdata);
> > 
> > And I don't understand what's going on here.  In this case, I *think*
> > "bridge" is an RCiEP and "bridge->rcec" is the related RCEC, so it
> > looks like we'll call report_frozen_detected(), report_mmio_enabled(),
> > etc for the RCEC driver.  I would think we'd want the RCiEP driver.
> 
> Indeed, the bridge->rcec here is the dev->rcec in which the dev is
> the RCiEP.
> 
> And we don’t need that conditional here, it should just hit the
> device driver’s routines.

So IIUC, the code would be:

  if (bridge->subordinate)
    pci_walk_bus(bridge->subordinate, cb, userdata);
  else
    cb(bridge, userdata);    /* RCEC or RCiEP */

Right?

I pushed a pci/err branch
(https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/err)
with some tweaks in these areas.  Diff from your v12 posting appended
below.  I split the RCEC/RCiEP error recovery pieces up a little bit
differently than in your posting.  Let me know if you see anything
that should be changed.  I dropped one of Jonathan's
reviewed/tested-by but probably should have dropped others to avoid
putting words in his mouth.

Not sure we're completely done, but we'll get there yet.  I definitely
want to make sure this happens this cycle.

Bjorn


diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index b86a92494345..4aa118edde35 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1366,33 +1366,38 @@ static int aer_probe(struct pcie_device *dev)
 }
 
 /**
- * aer_root_reset - reset link on Root Port
- * @dev: pointer to Root Port's pci_dev data structure
+ * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
+ * @dev: pointer to Root Port, RCEC, or RCiEP
  *
- * Invoked by Port Bus driver when performing link reset at Root Port.
+ * Invoked by Port Bus driver when performing reset.
  */
 static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
 	struct pci_dev *root;
-	int aer = 0;
-	int rc = 0;
+	int aer;
+	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
 	u32 reg32;
+	int rc;
 
+	/*
+	 * Only Root Ports and RCECs have AER Root Command and Root Status
+	 * registers.  If "dev" is an RCiEP, the relevant registers are in
+	 * the RCEC.
+	 */
 	if (type == PCI_EXP_TYPE_RC_END)
-		/*
-		 * The reset should only clear the Root Error Status
-		 * of the RCEC. Only perform this for the
-		 * native case, i.e., an RCEC is present.
-		 */
 		root = dev->rcec;
 	else
 		root = dev;
 
-	if (root)
-		aer = dev->aer_cap;
+	/*
+	 * If the platform retained control of AER, an RCiEP may not have
+	 * an RCEC visible to us, so dev->rcec ("root") may be NULL.  In
+	 * that case, firmware is responsible for these registers.
+	 */
+	aer = root ? root->aer_cap : 0;
 
-	if ((aer) && pcie_aer_is_native(dev)) {
+	if ((host->native_aer || pcie_ports_native) && aer) {
 		/* Disable Root's interrupt in response to error messages */
 		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
 		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
@@ -1403,13 +1408,15 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 		if (pcie_has_flr(dev)) {
 			rc = pcie_flr(dev);
 			pci_info(dev, "has been reset (%d)\n", rc);
+		} else {
+			pci_info(dev, "not reset (no FLR support)\n");
 		}
 	} else {
 		rc = pci_bus_error_reset(dev);
 		pci_info(dev, "Root Port link has been reset (%d)\n", rc);
 	}
 
-	if ((aer) && pcie_aer_is_native(dev)) {
+	if ((host->native_aer || pcie_ports_native) && aer) {
 		/* Clear Root Error Status */
 		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
 		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index cbc5abfe767b..510f31f0ef6d 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -148,30 +148,23 @@ static int report_resume(struct pci_dev *dev, void *data)
 
 /**
  * pci_walk_bridge - walk bridges potentially AER affected
- * @bridge   bridge which may be an RCEC with associated RCiEPs,
- *           or a Port.
- * @cb       callback to be called for each device found
- * @userdata arbitrary pointer to be passed to callback.
+ * @bridge:	bridge which may be a Port, an RCEC, or an RCiEP
+ * @cb:		callback to be called for each device found
+ * @userdata:	arbitrary pointer to be passed to callback
  *
  * If the device provided is a bridge, 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 callback on the
- * device itself.
+ * If the device provided has no subordinate bus, e.g., an RCEC or RCiEP,
+ * call the callback on the device itself.
  */
 static void pci_walk_bridge(struct pci_dev *bridge,
 			    int (*cb)(struct pci_dev *, void *),
 			    void *userdata)
 {
-	/*
-	 * In a non-native case where there is no OS-visible reporting
-	 * device the bridge will be NULL, i.e., no RCEC, no Downstream Port.
-	 */
 	if (bridge->subordinate)
 		pci_walk_bus(bridge->subordinate, cb, userdata);
-	else if (bridge->rcec)
-		cb(bridge->rcec, userdata);
 	else
 		cb(bridge, userdata);
 }
@@ -183,11 +176,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	int type = pci_pcie_type(dev);
 	struct pci_dev *bridge;
 	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
+	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
 
 	/*
-	 * Error recovery runs on all subordinates of the bridge.  If the
-	 * bridge detected the error, it is cleared at the end.  For RCiEPs
-	 * we should reset just the RCiEP itself.
+	 * If the error was detected by a Root Port, Downstream Port, RCEC,
+	 * or RCiEP, recovery runs on the device itself.  For Ports, that
+	 * also includes any subordinate devices.
+	 *
+	 * If it was detected by another device (Endpoint, etc), recovery
+	 * runs on the device and anything else under the same Port, i.e.,
+	 * everything under "bridge".
 	 */
 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
 	    type == PCI_EXP_TYPE_DOWNSTREAM ||
@@ -232,11 +230,15 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	pci_dbg(bridge, "broadcast resume message\n");
 	pci_walk_bridge(bridge, report_resume, &status);
 
-	if (type == PCI_EXP_TYPE_ROOT_PORT ||
-	    type == PCI_EXP_TYPE_DOWNSTREAM ||
-	    type == PCI_EXP_TYPE_RC_EC) {
-		if (pcie_aer_is_native(bridge))
-			pcie_clear_device_status(bridge);
+	/*
+	 * If we have native control of AER, clear error status in the Root
+	 * Port or Downstream Port that signaled the error.  If the
+	 * platform retained control of AER, it is responsible for clearing
+	 * this status.  In that case, the signaling device may not even be
+	 * visible to the OS.
+	 */
+	if (host->native_aer || pcie_ports_native) {
+		pcie_clear_device_status(bridge);
 		pci_aer_clear_nonfatal_status(bridge);
 	}
 	pci_info(bridge, "device recovery successful\n");

Re: [PATCH v12 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

[Kelley, Sean V]

Hi Bjorn,

> On Dec 3, 2020, at 4:01 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Thu, Dec 03, 2020 at 12:51:40AM +0000, Kelley, Sean V wrote:
>>> On Dec 2, 2020, at 3:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Fri, Nov 20, 2020 at 04:10:33PM -0800, Sean V Kelley 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.  In
>>>> some non-native cases in which there is no OS-visible device associated
>>>> with the RCiEP, there is nothing to act upon as the firmware is acting
>>>> before the OS.
>>>> 
>>>> Add handling for the linked RCEC in AER/ERR while taking into account
>>>> non-native cases.
>>>> 
>>>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>>>> Link: https://lore.kernel.org/r/20201002184735.1229220-12-seanvk.dev@oregontracks.org
>>>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>>>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> ---
>>>> drivers/pci/pcie/aer.c | 46 +++++++++++++++++++++++++++++++-----------
>>>> drivers/pci/pcie/err.c | 20 +++++++++---------
>>>> 2 files changed, 44 insertions(+), 22 deletions(-)
>>>> 
>>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>>> index 0ba0b47ae751..51389a6ee4ca 100644
>>>> --- a/drivers/pci/pcie/aer.c
>>>> +++ b/drivers/pci/pcie/aer.c
>>>> @@ -1358,29 +1358,51 @@ 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 type = pci_pcie_type(dev);
>>>> +	struct pci_dev *root;
>>>> +	int aer = 0;
>>>> +	int rc = 0;
>>>> 	u32 reg32;
>>>> -	int rc;
>>>> 
>>>> -	if (pcie_aer_is_native(dev)) {
>>>> +	if (type == PCI_EXP_TYPE_RC_END)
>>>> +		/*
>>>> +		 * The reset should only clear the Root Error Status
>>>> +		 * of the RCEC. Only perform this for the
>>>> +		 * native case, i.e., an RCEC is present.
>>>> +		 */
>>>> +		root = dev->rcec;
>>>> +	else
>>>> +		root = dev;
>>>> +
>>>> +	if (root)
>>>> +		aer = dev->aer_cap;
>>>> +
>>>> +	if ((aer) && pcie_aer_is_native(dev)) {
>>>> 		/* Disable Root's interrupt in response to error messages */
>>>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>>>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>>>> 		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
>>>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>>>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
>>>> 	}
>>>> 
>>>> -	rc = pci_bus_error_reset(dev);
>>>> -	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>>>> +	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
>>>> +		if (pcie_has_flr(dev)) {
>>>> +			rc = pcie_flr(dev);
>>>> +			pci_info(dev, "has been reset (%d)\n", rc);
>>> 
>>> Maybe:
>>> 
>>> +             } else {
>>> +                     rc = -ENOTTY;
>>> +                     pci_info(dev, "not reset (no FLR support)\n");
>>> 
>>> Or do we want to pretend the device was reset and return
>>> PCI_ERS_RESULT_RECOVERED?
>> 
>> We are currently doing the latter now with the default of rc = 0
>> above and so  I’m not sure the extra detail here on the absence of
>> FLR support is of value.
> 
> So to make sure I understand the proposal here, for RCECs and RCiEPs
> that don't support FLR, you're saying you want to continue silently
> and return PCI_ERS_RESULT_RECOVERED and let the drivers assume their
> device was reset when it was not?

The setting of the  ‘rc’  on the FLR support is  fine to add to the else condition.
I had simply recalled in earlier discussion that pcie_has_flr() was needed due to quirky
behavior in some hardware and so was not sure that detail of having or not having flr was in fact
consitent/accurate.

> 
>>>> +	} else {
>>>> +		rc = pci_bus_error_reset(dev);
>>>> +		pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>>>> +	}
>>>> 
>>>> -	if (pcie_aer_is_native(dev)) {
>>>> +	if ((aer) && pcie_aer_is_native(dev)) {
>>>> 		/* Clear Root Error Status */
>>>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
>>>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
>>>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
>>>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
>>>> 
>>>> 		/* Enable Root Port's interrupt in response to error messages */
>>>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>>>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>>>> 		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
>>>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>>>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
>>>> 	}
>>>> 
>>>> 	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> 
>>>> @@ -164,8 +164,14 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>>>> 			    int (*cb)(struct pci_dev *, void *),
>>>> 			    void *userdata)
>>>> {
>>>> +	/*
>>>> +	 * In a non-native case where there is no OS-visible reporting
>>>> +	 * device the bridge will be NULL, i.e., no RCEC, no Downstream Port.
>>> 
>>> I don't quite understand this comment.  I see that in the non-native
>>> case, the reporting device may not be OS-visible.  But I don't
>>> understand why the comment is *here*.
>>> 
>>> If "bridge" can be NULL here, we should test that before dereferencing
>>> "bridge->subordinate".
>> 
>> Wrongly worded.  The subordinate may be NULL or the associated RCEC
>> may be NULL, not the “bridge”.  However, per below, we should not be
>> trying to call report_frozen_detected(), report_mmio_enabled() via
>> the associated RCEC’s driver, but rather the CB for the RCiEP
>> itself.
> 
> OK, so if we want a comment here, I assume it would be along the lines
> of:
> 
>  If "bridge" has no subordinate bus, it's an RCEC or an RCiEP.  In
>  either of those cases, we want to call the callback on "bridge"
>  itself.
> 

Correct.

>> Going back to this conversation,
>> 
>> https://lore.kernel.org/linux-pci/20201016172210.GA86168@bjorn-Precision-5520/
>> 
>> "Looks like *this* is the patch where the "no subordinate bus" case
>> becomes possible?  If you agree, I can just move the test here, no
>> need to repost.”
>> 
>> It is actually the case we are only dealing with the absence of a
>> subordinate bus.
>> 
>>>> 	if (bridge->subordinate)
>>>> 		pci_walk_bus(bridge->subordinate, cb, userdata);
>>>> +	else if (bridge->rcec)
>>>> +		cb(bridge->rcec, userdata);
>>> 
>>> And I don't understand what's going on here.  In this case, I *think*
>>> "bridge" is an RCiEP and "bridge->rcec" is the related RCEC, so it
>>> looks like we'll call report_frozen_detected(), report_mmio_enabled(),
>>> etc for the RCEC driver.  I would think we'd want the RCiEP driver.
>> 
>> Indeed, the bridge->rcec here is the dev->rcec in which the dev is
>> the RCiEP.
>> 
>> And we don’t need that conditional here, it should just hit the
>> device driver’s routines.
> 
> So IIUC, the code would be:
> 
>  if (bridge->subordinate)
>    pci_walk_bus(bridge->subordinate, cb, userdata);
>  else
>    cb(bridge, userdata);    /* RCEC or RCiEP */
> 
> Right?

Right, as before.

> 
> I pushed a pci/err branch
> (https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/err)
> with some tweaks in these areas.  Diff from your v12 posting appended
> below.  I split the RCEC/RCiEP error recovery pieces up a little bit
> differently than in your posting.  Let me know if you see anything
> that should be changed.  I dropped one of Jonathan's
> reviewed/tested-by but probably should have dropped others to avoid
> putting words in his mouth.

Thanks very much for doing this update.  It looks good to me.

> 
> Not sure we're completely done, but we'll get there yet.  I definitely
> want to make sure this happens this cycle.

I’ll retest today.

Thanks,

Sean


> 
> Bjorn
> 
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index b86a92494345..4aa118edde35 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1366,33 +1366,38 @@ static int aer_probe(struct pcie_device *dev)
> }
> 
> /**
> - * aer_root_reset - reset link on Root Port
> - * @dev: pointer to Root Port's pci_dev data structure
> + * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
> + * @dev: pointer to Root Port, RCEC, or RCiEP
>  *
> - * Invoked by Port Bus driver when performing link reset at Root Port.
> + * Invoked by Port Bus driver when performing reset.
>  */
> static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
> {
> 	int type = pci_pcie_type(dev);
> 	struct pci_dev *root;
> -	int aer = 0;
> -	int rc = 0;
> +	int aer;
> +	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> 	u32 reg32;
> +	int rc;
> 
> +	/*
> +	 * Only Root Ports and RCECs have AER Root Command and Root Status
> +	 * registers.  If "dev" is an RCiEP, the relevant registers are in
> +	 * the RCEC.
> +	 */
> 	if (type == PCI_EXP_TYPE_RC_END)
> -		/*
> -		 * The reset should only clear the Root Error Status
> -		 * of the RCEC. Only perform this for the
> -		 * native case, i.e., an RCEC is present.
> -		 */
> 		root = dev->rcec;
> 	else
> 		root = dev;
> 
> -	if (root)
> -		aer = dev->aer_cap;
> +	/*
> +	 * If the platform retained control of AER, an RCiEP may not have
> +	 * an RCEC visible to us, so dev->rcec ("root") may be NULL.  In
> +	 * that case, firmware is responsible for these registers.
> +	 */
> +	aer = root ? root->aer_cap : 0;
> 
> -	if ((aer) && pcie_aer_is_native(dev)) {
> +	if ((host->native_aer || pcie_ports_native) && aer) {
> 		/* Disable Root's interrupt in response to error messages */
> 		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> 		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> @@ -1403,13 +1408,15 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
> 		if (pcie_has_flr(dev)) {
> 			rc = pcie_flr(dev);
> 			pci_info(dev, "has been reset (%d)\n", rc);
> +		} else {
> +			pci_info(dev, "not reset (no FLR support)\n");
> 		}
> 	} else {
> 		rc = pci_bus_error_reset(dev);
> 		pci_info(dev, "Root Port link has been reset (%d)\n", rc);
> 	}
> 
> -	if ((aer) && pcie_aer_is_native(dev)) {
> +	if ((host->native_aer || pcie_ports_native) && aer) {
> 		/* Clear Root Error Status */
> 		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
> 		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index cbc5abfe767b..510f31f0ef6d 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -148,30 +148,23 @@ static int report_resume(struct pci_dev *dev, void *data)
> 
> /**
>  * pci_walk_bridge - walk bridges potentially AER affected
> - * @bridge   bridge which may be an RCEC with associated RCiEPs,
> - *           or a Port.
> - * @cb       callback to be called for each device found
> - * @userdata arbitrary pointer to be passed to callback.
> + * @bridge:	bridge which may be a Port, an RCEC, or an RCiEP
> + * @cb:		callback to be called for each device found
> + * @userdata:	arbitrary pointer to be passed to callback
>  *
>  * If the device provided is a bridge, 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 callback on the
> - * device itself.
> + * If the device provided has no subordinate bus, e.g., an RCEC or RCiEP,
> + * call the callback on the device itself.
>  */
> static void pci_walk_bridge(struct pci_dev *bridge,
> 			    int (*cb)(struct pci_dev *, void *),
> 			    void *userdata)
> {
> -	/*
> -	 * In a non-native case where there is no OS-visible reporting
> -	 * device the bridge will be NULL, i.e., no RCEC, no Downstream Port.
> -	 */
> 	if (bridge->subordinate)
> 		pci_walk_bus(bridge->subordinate, cb, userdata);
> -	else if (bridge->rcec)
> -		cb(bridge->rcec, userdata);
> 	else
> 		cb(bridge, userdata);
> }
> @@ -183,11 +176,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> 	int type = pci_pcie_type(dev);
> 	struct pci_dev *bridge;
> 	pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
> +	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> 
> 	/*
> -	 * Error recovery runs on all subordinates of the bridge.  If the
> -	 * bridge detected the error, it is cleared at the end.  For RCiEPs
> -	 * we should reset just the RCiEP itself.
> +	 * If the error was detected by a Root Port, Downstream Port, RCEC,
> +	 * or RCiEP, recovery runs on the device itself.  For Ports, that
> +	 * also includes any subordinate devices.
> +	 *
> +	 * If it was detected by another device (Endpoint, etc), recovery
> +	 * runs on the device and anything else under the same Port, i.e.,
> +	 * everything under "bridge".
> 	 */
> 	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> 	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> @@ -232,11 +230,15 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> 	pci_dbg(bridge, "broadcast resume message\n");
> 	pci_walk_bridge(bridge, report_resume, &status);
> 
> -	if (type == PCI_EXP_TYPE_ROOT_PORT ||
> -	    type == PCI_EXP_TYPE_DOWNSTREAM ||
> -	    type == PCI_EXP_TYPE_RC_EC) {
> -		if (pcie_aer_is_native(bridge))
> -			pcie_clear_device_status(bridge);
> +	/*
> +	 * If we have native control of AER, clear error status in the Root
> +	 * Port or Downstream Port that signaled the error.  If the
> +	 * platform retained control of AER, it is responsible for clearing
> +	 * this status.  In that case, the signaling device may not even be
> +	 * visible to the OS.
> +	 */
> +	if (host->native_aer || pcie_ports_native) {
> +		pcie_clear_device_status(bridge);
> 		pci_aer_clear_nonfatal_status(bridge);
> 	}
> 	pci_info(bridge, "device recovery successful\n");

Re: [PATCH v12 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

[Bjorn Helgaas]

On Fri, Dec 04, 2020 at 05:17:58PM +0000, Kelley, Sean V wrote:
> On Dec 3, 2020, at 4:01 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:

> > OK, so if we want a comment here, I assume it would be along the lines
> > of:
> > 
> >  If "bridge" has no subordinate bus, it's an RCEC or an RCiEP.  In
> >  either of those cases, we want to call the callback on "bridge"
> >  itself.
> 
> Correct.

OK, good.  I think the function comment now captures this.

> > So IIUC, the code would be:
> > 
> >  if (bridge->subordinate)
> >    pci_walk_bus(bridge->subordinate, cb, userdata);
> >  else
> >    cb(bridge, userdata);    /* RCEC or RCiEP */
> > 
> > Right?
> 
> Right, as before.

Updated to match this.

> > I pushed a pci/err branch
> > (https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/err)
> > with some tweaks in these areas.  Diff from your v12 posting appended
> > below.  I split the RCEC/RCiEP error recovery pieces up a little bit
> > differently than in your posting.  Let me know if you see anything
> > that should be changed.  I dropped one of Jonathan's
> > reviewed/tested-by but probably should have dropped others to avoid
> > putting words in his mouth.
> 
> Thanks very much for doing this update.  It looks good to me.

I just updated this for the "rc used before initialization" error.
Current head f74d7cf9f2bc ("PCI/AER: Add RCEC AER error injection
support").

Re: [PATCH v12 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

[Bjorn Helgaas]

On Fri, Dec 04, 2020 at 05:17:58PM +0000, Kelley, Sean V wrote:
> > On Dec 3, 2020, at 4:01 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Dec 03, 2020 at 12:51:40AM +0000, Kelley, Sean V wrote:
> >>> On Dec 2, 2020, at 3:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>> On Fri, Nov 20, 2020 at 04:10:33PM -0800, Sean V Kelley 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.  In
> >>>> some non-native cases in which there is no OS-visible device associated
> >>>> with the RCiEP, there is nothing to act upon as the firmware is acting
> >>>> before the OS.
> >>>> 
> >>>> Add handling for the linked RCEC in AER/ERR while taking into account
> >>>> non-native cases.
> >>>> 
> >>>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
> >>>> Link: https://lore.kernel.org/r/20201002184735.1229220-12-seanvk.dev@oregontracks.org
> >>>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> >>>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> >>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>>> ---
> >>>> drivers/pci/pcie/aer.c | 46 +++++++++++++++++++++++++++++++-----------
> >>>> drivers/pci/pcie/err.c | 20 +++++++++---------
> >>>> 2 files changed, 44 insertions(+), 22 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> >>>> index 0ba0b47ae751..51389a6ee4ca 100644
> >>>> --- a/drivers/pci/pcie/aer.c
> >>>> +++ b/drivers/pci/pcie/aer.c
> >>>> @@ -1358,29 +1358,51 @@ 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 type = pci_pcie_type(dev);
> >>>> +	struct pci_dev *root;
> >>>> +	int aer = 0;
> >>>> +	int rc = 0;
> >>>> 	u32 reg32;
> >>>> -	int rc;
> >>>> 
> >>>> -	if (pcie_aer_is_native(dev)) {
> >>>> +	if (type == PCI_EXP_TYPE_RC_END)
> >>>> +		/*
> >>>> +		 * The reset should only clear the Root Error Status
> >>>> +		 * of the RCEC. Only perform this for the
> >>>> +		 * native case, i.e., an RCEC is present.
> >>>> +		 */
> >>>> +		root = dev->rcec;
> >>>> +	else
> >>>> +		root = dev;
> >>>> +
> >>>> +	if (root)
> >>>> +		aer = dev->aer_cap;
> >>>> +
> >>>> +	if ((aer) && pcie_aer_is_native(dev)) {
> >>>> 		/* Disable Root's interrupt in response to error messages */
> >>>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> >>>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> >>>> 		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> >>>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> >>>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> >>>> 	}
> >>>> 
> >>>> -	rc = pci_bus_error_reset(dev);
> >>>> -	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
> >>>> +	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
> >>>> +		if (pcie_has_flr(dev)) {
> >>>> +			rc = pcie_flr(dev);
> >>>> +			pci_info(dev, "has been reset (%d)\n", rc);
> >>> 
> >>> Maybe:
> >>> 
> >>> +             } else {
> >>> +                     rc = -ENOTTY;
> >>> +                     pci_info(dev, "not reset (no FLR support)\n");
> >>> 
> >>> Or do we want to pretend the device was reset and return
> >>> PCI_ERS_RESULT_RECOVERED?
> >> 
> >> We are currently doing the latter now with the default of rc = 0
> >> above and so  I’m not sure the extra detail here on the absence of
> >> FLR support is of value.
> > 
> > So to make sure I understand the proposal here, for RCECs and RCiEPs
> > that don't support FLR, you're saying you want to continue silently
> > and return PCI_ERS_RESULT_RECOVERED and let the drivers assume their
> > device was reset when it was not?
> 
> The setting of the ‘rc’ on the FLR support is fine to add to the
> else condition.  I had simply recalled in earlier discussion that
> pcie_has_flr() was needed due to quirky behavior in some hardware
> and so was not sure that detail of having or not having flr was in
> fact consitent/accurate.

I think we should do the following, unless you object:

    if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
	    if (pcie_has_flr(dev)) {
		    rc = pcie_flr(dev);
		    pci_info(dev, "has been reset (%d)\n", rc);
	    } else {
		    pci_info(dev, "not reset (no FLR support)\n");
		    rc = -ENOTTY;
	    }
    } else {
	    rc = pci_bus_error_reset(dev);
	    pci_info(dev, "Root Port link has been reset (%d)\n", rc);
    }
    ...
    return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;

Sorry, I should have done that in the proposed patch earlier; it's
what I was *thinking* but didn't get it transcribed into the code.

Bjorn

Re: [PATCH v12 12/15] PCI/RCEC: Add RCiEP's linked RCEC to AER/ERR

[Kelley, Sean V]

Hi Bjorn,

> On Dec 5, 2020, at 1:30 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Fri, Dec 04, 2020 at 05:17:58PM +0000, Kelley, Sean V wrote:
>>> On Dec 3, 2020, at 4:01 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Thu, Dec 03, 2020 at 12:51:40AM +0000, Kelley, Sean V wrote:
>>>>> On Dec 2, 2020, at 3:44 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>> On Fri, Nov 20, 2020 at 04:10:33PM -0800, Sean V Kelley 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.  In
>>>>>> some non-native cases in which there is no OS-visible device associated
>>>>>> with the RCiEP, there is nothing to act upon as the firmware is acting
>>>>>> before the OS.
>>>>>> 
>>>>>> Add handling for the linked RCEC in AER/ERR while taking into account
>>>>>> non-native cases.
>>>>>> 
>>>>>> Co-developed-by: Sean V Kelley <sean.v.kelley@intel.com>
>>>>>> Link: https://lore.kernel.org/r/20201002184735.1229220-12-seanvk.dev@oregontracks.org
>>>>>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>>>>>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
>>>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>>> ---
>>>>>> drivers/pci/pcie/aer.c | 46 +++++++++++++++++++++++++++++++-----------
>>>>>> drivers/pci/pcie/err.c | 20 +++++++++---------
>>>>>> 2 files changed, 44 insertions(+), 22 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>>>>> index 0ba0b47ae751..51389a6ee4ca 100644
>>>>>> --- a/drivers/pci/pcie/aer.c
>>>>>> +++ b/drivers/pci/pcie/aer.c
>>>>>> @@ -1358,29 +1358,51 @@ 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 type = pci_pcie_type(dev);
>>>>>> +	struct pci_dev *root;
>>>>>> +	int aer = 0;
>>>>>> +	int rc = 0;
>>>>>> 	u32 reg32;
>>>>>> -	int rc;
>>>>>> 
>>>>>> -	if (pcie_aer_is_native(dev)) {
>>>>>> +	if (type == PCI_EXP_TYPE_RC_END)
>>>>>> +		/*
>>>>>> +		 * The reset should only clear the Root Error Status
>>>>>> +		 * of the RCEC. Only perform this for the
>>>>>> +		 * native case, i.e., an RCEC is present.
>>>>>> +		 */
>>>>>> +		root = dev->rcec;
>>>>>> +	else
>>>>>> +		root = dev;
>>>>>> +
>>>>>> +	if (root)
>>>>>> +		aer = dev->aer_cap;
>>>>>> +
>>>>>> +	if ((aer) && pcie_aer_is_native(dev)) {
>>>>>> 		/* Disable Root's interrupt in response to error messages */
>>>>>> -		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>>>>>> +		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
>>>>>> 		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
>>>>>> -		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>>>>>> +		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
>>>>>> 	}
>>>>>> 
>>>>>> -	rc = pci_bus_error_reset(dev);
>>>>>> -	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>>>>>> +	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
>>>>>> +		if (pcie_has_flr(dev)) {
>>>>>> +			rc = pcie_flr(dev);
>>>>>> +			pci_info(dev, "has been reset (%d)\n", rc);
>>>>> 
>>>>> Maybe:
>>>>> 
>>>>> +             } else {
>>>>> +                     rc = -ENOTTY;
>>>>> +                     pci_info(dev, "not reset (no FLR support)\n");
>>>>> 
>>>>> Or do we want to pretend the device was reset and return
>>>>> PCI_ERS_RESULT_RECOVERED?
>>>> 
>>>> We are currently doing the latter now with the default of rc = 0
>>>> above and so  I’m not sure the extra detail here on the absence of
>>>> FLR support is of value.
>>> 
>>> So to make sure I understand the proposal here, for RCECs and RCiEPs
>>> that don't support FLR, you're saying you want to continue silently
>>> and return PCI_ERS_RESULT_RECOVERED and let the drivers assume their
>>> device was reset when it was not?
>> 
>> The setting of the ‘rc’ on the FLR support is fine to add to the
>> else condition.  I had simply recalled in earlier discussion that
>> pcie_has_flr() was needed due to quirky behavior in some hardware
>> and so was not sure that detail of having or not having flr was in
>> fact consitent/accurate.
> 
> I think we should do the following, unless you object:
> 
>    if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
> 	    if (pcie_has_flr(dev)) {
> 		    rc = pcie_flr(dev);
> 		    pci_info(dev, "has been reset (%d)\n", rc);
> 	    } else {
> 		    pci_info(dev, "not reset (no FLR support)\n");
> 		    rc = -ENOTTY;
> 	    }
>    } else {
> 	    rc = pci_bus_error_reset(dev);
> 	    pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>    }
>    ...
>    return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> 
> Sorry, I should have done that in the proposed patch earlier; it's
> what I was *thinking* but didn't get it transcribed into the code.


Looks good to me.

Thanks,

Sean


> 
> Bjorn