Skip to contentSkip to conversation

Address error and recovery for AER and DPC

PATCH Superseded Oza Pawandeep 2 participants 15 messages Feb 22, 2018 – Feb 23, 2018

Subsystems (4)

v9 of 17 Superseded Latest is v17 →

lore
Share & tools
lore
#0 lore raw

v9 cover Address error and recovery for AER and DPC

This patch set brings in error handling support for DPC

The current implementation of AER and error message broadcasting to the EP driver is tightly coupled and limited to AER service driver. It is important to factor out broadcasting and other link handling callbacks. So that not only when AER gets triggered, but also when DPC get triggered (for e.g. ERR_FATAL), callbacks are handled appropriately.

DPC should enumerate the devices after recovering the link, which is achieved by implementing error_resume callback.

Changes since v8:
Fixed Kbuild errors.
Changes since v7:
Rebased the code on pci master.
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/helgaas/pci
Changes since v6:
Sinan's and Stefan's comments implemented.
> reordered patch 6 and 7
> cleaned up
Changes since v5:
Sinan's and Keith's comments incorporated.
> made separate patch for mutex
> unified error repotting codes into driver/pci/pci.h
> got rid of wait link active/inactive and
made generic function in driver/pci/pci.c
Changes since v4:
Bjorn's comments incorporated.
> Renamed only do_recovery.
> moved the things more locally to drivers/pci/pci.h
Changes since v3:
Bjorn's comments incorporated.
> Made separate patch renaming generic pci_err.c
> Introduce pci_err.h to contain all the error types and recovery > removed all the dependencies on pci.h
Changes since v2:
Based on feedback from Keith:
"
When DPC is triggered due to receipt of an uncorrectable error Message, the Requester ID from the Message is recorded in the DPC Error Source ID register and that Message is discarded and not forwarded Upstream. "
Removed the patch where AER checks if DPC service is active
Changes since v1:
Kbuild errors fixed:
> pci_find_dpc_dev made static
> ras_event.h updated
> pci_find_aer_service call with CONFIG check
> pci_find_dpc_service call with CONFIG check

Oza Pawandeep (7):
PCI/AER: Rename error recovery to generic pci naming PCI/AER: factor out error reporting from AER PCI/ERR: add mutex to synchronize recovery PCI/DPC: Unify and plumb error handling into DPC PCI/AER: Unify aer error defines at single space PCI/DPC: Enumerate the devices after DPC trigger event
PCI: Unify wait for link active into generic pci

 drivers/acpi/apei/ghes.c               |   1 +
 drivers/pci/hotplug/pciehp_hpc.c       |  21 +-
 drivers/pci/pci.c                      |  39 +++-
 drivers/pci/pci.h                      |  11 +
 drivers/pci/pcie/Makefile              |   2 +-
 drivers/pci/pcie/aer/aerdrv.h          |  30 ---
 drivers/pci/pcie/aer/aerdrv_core.c     | 293 +-------------------------
 drivers/pci/pcie/aer/aerdrv_errprint.c |   1 +
 drivers/pci/pcie/pcie-dpc.c            | 115 ++++++++++-
 drivers/pci/pcie/pcie-err.c            | 366 +++++++++++++++++++++++++++++++++
 drivers/pci/pcie/portdrv.h             |   2 +
 include/linux/aer.h                    |   4 -
 include/linux/pci.h                    |   1 +
 13 files changed, 534 insertions(+), 352 deletions(-)
 create mode 100644 drivers/pci/pcie/pcie-err.c
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
#1 lore raw

PATCH v9 1/7 PCI/AER: Rename error recovery to generic pci naming

This patch renames error recovery to generic name with pci prefix

drivers/pci/pcie/aer/aerdrv_core.c +4 -4
index a4bfea5..306bf2f 100644
@@ -478,7 +478,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
 }
 
 /**
- * do_recovery - handle nonfatal/fatal error recovery process
+ * pci_do_recovery - handle nonfatal/fatal error recovery process
  * @dev: pointer to a pci_dev data structure of agent detecting an error
  * @severity: error severity type
  *
@@ -486,7 +486,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
  * error detected message to all downstream drivers within a hierarchy in
  * question and return the returned code.
  */
-static void do_recovery(struct pci_dev *dev, int severity)
+static void pci_do_recovery(struct pci_dev *dev, int severity)
 {
 	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
 	enum pci_channel_state state;
@@ -566,7 +566,7 @@ static void handle_error_source(struct pcie_device *aerdev,
 			pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
 					info->status);
 	} else
-		do_recovery(dev, info->severity);
+		pci_do_recovery(dev, info->severity);
 }
 
 #ifdef CONFIG_ACPI_APEI_PCIEAER
@@ -631,7 +631,7 @@ static void aer_recover_work_func(struct work_struct *work)
 		}
 		cper_print_aer(pdev, entry.severity, entry.regs);
 		if (entry.severity != AER_CORRECTABLE)
-			do_recovery(pdev, entry.severity);
+			pci_do_recovery(pdev, entry.severity);
 		pci_dev_put(pdev);
 	}
 }
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
#2 lore raw

PATCH v9 2/7 PCI/AER: factor out error reporting from AER

In reply to Oza Pawandeep

This patch factors out error reporting callbacks, which are currently tightly coupled with AER.
DPC should be able to register callbacks and attmept recovery when DPC trigger event occurs.

drivers/pci/pci.h +3 -0
index fcd8191..a5a79f0 100644
@@ -342,6 +342,9 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
 
 void pci_enable_acs(struct pci_dev *dev);
 
+/* PCI error reporting and recovery */
+void pci_do_recovery(struct pci_dev *dev, int severity);
+
 #ifdef CONFIG_PCIEASPM
 void pcie_aspm_init_link_state(struct pci_dev *pdev);
 void pcie_aspm_exit_link_state(struct pci_dev *pdev);
drivers/pci/pcie/Makefile +1 -1
index 223e4c3..d669497 100644
@@ -6,7 +6,7 @@
 # Build PCI Express ASPM if needed
 obj-$(CONFIG_PCIEASPM)		+= aspm.o
 
-pcieportdrv-y			:= portdrv_core.o portdrv_pci.o portdrv_bus.o
+pcieportdrv-y			:= portdrv_core.o portdrv_pci.o portdrv_bus.o pcie-err.o
 pcieportdrv-$(CONFIG_ACPI)	+= portdrv_acpi.o
 
 obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o
drivers/pci/pcie/aer/aerdrv.h +0 -30
index 5449e5c..bc9db53 100644
@@ -76,36 +76,6 @@ struct aer_rpc {
 					 */
 };
 
-struct aer_broadcast_data {
-	enum pci_channel_state state;
-	enum pci_ers_result result;
-};
-
-static inline pci_ers_result_t merge_result(enum pci_ers_result orig,
-		enum pci_ers_result new)
-{
-	if (new == PCI_ERS_RESULT_NO_AER_DRIVER)
-		return PCI_ERS_RESULT_NO_AER_DRIVER;
-
-	if (new == PCI_ERS_RESULT_NONE)
-		return orig;
-
-	switch (orig) {
-	case PCI_ERS_RESULT_CAN_RECOVER:
-	case PCI_ERS_RESULT_RECOVERED:
-		orig = new;
-		break;
-	case PCI_ERS_RESULT_DISCONNECT:
-		if (new == PCI_ERS_RESULT_NEED_RESET)
-			orig = PCI_ERS_RESULT_NEED_RESET;
-		break;
-	default:
-		break;
-	}
-
-	return orig;
-}
-
 extern struct bus_type pcie_port_bus_type;
 void aer_isr(struct work_struct *work);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
drivers/pci/pcie/aer/aerdrv_core.c +3 -287

314 lines not on this page · raw message

drivers/pci/pcie/pcie-err.c new file +334 -0

334 lines not on this page · raw message

drivers/pci/pcie/portdrv.h +1 -0
index a854bc5..4f1992d 100644
@@ -79,4 +79,5 @@ static inline void pcie_port_platform_notify(struct pci_dev *port, int *mask)
 static inline void pcie_port_platform_notify(struct pci_dev *port, int *mask){}
 #endif /* !CONFIG_ACPI */
 
+struct pcie_port_service_driver *pci_find_aer_service(struct pci_dev *dev);
 #endif /* _PORTDRV_H_ */
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
#3 lore raw

Re: [PATCH v9 2/7] PCI/AER: factor out error reporting from AER

On 02/21/2018 11:46 PM, Oza Pawandeep wrote:

Hi,
Just minor stuff:

Quoted 44 lines
drivers/pci/pcie/pcie-err.c new file +37 -0
new file mode 100644
index 0000000..a532fe0
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This file implements the error recovery as a core part of PCIe error reporting.
+ * When a PCIe error is delivered, an error message will be collected and printed
+ * to console, then, an error recovery procedure will be executed by following
+ * the PCI error recovery rules.
+ *
+ * Copyright (C) 2006 Intel Corp.
+ *	Tom Long Nguyen ([email protected])
+ *	Zhang Yanmin ([email protected])
+ *
+ */
+
+#include <linux/pci.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/aer.h>
+#include <linux/pcieport_if.h>
+#include "portdrv.h"
 
+static int report_error_detected(struct pci_dev *dev, void *data)
+{
+	pci_ers_result_t vote;
+	const struct pci_error_handlers *err_handler;
+	struct aer_broadcast_data *result_data;
+
+	result_data = (struct aer_broadcast_data *) data;
+
+	device_lock(&dev->dev);
+	dev->error_state = result_data->state;
+
+	if (!dev->driver ||
+		!dev->driver->err_handler ||
+		!dev->driver->err_handler->error_detected) {
+		if (result_data->state == pci_channel_io_frozen &&
+			dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {

drivers/pci/pcie/pcie-err.c line 141

Quoted 10 lines
drivers/pci/pcie/pcie-err.c new file +10 -0
+			/*
+			 * In case of fatal recovery, if one of down-
+			 * stream device has no driver. We might be
+			 * unable to recover because a later insmod
+			 * of a driver for this device is unaware of
+			 * its hw state.
+			 */
+			dev_printk(KERN_DEBUG, &dev->dev, "device has %s\n",
+				   dev->driver ?
+				   "no error-aware driver" : "no driver");

or:

			dev_printk(KERN_DEBUG, &dev->dev, "device has no%s driver\n",
				   dev->driver ? " error-aware" : "");
Quoted 19 lines
drivers/pci/pcie/pcie-err.c new file +19 -0
+		}
+
+		/*
+		 * If there's any device in the subtree that does not
+		 * have an error_detected callback, returning
+		 * PCI_ERS_RESULT_NO_AER_DRIVER prevents calling of
+		 * the subsequent mmio_enabled/slot_reset/resume
+		 * callbacks of "any" device in the subtree. All the
+		 * devices in the subtree are left in the error state
+		 * without recovery.
+		 */
+
+		if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
+			vote = PCI_ERS_RESULT_NO_AER_DRIVER;
+		else
+			vote = PCI_ERS_RESULT_NONE;
+	} else {
+		err_handler = dev->driver->err_handler;
+		vote = err_handler->error_detected(dev, result_data->state);

drivers/pci/pcie/pcie-err.c line 224

Quoted 10 lines
drivers/pci/pcie/pcie-err.c new file +9 -0
+	}
+
+	result_data->result = merge_result(result_data->result, vote);
+	device_unlock(&dev->dev);
+	return 0;
+}
 
+/**
+ * broadcast_error_message - handle message broadcast to downstream drivers
+ * @dev: pointer to from where in a hierarchy message is broadcasted down

I would drop ^^ "from" ... is broadcast downstream

Quoted 3 lines

+ * @state: error state
+ * @error_mesg: message to print
+ * @cb: callback to be broadcasted

to be broadcast

Quoted 3 lines

+ *
+ * Invoked during error recovery process. Once being invoked, the content
+ * of error severity will be broadcasted to all downstream drivers in a

will be broadcast

Quoted 8 lines

+ * hierarchy in question.
+ */

drivers/pci/pcie/pcie-err.c new file +6 -0
+static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
+	enum pci_channel_state state,
+	char *error_mesg,
+	int (*cb)(struct pci_dev *, void *))
+{
+	struct aer_broadcast_data result_data;
Signature · 1 line
~Randy
#4 lore raw

Re: [PATCH v9 2/7] PCI/AER: factor out error reporting from AER

Quoted 116 lines · Randy Dunlap

On 02/21/2018 11:46 PM, Oza Pawandeep wrote:

Hi,
Just minor stuff:

Quoted 47 lines
drivers/pci/pcie/pcie-err.c new file +3 -0
new file mode 100644
index 0000000..a532fe0
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This file implements the error recovery as a core part of PCIe 

error reporting.
+ * When a PCIe error is delivered, an error message will be collected and printed
+ * to console, then, an error recovery procedure will be executed by following
+ * the PCI error recovery rules.
+ *
+ * Copyright (C) 2006 Intel Corp.
+ * Tom Long Nguyen ([email protected])
+ * Zhang Yanmin ([email protected])
+ *
+ */

drivers/pci/pcie/pcie-err.c new file +25 -0
+
+#include <linux/pci.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/aer.h>
+#include <linux/pcieport_if.h>
+#include "portdrv.h"
 
+static int report_error_detected(struct pci_dev *dev, void *data)
+{
+	pci_ers_result_t vote;
+	const struct pci_error_handlers *err_handler;
+	struct aer_broadcast_data *result_data;
+
+	result_data = (struct aer_broadcast_data *) data;
+
+	device_lock(&dev->dev);
+	dev->error_state = result_data->state;
+
+	if (!dev->driver ||
+		!dev->driver->err_handler ||
+		!dev->driver->err_handler->error_detected) {
+		if (result_data->state == pci_channel_io_frozen &&
+			dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {

drivers/pci/pcie/pcie-err.c line 141

Quoted 10 lines
drivers/pci/pcie/pcie-err.c new file +10 -0
+			/*
+			 * In case of fatal recovery, if one of down-
+			 * stream device has no driver. We might be
+			 * unable to recover because a later insmod
+			 * of a driver for this device is unaware of
+			 * its hw state.
+			 */
+			dev_printk(KERN_DEBUG, &dev->dev, "device has %s\n",
+				   dev->driver ?
+				   "no error-aware driver" : "no driver");

or:

			dev_printk(KERN_DEBUG, &dev->dev, "device has no%s driver\n",
				   dev->driver ? " error-aware" : "");
Quoted 28 lines
drivers/pci/pcie/pcie-err.c new file +27 -0
+		}
+
+		/*
+		 * If there's any device in the subtree that does not
+		 * have an error_detected callback, returning
+		 * PCI_ERS_RESULT_NO_AER_DRIVER prevents calling of
+		 * the subsequent mmio_enabled/slot_reset/resume
+		 * callbacks of "any" device in the subtree. All the
+		 * devices in the subtree are left in the error state
+		 * without recovery.
+		 */
+
+		if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
+			vote = PCI_ERS_RESULT_NO_AER_DRIVER;
+		else
+			vote = PCI_ERS_RESULT_NONE;
+	} else {
+		err_handler = dev->driver->err_handler;
+		vote = err_handler->error_detected(dev, result_data->state);
+	}
+
+	result_data->result = merge_result(result_data->result, vote);
+	device_unlock(&dev->dev);
+	return 0;
+}
 
+/**
+ * broadcast_error_message - handle message broadcast to downstream 
Quoted 3 lines

drivers
+ * @dev: pointer to from where in a hierarchy message is broadcasted down

I would drop ^^ "from" ... is broadcast
downstream

Quoted 3 lines

+ * @state: error state
+ * @error_mesg: message to print
+ * @cb: callback to be broadcasted

to be broadcast

Quoted 5 lines

+ *
+ * Invoked during error recovery process. Once being invoked, the content
+ * of error severity will be broadcasted to all downstream drivers in a

will be broadcast

Quoted 8 lines

drivers/pci/pcie/pcie-err.c line 238

Quoted 8 lines

+ * hierarchy in question.
+ */

drivers/pci/pcie/pcie-err.c new file +6 -0
+static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
+	enum pci_channel_state state,
+	char *error_mesg,
+	int (*cb)(struct pci_dev *, void *))
+{
+	struct aer_broadcast_data result_data;

Thanks for the comments, will take care of it.

#5 lore raw

Re: [PATCH v9 2/7] PCI/AER: factor out error reporting from AER

In reply to Randy Dunlap
Quoted 64 lines · Randy Dunlap

On 02/21/2018 11:46 PM, Oza Pawandeep wrote:

Hi,
Just minor stuff:

Quoted 47 lines
drivers/pci/pcie/pcie-err.c new file +3 -0
new file mode 100644
index 0000000..a532fe0
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This file implements the error recovery as a core part of PCIe 

error reporting.
+ * When a PCIe error is delivered, an error message will be collected and printed
+ * to console, then, an error recovery procedure will be executed by following
+ * the PCI error recovery rules.
+ *
+ * Copyright (C) 2006 Intel Corp.
+ * Tom Long Nguyen ([email protected])
+ * Zhang Yanmin ([email protected])
+ *
+ */

drivers/pci/pcie/pcie-err.c new file +25 -0
+
+#include <linux/pci.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/aer.h>
+#include <linux/pcieport_if.h>
+#include "portdrv.h"
 
+static int report_error_detected(struct pci_dev *dev, void *data)
+{
+	pci_ers_result_t vote;
+	const struct pci_error_handlers *err_handler;
+	struct aer_broadcast_data *result_data;
+
+	result_data = (struct aer_broadcast_data *) data;
+
+	device_lock(&dev->dev);
+	dev->error_state = result_data->state;
+
+	if (!dev->driver ||
+		!dev->driver->err_handler ||
+		!dev->driver->err_handler->error_detected) {
+		if (result_data->state == pci_channel_io_frozen &&
+			dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {

drivers/pci/pcie/pcie-err.c line 141

Quoted 10 lines
drivers/pci/pcie/pcie-err.c new file +10 -0
+			/*
+			 * In case of fatal recovery, if one of down-
+			 * stream device has no driver. We might be
+			 * unable to recover because a later insmod
+			 * of a driver for this device is unaware of
+			 * its hw state.
+			 */
+			dev_printk(KERN_DEBUG, &dev->dev, "device has %s\n",
+				   dev->driver ?
+				   "no error-aware driver" : "no driver");

or:

Quoted 3 lines
			dev_printk(KERN_DEBUG, &dev->dev, "device has no%s driver\n",
				   dev->driver ? " error-aware" : "");

I might not be able to take care of this, because all the later patches are dependent on this, and probably I might have to manually merge. though I will give it a try :)

Quoted 57 lines
Quoted 28 lines
drivers/pci/pcie/pcie-err.c new file +27 -0
+		}
+
+		/*
+		 * If there's any device in the subtree that does not
+		 * have an error_detected callback, returning
+		 * PCI_ERS_RESULT_NO_AER_DRIVER prevents calling of
+		 * the subsequent mmio_enabled/slot_reset/resume
+		 * callbacks of "any" device in the subtree. All the
+		 * devices in the subtree are left in the error state
+		 * without recovery.
+		 */
+
+		if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
+			vote = PCI_ERS_RESULT_NO_AER_DRIVER;
+		else
+			vote = PCI_ERS_RESULT_NONE;
+	} else {
+		err_handler = dev->driver->err_handler;
+		vote = err_handler->error_detected(dev, result_data->state);
+	}
+
+	result_data->result = merge_result(result_data->result, vote);
+	device_unlock(&dev->dev);
+	return 0;
+}
 
+/**
+ * broadcast_error_message - handle message broadcast to downstream 
Quoted 3 lines

drivers
+ * @dev: pointer to from where in a hierarchy message is broadcasted down

I would drop ^^ "from" ... is broadcast
downstream

Quoted 3 lines

+ * @state: error state
+ * @error_mesg: message to print
+ * @cb: callback to be broadcasted

to be broadcast

Quoted 5 lines

+ *
+ * Invoked during error recovery process. Once being invoked, the content
+ * of error severity will be broadcasted to all downstream drivers in a

will be broadcast

Quoted 8 lines

+ * hierarchy in question.
+ */

drivers/pci/pcie/pcie-err.c new file +6 -0
+static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
+	enum pci_channel_state state,
+	char *error_mesg,
+	int (*cb)(struct pci_dev *, void *))
+{
+	struct aer_broadcast_data result_data;
#6 lore raw

PATCH v9 3/7 PCI/ERR: add mutex to synchronize recovery

In reply to Oza Pawandeep

This patch protects pci_do_recovery with mutex.

drivers/pci/pcie/pcie-err.c +6 -0
index a532fe0..8318c84 100644
@@ -20,6 +20,8 @@
 #include <linux/pcieport_if.h>
 #include "portdrv.h"
 
+static DEFINE_MUTEX(pci_err_recovery_lock);
+
 struct aer_broadcast_data {
 	enum pci_channel_state state;
 	enum pci_ers_result result;
@@ -283,6 +285,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
 	enum pci_channel_state state;
 
+	mutex_lock(&pci_err_recovery_lock);
+
 	if (severity == AER_FATAL)
 		state = pci_channel_io_frozen;
 	else
@@ -326,9 +330,11 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 				report_resume);
 
 	dev_info(&dev->dev, "Device recovery successful\n");
+	mutex_unlock(&pci_err_recovery_lock);
 	return;
 
 failed:
 	/* TODO: Should kernel panic here? */
 	dev_info(&dev->dev, "Device recovery failed\n");
+	mutex_unlock(&pci_err_recovery_lock);
 }
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
#7 lore raw

PATCH v9 4/7 PCI/DPC: Unify and plumb error handling into DPC

In reply to Oza Pawandeep

Current DPC driver does not do recovery, e.g. calling end-point's driver's callbacks, which sanitize the sw.

DPC driver implements link_reset callback, and calls pci_do_recovery.

drivers/pci/pci.h +2 -0
index a5a79f0..124f42e 100644
@@ -343,6 +343,8 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
 void pci_enable_acs(struct pci_dev *dev);
 
 /* PCI error reporting and recovery */
+#define DPC_FATAL	4
+
 void pci_do_recovery(struct pci_dev *dev, int severity);
 
 #ifdef CONFIG_PCIEASPM
drivers/pci/pcie/pcie-dpc.c +86 -7

137 lines not on this page · raw message

drivers/pci/pcie/pcie-err.c +21 -6
index 8318c84..6844347 100644
@@ -19,6 +19,7 @@
 #include <linux/aer.h>
 #include <linux/pcieport_if.h>
 #include "portdrv.h"
+#include "./../pci.h"
 
 static DEFINE_MUTEX(pci_err_recovery_lock);
 
@@ -181,7 +182,7 @@ static pci_ers_result_t default_reset_link(struct pci_dev *dev)
 	return PCI_ERS_RESULT_RECOVERED;
 }
 
-static pci_ers_result_t reset_link(struct pci_dev *dev)
+static pci_ers_result_t reset_link(struct pci_dev *dev, int severity)
 {
 	struct pci_dev *udev;
 	pci_ers_result_t status;
@@ -195,9 +196,17 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
 		udev = dev->bus->self;
 	}
 
+
+	/* Use the service driver of the component firstly */
+#if IS_ENABLED(CONFIG_PCIE_DPC)
+	if (severity == DPC_FATAL)
+		driver = pci_find_dpc_service(udev);
+#endif
 #if IS_ENABLED(CONFIG_PCIEAER)
-	/* Use the aer driver of the component firstly */
-	driver = pci_find_aer_service(udev);
+	if ((severity == AER_FATAL) ||
+	    (severity == AER_NONFATAL) ||
+	    (severity == AER_CORRECTABLE))
+		driver = pci_find_aer_service(udev);
 #endif
 
 	if (driver && driver->reset_link) {
@@ -287,7 +296,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 
 	mutex_lock(&pci_err_recovery_lock);
 
-	if (severity == AER_FATAL)
+	if ((severity == AER_FATAL) ||
+	    (severity == DPC_FATAL))
 		state = pci_channel_io_frozen;
 	else
 		state = pci_channel_io_normal;
@@ -297,10 +307,14 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 			"error_detected",
 			report_error_detected);
 
-	if (severity == AER_FATAL) {
-		result = reset_link(dev);
+	if ((severity == AER_FATAL) ||
+	    (severity == DPC_FATAL)) {
+		result = reset_link(dev, severity);
 		if (result != PCI_ERS_RESULT_RECOVERED)
 			goto failed;
+		else if (severity == DPC_FATAL)
+			goto resume;
+
 	}
 
 	if (status == PCI_ERS_RESULT_CAN_RECOVER)
@@ -324,6 +338,7 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 	if (status != PCI_ERS_RESULT_RECOVERED)
 		goto failed;
 
+resume:
 	broadcast_error_message(dev,
 				state,
 				"resume",
drivers/pci/pcie/portdrv.h +1 -0
index 4f1992d..b013e24 100644
@@ -80,4 +80,5 @@ static inline void pcie_port_platform_notify(struct pci_dev *port, int *mask){}
 #endif /* !CONFIG_ACPI */
 
 struct pcie_port_service_driver *pci_find_aer_service(struct pci_dev *dev);
+struct pcie_port_service_driver *pci_find_dpc_service(struct pci_dev *dev);
 #endif /* _PORTDRV_H_ */
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
#8 lore raw

Re: [PATCH v9 4/7] PCI/DPC: Unify and plumb error handling into DPC

Quoted 4 lines · Oza Pawandeep

Current DPC driver does not do recovery, e.g. calling end-point's driver's callbacks, which sanitize the sw.

DPC driver implements link_reset callback, and calls pci_do_recovery.

Hi,
What does DPC mean?

Quoted 9 lines
drivers/pci/pcie/pcie-dpc.c +0 -0
index 38e40c6..208b427 100644

drivers/pci/pcie/pcie-dpc.c static void dpc_wait_link_inactive(struct dpc_dev *dpc) · line 141

Quoted 5 lines
drivers/pci/pcie/pcie-dpc.c +2 -0
@@ -82,12 +137,25 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
 		dev_warn(dev, "Link state not disabled for DPC event\n");
 }
 
+/**
+ * dpc_reset_link - reset link DPC  routine

+ * @dev: pointer to Root Port's pci_dev data structure

* @pdev: ...

drivers/pci/pcie/pcie-dpc.c static void dpc_wait_link_inactive(struct dpc_dev *dpc) · line 147

Quoted 5 lines
drivers/pci/pcie/pcie-dpc.c +4 -0
+ *
+ * Invoked by Port Bus driver when performing link reset at Root Port.
+ */
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 {

You can use 'make W=1 ...' to find kernel-doc warnings like this.

thanks,

Signature · 1 line
~Randy
#9 lore raw

Re: [PATCH v9 4/7] PCI/DPC: Unify and plumb error handling into DPC

Quoted 9 lines · Randy Dunlap
Quoted 4 lines · Oza Pawandeep

Current DPC driver does not do recovery, e.g. calling end-point's driver's callbacks, which sanitize the sw.

DPC driver implements link_reset callback, and calls pci_do_recovery.

Hi,
What does DPC mean?

DPC (Downstream port containment) is PCIe RP or Switch feature, which if detects FATAL error messages halts the outgoing traffic, and PCIe link reset and quiescence activities are taken care by HW, and recovery is made. followed by that; normal operation can be resumed.

Quoted 19 lines
Quoted 9 lines
drivers/pci/pcie/pcie-dpc.c +0 -0
index 38e40c6..208b427 100644

drivers/pci/pcie/pcie-dpc.c static void dpc_wait_link_inactive(struct dpc_dev · line 142

Quoted 6 lines
drivers/pci/pcie/pcie-dpc.c +2 -0
@@ -82,12 +137,25 @@ static void dpc_wait_link_inactive(struct dpc_dev 
 *dpc)
 		dev_warn(dev, "Link state not disabled for DPC event\n");
 }
 
+/**
+ * dpc_reset_link - reset link DPC  routine

+ * @dev: pointer to Root Port's pci_dev data structure

* @pdev: ...

Quoted 9 lines
Quoted 6 lines
drivers/pci/pcie/pcie-dpc.c +2 -0
+ *
+ * Invoked by Port Bus driver when performing link reset at Root 

Port.
+ */
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
{

You can use 'make W=1 ...' to find kernel-doc warnings like this.

sure thanks for this tip.

ps: I am going to post v11 to address some comments.

Quoted 1 line

thanks,

#10 lore raw

PATCH v9 5/7 PCI/AER: Unify aer error defines at single space

In reply to Oza Pawandeep

This patch moves AER error defines to drivers/pci/pci.h.
So that it unifies the error repoting codes at single place along with dpc

drivers/acpi/apei/ghes.c +1 -0
index 1efefe9..7ae9bb3 100644
@@ -56,6 +56,7 @@
 #include <ras/ras_event.h>
 
 #include "apei-internal.h"
+#include "../../pci/pci.h"
 
 #define GHES_PFX	"GHES: "
 
drivers/pci/pci.h +5 -1
index 124f42e..b0e63b5 100644
@@ -343,7 +343,11 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
 void pci_enable_acs(struct pci_dev *dev);
 
 /* PCI error reporting and recovery */
-#define DPC_FATAL	4
+#define AER_NONFATAL		0
+#define AER_FATAL		1
+#define AER_CORRECTABLE		2
+
+#define DPC_FATAL		4
 
 void pci_do_recovery(struct pci_dev *dev, int severity);
 
drivers/pci/pcie/aer/aerdrv_errprint.c +1 -0
index 6a352e6..4c59f37 100644
@@ -19,6 +19,7 @@
 #include <linux/cper.h>
 
 #include "aerdrv.h"
+#include "../../pci.h"
 #include <ras/ras_event.h>
 
 #define AER_AGENT_RECEIVER		0
include/linux/aer.h +0 -4
index 8f87bbe..3eac8ed 100644
@@ -11,10 +11,6 @@
 #include <linux/errno.h>
 #include <linux/types.h>
 
-#define AER_NONFATAL			0
-#define AER_FATAL			1
-#define AER_CORRECTABLE			2
-
 struct pci_dev;
 
 struct aer_header_log_regs {
include/ras/ras_event.h +1 -0
index 9c68986..d75c75b 100644
@@ -13,6 +13,7 @@
 #include <linux/aer.h>
 #include <linux/cper.h>
 #include <linux/mm.h>
+#include "../../../drivers/pci/pci.h"
 
 /*
  * MCE Extended Error Log trace event
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
#11 lore raw

PATCH v9 6/7 PCI: Unify wait for link active into generic pci

In reply to Oza Pawandeep

Clients such as pciehp, dpc are using pcie_wait_link_active, which waits till the link becomes active or inactive.

Made generic function and moved it to drivers/pci/pci.c

drivers/pci/hotplug/pciehp_hpc.c +4 -17
index 18a42f8..a133b8b 100644
@@ -231,25 +231,12 @@ bool pciehp_check_link_active(struct controller *ctrl)
 	return ret;
 }
 
-static void __pcie_wait_link_active(struct controller *ctrl, bool active)
+static bool pcie_wait_link_active(struct controller *ctrl)
 {
-	int timeout = 1000;
-
-	if (pciehp_check_link_active(ctrl) == active)
-		return;
-	while (timeout > 0) {
-		msleep(10);
-		timeout -= 10;
-		if (pciehp_check_link_active(ctrl) == active)
-			return;
-	}
-	ctrl_dbg(ctrl, "Data Link Layer Link Active not %s in 1000 msec\n",
-			active ? "set" : "cleared");
-}
+	struct pci_dev *pdev = ctrl_dev(ctrl);
+	bool active = true;
 
-static void pcie_wait_link_active(struct controller *ctrl)
-{
-	__pcie_wait_link_active(ctrl, true);
+	return pci_wait_for_link(pdev, active);
 }
 
 static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
drivers/pci/pci.c +34 -0
index f6a4dd1..5440696 100644
@@ -4176,6 +4176,40 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
 	return 0;
 }
 
+/**
+ * pci__wait_for_link - Wait for link till its active/inactive
+ * @pdev: Bridge device
+ * @active: waiting for active or inactive ?
+ *
+ * Use this to wait till link becomes active or inactive.
+ */
+bool pci_wait_for_link(struct pci_dev *pdev, bool active)
+{
+	int timeout = 1000;
+	bool ret;
+	u16 lnk_status;
+
+	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
+	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
+
+	while ((ret != active) && (timeout > 0)) {
+		msleep(10);
+		timeout -= 10;
+		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
+		ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
+	}
+
+	if (ret == active)
+		return true;
+
+	dev_printk(KERN_DEBUG, &pdev->dev,
+		   "Data Link Layer Link Active not %s in 1000 msec\n",
+		   active ? "set" : "cleared");
+
+	return false;
+}
+EXPORT_SYMBOL(pci_wait_for_link);
+
 void pci_reset_secondary_bus(struct pci_dev *dev)
 {
 	u16 ctrl;
drivers/pci/pcie/pcie-dpc.c +2 -11
index 208b427..fce4518 100644
@@ -122,19 +122,10 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 
 static void dpc_wait_link_inactive(struct dpc_dev *dpc)
 {
-	unsigned long timeout = jiffies + HZ;
 	struct pci_dev *pdev = dpc->dev->port;
-	struct device *dev = &dpc->dev->device;
-	u16 lnk_status;
+	bool active = false;
 
-	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
-	while (lnk_status & PCI_EXP_LNKSTA_DLLLA &&
-					!time_after(jiffies, timeout)) {
-		msleep(10);
-		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
-	}
-	if (lnk_status & PCI_EXP_LNKSTA_DLLLA)
-		dev_warn(dev, "Link state not disabled for DPC event\n");
+	pci_wait_for_link(pdev, active);
 }
 
 /**
include/linux/pci.h +1 -0
index 024a1be..cb674c3 100644
@@ -1195,6 +1195,7 @@ int pci_add_ext_cap_save_buffer(struct pci_dev *dev,
 int pci_request_selected_regions(struct pci_dev *, int, const char *);
 int pci_request_selected_regions_exclusive(struct pci_dev *, int, const char *);
 void pci_release_selected_regions(struct pci_dev *, int);
+bool pci_wait_for_link(struct pci_dev *pdev, bool active);
 
 /* drivers/pci/bus.c */
 struct pci_bus *pci_bus_get(struct pci_bus *bus);
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
#12 lore raw

PATCH v9 7/7 PCI/DPC: Enumerate the devices after DPC trigger event

In reply to Oza Pawandeep

Implement error_resume callback in DPC so, after DPC trigger event enumerates the devices beneath.

drivers/pci/pcie/pcie-dpc.c +18 -0
index fce4518..59c01c7 100644
@@ -129,6 +129,23 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
 }
 
 /**
+ * dpc_error_resume - enumerate the devices beneath
+ * @dev: pointer to Root Port's pci_dev data structure
+ *
+ * Invoked by Port Bus driver during nonfatal recovery.
+ */
+static void dpc_error_resume(struct pci_dev *pdev)
+{
+	bool active = true;
+
+	if (pci_wait_for_link(pdev, active)) {
+		pci_lock_rescan_remove();
+		pci_rescan_bus(pdev->bus);
+		pci_unlock_rescan_remove();
+	}
+}
+
+/**
  * dpc_reset_link - reset link DPC  routine
  * @dev: pointer to Root Port's pci_dev data structure
  *
@@ -366,6 +383,7 @@ static void dpc_remove(struct pcie_device *dev)
 	.service	= PCIE_PORT_SERVICE_DPC,
 	.probe		= dpc_probe,
 	.remove		= dpc_remove,
+	.error_resume	= dpc_error_resume,
 	.reset_link     = dpc_reset_link,
 };
 
drivers/pci/pcie/pcie-err.c +20 -5
index 6844347..4950f49 100644
@@ -236,6 +236,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, int severity)
  * @state: error state
  * @error_mesg: message to print
  * @cb: callback to be broadcasted
+ * @severity: error severity
  *
  * Invoked during error recovery process. Once being invoked, the content
  * of error severity will be broadcasted to all downstream drivers in a
@@ -244,7 +245,8 @@ static pci_ers_result_t reset_link(struct pci_dev *dev, int severity)
 static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
 	enum pci_channel_state state,
 	char *error_mesg,
-	int (*cb)(struct pci_dev *, void *))
+	int (*cb)(struct pci_dev *, void *),
+	int severity)
 {
 	struct aer_broadcast_data result_data;
 
@@ -256,6 +258,15 @@ static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
 		result_data.result = PCI_ERS_RESULT_RECOVERED;
 
 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+		/* If DPC is triggered, call resume error hanlder
+		 * because, at this point we can safely assume that
+		 * link recovery has happened.
+		 */
+		if ((severity == DPC_FATAL) &&
+			(cb == report_resume)) {
+			cb(dev, NULL);
+			return PCI_ERS_RESULT_RECOVERED;
+		}
 		/*
 		 * If the error is reported by a bridge, we think this error
 		 * is related to the downstream link of the bridge, so we
@@ -305,7 +316,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 	status = broadcast_error_message(dev,
 			state,
 			"error_detected",
-			report_error_detected);
+			report_error_detected,
+			severity);
 
 	if ((severity == AER_FATAL) ||
 	    (severity == DPC_FATAL)) {
@@ -321,7 +333,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 		status = broadcast_error_message(dev,
 				state,
 				"mmio_enabled",
-				report_mmio_enabled);
+				report_mmio_enabled,
+				severity);
 
 	if (status == PCI_ERS_RESULT_NEED_RESET) {
 		/*
@@ -332,7 +345,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 		status = broadcast_error_message(dev,
 				state,
 				"slot_reset",
-				report_slot_reset);
+				report_slot_reset,
+				severity);
 	}
 
 	if (status != PCI_ERS_RESULT_RECOVERED)
@@ -342,7 +356,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 	broadcast_error_message(dev,
 				state,
 				"resume",
-				report_resume);
+				report_resume,
+				severity);
 
 	dev_info(&dev->dev, "Device recovery successful\n");
 	mutex_unlock(&pci_err_recovery_lock);
Signature · 2 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.,
a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
#13 lore raw

Re: [PATCH v9 7/7] PCI/DPC: Enumerate the devices after DPC trigger event

Quoted 10 lines · Oza Pawandeep

Implement error_resume callback in DPC so, after DPC trigger event enumerates the devices beneath.

drivers/pci/pcie/pcie-dpc.c +0 -0
index fce4518..59c01c7 100644

drivers/pci/pcie/pcie-dpc.c static void dpc_wait_link_inactive(struct dpc_dev *dpc) · line 133

Quoted 5 lines
drivers/pci/pcie/pcie-dpc.c +2 -0
@@ -129,6 +129,23 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
 }
 
 /**
+ * dpc_error_resume - enumerate the devices beneath
+ * @dev: pointer to Root Port's pci_dev data structure

* @pdev: ...

Quoted 23 lines
drivers/pci/pcie/pcie-dpc.c +15 -0
+ *
+ * Invoked by Port Bus driver during nonfatal recovery.
+ */
+static void dpc_error_resume(struct pci_dev *pdev)
+{
+	bool active = true;
+
+	if (pci_wait_for_link(pdev, active)) {
+		pci_lock_rescan_remove();
+		pci_rescan_bus(pdev->bus);
+		pci_unlock_rescan_remove();
+	}
+}
+
+/**
  * dpc_reset_link - reset link DPC  routine
  * @dev: pointer to Root Port's pci_dev data structure
  *
drivers/pci/pcie/pcie-err.c +0 -0
index 6844347..4950f49 100644

drivers/pci/pcie/pcie-err.c static pci_ers_result_t broadcast_error_message(struct pci_dev *dev, · line 261

Quoted 4 lines
drivers/pci/pcie/pcie-err.c +1 -0
@@ -256,6 +258,15 @@ static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
 		result_data.result = PCI_ERS_RESULT_RECOVERED;
 
 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+		/* If DPC is triggered, call resume error hanlder

handler

Kernel multi-line comment style is:

		/*
		 * Begin comment here.
		 * foo bar blah
		 */

drivers/pci/pcie/pcie-err.c static pci_ers_result_t broadcast_error_message(struct pci_dev *dev, · line 272

Quoted 11 lines
drivers/pci/pcie/pcie-err.c +8 -0
+		 * because, at this point we can safely assume that
+		 * link recovery has happened.
+		 */
+		if ((severity == DPC_FATAL) &&
+			(cb == report_resume)) {
+			cb(dev, NULL);
+			return PCI_ERS_RESULT_RECOVERED;
+		}
 		/*
 		 * If the error is reported by a bridge, we think this error
 		 * is related to the downstream link of the bridge, so we

thanks,

Signature · 1 line
~Randy
#14 lore raw

Re: [PATCH v9 7/7] PCI/DPC: Enumerate the devices after DPC trigger event

Quoted 57 lines · Randy Dunlap
Quoted 10 lines · Oza Pawandeep

Implement error_resume callback in DPC so, after DPC trigger event enumerates the devices beneath.

drivers/pci/pcie/pcie-dpc.c +0 -0
index fce4518..59c01c7 100644

drivers/pci/pcie/pcie-dpc.c static void dpc_wait_link_inactive(struct dpc_dev · line 134

Quoted 6 lines
drivers/pci/pcie/pcie-dpc.c +2 -0
@@ -129,6 +129,23 @@ static void dpc_wait_link_inactive(struct dpc_dev 
 *dpc)
 }
 
 /**
+ * dpc_error_resume - enumerate the devices beneath
+ * @dev: pointer to Root Port's pci_dev data structure

* @pdev: ...

Quoted 23 lines
drivers/pci/pcie/pcie-dpc.c +15 -0
+ *
+ * Invoked by Port Bus driver during nonfatal recovery.
+ */
+static void dpc_error_resume(struct pci_dev *pdev)
+{
+	bool active = true;
+
+	if (pci_wait_for_link(pdev, active)) {
+		pci_lock_rescan_remove();
+		pci_rescan_bus(pdev->bus);
+		pci_unlock_rescan_remove();
+	}
+}
+
+/**
  * dpc_reset_link - reset link DPC  routine
  * @dev: pointer to Root Port's pci_dev data structure

*

drivers/pci/pcie/pcie-err.c +0 -0
index 6844347..4950f49 100644

drivers/pci/pcie/pcie-err.c static pci_ers_result_t · line 262

Quoted 5 lines
drivers/pci/pcie/pcie-err.c +1 -0
@@ -256,6 +258,15 @@ static pci_ers_result_t 
 broadcast_error_message(struct pci_dev *dev,
 		result_data.result = PCI_ERS_RESULT_RECOVERED;
 
 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+		/* If DPC is triggered, call resume error hanlder

handler

Kernel multi-line comment style is:

		/*
		 * Begin comment here.
		 * foo bar blah
		 */
Quoted 13 lines
Quoted 11 lines
drivers/pci/pcie/pcie-err.c +8 -0
+		 * because, at this point we can safely assume that
+		 * link recovery has happened.
+		 */
+		if ((severity == DPC_FATAL) &&
+			(cb == report_resume)) {
+			cb(dev, NULL);
+			return PCI_ERS_RESULT_RECOVERED;
+		}
 		/*
 		 * If the error is reported by a bridge, we think this error

* is related to the downstream link of the bridge, so we

thanks,

Thanks for the comments,
All your comments are taken care in v10, though I will post v11.