Skip to contentSkip to conversation

Address error and recovery for AER and DPC

PATCH Superseded Oza Pawandeep 3 participants 7 messages Jan 8, 2018 – Jan 14, 2018

Subsystems (4)
ACPI PCI ACPI APEI RAS infrastructure

v3 of 17 Superseded Latest is v17 →

lore
Share & tools
lore

Oza Pawandeep

author
#0 lore raw

v3 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 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 (4):
PCI/AER: factor out error reporting from AER PCI/DPC/AER: Address Concurrency between AER and DPC PCI/ERR: Do not do recovery if DPC service is active PCI/DPC: Enumerate the devices after DPC trigger event

 drivers/acpi/apei/ghes.c               |   2 +-
 drivers/pci/pcie/Makefile              |   2 +-
 drivers/pci/pcie/aer/aerdrv.h          |  30 ---
 drivers/pci/pcie/aer/aerdrv_core.c     | 306 +------------------------
 drivers/pci/pcie/aer/aerdrv_errprint.c |  27 ++-
 drivers/pci/pcie/pcie-dpc.c            | 127 ++++++++++-
 drivers/pci/pcie/pcie-err.c            | 399 +++++++++++++++++++++++++++++++++
 drivers/pci/pcie/portdrv.h             |   2 +
 include/linux/aer.h                    |   4 -
 include/linux/pci.h                    |  23 ++
 include/ras/ras_event.h                |   6 +-
 11 files changed, 579 insertions(+), 349 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.
author
#1 lore raw

PATCH v3 1/3 PCI/AER: factor out error reporting from AER

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

Signed-off-by Oza Pawandeep <[email protected]>
drivers/acpi/apei/ghes.c +1 -1
index 6402f7f..fd053e5 100644
@@ -462,7 +462,7 @@ static void ghes_do_proc(struct ghes *ghes,
 				 * use, so treat it as a fatal AER error.
 				 */
 				if (gdata->flags & CPER_SEC_RESET)
-					aer_severity = AER_FATAL;
+					aer_severity = PCI_ERR_AER_FATAL;
 
 				aer_recover_queue(pcie_err->device_id.segment,
 						  pcie_err->device_id.bus,
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 +11 -295

373 lines not on this page · raw message

drivers/pci/pcie/aer/aerdrv_errprint.c +16 -11
index 54c4b69..222c56c 100644
@@ -29,11 +29,14 @@
 #define AER_AGENT_COMPLETER		2
 #define AER_AGENT_TRANSMITTER		3
 
-#define AER_AGENT_REQUESTER_MASK(t)	((t == AER_CORRECTABLE) ?	\
+#define AER_AGENT_REQUESTER_MASK(t)		\
+	((t == PCI_ERR_AER_CORRECTABLE) ?	\
 	0 : (PCI_ERR_UNC_COMP_TIME|PCI_ERR_UNC_UNSUP))
-#define AER_AGENT_COMPLETER_MASK(t)	((t == AER_CORRECTABLE) ?	\
+#define AER_AGENT_COMPLETER_MASK(t)		\
+	((t == PCI_ERR_AER_CORRECTABLE) ?	\
 	0 : PCI_ERR_UNC_COMP_ABORT)
-#define AER_AGENT_TRANSMITTER_MASK(t)	((t == AER_CORRECTABLE) ?	\
+#define AER_AGENT_TRANSMITTER_MASK(t)		\
+	((t == PCI_ERR_AER_CORRECTABLE) ?	\
 	(PCI_ERR_COR_REP_ROLL|PCI_ERR_COR_REP_TIMER) : 0)
 
 #define AER_GET_AGENT(t, e)						\

@@ -46,9 +49,11 @@
 #define AER_DATA_LINK_LAYER_ERROR	1
 #define AER_TRANSACTION_LAYER_ERROR	2
 
-#define AER_PHYSICAL_LAYER_ERROR_MASK(t) ((t == AER_CORRECTABLE) ?	\
+#define AER_PHYSICAL_LAYER_ERROR_MASK(t)	\
+	((t == PCI_ERR_AER_CORRECTABLE) ?	\
 	PCI_ERR_COR_RCVR : 0)
-#define AER_DATA_LINK_LAYER_ERROR_MASK(t) ((t == AER_CORRECTABLE) ?	\
+#define AER_DATA_LINK_LAYER_ERROR_MASK(t)	\
+	((t == PCI_ERR_AER_CORRECTABLE) ?	\
 	(PCI_ERR_COR_BAD_TLP|						\
 	PCI_ERR_COR_BAD_DLLP|						\
 	PCI_ERR_COR_REP_ROLL|						\

@@ -147,7 +152,7 @@ static void __aer_print_error(struct pci_dev *dev,
 		if (!(status & (1 << i)))
 			continue;
 
-		if (info->severity == AER_CORRECTABLE)
+		if (info->severity == PCI_ERR_AER_CORRECTABLE)
 			errmsg = i < ARRAY_SIZE(aer_correctable_error_string) ?
 				aer_correctable_error_string[i] : NULL;
 		else

@@ -210,11 +215,11 @@ int cper_severity_to_aer(int cper_severity)
 {
 	switch (cper_severity) {
 	case CPER_SEV_RECOVERABLE:
-		return AER_NONFATAL;
+		return PCI_ERR_AER_NONFATAL;
 	case CPER_SEV_FATAL:
-		return AER_FATAL;
+		return PCI_ERR_AER_FATAL;
 	default:
-		return AER_CORRECTABLE;
+		return PCI_ERR_AER_CORRECTABLE;
 	}
 }
 EXPORT_SYMBOL_GPL(cper_severity_to_aer);

@@ -226,7 +231,7 @@ void cper_print_aer(struct pci_dev *dev, int aer_severity,
 	u32 status, mask;
 	const char **status_strs;
 
-	if (aer_severity == AER_CORRECTABLE) {
+	if (aer_severity == PCI_ERR_AER_CORRECTABLE) {
 		status = aer->cor_status;
 		mask = aer->cor_mask;
 		status_strs = aer_correctable_error_string;

@@ -247,7 +252,7 @@ void cper_print_aer(struct pci_dev *dev, int aer_severity,
 	dev_err(&dev->dev, "aer_layer=%s, aer_agent=%s\n",
 		aer_error_layer[layer], aer_agent_string[agent]);
 
-	if (aer_severity != AER_CORRECTABLE)
+	if (aer_severity != PCI_ERR_AER_CORRECTABLE)
 		dev_err(&dev->dev, "aer_uncor_severity: 0x%08x\n",
 			aer->uncor_severity);
drivers/pci/pcie/pcie-err.c new file +335 -0

335 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_ */
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/linux/pci.h +21 -0
index c170c92..083408e 100644
@@ -739,6 +739,10 @@ struct pci_error_handlers {
 	void (*resume)(struct pci_dev *dev);
 };
 
+struct pci_err_broadcast_data {
+	enum pci_channel_state state;
+	enum pci_ers_result result;
+};
 
 struct module;
 struct pci_driver {

@@ -1998,6 +2002,23 @@ static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res
 void pci_hp_remove_module_link(struct pci_slot *pci_slot);
 #endif
 
+#define PCI_ERR_AER_NONFATAL		0
+#define PCI_ERR_AER_FATAL		1
+#define PCI_ERR_AER_CORRECTABLE		2
+
+pci_ers_result_t pci_broadcast_error_message(struct pci_dev *dev,
+					enum pci_channel_state state,
+					char *error_mesg,
+					int (*cb)(struct pci_dev *, void *));
+int pci_report_mmio_enabled(struct pci_dev *dev, void *data);
+int pci_report_slot_reset(struct pci_dev *dev, void *data);
+int pci_report_resume(struct pci_dev *dev, void *data);
+int pci_report_error_detected(struct pci_dev *dev, void *data);
+pci_ers_result_t pci_reset_link(struct pci_dev *dev);
+pci_ers_result_t pci_merge_result(enum pci_ers_result orig,
+				enum pci_ers_result new);
+void pci_do_recovery(struct pci_dev *dev, int severity);
+
 /**
  * pci_pcie_cap - get the saved PCIe capability offset
  * @dev: PCI device
include/ras/ras_event.h +3 -3
index 9c68986..6176e90 100644
@@ -316,10 +316,10 @@
 
 	TP_printk("%s PCIe Bus Error: severity=%s, %s\n",
 		__get_str(dev_name),
-		__entry->severity == AER_CORRECTABLE ? "Corrected" :
-			__entry->severity == AER_FATAL ?
+		__entry->severity == PCI_ERR_AER_CORRECTABLE ? "Corrected" :
+			__entry->severity == PCI_ERR_AER_FATAL ?
 			"Fatal" : "Uncorrected, non-fatal",
-		__entry->severity == AER_CORRECTABLE ?
+		__entry->severity == PCI_ERR_AER_CORRECTABLE ?
 		__print_flags(__entry->status, "|", aer_correctable_errors) :
 		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
 );
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

Re: [PATCH v3 1/3] PCI/AER: factor out error reporting from AER

Oza,

Quoted 5 lines · Oza Pawandeep

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

Signed-off-by Oza Pawandeep <[email protected]>

<snip>

Quoted 5 lines
drivers/pci/pcie/pcie-err.c new file +2 -0
@@ -0,0 +1,335 @@
+/*
+ * Copyright (c) 2017, The Linux Foundation. All rights reserved.

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

Quoted 10 lines
drivers/pci/pcie/pcie-err.c new file +10 -0
+
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */

Do you mind using an SPDX tag there instead of this fine legalese?

Signature · 2 lines
Cordially
Philippe Ombredanne
#3 lore raw

Re: [PATCH v3 1/3] PCI/AER: factor out error reporting from AER

In reply to Oza Pawandeep
Quoted 11 lines · Oza Pawandeep

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

Signed-off-by Oza Pawandeep <[email protected]>
drivers/acpi/apei/ghes.c +0 -0
index 6402f7f..fd053e5 100644

drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 465

Quoted 5 lines
drivers/acpi/apei/ghes.c +1 -1
@@ -462,7 +462,7 @@ static void ghes_do_proc(struct ghes *ghes,
 				 * use, so treat it as a fatal AER error.
 				 */
 				if (gdata->flags & CPER_SEC_RESET)
-					aer_severity = AER_FATAL;
+					aer_severity = PCI_ERR_AER_FATAL;

Please split the s/AER_FATAL/PCI_ERR_AER_FATAL/ changes into a separate patch to reduce the size of this patch.

I would name them PCI_ERR_FATAL and PCI_ERR_NONFATAL because that matches the usage in the spec, e.g., PCIe r4.0, sec 6.2.2, and the symbols like PCI_ERR_UNC_UND in pci_regs.h.

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

Quoted 8 lines
drivers/pci/pcie/pcie-err.c new file +2 -0
new file mode 100644
index 0000000..a76a8bf
@@ -0,0 +1,335 @@
+/*
+ * Copyright (c) 2017, The Linux Foundation. All rights reserved.

Somebody already mentioned using the SPDX thing, which I think you should do.

But I'm confused about this copyright line. As far as I can tell, this basically moves code from aerdrv_core.c to pcie-err.c, which is not enough to change the copyright ownership. But it drops the copyright lines from aerdrv.core.c and replaces them with "(c) 2017, The Linux Foundation". ?? Where did that come from?

If you *add* something non-trivial, I think it's OK to add your own new copyright info (though I don't think this is really necessary), but I don't think we should *remove* information about other copyright owners.

Quoted 13 lines
drivers/pci/pcie/pcie-err.c new file +13 -0
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/pci.h>
+#include <linux/module.h>
+#include <linux/pci.h>

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

Quoted 10 lines
drivers/pci/pcie/pcie-err.c new file +10 -0
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/aer.h>
+#include <linux/pcieport_if.h>
+#include "portdrv.h"
+
+static DEFINE_MUTEX(pci_err_recovery_lock);
+
+pci_ers_result_t pci_merge_result(enum pci_ers_result orig,
+				  enum pci_ers_result new)

Please do all the renames, e.g., s/merge_result/pci_merge_result/, in a separate patch, followed by one that only moves code between files.

These initial ones will seem trivial, and they are, which is perfect. That makes them easy to review, and it will make the "interesting" patches much smaller and also easier to review.

The you can follow up with more patches that do things like add the mutex. It's too hard to review (or even notice) things like that when everything is squashed together.

Quoted 20 lines
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/linux/pci.h +0 -0
index c170c92..083408e 100644

include/linux/pci.h struct pci_error_handlers { · line 745

Quoted 7 lines
include/linux/pci.h +4 -0
@@ -739,6 +739,10 @@ struct pci_error_handlers {
 	void (*resume)(struct pci_dev *dev);
 };
 
+struct pci_err_broadcast_data {
+	enum pci_channel_state state;
+	enum pci_ers_result result;
+};

Only used in pcie-err.c; should be declared there.

include/linux/pci.h static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res · line 2007

Quoted 9 lines
include/linux/pci.h +3 -0
 struct module;
 struct pci_driver {

@@ -1998,6 +2002,23 @@ static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res
 void pci_hp_remove_module_link(struct pci_slot *pci_slot);
 #endif
 
+#define PCI_ERR_AER_NONFATAL		0
+#define PCI_ERR_AER_FATAL		1
+#define PCI_ERR_AER_CORRECTABLE		2

Why do these need to be moved to include/linux/pci.h? I don't really want them in include/linux at all. The only uses outside drivers/pci are in ras_event.h and acpi/apei/ghes.c. I'd rather keep them in aer.h with the hope of being able to move them into drivers/pci/pci.h eventually.

include/linux/pci.h static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res · line 2019

Quoted 11 lines
include/linux/pci.h +11 -0
+pci_ers_result_t pci_broadcast_error_message(struct pci_dev *dev,
+					enum pci_channel_state state,
+					char *error_mesg,
+					int (*cb)(struct pci_dev *, void *));
+int pci_report_mmio_enabled(struct pci_dev *dev, void *data);
+int pci_report_slot_reset(struct pci_dev *dev, void *data);
+int pci_report_resume(struct pci_dev *dev, void *data);
+int pci_report_error_detected(struct pci_dev *dev, void *data);
+pci_ers_result_t pci_reset_link(struct pci_dev *dev);
+pci_ers_result_t pci_merge_result(enum pci_ers_result orig,
+				enum pci_ers_result new);

The above are only used in pcie-err.c and should be static and not declared here.

Quoted 27 lines
include/linux/pci.h +2 -0
+void pci_do_recovery(struct pci_dev *dev, int severity);
+
 /**
  * pci_pcie_cap - get the saved PCIe capability offset
  * @dev: PCI device
include/ras/ras_event.h +3 -3
index 9c68986..6176e90 100644
@@ -316,10 +316,10 @@
 
 	TP_printk("%s PCIe Bus Error: severity=%s, %s\n",
 		__get_str(dev_name),
-		__entry->severity == AER_CORRECTABLE ? "Corrected" :
-			__entry->severity == AER_FATAL ?
+		__entry->severity == PCI_ERR_AER_CORRECTABLE ? "Corrected" :
+			__entry->severity == PCI_ERR_AER_FATAL ?
 			"Fatal" : "Uncorrected, non-fatal",
-		__entry->severity == AER_CORRECTABLE ?
+		__entry->severity == PCI_ERR_AER_CORRECTABLE ?
 		__print_flags(__entry->status, "|", aer_correctable_errors) :
 		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
 );
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.
author
#4 lore raw

Re: [PATCH v3 1/3] PCI/AER: factor out error reporting from AER

Quoted 7 lines · Bjorn Helgaas
Quoted 6 lines · Oza Pawandeep

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

Signed-off-by Oza Pawandeep <[email protected]>
Quoted 14 lines

drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 465

Quoted 10 lines
drivers/acpi/apei/ghes.c +1 -1
index 6402f7f..fd053e5 100644
@@ -462,7 +462,7 @@ static void ghes_do_proc(struct ghes *ghes,
 				 * use, so treat it as a fatal AER error.
 				 */
 				if (gdata->flags & CPER_SEC_RESET)
-					aer_severity = AER_FATAL;
+					aer_severity = PCI_ERR_AER_FATAL;

Please split the s/AER_FATAL/PCI_ERR_AER_FATAL/ changes into a separate patch to reduce the size of this patch.

will do.

Quoted 4 lines

I would name them PCI_ERR_FATAL and PCI_ERR_NONFATAL because that matches the usage in the spec, e.g., PCIe r4.0, sec 6.2.2, and the symbols like PCI_ERR_UNC_UND in pci_regs.h.

ok will work on that.

Quoted 18 lines

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

Quoted 8 lines
drivers/pci/pcie/pcie-err.c new file +2 -0
new file mode 100644
index 0000000..a76a8bf
@@ -0,0 +1,335 @@
+/*
+ * Copyright (c) 2017, The Linux Foundation. All rights reserved.

Somebody already mentioned using the SPDX thing, which I think you should do.

But I'm confused about this copyright line. As far as I can tell, this basically moves code from aerdrv_core.c to pcie-err.c, which is not enough to change the copyright ownership. But it drops the copyright lines from aerdrv.core.c and replaces them with "(c) 2017, The Linux Foundation". ?? Where did that come from?

Quoted 5 lines

If you *add* something non-trivial, I think it's OK to add your own new copyright info (though I don't think this is really necessary), but I don't think we should *remove* information about other copyright owners.

sure will keep original copyright owners info, and SPDX as well.

Quoted 28 lines
Quoted 14 lines
drivers/pci/pcie/pcie-err.c new file +1 -0
+ * This program is free software; you can redistribute it and/or

modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.

drivers/pci/pcie/pcie-err.c new file +10 -0
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/pci.h>
+#include <linux/module.h>
+#include <linux/pci.h>

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

Quoted 10 lines
drivers/pci/pcie/pcie-err.c new file +10 -0
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/aer.h>
+#include <linux/pcieport_if.h>
+#include "portdrv.h"
+
+static DEFINE_MUTEX(pci_err_recovery_lock);
+
+pci_ers_result_t pci_merge_result(enum pci_ers_result orig,
+				  enum pci_ers_result new)

Please do all the renames, e.g., s/merge_result/pci_merge_result/, in a separate patch, followed by one that only moves code between files.

Quoted 4 lines

These initial ones will seem trivial, and they are, which is perfect. That makes them easy to review, and it will make the "interesting" patches much smaller and also easier to review.

ok all the renamed will be made in a separate patch.

Quoted 4 lines

The you can follow up with more patches that do things like add the mutex. It's too hard to review (or even notice) things like that when everything is squashed together.

sure.

Quoted 37 lines
Quoted 20 lines
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/linux/pci.h +0 -0
index c170c92..083408e 100644

include/linux/pci.h struct pci_error_handlers { · line 745

Quoted 7 lines
include/linux/pci.h +4 -0
@@ -739,6 +739,10 @@ struct pci_error_handlers {
 	void (*resume)(struct pci_dev *dev);
 };
 
+struct pci_err_broadcast_data {
+	enum pci_channel_state state;
+	enum pci_ers_result result;
+};

Only used in pcie-err.c; should be declared there.

Quoted 7 lines
include/linux/pci.h +0 -0
 struct module;
 struct pci_driver {

@@ -1998,6 +2002,23 @@ static inline resource_size_t

pci_iov_resource_size(struct pci_dev *dev, int res
void pci_hp_remove_module_link(struct pci_slot *pci_slot); #endif

Quoted 9 lines

include/linux/pci.h static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res · line 2007

Quoted 3 lines
include/linux/pci.h +3 -0
+#define PCI_ERR_AER_NONFATAL		0
+#define PCI_ERR_AER_FATAL		1
+#define PCI_ERR_AER_CORRECTABLE		2

Why do these need to be moved to include/linux/pci.h? I don't really want them in include/linux at all. The only uses outside drivers/pci are in ras_event.h and acpi/apei/ghes.c. I'd rather keep them in aer.h with the hope of being able to move them into drivers/pci/pci.h eventually.

but if I do PCI_ERR_FATAL in aer.h
how dpc can use that ?
let me see what best I can do because PCI_ERR_AER_FATAL if renamed to PCI_ERR_FATAL then both aer and dpc will use that.

Quoted 43 lines

include/linux/pci.h static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res · line 2019

Quoted 12 lines
include/linux/pci.h +11 -0
+pci_ers_result_t pci_broadcast_error_message(struct pci_dev *dev,
+					enum pci_channel_state state,
+					char *error_mesg,
+					int (*cb)(struct pci_dev *, void *));
+int pci_report_mmio_enabled(struct pci_dev *dev, void *data);
+int pci_report_slot_reset(struct pci_dev *dev, void *data);
+int pci_report_resume(struct pci_dev *dev, void *data);
+int pci_report_error_detected(struct pci_dev *dev, void *data);
+pci_ers_result_t pci_reset_link(struct pci_dev *dev);
+pci_ers_result_t pci_merge_result(enum pci_ers_result orig,
+				enum pci_ers_result new);

The above are only used in pcie-err.c and should be static and not declared here.

Quoted 27 lines
include/linux/pci.h +2 -0
+void pci_do_recovery(struct pci_dev *dev, int severity);
+
 /**
  * pci_pcie_cap - get the saved PCIe capability offset
  * @dev: PCI device
include/ras/ras_event.h +3 -4
index 9c68986..6176e90 100644
@@ -316,10 +316,10 @@
 
 	TP_printk("%s PCIe Bus Error: severity=%s, %s\n",
 		__get_str(dev_name),
-		__entry->severity == AER_CORRECTABLE ? "Corrected" :
-			__entry->severity == AER_FATAL ?
+		__entry->severity == PCI_ERR_AER_CORRECTABLE ? "Corrected" :
+			__entry->severity == PCI_ERR_AER_FATAL ?
 			"Fatal" : "Uncorrected, non-fatal",
-		__entry->severity == AER_CORRECTABLE ?
+		__entry->severity == PCI_ERR_AER_CORRECTABLE ?
 		__print_flags(__entry->status, "|", aer_correctable_errors) :
 		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
 );
--

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.

author
#5 lore raw

PATCH v3 2/3 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.

Signed-off-by Oza Pawandeep <[email protected]>
drivers/pci/pcie/pcie-dpc.c +85 -4
index 2d976a6..68296ec 100644
@@ -15,6 +15,9 @@
 #include <linux/pci.h>
 #include <linux/pcieport_if.h>
 #include "../pci.h"
+#include "portdrv.h"
+
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
 
 struct rp_pio_header_log_regs {
 	u32 dw0;

@@ -67,6 +70,60 @@ struct dpc_dev {
 	"Memory Request Completion Timeout",		 /* Bit Position 18 */
 };
 
+static int find_dpc_dev_iter(struct device *device, void *data)
+{
+	struct pcie_port_service_driver *service_driver;
+	struct device **dev;
+
+	dev = (struct device **) data;
+
+	if (device->bus == &pcie_port_bus_type && device->driver) {
+		service_driver = to_service_driver(device->driver);
+		if (service_driver->service == PCIE_PORT_SERVICE_DPC) {
+			*dev = device;
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+static struct device *pci_find_dpc_dev(struct pci_dev *pdev)
+{
+	struct device *dev = NULL;
+
+	device_for_each_child(&pdev->dev, &dev, find_dpc_dev_iter);
+
+	return dev;
+}
+
+static int find_dpc_service_iter(struct device *device, void *data)
+{
+	struct pcie_port_service_driver *service_driver, **drv;
+
+	drv = (struct pcie_port_service_driver **) data;
+
+	if (device->bus == &pcie_port_bus_type && device->driver) {
+		service_driver = to_service_driver(device->driver);
+		if (service_driver->service == PCIE_PORT_SERVICE_DPC) {
+			*drv = service_driver;
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+struct pcie_port_service_driver *pci_find_dpc_service(struct pci_dev *dev)
+{
+	struct pcie_port_service_driver *drv = NULL;
+
+	device_for_each_child(&dev->dev, &drv, find_dpc_service_iter);
+
+	return drv;
+}
+EXPORT_SYMBOL(pci_find_dpc_service);
+
 static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 {
 	unsigned long timeout = jiffies + HZ;

@@ -104,11 +161,23 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
 		dev_warn(dev, "Link state not disabled for DPC event\n");
 }
 
-static void interrupt_event_handler(struct work_struct *work)
+/**
+ * dpc_reset_link - reset link DPC  routine
+ * @dev: pointer to Root Port's pci_dev data structure
+ *
+ * Invoked by Port Bus driver when performing link reset at Root Port.
+ */
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 {
-	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
-	struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
 	struct pci_bus *parent = pdev->subordinate;
+	struct pci_dev *dev, *temp;
+	struct dpc_dev *dpc;
+	struct pcie_device *pciedev;
+	struct device *devdpc;
+
+	devdpc = pci_find_dpc_dev(pdev);
+	pciedev = to_pcie_device(devdpc);
+	dpc = get_service_data(pciedev);
 
 	pci_lock_rescan_remove();
 	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,

@@ -125,7 +194,7 @@ static void interrupt_event_handler(struct work_struct *work)
 
 	dpc_wait_link_inactive(dpc);
 	if (dpc->rp && dpc_wait_rp_inactive(dpc))
-		return;
+		return PCI_ERS_RESULT_DISCONNECT;
 	if (dpc->rp && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev,
 				      dpc->cap_pos + PCI_EXP_DPC_RP_PIO_STATUS,

@@ -135,6 +204,17 @@ static void interrupt_event_handler(struct work_struct *work)
 
 	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS,
 		PCI_EXP_DPC_STATUS_TRIGGER | PCI_EXP_DPC_STATUS_INTERRUPT);
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void interrupt_event_handler(struct work_struct *work)
+{
+	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
+	struct pci_dev *pdev = dpc->dev->port;
+
+	/* From DPC point of view error is always FATAL. */
+	pci_do_recovery(pdev, PCI_ERR_DPC_FATAL);
 }
 
 static void dpc_rp_pio_print_tlp_header(struct device *dev,

@@ -339,6 +419,7 @@ static void dpc_remove(struct pcie_device *dev)
 	.service	= PCIE_PORT_SERVICE_DPC,
 	.probe		= dpc_probe,
 	.remove		= dpc_remove,
+	.reset_link     = dpc_reset_link,
 };
 
 static int __init dpc_service_init(void)
drivers/pci/pcie/pcie-err.c +16 -6
index a76a8bf..858c94c 100644
@@ -176,7 +176,7 @@ static pci_ers_result_t pci_default_reset_link(struct pci_dev *dev)
 	return PCI_ERS_RESULT_RECOVERED;
 }
 
-pci_ers_result_t pci_reset_link(struct pci_dev *dev)
+pci_ers_result_t pci_reset_link(struct pci_dev *dev, int severity)
 {
 	struct pci_dev *udev;
 	pci_ers_result_t status;

@@ -190,9 +190,17 @@ pci_ers_result_t pci_reset_link(struct pci_dev *dev)
 		udev = dev->bus->self;
 	}
 
+
+	/* Use the service driver of the component firstly */
+#if IS_ENABLED(CONFIG_PCIEDPC)
+	if (severity == PCI_ERR_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 == PCI_ERR_AER_FATAL) ||
+	    (severity == PCI_ERR_AER_NONFATAL) ||
+	    (severity == PCI_ERR_AER_CORRECTABLE))
+		driver = pci_find_aer_service(udev);
 #endif
 
 	if (driver && driver->reset_link) {

@@ -282,7 +290,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 
 	mutex_lock(&pci_err_recovery_lock);
 
-	if (severity == PCI_ERR_AER_FATAL)
+	if ((severity == PCI_ERR_AER_FATAL) ||
+	    (severity == PCI_ERR_DPC_FATAL))
 		state = pci_channel_io_frozen;
 	else
 		state = pci_channel_io_normal;

@@ -292,8 +301,9 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 			"error_detected",
 			pci_report_error_detected);
 
-	if (severity == PCI_ERR_AER_FATAL) {
-		result = pci_reset_link(dev);
+	if ((severity == PCI_ERR_AER_FATAL) ||
+	    (severity == PCI_ERR_DPC_FATAL)) {
+		result = pci_reset_link(dev, severity);
 		if (result != PCI_ERS_RESULT_RECOVERED)
 			goto failed;
 	}
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_ */
include/linux/pci.h +2 -1
index 083408e..123ee15 100644
@@ -2005,6 +2005,7 @@ static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res
 #define PCI_ERR_AER_NONFATAL		0
 #define PCI_ERR_AER_FATAL		1
 #define PCI_ERR_AER_CORRECTABLE		2
+#define PCI_ERR_DPC_FATAL		4
 
 pci_ers_result_t pci_broadcast_error_message(struct pci_dev *dev,
 					enum pci_channel_state state,

@@ -2014,7 +2015,7 @@ pci_ers_result_t pci_broadcast_error_message(struct pci_dev *dev,
 int pci_report_slot_reset(struct pci_dev *dev, void *data);
 int pci_report_resume(struct pci_dev *dev, void *data);
 int pci_report_error_detected(struct pci_dev *dev, void *data);
-pci_ers_result_t pci_reset_link(struct pci_dev *dev);
+pci_ers_result_t pci_reset_link(struct pci_dev *dev, int severity);
 pci_ers_result_t pci_merge_result(enum pci_ers_result orig,
 				enum pci_ers_result new);
 void pci_do_recovery(struct pci_dev *dev, int severity);
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.
author
#6 lore raw

PATCH v3 3/3 PCI/DPC: Enumerate the devices after DPC trigger event

In reply to Oza Pawandeep

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

Signed-off-by Oza Pawandeep <[email protected]>
drivers/pci/pcie/pcie-dpc.c +38 -0
index 68296ec..4c6bef3 100644
@@ -161,6 +161,43 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
 		dev_warn(dev, "Link state not disabled for DPC event\n");
 }
 
+static bool dpc_wait_link_active(struct pci_dev *pdev)
+{
+	unsigned long timeout = jiffies + HZ;
+	u16 lnk_status;
+	bool ret = true;
+
+	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(&pdev->dev, "Link state not enabled after DPC event\n");
+		ret = false;
+	}
+
+	return ret;
+}
+
+/**
+ * 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)
+{
+	if (dpc_wait_link_active(pdev)) {
+		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

@@ -419,6 +456,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 +23 -5
index 858c94c..7d1d20e 100644
@@ -238,7 +238,8 @@ pci_ers_result_t pci_reset_link(struct pci_dev *dev, int severity)
 pci_ers_result_t pci_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 pci_err_broadcast_data result_data;
 

@@ -250,6 +251,15 @@ pci_ers_result_t pci_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 == PCI_ERR_DPC_FATAL) &&
+			(cb == pci_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

@@ -299,7 +309,8 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 	status = pci_broadcast_error_message(dev,
 			state,
 			"error_detected",
-			pci_report_error_detected);
+			pci_report_error_detected,
+			severity);
 
 	if ((severity == PCI_ERR_AER_FATAL) ||
 	    (severity == PCI_ERR_DPC_FATAL)) {

@@ -308,11 +319,15 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 			goto failed;
 	}
 
+	if (severity == PCI_ERR_DPC_FATAL)
+		goto resume;
+
 	if (status == PCI_ERS_RESULT_CAN_RECOVER)
 		status = pci_broadcast_error_message(dev,
 				state,
 				"mmio_enabled",
-				pci_report_mmio_enabled);
+				pci_report_mmio_enabled,
+				severity);
 
 	if (status == PCI_ERS_RESULT_NEED_RESET) {
 		/*

@@ -323,16 +338,19 @@ void pci_do_recovery(struct pci_dev *dev, int severity)
 		status = pci_broadcast_error_message(dev,
 				state,
 				"slot_reset",
-				pci_report_slot_reset);
+				pci_report_slot_reset,
+				severity);
 	}
 
 	if (status != PCI_ERS_RESULT_RECOVERED)
 		goto failed;
 
+resume:
 	pci_broadcast_error_message(dev,
 				state,
 				"resume",
-				pci_report_resume);
+				pci_report_resume,
+				severity);
 
 	dev_info(&dev->dev, "Device recovery successful\n");
 	mutex_unlock(&pci_err_recovery_lock);
include/linux/pci.h +2 -1
index 123ee15..46e2526 100644
@@ -2010,7 +2010,8 @@ static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int res
 pci_ers_result_t pci_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);
 int pci_report_mmio_enabled(struct pci_dev *dev, void *data);
 int pci_report_slot_reset(struct pci_dev *dev, void *data);
 int pci_report_resume(struct pci_dev *dev, void *data);
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.