[PATCH v6 0/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[PATCH v6 0/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Shuai Xue]

changes since v5:
- rebase to 6.18-rc1
- add separate patches to clear device fatal status per Kuppuswamy

changes since v4:
- rebase to 6.17-rc6
- pick up Reviewed-by tag for PATCH[2] from Sathyanarayanan
- minor typos in commit log per Manivannan

changes since v3:
- squash patch 1 and 2 into one patch per Sathyanarayanan
- add comments note for dpc_process_error per Sathyanarayanan
- pick up Reviewed-by tag for PATCH[1] from Sathyanarayanan

changes since v2:
- moving the "err_port" rename to a separate patch per Sathyanarayanan
- rewrite comments of dpc_process_error per Sathyanarayanan
- remove NULL initialization for err_dev per Sathyanarayanan

changes since v1:
- rewrite commit log per Bjorn
- refactor aer_get_device_error_info to reduce duplication per Keith
- fix to avoid reporting fatal errors twice for root and downstream ports per Keith

The AER driver has historically avoided reading the configuration space of an
endpoint or RCiEP that reported a fatal error, considering the link to that
device unreliable. Consequently, when a fatal error occurs, the AER and DPC
drivers do not report specific error types, resulting in logs like:

  pcieport 0015:00:00.0: EDR: EDR event received
  pcieport 0015:00:00.0: EDR: Reported EDR dev: 0015:00:00.0
  pcieport 0015:00:00.0: DPC: containment event, status:0x200d, ERR_FATAL received from 0015:01:00.0
  pcieport 0015:00:00.0: AER: broadcast error_detected message
  pcieport 0015:00:00.0: AER: broadcast mmio_enabled message
  pcieport 0015:00:00.0: AER: broadcast resume message
  pcieport 0015:00:00.0: pciehp: Slot(21): Link Down/Up ignored
  pcieport 0015:00:00.0: AER: device recovery successful
  pcieport 0015:00:00.0: EDR: DPC port successfully recovered
  pcieport 0015:00:00.0: EDR: Status for 0015:00:00.0: 0x80

AER status registers are sticky and Write-1-to-clear. If the link recovered
after hot reset, we can still safely access AER status and TLP header of the
error device. In such case, report fatal errors which helps to figure out the
error root case.

After this patch, the logs like:

  pcieport 0015:00:00.0: EDR: EDR event received
  pcieport 0015:00:00.0: EDR: Reported EDR dev: 0015:00:00.0
  pcieport 0015:00:00.0: DPC: containment event, status:0x200d, ERR_FATAL received from 0015:01:00.0
  pcieport 0015:00:00.0: AER: broadcast error_detected message
  vfio-pci 0015:01:00.0: PCIe Bus Error: severity=Uncorrectable (Fatal), type=Transaction Layer, (Receiver ID)
  pcieport 0015:00:00.0: pciehp: Slot(21): Link Down/Up ignored
  vfio-pci 0015:01:00.0:   device [144d:a80a] error status/mask=00001000/00400000
  vfio-pci 0015:01:00.0:    [12] TLP                    (First)
  vfio-pci 0015:01:00.0: AER:   TLP Header: 0x4a004010 0x00000040 0x01000000 0xffffffff
  pcieport 0015:00:00.0: AER: broadcast mmio_enabled message
  pcieport 0015:00:00.0: AER: broadcast resume message
  pcieport 0015:00:00.0: AER: device recovery successful
  pcieport 0015:00:00.0: EDR: DPC port successfully recovered
  pcieport 0015:00:00.0: EDR: Status for 0015:00:00.0: 0x80

Shuai Xue (5):
  PCI/DPC: Clarify naming for error port in DPC Handling
  PCI/DPC: Run recovery on device that detected the error
  PCI/AER: Report fatal errors of RCiEP and EP if link recoverd
  PCI/ERR: Use pcie_aer_is_native() to check for native AER control
  PCI/AER: Clear both AER fatal and non-fatal status

 drivers/pci/pci.h      |  6 ++++--
 drivers/pci/pcie/aer.c | 18 +++++++++++-------
 drivers/pci/pcie/dpc.c | 31 ++++++++++++++++++++++++-------
 drivers/pci/pcie/edr.c | 35 ++++++++++++++++++-----------------
 drivers/pci/pcie/err.c | 16 +++++++++++++---
 5 files changed, 70 insertions(+), 36 deletions(-)

-- 
2.39.3

[PATCH v6 1/5] PCI/DPC: Clarify naming for error port in DPC Handling

[Shuai Xue]

dpc_handler() is registered for error port which recevie DPC interrupt
and acpi_dpc_port_get() locate the port that experienced the containment
event.

Rename edev and pdev to err_port for clear so that later patch will
avoid misused err_port in pcie_do_recovery().

No functional changes intended.

Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/dpc.c | 10 +++++-----
 drivers/pci/pcie/edr.c | 34 +++++++++++++++++-----------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index fc18349614d7..bff29726c6a5 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -361,21 +361,21 @@ static bool dpc_is_surprise_removal(struct pci_dev *pdev)
 
 static irqreturn_t dpc_handler(int irq, void *context)
 {
-	struct pci_dev *pdev = context;
+	struct pci_dev *err_port = context;
 
 	/*
 	 * According to PCIe r6.0 sec 6.7.6, errors are an expected side effect
 	 * of async removal and should be ignored by software.
 	 */
-	if (dpc_is_surprise_removal(pdev)) {
-		dpc_handle_surprise_removal(pdev);
+	if (dpc_is_surprise_removal(err_port)) {
+		dpc_handle_surprise_removal(err_port);
 		return IRQ_HANDLED;
 	}
 
-	dpc_process_error(pdev);
+	dpc_process_error(err_port);
 
 	/* We configure DPC so it only triggers on ERR_FATAL */
-	pcie_do_recovery(pdev, pci_channel_io_frozen, dpc_reset_link);
+	pcie_do_recovery(err_port, pci_channel_io_frozen, dpc_reset_link);
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
index e86298dbbcff..521fca2f40cb 100644
--- a/drivers/pci/pcie/edr.c
+++ b/drivers/pci/pcie/edr.c
@@ -150,7 +150,7 @@ static int acpi_send_edr_status(struct pci_dev *pdev, struct pci_dev *edev,
 
 static void edr_handle_event(acpi_handle handle, u32 event, void *data)
 {
-	struct pci_dev *pdev = data, *edev;
+	struct pci_dev *pdev = data, *err_port;
 	pci_ers_result_t estate = PCI_ERS_RESULT_DISCONNECT;
 	u16 status;
 
@@ -169,36 +169,36 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data)
 	 * may be that port or a parent of it (PCI Firmware r3.3, sec
 	 * 4.6.13).
 	 */
-	edev = acpi_dpc_port_get(pdev);
-	if (!edev) {
+	err_port = acpi_dpc_port_get(pdev);
+	if (!err_port) {
 		pci_err(pdev, "Firmware failed to locate DPC port\n");
 		return;
 	}
 
-	pci_dbg(pdev, "Reported EDR dev: %s\n", pci_name(edev));
+	pci_dbg(pdev, "Reported EDR dev: %s\n", pci_name(err_port));
 
 	/* If port does not support DPC, just send the OST */
-	if (!edev->dpc_cap) {
-		pci_err(edev, FW_BUG "This device doesn't support DPC\n");
+	if (!err_port->dpc_cap) {
+		pci_err(err_port, FW_BUG "This device doesn't support DPC\n");
 		goto send_ost;
 	}
 
 	/* Check if there is a valid DPC trigger */
-	pci_read_config_word(edev, edev->dpc_cap + PCI_EXP_DPC_STATUS, &status);
+	pci_read_config_word(err_port, err_port->dpc_cap + PCI_EXP_DPC_STATUS, &status);
 	if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
-		pci_err(edev, "Invalid DPC trigger %#010x\n", status);
+		pci_err(err_port, "Invalid DPC trigger %#010x\n", status);
 		goto send_ost;
 	}
 
-	dpc_process_error(edev);
-	pci_aer_raw_clear_status(edev);
+	dpc_process_error(err_port);
+	pci_aer_raw_clear_status(err_port);
 
 	/*
 	 * Irrespective of whether the DPC event is triggered by ERR_FATAL
 	 * or ERR_NONFATAL, since the link is already down, use the FATAL
 	 * error recovery path for both cases.
 	 */
-	estate = pcie_do_recovery(edev, pci_channel_io_frozen, dpc_reset_link);
+	estate = pcie_do_recovery(err_port, pci_channel_io_frozen, dpc_reset_link);
 
 send_ost:
 
@@ -207,15 +207,15 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data)
 	 * to firmware. If not successful, send _OST(0xF, BDF << 16 | 0x81).
 	 */
 	if (estate == PCI_ERS_RESULT_RECOVERED) {
-		pci_dbg(edev, "DPC port successfully recovered\n");
-		pcie_clear_device_status(edev);
-		acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS);
+		pci_dbg(err_port, "DPC port successfully recovered\n");
+		pcie_clear_device_status(err_port);
+		acpi_send_edr_status(pdev, err_port, EDR_OST_SUCCESS);
 	} else {
-		pci_dbg(edev, "DPC port recovery failed\n");
-		acpi_send_edr_status(pdev, edev, EDR_OST_FAILED);
+		pci_dbg(err_port, "DPC port recovery failed\n");
+		acpi_send_edr_status(pdev, err_port, EDR_OST_FAILED);
 	}
 
-	pci_dev_put(edev);
+	pci_dev_put(err_port);
 }
 
 void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
-- 
2.39.3

[PATCH v6 2/5] PCI/DPC: Run recovery on device that detected the error

[Shuai Xue]

The current implementation of pcie_do_recovery() assumes that the
recovery process is executed for the device that detected the error.
However, the DPC driver currently passes the error port that experienced
the DPC event to pcie_do_recovery().

Use the SOURCE ID register to correctly identify the device that
detected the error. When passing the error device, the
pcie_do_recovery() will find the upstream bridge and walk bridges
potentially AER affected. And subsequent commits will be able to
accurately access AER status of the error device.

Should not observe any functional changes.

Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pci.h      |  2 +-
 drivers/pci/pcie/dpc.c | 25 +++++++++++++++++++++----
 drivers/pci/pcie/edr.c |  7 ++++---
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fff30521ed83..6b0c55bed15b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -764,7 +764,7 @@ struct rcec_ea {
 void pci_save_dpc_state(struct pci_dev *dev);
 void pci_restore_dpc_state(struct pci_dev *dev);
 void pci_dpc_init(struct pci_dev *pdev);
-void dpc_process_error(struct pci_dev *pdev);
+struct pci_dev *dpc_process_error(struct pci_dev *pdev);
 pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
 bool pci_dpc_recovered(struct pci_dev *pdev);
 unsigned int dpc_tlp_log_len(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index bff29726c6a5..f6069f621683 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -260,10 +260,20 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
 	return 1;
 }
 
-void dpc_process_error(struct pci_dev *pdev)
+/**
+ * dpc_process_error - handle the DPC error status
+ * @pdev: the port that experienced the containment event
+ *
+ * Return: the device that detected the error.
+ *
+ * NOTE: The device reference count is increased, the caller must decrement
+ * the reference count by calling pci_dev_put().
+ */
+struct pci_dev *dpc_process_error(struct pci_dev *pdev)
 {
 	u16 cap = pdev->dpc_cap, status, source, reason, ext_reason;
 	struct aer_err_info info = {};
+	struct pci_dev *err_dev;
 
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 
@@ -279,6 +289,7 @@ void dpc_process_error(struct pci_dev *pdev)
 			pci_aer_clear_nonfatal_status(pdev);
 			pci_aer_clear_fatal_status(pdev);
 		}
+		err_dev = pci_dev_get(pdev);
 		break;
 	case PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE:
 	case PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE:
@@ -290,6 +301,8 @@ void dpc_process_error(struct pci_dev *pdev)
 				"ERR_FATAL" : "ERR_NONFATAL",
 			 pci_domain_nr(pdev->bus), PCI_BUS_NUM(source),
 			 PCI_SLOT(source), PCI_FUNC(source));
+		err_dev = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
+					    PCI_BUS_NUM(source), source & 0xff);
 		break;
 	case PCI_EXP_DPC_STATUS_TRIGGER_RSN_IN_EXT:
 		ext_reason = status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT;
@@ -304,8 +317,11 @@ void dpc_process_error(struct pci_dev *pdev)
 		if (ext_reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO &&
 		    pdev->dpc_rp_extensions)
 			dpc_process_rp_pio_error(pdev);
+		err_dev = pci_dev_get(pdev);
 		break;
 	}
+
+	return err_dev;
 }
 
 static void pci_clear_surpdn_errors(struct pci_dev *pdev)
@@ -361,7 +377,7 @@ static bool dpc_is_surprise_removal(struct pci_dev *pdev)
 
 static irqreturn_t dpc_handler(int irq, void *context)
 {
-	struct pci_dev *err_port = context;
+	struct pci_dev *err_port = context, *err_dev;
 
 	/*
 	 * According to PCIe r6.0 sec 6.7.6, errors are an expected side effect
@@ -372,10 +388,11 @@ static irqreturn_t dpc_handler(int irq, void *context)
 		return IRQ_HANDLED;
 	}
 
-	dpc_process_error(err_port);
+	err_dev = dpc_process_error(err_port);
 
 	/* We configure DPC so it only triggers on ERR_FATAL */
-	pcie_do_recovery(err_port, pci_channel_io_frozen, dpc_reset_link);
+	pcie_do_recovery(err_dev, pci_channel_io_frozen, dpc_reset_link);
+	pci_dev_put(err_dev);
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
index 521fca2f40cb..b6e9d652297e 100644
--- a/drivers/pci/pcie/edr.c
+++ b/drivers/pci/pcie/edr.c
@@ -150,7 +150,7 @@ static int acpi_send_edr_status(struct pci_dev *pdev, struct pci_dev *edev,
 
 static void edr_handle_event(acpi_handle handle, u32 event, void *data)
 {
-	struct pci_dev *pdev = data, *err_port;
+	struct pci_dev *pdev = data, *err_port, *err_dev;
 	pci_ers_result_t estate = PCI_ERS_RESULT_DISCONNECT;
 	u16 status;
 
@@ -190,7 +190,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data)
 		goto send_ost;
 	}
 
-	dpc_process_error(err_port);
+	err_dev = dpc_process_error(err_port);
 	pci_aer_raw_clear_status(err_port);
 
 	/*
@@ -198,7 +198,8 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data)
 	 * or ERR_NONFATAL, since the link is already down, use the FATAL
 	 * error recovery path for both cases.
 	 */
-	estate = pcie_do_recovery(err_port, pci_channel_io_frozen, dpc_reset_link);
+	estate = pcie_do_recovery(err_dev, pci_channel_io_frozen, dpc_reset_link);
+	pci_dev_put(err_dev);
 
 send_ost:
 
-- 
2.39.3

[PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Shuai Xue]

The AER driver has historically avoided reading the configuration space of
an endpoint or RCiEP that reported a fatal error, considering the link to
that device unreliable. Consequently, when a fatal error occurs, the AER
and DPC drivers do not report specific error types, resulting in logs like:

  pcieport 0015:00:00.0: EDR: EDR event received
  pcieport 0015:00:00.0: EDR: Reported EDR dev: 0015:00:00.0
  pcieport 0015:00:00.0: DPC: containment event, status:0x200d, ERR_FATAL received from 0015:01:00.0
  pcieport 0015:00:00.0: AER: broadcast error_detected message
  pcieport 0015:00:00.0: AER: broadcast mmio_enabled message
  pcieport 0015:00:00.0: AER: broadcast resume message
  pcieport 0015:00:00.0: pciehp: Slot(21): Link Down/Up ignored
  pcieport 0015:00:00.0: AER: device recovery successful
  pcieport 0015:00:00.0: EDR: DPC port successfully recovered
  pcieport 0015:00:00.0: EDR: Status for 0015:00:00.0: 0x80

AER status registers are sticky and Write-1-to-clear. If the link recovered
after hot reset, we can still safely access AER status and TLP header of the
error device. In such case, report fatal errors which helps to figure out the
error root case.

After this patch, the logs like:

  pcieport 0015:00:00.0: EDR: EDR event received
  pcieport 0015:00:00.0: EDR: Reported EDR dev: 0015:00:00.0
  pcieport 0015:00:00.0: DPC: containment event, status:0x200d, ERR_FATAL received from 0015:01:00.0
  pcieport 0015:00:00.0: AER: broadcast error_detected message
  vfio-pci 0015:01:00.0: PCIe Bus Error: severity=Uncorrectable (Fatal), type=Transaction Layer, (Receiver ID)
  pcieport 0015:00:00.0: pciehp: Slot(21): Link Down/Up ignored
  vfio-pci 0015:01:00.0:   device [144d:a80a] error status/mask=00001000/00400000
  vfio-pci 0015:01:00.0:    [12] TLP                    (First)
  vfio-pci 0015:01:00.0: AER:   TLP Header: 0x4a004010 0x00000040 0x01000000 0xffffffff
  pcieport 0015:00:00.0: AER: broadcast mmio_enabled message
  pcieport 0015:00:00.0: AER: broadcast resume message
  pcieport 0015:00:00.0: AER: device recovery successful
  pcieport 0015:00:00.0: EDR: DPC port successfully recovered
  pcieport 0015:00:00.0: EDR: Status for 0015:00:00.0: 0x80

Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 drivers/pci/pci.h      |  4 +++-
 drivers/pci/pcie/aer.c | 18 +++++++++++-------
 drivers/pci/pcie/dpc.c |  2 +-
 drivers/pci/pcie/err.c | 11 +++++++++++
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6b0c55bed15b..3eccef2d25a3 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -739,8 +739,10 @@ struct aer_err_info {
 	struct pcie_tlp_log tlp;	/* TLP Header */
 };
 
-int aer_get_device_error_info(struct aer_err_info *info, int i);
+int aer_get_device_error_info(struct aer_err_info *info, int i,
+			      bool link_healthy);
 void aer_print_error(struct aer_err_info *info, int i);
+int aer_add_error_device(struct aer_err_info *e_info, struct pci_dev *dev);
 
 int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
 		      unsigned int tlp_len, bool flit,
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 0b5ed4722ac3..aaea9902cbb7 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -978,7 +978,7 @@ EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
  * @e_info: pointer to error info
  * @dev: pointer to pci_dev to be added
  */
-static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
+int aer_add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
 {
 	int i = e_info->error_dev_num;
 
@@ -1068,7 +1068,7 @@ static int find_device_iter(struct pci_dev *dev, void *data)
 
 	if (is_error_source(dev, e_info)) {
 		/* List this device */
-		if (add_error_device(e_info, dev)) {
+		if (aer_add_error_device(e_info, dev)) {
 			/* We cannot handle more... Stop iteration */
 			pci_err(dev, "Exceeded max supported (%d) devices with errors logged\n",
 				AER_MAX_MULTI_ERR_DEVICES);
@@ -1382,12 +1382,14 @@ EXPORT_SYMBOL_GPL(aer_recover_queue);
  * aer_get_device_error_info - read error status from dev and store it to info
  * @info: pointer to structure to store the error record
  * @i: index into info->dev[]
+ * @link_healthy: link is healthy or not
  *
  * Return: 1 on success, 0 on error.
  *
  * Note that @info is reused among all error devices. Clear fields properly.
  */
-int aer_get_device_error_info(struct aer_err_info *info, int i)
+int aer_get_device_error_info(struct aer_err_info *info, int i,
+			      bool link_healthy)
 {
 	struct pci_dev *dev;
 	int type, aer;
@@ -1415,10 +1417,12 @@ int aer_get_device_error_info(struct aer_err_info *info, int i)
 			&info->mask);
 		if (!(info->status & ~info->mask))
 			return 0;
+		info->level = KERN_WARNING;
 	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
 		   type == PCI_EXP_TYPE_RC_EC ||
 		   type == PCI_EXP_TYPE_DOWNSTREAM ||
-		   info->severity == AER_NONFATAL) {
+		   info->severity == AER_NONFATAL ||
+		   (info->severity == AER_FATAL && link_healthy)) {
 
 		/* Link is still healthy for IO reads */
 		pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,
@@ -1427,7 +1431,7 @@ int aer_get_device_error_info(struct aer_err_info *info, int i)
 			&info->mask);
 		if (!(info->status & ~info->mask))
 			return 0;
-
+		info->level = KERN_ERR;
 		/* Get First Error Pointer */
 		pci_read_config_dword(dev, aer + PCI_ERR_CAP, &aercc);
 		info->first_error = PCI_ERR_CAP_FEP(aercc);
@@ -1451,11 +1455,11 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info)
 
 	/* Report all before handling them, to not lose records by reset etc. */
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
-		if (aer_get_device_error_info(e_info, i))
+		if (aer_get_device_error_info(e_info, i, false))
 			aer_print_error(e_info, i);
 	}
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
-		if (aer_get_device_error_info(e_info, i))
+		if (aer_get_device_error_info(e_info, i, false))
 			handle_error_source(e_info->dev[i], e_info);
 	}
 }
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index f6069f621683..21c4e8371279 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -284,7 +284,7 @@ struct pci_dev *dpc_process_error(struct pci_dev *pdev)
 		pci_warn(pdev, "containment event, status:%#06x: unmasked uncorrectable error detected\n",
 			 status);
 		if (dpc_get_aer_uncorrect_severity(pdev, &info) &&
-		    aer_get_device_error_info(&info, 0)) {
+		    aer_get_device_error_info(&info, 0, false)) {
 			aer_print_error(&info, 0);
 			pci_aer_clear_nonfatal_status(pdev);
 			pci_aer_clear_fatal_status(pdev);
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index bebe4bc111d7..4e65eac809d1 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -215,6 +215,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *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);
+	struct aer_err_info info;
 
 	/*
 	 * If the error was detected by a Root Port, Downstream Port, RCEC,
@@ -253,6 +254,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 			pci_warn(bridge, "subordinate device reset failed\n");
 			goto failed;
 		}
+
+		/* Link recovered, report fatal errors of RCiEP or EP */
+		if (state == pci_channel_io_frozen &&
+		    (type == PCI_EXP_TYPE_ENDPOINT || type == PCI_EXP_TYPE_RC_END)) {
+			aer_add_error_device(&info, dev);
+			info.severity = AER_FATAL;
+			if (aer_get_device_error_info(&info, 0, true))
+				aer_print_error(&info, 0);
+			pci_dev_put(dev);
+		}
 	}
 
 	if (status == PCI_ERS_RESULT_NEED_RESET) {
-- 
2.39.3

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Lukas Wunner]

On Wed, Oct 15, 2025 at 10:41:57AM +0800, Shuai Xue wrote:
> +++ b/drivers/pci/pcie/err.c
> @@ -253,6 +254,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  			pci_warn(bridge, "subordinate device reset failed\n");
>  			goto failed;
>  		}
> +
> +		/* Link recovered, report fatal errors of RCiEP or EP */
> +		if (state == pci_channel_io_frozen &&
> +		    (type == PCI_EXP_TYPE_ENDPOINT || type == PCI_EXP_TYPE_RC_END)) {
> +			aer_add_error_device(&info, dev);
> +			info.severity = AER_FATAL;
> +			if (aer_get_device_error_info(&info, 0, true))
> +				aer_print_error(&info, 0);
> +			pci_dev_put(dev);
> +		}
>  	}

Where is the the pci_dev_get() to balance the pci_dev_put() here?

It feels awkward to leak AER-specific details into pcie_do_recovery().
That function is supposed to implement the flow described in
Documentation/PCI/pci-error-recovery.rst in a platform-agnostic way
so that powerpc (EEH) and s390 could conceivably take advantage of it.

Can you find a way to avoid this, e.g. report errors after
pcie_do_recovery() has concluded?

I'm also worried that errors are reported *during* recovery.
I imagine this looks confusing to a user.  The logged messages
should make it clear that these are errors that occurred *earlier*
and are reported belatedly.

Thanks,

Lukas

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Shuai Xue]

在 2025/10/20 18:10, Lukas Wunner 写道:
> On Wed, Oct 15, 2025 at 10:41:57AM +0800, Shuai Xue wrote:
>> +++ b/drivers/pci/pcie/err.c
>> @@ -253,6 +254,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>   			pci_warn(bridge, "subordinate device reset failed\n");
>>   			goto failed;
>>   		}
>> +
>> +		/* Link recovered, report fatal errors of RCiEP or EP */
>> +		if (state == pci_channel_io_frozen &&
>> +		    (type == PCI_EXP_TYPE_ENDPOINT || type == PCI_EXP_TYPE_RC_END)) {
>> +			aer_add_error_device(&info, dev);
>> +			info.severity = AER_FATAL;
>> +			if (aer_get_device_error_info(&info, 0, true))
>> +				aer_print_error(&info, 0);
>> +			pci_dev_put(dev);
>> +		}
>>   	}

Hi, Lukas,

> 
> Where is the the pci_dev_get() to balance the pci_dev_put() here?

The corresponding pci_dev_get() is called in add_error_device(). Please
refer to commit 60271ab044a5 ("PCI/AER: Take reference on error
devices") which introduced this reference counting mechanism.

> 
> It feels awkward to leak AER-specific details into pcie_do_recovery().
> That function is supposed to implement the flow described in
> Documentation/PCI/pci-error-recovery.rst in a platform-agnostic way
> so that powerpc (EEH) and s390 could conceivably take advantage of it.
> 
> Can you find a way to avoid this, e.g. report errors after
> pcie_do_recovery() has concluded?

I understand your concern about keeping pcie_do_recovery()
platform-agnostic. I explored the possibility of reporting errors after
recovery concludes, but unfortunately, this approach isn't feasible due
to the recovery sequence. The issue is that most drivers'
pci_error_handlers implement .slot_reset() which internally calls
pci_restore_state() to restore the device's configuration space and
state. This function also clears the device's AER status registers:

   .slot_reset()
     => pci_restore_state()
       => pci_aer_clear_status()

Therefore, the only window to capture and report the original error
information is between link recovery (after reset_subordinates()) and
before .slot_reset() is called. Once .slot_reset() executes, the error
status is cleared and lost forever.

> 
> I'm also worried that errors are reported *during* recovery.
> I imagine this looks confusing to a user.  The logged messages
> should make it clear that these are errors that occurred *earlier*
> and are reported belatedly.

You raise an excellent point about potential user confusion. The current
aer_print_error() interface doesn't indicate that these are historical
errors being reported belatedly. Would it be acceptable to add a
clarifying message before calling aer_print_error()? For example:

   pci_err(dev, "Reporting error that occurred before recovery:\n");

Alternatively, if you have suggestions for a better approach to make
this timing clear to users, I'd be happy to implement them.

> 
> Thanks,
> 
> Lukas

Thanks for valuable comments.

Best Regards,
Shuai

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Lukas Wunner]

On Mon, Oct 20, 2025 at 08:58:55PM +0800, Shuai Xue wrote:
> ??? 2025/10/20 18:10, Lukas Wunner ??????:
> > On Wed, Oct 15, 2025 at 10:41:57AM +0800, Shuai Xue wrote:
> > > +++ b/drivers/pci/pcie/err.c
> > > @@ -253,6 +254,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> > >   			pci_warn(bridge, "subordinate device reset failed\n");
> > >   			goto failed;
> > >   		}
> > > +
> > > +		/* Link recovered, report fatal errors of RCiEP or EP */
> > > +		if (state == pci_channel_io_frozen &&
> > > +		    (type == PCI_EXP_TYPE_ENDPOINT || type == PCI_EXP_TYPE_RC_END)) {
> > > +			aer_add_error_device(&info, dev);
> > > +			info.severity = AER_FATAL;
> > > +			if (aer_get_device_error_info(&info, 0, true))
> > > +				aer_print_error(&info, 0);
> > > +			pci_dev_put(dev);
> > > +		}
> > 
> > Where is the the pci_dev_get() to balance the pci_dev_put() here?
> 
> The corresponding pci_dev_get() is called in add_error_device(). Please
> refer to commit 60271ab044a5 ("PCI/AER: Take reference on error
> devices") which introduced this reference counting mechanism.

That is non-obvious and needs a code comment.

> > It feels awkward to leak AER-specific details into pcie_do_recovery().
> > That function is supposed to implement the flow described in
> > Documentation/PCI/pci-error-recovery.rst in a platform-agnostic way
> > so that powerpc (EEH) and s390 could conceivably take advantage of it.
> > 
> > Can you find a way to avoid this, e.g. report errors after
> > pcie_do_recovery() has concluded?
> 
> I understand your concern about keeping pcie_do_recovery()
> platform-agnostic.

The code you're adding above, with the exception of the check for
pci_channel_io_frozen, should live in a helper in aer.c.
Then you also don't need to rename add_error_device().

> I explored the possibility of reporting errors after
> recovery concludes, but unfortunately, this approach isn't feasible due
> to the recovery sequence. The issue is that most drivers'
> pci_error_handlers implement .slot_reset() which internally calls
> pci_restore_state() to restore the device's configuration space and
> state. This function also clears the device's AER status registers:
> 
>   .slot_reset()
>     => pci_restore_state()
>       => pci_aer_clear_status()

This was added in 2015 by b07461a8e45b.  The commit claims that
the errors are stale and can be ignored.  It turns out they cannot.

So maybe pci_restore_state() should print information about the
errors before clearing them?

Actually pci_restore_state() is only supposed to restore state,
as the name implies, and not clear errors.  It seems questionable
that the commit amended it to do that.

> > I'm also worried that errors are reported *during* recovery.
> > I imagine this looks confusing to a user.  The logged messages
> > should make it clear that these are errors that occurred *earlier*
> > and are reported belatedly.
> 
> You raise an excellent point about potential user confusion. The current
> aer_print_error() interface doesn't indicate that these are historical
> errors being reported belatedly. Would it be acceptable to add a
> clarifying message before calling aer_print_error()? For example:
> 
>   pci_err(dev, "Reporting error that occurred before recovery:\n");

Yes, something like that.  "Errors reported prior to reset"?  Dunno.

Thanks,

Lukas

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Shuai Xue]

Hi, Lukas,

在 2025/10/20 21:54, Lukas Wunner 写道:
> On Mon, Oct 20, 2025 at 08:58:55PM +0800, Shuai Xue wrote:
>> ??? 2025/10/20 18:10, Lukas Wunner ??????:
>>> On Wed, Oct 15, 2025 at 10:41:57AM +0800, Shuai Xue wrote:
>>>> +++ b/drivers/pci/pcie/err.c
>>>> @@ -253,6 +254,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>    			pci_warn(bridge, "subordinate device reset failed\n");
>>>>    			goto failed;
>>>>    		}
>>>> +
>>>> +		/* Link recovered, report fatal errors of RCiEP or EP */
>>>> +		if (state == pci_channel_io_frozen &&
>>>> +		    (type == PCI_EXP_TYPE_ENDPOINT || type == PCI_EXP_TYPE_RC_END)) {
>>>> +			aer_add_error_device(&info, dev);
>>>> +			info.severity = AER_FATAL;
>>>> +			if (aer_get_device_error_info(&info, 0, true))
>>>> +				aer_print_error(&info, 0);
>>>> +			pci_dev_put(dev);
>>>> +		}
>>>
>>> Where is the the pci_dev_get() to balance the pci_dev_put() here?
>>
>> The corresponding pci_dev_get() is called in add_error_device(). Please
>> refer to commit 60271ab044a5 ("PCI/AER: Take reference on error
>> devices") which introduced this reference counting mechanism.
> 
> That is non-obvious and needs a code comment.

Agreed. I'll add a comment to clarify the reference counting relationship.

> 
>>> It feels awkward to leak AER-specific details into pcie_do_recovery().
>>> That function is supposed to implement the flow described in
>>> Documentation/PCI/pci-error-recovery.rst in a platform-agnostic way
>>> so that powerpc (EEH) and s390 could conceivably take advantage of it.
>>>
>>> Can you find a way to avoid this, e.g. report errors after
>>> pcie_do_recovery() has concluded?
>>
>> I understand your concern about keeping pcie_do_recovery()
>> platform-agnostic.
> 
> The code you're adding above, with the exception of the check for
> pci_channel_io_frozen, should live in a helper in aer.c.
> Then you also don't need to rename add_error_device().

Good point.

That's a much cleaner approach. I'll create a helper function in aer.c,
something like:

void aer_report_frozen_error(struct pci_dev *dev)
{
     struct aer_err_info info;

     if (dev->pci_type != PCI_EXP_TYPE_ENDPOINT &&
         dev->pci_type != PCI_EXP_TYPE_RC_END)
         return;

     aer_info_init(&info);
     aer_add_error_device(&info, dev);
     info.severity = AER_FATAL;
     if (aer_get_device_error_info(&info, 0, true))
         aer_print_error(&info, 0);

     /* pci_dev_put() pairs with pci_dev_get() in aer_add_error_device() */
     pci_dev_put(dev);
}

>> I explored the possibility of reporting errors after
>> recovery concludes, but unfortunately, this approach isn't feasible due
>> to the recovery sequence. The issue is that most drivers'
>> pci_error_handlers implement .slot_reset() which internally calls
>> pci_restore_state() to restore the device's configuration space and
>> state. This function also clears the device's AER status registers:
>>
>>    .slot_reset()
>>      => pci_restore_state()
>>        => pci_aer_clear_status()
> 
> This was added in 2015 by b07461a8e45b.  The commit claims that
> the errors are stale and can be ignored.  It turns out they cannot.
> 
> So maybe pci_restore_state() should print information about the
> errors before clearing them?

While that could work, we would lose the error severity information at
that point, which could lead to duplicate or less informative error
messages compared to what the AER driver provides. The helper function
approach preserves all error details for proper reporting.

> 
> Actually pci_restore_state() is only supposed to restore state,
> as the name implies, and not clear errors.  It seems questionable
> that the commit amended it to do that.
> 
>>> I'm also worried that errors are reported *during* recovery.
>>> I imagine this looks confusing to a user.  The logged messages
>>> should make it clear that these are errors that occurred *earlier*
>>> and are reported belatedly.
>>
>> You raise an excellent point about potential user confusion. The current
>> aer_print_error() interface doesn't indicate that these are historical
>> errors being reported belatedly. Would it be acceptable to add a
>> clarifying message before calling aer_print_error()? For example:
>>
>>    pci_err(dev, "Reporting error that occurred before recovery:\n");
> 
> Yes, something like that.  "Errors reported prior to reset"?  Dunno.

I'll use "Errors reported prior to reset" - it's clear and concise.

> 
> Thanks,
> 
> Lukas

Thanks.
Shuai

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Lukas Wunner]

On Mon, Oct 20, 2025 at 10:17:10PM +0800, Shuai Xue wrote:
> void aer_report_frozen_error(struct pci_dev *dev)
> {
>     struct aer_err_info info;
> 
>     if (dev->pci_type != PCI_EXP_TYPE_ENDPOINT &&
>         dev->pci_type != PCI_EXP_TYPE_RC_END)
>         return;
> 
>     aer_info_init(&info);
>     aer_add_error_device(&info, dev);
>     info.severity = AER_FATAL;
>     if (aer_get_device_error_info(&info, 0, true))
>         aer_print_error(&info, 0);
> 
>     /* pci_dev_put() pairs with pci_dev_get() in aer_add_error_device() */
>     pci_dev_put(dev);
> }

Much better.  Again, I think you don't need to rename add_error_device()
and then the code comment even fits on the same line:

	pci_dev_put(dev);  /* pairs with pci_dev_get() in add_error_device() */

> > >    .slot_reset()
> > >      => pci_restore_state()
> > >        => pci_aer_clear_status()
> > 
> > This was added in 2015 by b07461a8e45b.  The commit claims that
> > the errors are stale and can be ignored.  It turns out they cannot.
> > 
> > So maybe pci_restore_state() should print information about the
> > errors before clearing them?
> 
> While that could work, we would lose the error severity information at

Wait, we've got that saved in pci_cap_saved_state, so we could restore
the severity register, report leftover errors, then clear those errors?

Thanks,

Lukas

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Shuai Xue]

在 2025/10/20 22:24, Lukas Wunner 写道:
> On Mon, Oct 20, 2025 at 10:17:10PM +0800, Shuai Xue wrote:
>> void aer_report_frozen_error(struct pci_dev *dev)
>> {
>>      struct aer_err_info info;
>>
>>      if (dev->pci_type != PCI_EXP_TYPE_ENDPOINT &&
>>          dev->pci_type != PCI_EXP_TYPE_RC_END)
>>          return;
>>
>>      aer_info_init(&info);
>>      aer_add_error_device(&info, dev);
>>      info.severity = AER_FATAL;
>>      if (aer_get_device_error_info(&info, 0, true))
>>          aer_print_error(&info, 0);
>>
>>      /* pci_dev_put() pairs with pci_dev_get() in aer_add_error_device() */
>>      pci_dev_put(dev);
>> }

Hi Lukas,
> 
> Much better.  Again, I think you don't need to rename add_error_device()
> and then the code comment even fits on the same line:

Good point. I'll keep the original function name and use the one-line
comment format.

> 
> 	pci_dev_put(dev);  /* pairs with pci_dev_get() in add_error_device() */
> 
>>>>     .slot_reset()
>>>>       => pci_restore_state()
>>>>         => pci_aer_clear_status()
>>>
>>> This was added in 2015 by b07461a8e45b.  The commit claims that
>>> the errors are stale and can be ignored.  It turns out they cannot.
>>>
>>> So maybe pci_restore_state() should print information about the
>>> errors before clearing them?
>>
>> While that could work, we would lose the error severity information at
> 
> Wait, we've got that saved in pci_cap_saved_state, so we could restore
> the severity register, report leftover errors, then clear those errors?

You're right that the severity register is also sticky, so we could
retrieve error severity directly from AER registers.

However, I have concerns about implementing this approach:

1. Scope creep: The current implementation is explicit - we know we're
in an AER error recovery path and intentionally report the missed fatal
errors. Your suggestion would make error reporting implicit during any
device state restoration.

2. Wider impact: pci_save_state() and pci_restore_state() are used
extensively beyond AER error handling - in power management, reset
operations, and various driver flows. Adding error reporting to
pci_restore_state() could generate unexpected error messages in
non-error scenarios.

3. Architectural consistency: As you noted earlier, "pci_restore_state()
is only supposed to restore state, as the name implies, and not clear
errors." Adding error reporting to this function would further violate
this principle - we'd be making it do even more than just restore state.

Would you prefer I implement this broader change, or shall we proceed
with the targeted helper function approach for now? The helper function
solves the immediate problem while keeping the changes focused on the
AER recovery path.

> Thanks,
> 
> Lukas

Thanks.
Shuai

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Lukas Wunner]

On Mon, Oct 20, 2025 at 11:20:58PM +0800, Shuai Xue wrote:
> 2025/10/20 22:24, Lukas Wunner:
> > On Mon, Oct 20, 2025 at 10:17:10PM +0800, Shuai Xue wrote:
> > > > >     .slot_reset()
> > > > >       => pci_restore_state()
> > > > >         => pci_aer_clear_status()
> > > > 
> > > > This was added in 2015 by b07461a8e45b.  The commit claims that
> > > > the errors are stale and can be ignored.  It turns out they cannot.
> > > > 
> > > > So maybe pci_restore_state() should print information about the
> > > > errors before clearing them?
> > > 
> > > While that could work, we would lose the error severity information at
> > 
> > Wait, we've got that saved in pci_cap_saved_state, so we could restore
> > the severity register, report leftover errors, then clear those errors?
> 
> You're right that the severity register is also sticky, so we could
> retrieve error severity directly from AER registers.
> 
> However, I have concerns about implementing this approach:
[...]
> 3. Architectural consistency: As you noted earlier, "pci_restore_state()
> is only supposed to restore state, as the name implies, and not clear
> errors." Adding error reporting to this function would further violate
> this principle - we'd be making it do even more than just restore state.
> 
> Would you prefer I implement this broader change, or shall we proceed
> with the targeted helper function approach for now? The helper function
> solves the immediate problem while keeping the changes focused on the
> AER recovery path.

My opinion is that b07461a8e45b was wrong and that reported errors
should not be silently ignored.  What I'd prefer is that if
pci_restore_state() discovers unreported errors, it asks the AER driver
to report them.

We've already got a helper to do that:  aer_recover_queue()
It queues up an entry in AER's kfifo and asks AER to report it.

So far the function is only used by GHES.  GHES allocates the
aer_regs argument from ghes_estatus_pool using gen_pool_alloc().
Consequently aer_recover_work_func() uses ghes_estatus_pool_region_free()
to free the allocation.  That prevents using aer_recover_queue()
for anything else than GHES.  It would first be necessary to
refactor aer_recover_queue() + aer_recover_work_func() such that
it can cope with arbitrary allocations (e.g. kmalloc()).

Thanks,

Lukas

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Shuai Xue]

在 2025/10/23 18:48, Lukas Wunner 写道:
> On Mon, Oct 20, 2025 at 11:20:58PM +0800, Shuai Xue wrote:
>> 2025/10/20 22:24, Lukas Wunner:
>>> On Mon, Oct 20, 2025 at 10:17:10PM +0800, Shuai Xue wrote:
>>>>>>      .slot_reset()
>>>>>>        => pci_restore_state()
>>>>>>          => pci_aer_clear_status()
>>>>>
>>>>> This was added in 2015 by b07461a8e45b.  The commit claims that
>>>>> the errors are stale and can be ignored.  It turns out they cannot.
>>>>>
>>>>> So maybe pci_restore_state() should print information about the
>>>>> errors before clearing them?
>>>>
>>>> While that could work, we would lose the error severity information at
>>>
>>> Wait, we've got that saved in pci_cap_saved_state, so we could restore
>>> the severity register, report leftover errors, then clear those errors?
>>
>> You're right that the severity register is also sticky, so we could
>> retrieve error severity directly from AER registers.
>>
>> However, I have concerns about implementing this approach:
> [...]
>> 3. Architectural consistency: As you noted earlier, "pci_restore_state()
>> is only supposed to restore state, as the name implies, and not clear
>> errors." Adding error reporting to this function would further violate
>> this principle - we'd be making it do even more than just restore state.
>>
>> Would you prefer I implement this broader change, or shall we proceed
>> with the targeted helper function approach for now? The helper function
>> solves the immediate problem while keeping the changes focused on the
>> AER recovery path.
> 
> My opinion is that b07461a8e45b was wrong and that reported errors
> should not be silently ignored. 

Thanks for your input and for discussing the history of commit
b07461a8e45b. I understand its intention to ignore errors specifically
during enumeration. As far as I know, AdvNonFatalErr events can occur in
this phase and typically should be ignored to simplify handling.

> What I'd prefer is that if
> pci_restore_state() discovers unreported errors, it asks the AER driver
> to report them.
> 
> We've already got a helper to do that:  aer_recover_queue()
> It queues up an entry in AER's kfifo and asks AER to report it.
> 
> So far the function is only used by GHES.  GHES allocates the
> aer_regs argument from ghes_estatus_pool using gen_pool_alloc().
> Consequently aer_recover_work_func() uses ghes_estatus_pool_region_free()
> to free the allocation.  That prevents using aer_recover_queue()
> for anything else than GHES.  It would first be necessary to
> refactor aer_recover_queue() + aer_recover_work_func() such that
> it can cope with arbitrary allocations (e.g. kmalloc()).

I agree that aer_recover_queue() and aer_recover_work_func() offer a
generalized way to report errors.

However, I’d like to highlight some concerns regarding error discovery
during pci_restore_state():

- Errors During Enumeration via Hotplug: Errors such as AdvNonFatalErr
   seen during enumeration or hotplug are generally intended to be
   ignored, as handling them adds unnecessary complexity without
   practical benefits.

- Errors During Downstream Port Containment (DPC): When an error is
   detected and not masked, it is expected to propagate through the usual
   AER path, either reported directly to the OS or to the firmware.
   Finally, these errors should be cleared and reported in a single
   cohesive step.

For missed fatal errors during DPC, queuing additional work to report
these errors using aer_recover_queue() could introduce significant
overhead. Specifically: It may result in the bus being reset and the
device reset again, which could unnecessarily disrupt system operation.

Do we really need the heavy way?

I would appreciate more feedback from the community on whether queuing
another recovery task for errors detected during pci_restore_state()

Thanks
Shuai

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Kuppuswamy Sathyanarayanan]

On 10/14/25 19:41, Shuai Xue wrote:
> The AER driver has historically avoided reading the configuration space of
> an endpoint or RCiEP that reported a fatal error, considering the link to
> that device unreliable. Consequently, when a fatal error occurs, the AER
> and DPC drivers do not report specific error types, resulting in logs like:
>
>    pcieport 0015:00:00.0: EDR: EDR event received
>    pcieport 0015:00:00.0: EDR: Reported EDR dev: 0015:00:00.0
>    pcieport 0015:00:00.0: DPC: containment event, status:0x200d, ERR_FATAL received from 0015:01:00.0
>    pcieport 0015:00:00.0: AER: broadcast error_detected message
>    pcieport 0015:00:00.0: AER: broadcast mmio_enabled message
>    pcieport 0015:00:00.0: AER: broadcast resume message
>    pcieport 0015:00:00.0: pciehp: Slot(21): Link Down/Up ignored
>    pcieport 0015:00:00.0: AER: device recovery successful
>    pcieport 0015:00:00.0: EDR: DPC port successfully recovered
>    pcieport 0015:00:00.0: EDR: Status for 0015:00:00.0: 0x80
>
> AER status registers are sticky and Write-1-to-clear. If the link recovered
> after hot reset, we can still safely access AER status and TLP header of the
> error device. In such case, report fatal errors which helps to figure out the
> error root case.
>
> After this patch, the logs like:
>
>    pcieport 0015:00:00.0: EDR: EDR event received
>    pcieport 0015:00:00.0: EDR: Reported EDR dev: 0015:00:00.0
>    pcieport 0015:00:00.0: DPC: containment event, status:0x200d, ERR_FATAL received from 0015:01:00.0
>    pcieport 0015:00:00.0: AER: broadcast error_detected message
>    vfio-pci 0015:01:00.0: PCIe Bus Error: severity=Uncorrectable (Fatal), type=Transaction Layer, (Receiver ID)
>    pcieport 0015:00:00.0: pciehp: Slot(21): Link Down/Up ignored

It would be more clear if you follow the same order of the log as before section
and highlight the new logs that are getting added.

>    vfio-pci 0015:01:00.0:   device [144d:a80a] error status/mask=00001000/00400000
>    vfio-pci 0015:01:00.0:    [12] TLP                    (First)
>    vfio-pci 0015:01:00.0: AER:   TLP Header: 0x4a004010 0x00000040 0x01000000 0xffffffff
>    pcieport 0015:00:00.0: AER: broadcast mmio_enabled message
>    pcieport 0015:00:00.0: AER: broadcast resume message
>    pcieport 0015:00:00.0: AER: device recovery successful
>    pcieport 0015:00:00.0: EDR: DPC port successfully recovered
>    pcieport 0015:00:00.0: EDR: Status for 0015:00:00.0: 0x80
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> ---
>   drivers/pci/pci.h      |  4 +++-
>   drivers/pci/pcie/aer.c | 18 +++++++++++-------
>   drivers/pci/pcie/dpc.c |  2 +-
>   drivers/pci/pcie/err.c | 11 +++++++++++
>   4 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6b0c55bed15b..3eccef2d25a3 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -739,8 +739,10 @@ struct aer_err_info {
>   	struct pcie_tlp_log tlp;	/* TLP Header */
>   };
>   
> -int aer_get_device_error_info(struct aer_err_info *info, int i);
> +int aer_get_device_error_info(struct aer_err_info *info, int i,
> +			      bool link_healthy);
>   void aer_print_error(struct aer_err_info *info, int i);
> +int aer_add_error_device(struct aer_err_info *e_info, struct pci_dev *dev);
>   
>   int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
>   		      unsigned int tlp_len, bool flit,
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 0b5ed4722ac3..aaea9902cbb7 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -978,7 +978,7 @@ EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
>    * @e_info: pointer to error info
>    * @dev: pointer to pci_dev to be added
>    */
> -static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
> +int aer_add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)

I don't think you need this rename.

>   {
>   	int i = e_info->error_dev_num;
>   
> @@ -1068,7 +1068,7 @@ static int find_device_iter(struct pci_dev *dev, void *data)
>   
>   	if (is_error_source(dev, e_info)) {
>   		/* List this device */
> -		if (add_error_device(e_info, dev)) {
> +		if (aer_add_error_device(e_info, dev)) {
>   			/* We cannot handle more... Stop iteration */
>   			pci_err(dev, "Exceeded max supported (%d) devices with errors logged\n",
>   				AER_MAX_MULTI_ERR_DEVICES);
> @@ -1382,12 +1382,14 @@ EXPORT_SYMBOL_GPL(aer_recover_queue);
>    * aer_get_device_error_info - read error status from dev and store it to info
>    * @info: pointer to structure to store the error record
>    * @i: index into info->dev[]
> + * @link_healthy: link is healthy or not
>    *
>    * Return: 1 on success, 0 on error.
>    *
>    * Note that @info is reused among all error devices. Clear fields properly.
>    */
> -int aer_get_device_error_info(struct aer_err_info *info, int i)
> +int aer_get_device_error_info(struct aer_err_info *info, int i,
> +			      bool link_healthy)
>   {
>   	struct pci_dev *dev;
>   	int type, aer;
> @@ -1415,10 +1417,12 @@ int aer_get_device_error_info(struct aer_err_info *info, int i)
>   			&info->mask);
>   		if (!(info->status & ~info->mask))
>   			return 0;
> +		info->level = KERN_WARNING;

I recommend setting this when initializing the info->level at the caller end (to match
other callers)

>   	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
>   		   type == PCI_EXP_TYPE_RC_EC ||
>   		   type == PCI_EXP_TYPE_DOWNSTREAM ||
> -		   info->severity == AER_NONFATAL) {
> +		   info->severity == AER_NONFATAL ||
> +		   (info->severity == AER_FATAL && link_healthy)) {
>   
>   		/* Link is still healthy for IO reads */
>   		pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,
> @@ -1427,7 +1431,7 @@ int aer_get_device_error_info(struct aer_err_info *info, int i)
>   			&info->mask);
>   		if (!(info->status & ~info->mask))
>   			return 0;
> -
> +		info->level = KERN_ERR;
>   		/* Get First Error Pointer */
>   		pci_read_config_dword(dev, aer + PCI_ERR_CAP, &aercc);
>   		info->first_error = PCI_ERR_CAP_FEP(aercc);
> @@ -1451,11 +1455,11 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info)
>   
>   	/* Report all before handling them, to not lose records by reset etc. */
>   	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
> -		if (aer_get_device_error_info(e_info, i))
> +		if (aer_get_device_error_info(e_info, i, false))
>   			aer_print_error(e_info, i);
>   	}
>   	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
> -		if (aer_get_device_error_info(e_info, i))
> +		if (aer_get_device_error_info(e_info, i, false))
>   			handle_error_source(e_info->dev[i], e_info);
>   	}
>   }
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index f6069f621683..21c4e8371279 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -284,7 +284,7 @@ struct pci_dev *dpc_process_error(struct pci_dev *pdev)
>   		pci_warn(pdev, "containment event, status:%#06x: unmasked uncorrectable error detected\n",
>   			 status);
>   		if (dpc_get_aer_uncorrect_severity(pdev, &info) &&
> -		    aer_get_device_error_info(&info, 0)) {
> +		    aer_get_device_error_info(&info, 0, false)) {
>   			aer_print_error(&info, 0);
>   			pci_aer_clear_nonfatal_status(pdev);
>   			pci_aer_clear_fatal_status(pdev);
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index bebe4bc111d7..4e65eac809d1 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -215,6 +215,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *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);
> +	struct aer_err_info info;
>   
>   	/*
>   	 * If the error was detected by a Root Port, Downstream Port, RCEC,
> @@ -253,6 +254,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>   			pci_warn(bridge, "subordinate device reset failed\n");
>   			goto failed;
>   		}
> +
> +		/* Link recovered, report fatal errors of RCiEP or EP */
> +		if (state == pci_channel_io_frozen &&
> +		    (type == PCI_EXP_TYPE_ENDPOINT || type == PCI_EXP_TYPE_RC_END)) {
> +			aer_add_error_device(&info, dev);
> +			info.severity = AER_FATAL;
info.level = KERN_ERR ?
> +			if (aer_get_device_error_info(&info, 0, true))
> +				aer_print_error(&info, 0);
> +			pci_dev_put(dev);

Like Lukas mentioned, it needs a comment about why you need this.

> +		}
>   	}
>   
>   	if (status == PCI_ERS_RESULT_NEED_RESET) {

Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

[Shuai Xue]

在 2025/10/21 02:38, Kuppuswamy Sathyanarayanan 写道:
> 
> On 10/14/25 19:41, Shuai Xue wrote:
>> The AER driver has historically avoided reading the configuration space of
>> an endpoint or RCiEP that reported a fatal error, considering the link to
>> that device unreliable. Consequently, when a fatal error occurs, the AER
>> and DPC drivers do not report specific error types, resulting in logs like:
>>
>>    pcieport 0015:00:00.0: EDR: EDR event received
>>    pcieport 0015:00:00.0: EDR: Reported EDR dev: 0015:00:00.0
>>    pcieport 0015:00:00.0: DPC: containment event, status:0x200d, ERR_FATAL received from 0015:01:00.0
>>    pcieport 0015:00:00.0: AER: broadcast error_detected message
>>    pcieport 0015:00:00.0: AER: broadcast mmio_enabled message
>>    pcieport 0015:00:00.0: AER: broadcast resume message
>>    pcieport 0015:00:00.0: pciehp: Slot(21): Link Down/Up ignored
>>    pcieport 0015:00:00.0: AER: device recovery successful
>>    pcieport 0015:00:00.0: EDR: DPC port successfully recovered
>>    pcieport 0015:00:00.0: EDR: Status for 0015:00:00.0: 0x80
>>
>> AER status registers are sticky and Write-1-to-clear. If the link recovered
>> after hot reset, we can still safely access AER status and TLP header of the
>> error device. In such case, report fatal errors which helps to figure out the
>> error root case.
>>
>> After this patch, the logs like:
>>
>>    pcieport 0015:00:00.0: EDR: EDR event received
>>    pcieport 0015:00:00.0: EDR: Reported EDR dev: 0015:00:00.0
>>    pcieport 0015:00:00.0: DPC: containment event, status:0x200d, ERR_FATAL received from 0015:01:00.0
>>    pcieport 0015:00:00.0: AER: broadcast error_detected message
>>    vfio-pci 0015:01:00.0: PCIe Bus Error: severity=Uncorrectable (Fatal), type=Transaction Layer, (Receiver ID)
>>    pcieport 0015:00:00.0: pciehp: Slot(21): Link Down/Up ignored
> 
> It would be more clear if you follow the same order of the log as before section
> and highlight the new logs that are getting added.

I see. I will reorder the pciehp log.

> 
>>    vfio-pci 0015:01:00.0:   device [144d:a80a] error status/mask=00001000/00400000
>>    vfio-pci 0015:01:00.0:    [12] TLP                    (First)
>>    vfio-pci 0015:01:00.0: AER:   TLP Header: 0x4a004010 0x00000040 0x01000000 0xffffffff
>>    pcieport 0015:00:00.0: AER: broadcast mmio_enabled message
>>    pcieport 0015:00:00.0: AER: broadcast resume message
>>    pcieport 0015:00:00.0: AER: device recovery successful
>>    pcieport 0015:00:00.0: EDR: DPC port successfully recovered
>>    pcieport 0015:00:00.0: EDR: Status for 0015:00:00.0: 0x80
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> ---
>>   drivers/pci/pci.h      |  4 +++-
>>   drivers/pci/pcie/aer.c | 18 +++++++++++-------
>>   drivers/pci/pcie/dpc.c |  2 +-
>>   drivers/pci/pcie/err.c | 11 +++++++++++
>>   4 files changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 6b0c55bed15b..3eccef2d25a3 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -739,8 +739,10 @@ struct aer_err_info {
>>       struct pcie_tlp_log tlp;    /* TLP Header */
>>   };
>> -int aer_get_device_error_info(struct aer_err_info *info, int i);
>> +int aer_get_device_error_info(struct aer_err_info *info, int i,
>> +                  bool link_healthy);
>>   void aer_print_error(struct aer_err_info *info, int i);
>> +int aer_add_error_device(struct aer_err_info *e_info, struct pci_dev *dev);
>>   int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
>>                 unsigned int tlp_len, bool flit,
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index 0b5ed4722ac3..aaea9902cbb7 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -978,7 +978,7 @@ EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
>>    * @e_info: pointer to error info
>>    * @dev: pointer to pci_dev to be added
>>    */
>> -static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>> +int aer_add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
> 
> I don't think you need this rename.

Yep, will rename back.

> 
>>   {
>>       int i = e_info->error_dev_num;
>> @@ -1068,7 +1068,7 @@ static int find_device_iter(struct pci_dev *dev, void *data)
>>       if (is_error_source(dev, e_info)) {
>>           /* List this device */
>> -        if (add_error_device(e_info, dev)) {
>> +        if (aer_add_error_device(e_info, dev)) {
>>               /* We cannot handle more... Stop iteration */
>>               pci_err(dev, "Exceeded max supported (%d) devices with errors logged\n",
>>                   AER_MAX_MULTI_ERR_DEVICES);
>> @@ -1382,12 +1382,14 @@ EXPORT_SYMBOL_GPL(aer_recover_queue);
>>    * aer_get_device_error_info - read error status from dev and store it to info
>>    * @info: pointer to structure to store the error record
>>    * @i: index into info->dev[]
>> + * @link_healthy: link is healthy or not
>>    *
>>    * Return: 1 on success, 0 on error.
>>    *
>>    * Note that @info is reused among all error devices. Clear fields properly.
>>    */
>> -int aer_get_device_error_info(struct aer_err_info *info, int i)
>> +int aer_get_device_error_info(struct aer_err_info *info, int i,
>> +                  bool link_healthy)
>>   {
>>       struct pci_dev *dev;
>>       int type, aer;
>> @@ -1415,10 +1417,12 @@ int aer_get_device_error_info(struct aer_err_info *info, int i)
>>               &info->mask);
>>           if (!(info->status & ~info->mask))
>>               return 0;
>> +        info->level = KERN_WARNING;
> 
> I recommend setting this when initializing the info->level at the caller end (to match
> other callers)

Good point, will drop this changes.

> 
>>       } else if (type == PCI_EXP_TYPE_ROOT_PORT ||
>>              type == PCI_EXP_TYPE_RC_EC ||
>>              type == PCI_EXP_TYPE_DOWNSTREAM ||
>> -           info->severity == AER_NONFATAL) {
>> +           info->severity == AER_NONFATAL ||
>> +           (info->severity == AER_FATAL && link_healthy)) {
>>           /* Link is still healthy for IO reads */
>>           pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,
>> @@ -1427,7 +1431,7 @@ int aer_get_device_error_info(struct aer_err_info *info, int i)
>>               &info->mask);
>>           if (!(info->status & ~info->mask))
>>               return 0;
>> -
>> +        info->level = KERN_ERR;
>>           /* Get First Error Pointer */
>>           pci_read_config_dword(dev, aer + PCI_ERR_CAP, &aercc);
>>           info->first_error = PCI_ERR_CAP_FEP(aercc);
>> @@ -1451,11 +1455,11 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info)
>>       /* Report all before handling them, to not lose records by reset etc. */
>>       for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
>> -        if (aer_get_device_error_info(e_info, i))
>> +        if (aer_get_device_error_info(e_info, i, false))
>>               aer_print_error(e_info, i);
>>       }
>>       for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
>> -        if (aer_get_device_error_info(e_info, i))
>> +        if (aer_get_device_error_info(e_info, i, false))
>>               handle_error_source(e_info->dev[i], e_info);
>>       }
>>   }
>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> index f6069f621683..21c4e8371279 100644
>> --- a/drivers/pci/pcie/dpc.c
>> +++ b/drivers/pci/pcie/dpc.c
>> @@ -284,7 +284,7 @@ struct pci_dev *dpc_process_error(struct pci_dev *pdev)
>>           pci_warn(pdev, "containment event, status:%#06x: unmasked uncorrectable error detected\n",
>>                status);
>>           if (dpc_get_aer_uncorrect_severity(pdev, &info) &&
>> -            aer_get_device_error_info(&info, 0)) {
>> +            aer_get_device_error_info(&info, 0, false)) {
>>               aer_print_error(&info, 0);
>>               pci_aer_clear_nonfatal_status(pdev);
>>               pci_aer_clear_fatal_status(pdev);
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index bebe4bc111d7..4e65eac809d1 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -215,6 +215,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *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);
>> +    struct aer_err_info info;
>>       /*
>>        * If the error was detected by a Root Port, Downstream Port, RCEC,
>> @@ -253,6 +254,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>               pci_warn(bridge, "subordinate device reset failed\n");
>>               goto failed;
>>           }
>> +
>> +        /* Link recovered, report fatal errors of RCiEP or EP */
>> +        if (state == pci_channel_io_frozen &&
>> +            (type == PCI_EXP_TYPE_ENDPOINT || type == PCI_EXP_TYPE_RC_END)) {
>> +            aer_add_error_device(&info, dev);
>> +            info.severity = AER_FATAL;
> info.level = KERN_ERR ?

Yep, will set level in caller site.

>> +            if (aer_get_device_error_info(&info, 0, true))
>> +                aer_print_error(&info, 0);
>> +            pci_dev_put(dev);
> 
> Like Lukas mentioned, it needs a comment about why you need this.
> 

Sure, will add it.

Thanks.
Shuai

[PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Shuai Xue]

Replace the manual checks for native AER control with the
pcie_aer_is_native() helper, which provides a more robust way
to determine if we have native control of AER.

No functional changes intended.

Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 drivers/pci/pcie/err.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 4e65eac809d1..097990094b71 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -214,7 +214,6 @@ 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);
 	struct aer_err_info info;
 
 	/*
@@ -289,7 +288,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	 * 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) {
+	if (pcie_aer_is_native(dev)) {
 		pcie_clear_device_status(dev);
 		pci_aer_clear_nonfatal_status(dev);
 	}
-- 
2.39.3

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Lukas Wunner]

On Wed, Oct 15, 2025 at 10:41:58AM +0800, Shuai Xue wrote:
> Replace the manual checks for native AER control with the
> pcie_aer_is_native() helper, which provides a more robust way
> to determine if we have native control of AER.

Why is it more robust?

Thanks,

Lukas

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Shuai Xue]

在 2025/10/20 18:17, Lukas Wunner 写道:
> On Wed, Oct 15, 2025 at 10:41:58AM +0800, Shuai Xue wrote:
>> Replace the manual checks for native AER control with the
>> pcie_aer_is_native() helper, which provides a more robust way
>> to determine if we have native control of AER.

Hi, Lukas,

Thank you for your question.

> 
> Why is it more robust?

IMHO, the pcie_aer_is_native() helper is more robust because it includes
additional safety checks that the manual approach lacks:

int pcie_aer_is_native(struct pci_dev *dev)
{
	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);

	if (!dev->aer_cap)
		return 0;

	return pcie_ports_native || host->native_aer;
}
EXPORT_SYMBOL_NS_GPL(pcie_aer_is_native, "CXL");

Specifically, it performs a sanity check for dev->aer_cap before
evaluating native AER control.

And I think this is more maintainable than manual checks scattered
throughout the code, as the helper encapsulates the exact conditions
required to determine native AER control.

> 
> Thanks,
> 
> Lukas

Thanks.

Shuai

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Lukas Wunner]

On Mon, Oct 20, 2025 at 09:09:41PM +0800, Shuai Xue wrote:
> ??? 2025/10/20 18:17, Lukas Wunner ??????:
> > On Wed, Oct 15, 2025 at 10:41:58AM +0800, Shuai Xue wrote:
> > > Replace the manual checks for native AER control with the
> > > pcie_aer_is_native() helper, which provides a more robust way
> > > to determine if we have native control of AER.
> > 
> > Why is it more robust?
> 
> IMHO, the pcie_aer_is_native() helper is more robust because it includes
> additional safety checks that the manual approach lacks:
[...]
> Specifically, it performs a sanity check for dev->aer_cap before
> evaluating native AER control.

I'm under the impression that aer_cap must be set, otherwise the
error wouldn't have been reported and we wouldn't be in this code path?

If we can end up in this code path without aer_cap set, your patch
would regress devices which are not AER-capable because it would
now skip clearing of errors in the Device Status register via
pcie_clear_device_status().

Thanks,

Lukas

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Shuai Xue]

在 2025/10/20 21:58, Lukas Wunner 写道:
> On Mon, Oct 20, 2025 at 09:09:41PM +0800, Shuai Xue wrote:
>> ??? 2025/10/20 18:17, Lukas Wunner ??????:
>>> On Wed, Oct 15, 2025 at 10:41:58AM +0800, Shuai Xue wrote:
>>>> Replace the manual checks for native AER control with the
>>>> pcie_aer_is_native() helper, which provides a more robust way
>>>> to determine if we have native control of AER.
>>>
>>> Why is it more robust?
>>
>> IMHO, the pcie_aer_is_native() helper is more robust because it includes
>> additional safety checks that the manual approach lacks:
> [...]
>> Specifically, it performs a sanity check for dev->aer_cap before
>> evaluating native AER control.
> 
> I'm under the impression that aer_cap must be set, otherwise the
> error wouldn't have been reported and we wouldn't be in this code path?
> 
> If we can end up in this code path without aer_cap set, your patch
> would regress devices which are not AER-capable because it would
> now skip clearing of errors in the Device Status register via
> pcie_clear_device_status().

Hi Lukas,

You raise an excellent point about the potential regression.

The origin code is:

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

This code clears both the PCIe Device Status register and AER status
registers when in native AER mode.

pcie_clear_device_status() is renamed from
pci_aer_clear_device_status(). Does it intends to clear only AER error
status?

- BIT 0: Correctable Error Detected
- BIT 1: Non-Fatal Error Detected
- BIT 2: Fatal Error Detected
- BIT 3: Unsupported Request Detected

 From PCIe spec, BIT 0-2 are logged for functions supporting Advanced
Error Handling.

I am not sure if we should clear BIT 3, and also BIT 6 (Emergency Power
Reduction Detected) and in case a AER error.

> 
> Thanks,
> 
> Lukas

Thanks.
Shuai

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Lukas Wunner]

On Mon, Oct 20, 2025 at 10:45:31PM +0800, Shuai Xue wrote:
> 	if (host->native_aer || pcie_ports_native) {
> 		pcie_clear_device_status(bridge);
> 		pci_aer_clear_nonfatal_status(bridge);
> 	}
> 
> This code clears both the PCIe Device Status register and AER status
> registers when in native AER mode.
> 
> pcie_clear_device_status() is renamed from
> pci_aer_clear_device_status(). Does it intends to clear only AER error
> status?
> 
> - BIT 0: Correctable Error Detected
> - BIT 1: Non-Fatal Error Detected
> - BIT 2: Fatal Error Detected
> - BIT 3: Unsupported Request Detected
> 
> From PCIe spec, BIT 0-2 are logged for functions supporting Advanced
> Error Handling.
> 
> I am not sure if we should clear BIT 3, and also BIT 6 (Emergency Power
> Reduction Detected) and in case a AER error.

AFAIUI, bits 0 to 3 are what the PCIe r7.0 sec 6.2.1 calls
"baseline capability" error reporting.  They're supported
even if AER is not supported.

Bit 6 has nothing to do with this AFAICS.

Thanks,

Lukas

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Shuai Xue]

在 2025/10/23 18:29, Lukas Wunner 写道:
> On Mon, Oct 20, 2025 at 10:45:31PM +0800, Shuai Xue wrote:
>> 	if (host->native_aer || pcie_ports_native) {
>> 		pcie_clear_device_status(bridge);
>> 		pci_aer_clear_nonfatal_status(bridge);
>> 	}
>>
>> This code clears both the PCIe Device Status register and AER status
>> registers when in native AER mode.
>>
>> pcie_clear_device_status() is renamed from
>> pci_aer_clear_device_status(). Does it intends to clear only AER error
>> status?
>>
>> - BIT 0: Correctable Error Detected
>> - BIT 1: Non-Fatal Error Detected
>> - BIT 2: Fatal Error Detected
>> - BIT 3: Unsupported Request Detected
>>
>>  From PCIe spec, BIT 0-2 are logged for functions supporting Advanced
>> Error Handling.
>>
>> I am not sure if we should clear BIT 3, and also BIT 6 (Emergency Power
>> Reduction Detected) and in case a AER error.
> 
> AFAIUI, bits 0 to 3 are what the PCIe r7.0 sec 6.2.1 calls
> "baseline capability" error reporting.  They're supported
> even if AER is not supported.
> 
> Bit 6 has nothing to do with this AFAICS.

Hi, Lukas,

Per PCIe r7.0 section 7.5.3.5:

   **For Functions supporting Advanced Error Handling**, errors are logged
   in this register regardless of the settings of the Uncorrectable Error
   Mask register. Default value of this bit is 0b.

 From this, it's clear that bits 0 to 2 are not logged unless AER is supported.

So, if dev->aer_cap is not true, there’s no need to clear bits 0 to 2.
This validates the dev->aer_cap sanity check in pcie_aer_is_native():

   int pcie_aer_is_native(struct pci_dev *dev)
   {
       struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);

       if (!dev->aer_cap)
           return 0;

       return pcie_ports_native || host->native_aer;
   }
   EXPORT_SYMBOL_NS_GPL(pcie_aer_is_native, "CXL");

Based on this, the introduction of pcie_aer_is_native() in the patch
seems reasonable and consistent with the PCIe specification.

Further, should we rename pcie_clear_device_status() back to
pci_aer_clear_device_status():

-void pcie_clear_device_status(struct pci_dev *dev)
+void pci_aer_clear_device_status(struct pci_dev *dev)
  {
         u16 sta;

         pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &sta);
-       pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
+       /* Bits 0-2 are logged if AER is supported */
+       pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta & 0x7);
  }

I am still uncertain whether bit 3 ("Unsupported Request Detected")
should be cleared in this function. It’s not directly tied to AER
capability.


I’d love to hear your thoughts, as well as @Bjorn’s, on both the renaming
suggestion and whether bit 3 should be cleared alongside bits 0 to 2.

Thanks.
Shuai

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Lukas Wunner]

On Fri, Oct 24, 2025 at 11:09:25AM +0800, Shuai Xue wrote:
> 2025/10/23 18:29, Lukas Wunner:
> > On Mon, Oct 20, 2025 at 10:45:31PM +0800, Shuai Xue wrote:
> > > From PCIe spec, BIT 0-2 are logged for functions supporting Advanced
> > > Error Handling.
> > > 
> > > I am not sure if we should clear BIT 3, and also BIT 6 (Emergency Power
> > > Reduction Detected) and in case a AER error.
> > 
> > AFAIUI, bits 0 to 3 are what the PCIe r7.0 sec 6.2.1 calls
> > "baseline capability" error reporting.  They're supported
> > even if AER is not supported.
> > 
> > Bit 6 has nothing to do with this AFAICS.
> 
> Per PCIe r7.0 section 7.5.3.5:
> 
>   **For Functions supporting Advanced Error Handling**, errors are logged
>   in this register regardless of the settings of the Uncorrectable Error
>   Mask register. Default value of this bit is 0b.
> 
> From this, it's clear that bits 0 to 2 are not logged unless AER is supported.

No.  It just means that if AER is supported, the Uncorrectable Error Mask
register has no bearing on whether the bits in the Device Status register
are set.  It does not mean that the bits are only set if AER is supported.

Thanks,

Lukas

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Shuai Xue]

在 2025/10/24 11:14, Lukas Wunner 写道:
> On Fri, Oct 24, 2025 at 11:09:25AM +0800, Shuai Xue wrote:
>> 2025/10/23 18:29, Lukas Wunner:
>>> On Mon, Oct 20, 2025 at 10:45:31PM +0800, Shuai Xue wrote:
>>>>  From PCIe spec, BIT 0-2 are logged for functions supporting Advanced
>>>> Error Handling.
>>>>
>>>> I am not sure if we should clear BIT 3, and also BIT 6 (Emergency Powerjj
>>>> Reduction Detected) and in case a AER error.
>>>
>>> AFAIUI, bits 0 to 3 are what the PCIe r7.0 sec 6.2.1 calls
>>> "baseline capability" error reporting.  They're supported
>>> even if AER is not supported.
>>>
>>> Bit 6 has nothing to do with this AFAICS.
>>
>> Per PCIe r7.0 section 7.5.3.5:
>>
>>    **For Functions supporting Advanced Error Handling**, errors are logged
>>    in this register regardless of the settings of the Uncorrectable Error
>>    Mask register. Default value of this bit is 0b.
>>
>>  From this, it's clear that bits 0 to 2 are not logged unless AER is supported.
> 
> No.  It just means that if AER is supported, the Uncorrectable Error Mask
> register has no bearing on whether the bits in the Device Status register
> are set.  It does not mean that the bits are only set if AER is supported.
> 

Thank you for pointing that out. I now understand that my interpretation
was incorrect.

As such, I will drop this patch that introduced the dev->aer_cap check.

The remaining question is whether it would make more sense to rename
pcie_clear_device_status() to pci_clear_device_error_status() and refine
its behavior by adding a mask specifically for bits 0 to 3. Here’s an
example of the proposed change:

-void pcie_clear_device_status(struct pci_dev *dev)
+void pci_clear_device_error_status(struct pci_dev *dev)
  {
         u16 sta;

         pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &sta);
-       pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
+       /* clear error-related bits: 0-3   */
+       pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta & 0xF);
  }

Renaming the function to pci_clear_device_error_status() better
reflects its current focus on clearing error-related bits, and
introducing the mask ensures that only those relevant bits (0-3) are
cleared, rather than modifying the entire register. What do you think
about these changes?

Thanks.
Shuai

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Lukas Wunner]

On Fri, Oct 24, 2025 at 11:38:10AM +0800, Shuai Xue wrote:
> The remaining question is whether it would make more sense to rename
> pcie_clear_device_status() to pci_clear_device_error_status() and refine
> its behavior by adding a mask specifically for bits 0 to 3. Here's an
> example of the proposed change:

I don't see much value in renaming the function.

However clearing only bits 0-3 makes sense.  PCIe r5.0 defined bit 6
as Emergency Power Reduction Detected with type RW1C in 2019.  The
last time we touched pcie_clear_device_status() was in 2018 with
ec752f5d54d7 and we've been clearing all bits since forever,
not foreseeing that new ones with type RW1C might be added later.

I suggest defining a new macro in include/uapi/linux/pci_regs.h
instead of using 0xf, say PCI_EXP_DEVSTA_ERR.  Then you don't
need the code comment because the code is self-explanatory.

Thanks,

Lukas

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Shuai Xue]

在 2025/10/24 12:03, Lukas Wunner 写道:
> On Fri, Oct 24, 2025 at 11:38:10AM +0800, Shuai Xue wrote:
>> The remaining question is whether it would make more sense to rename
>> pcie_clear_device_status() to pci_clear_device_error_status() and refine
>> its behavior by adding a mask specifically for bits 0 to 3. Here's an
>> example of the proposed change:
> 
> I don't see much value in renaming the function.
> 
> However clearing only bits 0-3 makes sense.  PCIe r5.0 defined bit 6
> as Emergency Power Reduction Detected with type RW1C in 2019.  The
> last time we touched pcie_clear_device_status() was in 2018 with
> ec752f5d54d7 and we've been clearing all bits since forever,
> not foreseeing that new ones with type RW1C might be added later.

Thank you for the detailed explanation and pointing out the history
behind bit 6 and the evolution since PCIe r5.0.

> 
> I suggest defining a new macro in include/uapi/linux/pci_regs.h
> instead of using 0xf, say PCI_EXP_DEVSTA_ERR.  Then you don't
> need the code comment because the code is self-explanatory.
> 

I’ll prepare a patch to implement this fix and submit it shortly.
Thanks again for the guidance!

Thanks.
Shuai

Re: [PATCH v6 4/5] PCI/ERR: Use pcie_aer_is_native() to check for native AER control

[Kuppuswamy Sathyanarayanan]

On 10/14/25 19:41, Shuai Xue wrote:
> Replace the manual checks for native AER control with the
> pcie_aer_is_native() helper, which provides a more robust way
> to determine if we have native control of AER.
>
> No functional changes intended.
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> ---

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>   drivers/pci/pcie/err.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 4e65eac809d1..097990094b71 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -214,7 +214,6 @@ 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);
>   	struct aer_err_info info;
>   
>   	/*
> @@ -289,7 +288,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>   	 * 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) {
> +	if (pcie_aer_is_native(dev)) {
>   		pcie_clear_device_status(dev);
>   		pci_aer_clear_nonfatal_status(dev);
>   	}

[PATCH v6 5/5] PCI/AER: Clear both AER fatal and non-fatal status

[Shuai Xue]

The DPC driver clears AER fatal status for the port that reported the
error, but not for the downstream device that deteced the error.  The
current recovery code only clears non-fatal AER status, leaving fatal
status bits set in the error device.

Use pci_aer_raw_clear_status() to clear both fatal and non-fatal error
status in the error device, ensuring all AER status bits are properly
cleared after recovery.

Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.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 097990094b71..28c5ca7d86ce 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -290,7 +290,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	 */
 	if (pcie_aer_is_native(dev)) {
 		pcie_clear_device_status(dev);
-		pci_aer_clear_nonfatal_status(dev);
+		pci_aer_raw_clear_status(dev);
 	}
 
 	pci_walk_bridge(bridge, pci_pm_runtime_put, NULL);
-- 
2.39.3

Re: [PATCH v6 5/5] PCI/AER: Clear both AER fatal and non-fatal status

[Kuppuswamy Sathyanarayanan]

On 10/14/25 19:41, Shuai Xue wrote:
> The DPC driver clears AER fatal status for the port that reported the
> error, but not for the downstream device that deteced the error.  The
> current recovery code only clears non-fatal AER status, leaving fatal
> status bits set in the error device.
>
> Use pci_aer_raw_clear_status() to clear both fatal and non-fatal error
> status in the error device, ensuring all AER status bits are properly
> cleared after recovery.
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> ---

I think it needs to go to stable tree. Any Fixes: commit ?

>   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 097990094b71..28c5ca7d86ce 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -290,7 +290,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>   	 */
>   	if (pcie_aer_is_native(dev)) {
>   		pcie_clear_device_status(dev);
> -		pci_aer_clear_nonfatal_status(dev);
> +		pci_aer_raw_clear_status(dev);
>   	}
>   
>   	pci_walk_bridge(bridge, pci_pm_runtime_put, NULL);

Re: [PATCH v6 5/5] PCI/AER: Clear both AER fatal and non-fatal status

[Shuai Xue]

在 2025/10/21 02:44, Kuppuswamy Sathyanarayanan 写道:
> 
> On 10/14/25 19:41, Shuai Xue wrote:
>> The DPC driver clears AER fatal status for the port that reported the
>> error, but not for the downstream device that deteced the error.  The
>> current recovery code only clears non-fatal AER status, leaving fatal
>> status bits set in the error device.
>>
>> Use pci_aer_raw_clear_status() to clear both fatal and non-fatal error
>> status in the error device, ensuring all AER status bits are properly
>> cleared after recovery.
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> ---
> 
> I think it needs to go to stable tree. Any Fixes: commit ?

Got it. I will add a Fixes tag and cc stable.

Thanks.
Shuai