Address error and recovery for AER and DPC

This patch alters the behavior of handling of ERR_FATAL, where removal of devices is initiated, followed by reset link, followed by re-enumeration.

So the errors are handled in a different way as follows:
ERR_NONFATAL => call driver recovery entry points
ERR_FATAL => remove and re-enumerate

please refer to Documentation/PCI/pci-error-recovery.txt for more details.

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

Reviewed-by Keith Busch <[email protected]>

drivers/pci/pcie/aer/aerdrv_core.c +26 -1

index 0ea5acc..649dd1f 100644
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/kfifo.h>
 #include "aerdrv.h"
+#include "../../pci.h"
 
 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)

@@ -475,35 +476,84 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
 }
 
 /**
- * do_recovery - handle nonfatal/fatal error recovery process
+ * do_fatal_recovery - handle fatal error recovery process
+ * @dev: pointer to a pci_dev data structure of agent detecting an error
+ *
+ * Invoked when an error is fatal. Once being invoked, removes the devices
+ * benetah this AER agent, followed by reset link e.g. secondary bus reset
+ * followed by re-enumeration of devices.
+ */
+
+static void do_fatal_recovery(struct pci_dev *dev)
+{
+	struct pci_dev *udev;
+	struct pci_bus *parent;
+	struct pci_dev *pdev, *temp;
+	pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+	struct aer_broadcast_data result_data;
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		udev = dev;
+	else
+		udev = dev->bus->self;
+
+	parent = udev->subordinate;
+	pci_lock_rescan_remove();
+	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+				 bus_list) {

drivers/pci/pcie/aer/aerdrv_core.c +35 -17

+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+		/*
+		 * If the error is reported by a bridge, we think this error
+		 * is related to the downstream link of the bridge, so we
+		 * do error recovery on all subordinates of the bridge instead
+		 * of the bridge and clear the error status of the bridge.
+		 */
+		pci_walk_bus(dev->subordinate, report_resume, &result_data);
+		pci_cleanup_aer_uncorrect_error_status(dev);
+	}
+
+	if (result == PCI_ERS_RESULT_RECOVERED) {
+		if (pcie_wait_for_link(udev, true))
+			pci_rescan_bus(udev->bus);
+	} else {
+		pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
+		pci_info(dev, "AER: Device recovery failed\n");
+	}
+
+	pci_unlock_rescan_remove();
+}
+
+/**
+ * do_nonfatal_recovery - handle nonfatal error recovery process
  * @dev: pointer to a pci_dev data structure of agent detecting an error
- * @severity: error severity type
  *
  * Invoked when an error is nonfatal/fatal. Once being invoked, broadcast
  * 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 do_nonfatal_recovery(struct pci_dev *dev)
 {
-	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
+	pci_ers_result_t status;
 	enum pci_channel_state state;
 
-	if (severity == AER_FATAL)
-		state = pci_channel_io_frozen;
-	else
-		state = pci_channel_io_normal;
+	state = pci_channel_io_normal;
 
 	status = broadcast_error_message(dev,
 			state,
 			"error_detected",
 			report_error_detected);
 
-	if (severity == AER_FATAL) {
-		result = reset_link(dev);
-		if (result != PCI_ERS_RESULT_RECOVERED)
-			goto failed;
-	}
-
 	if (status == PCI_ERS_RESULT_CAN_RECOVER)
 		status = broadcast_error_message(dev,
 				state,

@@ -562,8 +612,10 @@ static void handle_error_source(struct pcie_device *aerdev,
 		if (pos)
 			pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
 					info->status);
-	} else
-		do_recovery(dev, info->severity);
+	} else if (info->severity == AER_NONFATAL)
+		do_nonfatal_recovery(dev);
+	else if (info->severity == AER_FATAL)
+		do_fatal_recovery(dev);
 }
 
 #ifdef CONFIG_ACPI_APEI_PCIEAER

@@ -627,8 +679,10 @@ static void aer_recover_work_func(struct work_struct *work)
 			continue;
 		}
 		cper_print_aer(pdev, entry.severity, entry.regs);
-		if (entry.severity != AER_CORRECTABLE)
-			do_recovery(pdev, entry.severity);
+		if (entry.severity == AER_NONFATAL)
+			do_nonfatal_recovery(pdev);
+		else if (entry.severity == AER_FATAL)
+			do_fatal_recovery(pdev);
 		pci_dev_put(pdev);
 	}
 }

Signature · 1 line

2.7.4

Quoted 60 lines · Oza Pawandeep

This patch alters the behavior of handling of ERR_FATAL, where removal of devices is initiated, followed by reset link, followed by re-enumeration.

So the errors are handled in a different way as follows:
ERR_NONFATAL => call driver recovery entry points
ERR_FATAL => remove and re-enumerate

please refer to Documentation/PCI/pci-error-recovery.txt for more details.

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

Reviewed-by Keith Busch <[email protected]>

drivers/pci/pcie/aer/aerdrv_core.c +1 -0

index 0ea5acc..649dd1f 100644
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/kfifo.h>
 #include "aerdrv.h"
+#include "../../pci.h"
 
 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE 
 | \

drivers/pci/pcie/aer/aerdrv_core.c +2 -1

 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)

@@ -475,35 +476,84 @@ static pci_ers_result_t reset_link(struct 
 pci_dev *dev)
 }
 
 /**
- * do_recovery - handle nonfatal/fatal error recovery process
+ * do_fatal_recovery - handle fatal error recovery process
+ * @dev: pointer to a pci_dev data structure of agent detecting an 

error
+ *
+ * Invoked when an error is fatal. Once being invoked, removes the devices
+ * benetah this AER agent, followed by reset link e.g. secondary bus reset
+ * followed by re-enumeration of devices.
+ */

drivers/pci/pcie/aer/aerdrv_core.c +18 -0

+
+static void do_fatal_recovery(struct pci_dev *dev)
+{
+	struct pci_dev *udev;
+	struct pci_bus *parent;
+	struct pci_dev *pdev, *temp;
+	pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+	struct aer_broadcast_data result_data;
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		udev = dev;
+	else
+		udev = dev->bus->self;
+
+	parent = udev->subordinate;
+	pci_lock_rescan_remove();
+	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+				 bus_list) {

drivers/pci/pcie/aer/aerdrv_core.c static pci_ers_result_t reset_link(struct pci_dev *dev) · line 513

Quoted 10 lines

drivers/pci/pcie/aer/aerdrv_core.c +10 -0

+		pci_dev_get(pdev);
+		pci_dev_set_disconnected(pdev, NULL);
+		if (pci_has_subordinate(pdev))
+			pci_walk_bus(pdev->subordinate,
+				     pci_dev_set_disconnected, NULL);
+		pci_stop_and_remove_bus_device(pdev);
+		pci_dev_put(pdev);
+	}
+
+	result = reset_link(udev);

I don't like the fact that for DPC, the link reset happens before we call the driver .remove() methods, while for AER, the reset happens *after* the .remove() methods. That means the .remove() methods may work differently for AER vs. DPC, e.g., they may be able to access the device if AER is handling the error, but they would not be able to access it if DPC is handling it.

I don't know how to fix this, and I think we can keep this patch as it is for now, but I think we should fix it eventually.

Quoted 93 lines

drivers/pci/pcie/aer/aerdrv_core.c +24 -0

+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+		/*
+		 * If the error is reported by a bridge, we think this error
+		 * is related to the downstream link of the bridge, so we
+		 * do error recovery on all subordinates of the bridge instead
+		 * of the bridge and clear the error status of the bridge.
+		 */
+		pci_walk_bus(dev->subordinate, report_resume, &result_data);
+		pci_cleanup_aer_uncorrect_error_status(dev);
+	}
+
+	if (result == PCI_ERS_RESULT_RECOVERED) {
+		if (pcie_wait_for_link(udev, true))
+			pci_rescan_bus(udev->bus);
+	} else {
+		pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
+		pci_info(dev, "AER: Device recovery failed\n");
+	}
+
+	pci_unlock_rescan_remove();
+}
+
+/**
+ * do_nonfatal_recovery - handle nonfatal error recovery process
  * @dev: pointer to a pci_dev data structure of agent detecting an 

error
- * @severity: error severity type
*
* Invoked when an error is nonfatal/fatal. Once being invoked,
broadcast
* error detected message to all downstream drivers within a
hierarchy in
* question and return the returned code.
*/

drivers/pci/pcie/aer/aerdrv_core.c +7 -14

-static void do_recovery(struct pci_dev *dev, int severity)
+static void do_nonfatal_recovery(struct pci_dev *dev)
 {
-	pci_ers_result_t status, result = PCI_ERS_RESULT_RECOVERED;
+	pci_ers_result_t status;
 	enum pci_channel_state state;
 
-	if (severity == AER_FATAL)
-		state = pci_channel_io_frozen;
-	else
-		state = pci_channel_io_normal;
+	state = pci_channel_io_normal;
 
 	status = broadcast_error_message(dev,
 			state,
 			"error_detected",
 			report_error_detected);
 
-	if (severity == AER_FATAL) {
-		result = reset_link(dev);
-		if (result != PCI_ERS_RESULT_RECOVERED)
-			goto failed;
-	}
-
 	if (status == PCI_ERS_RESULT_CAN_RECOVER)
 		status = broadcast_error_message(dev,
 				state,

@@ -562,8 +612,10 @@ static void handle_error_source(struct 
 pcie_device *aerdev,
 		if (pos)
 			pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
 					info->status);
-	} else
-		do_recovery(dev, info->severity);
+	} else if (info->severity == AER_NONFATAL)
+		do_nonfatal_recovery(dev);
+	else if (info->severity == AER_FATAL)
+		do_fatal_recovery(dev);
 }
 

#ifdef CONFIG_ACPI_APEI_PCIEAER

drivers/pci/pcie/aer/aerdrv_core.c +4 -2

@@ -627,8 +679,10 @@ static void aer_recover_work_func(struct 
 work_struct *work)
 			continue;
 		}
 		cper_print_aer(pdev, entry.severity, entry.regs);
-		if (entry.severity != AER_CORRECTABLE)
-			do_recovery(pdev, entry.severity);
+		if (entry.severity == AER_NONFATAL)
+			do_nonfatal_recovery(pdev);
+		else if (entry.severity == AER_FATAL)
+			do_fatal_recovery(pdev);
 		pci_dev_put(pdev);
 	}

}
--
2.7.4

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

+void pcie_do_fatal_recovery(struct pci_dev *dev)
+{
+	struct pci_dev *udev;
+	struct pci_bus *parent;
+	struct pci_dev *pdev, *temp;
+	pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+	struct aer_broadcast_data result_data;
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		udev = dev;
+	else
+		udev = dev->bus->self;
+
+	parent = udev->subordinate;
+	pci_lock_rescan_remove();

Quoted 15 lines · Oza Pawandeep

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

+void pcie_do_fatal_recovery(struct pci_dev *dev)
+{
+	struct pci_dev *udev;
+	struct pci_bus *parent;
+	struct pci_dev *pdev, *temp;
+	pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+	struct aer_broadcast_data result_data;
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		udev = dev;
+	else
+		udev = dev->bus->self;
+
+	parent = udev->subordinate;
+	pci_lock_rescan_remove();

drivers/pci/pcie/err.c line 316

Quoted 10 lines

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

+	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+				 bus_list) {
+		pci_dev_get(pdev);
+		pci_dev_set_disconnected(pdev, NULL);
+		if (pci_has_subordinate(pdev))
+			pci_walk_bus(pdev->subordinate,
+				     pci_dev_set_disconnected, NULL);
+		pci_stop_and_remove_bus_device(pdev);
+		pci_dev_put(pdev);
+	}

Any reason not to simply call

	pci_walk_bus(udev->subordinate, pci_dev_set_disconnected, NULL);

before the list_for_each_entry_safe_reverse() iteration, instead of calling it for each device on the subordinate bus and for each device's children? Should be semantically identical, saves 3 LoC and saves wasted cycles of acquiring pci_bus_sem over and over again for each device on the subordinate bus.

Thanks,

Lukas

Quoted 29 lines · Lukas Wunner

Quoted 15 lines · Oza Pawandeep

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

+void pcie_do_fatal_recovery(struct pci_dev *dev)
+{
+	struct pci_dev *udev;
+	struct pci_bus *parent;
+	struct pci_dev *pdev, *temp;
+	pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+	struct aer_broadcast_data result_data;
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		udev = dev;
+	else
+		udev = dev->bus->self;
+
+	parent = udev->subordinate;
+	pci_lock_rescan_remove();

drivers/pci/pcie/err.c line 316

Quoted 10 lines

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

+	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+				 bus_list) {
+		pci_dev_get(pdev);
+		pci_dev_set_disconnected(pdev, NULL);
+		if (pci_has_subordinate(pdev))
+			pci_walk_bus(pdev->subordinate,
+				     pci_dev_set_disconnected, NULL);
+		pci_stop_and_remove_bus_device(pdev);
+		pci_dev_put(pdev);
+	}

Any reason not to simply call

Quoted 7 lines

	pci_walk_bus(udev->subordinate, pci_dev_set_disconnected, NULL);

before the list_for_each_entry_safe_reverse() iteration, instead of calling it for each device on the subordinate bus and for each device's children? Should be semantically identical, saves 3 LoC and saves wasted cycles of acquiring pci_bus_sem over and over again for each device on the subordinate bus.

Well this is borrowed code from DPC driver, hence I thought to keep the same.
but to me it looks like its taking care of PCIe switch where is goes through all the subordinates, and which could turn out to be more swicthes down the line, and son on...
it goes all the way down to the tree

Quoted 38 lines · On Fri, May 11, 2018 at 09:04:36PM +0530, [email protected] wrote:

Quoted 29 lines · Lukas Wunner

Quoted 15 lines · Oza Pawandeep

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

+void pcie_do_fatal_recovery(struct pci_dev *dev)
+{
+	struct pci_dev *udev;
+	struct pci_bus *parent;
+	struct pci_dev *pdev, *temp;
+	pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+	struct aer_broadcast_data result_data;
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		udev = dev;
+	else
+		udev = dev->bus->self;
+
+	parent = udev->subordinate;
+	pci_lock_rescan_remove();

drivers/pci/pcie/err.c line 316

Quoted 10 lines

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

+	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+				 bus_list) {
+		pci_dev_get(pdev);
+		pci_dev_set_disconnected(pdev, NULL);
+		if (pci_has_subordinate(pdev))
+			pci_walk_bus(pdev->subordinate,
+				     pci_dev_set_disconnected, NULL);
+		pci_stop_and_remove_bus_device(pdev);
+		pci_dev_put(pdev);
+	}

Any reason not to simply call

Quoted 7 lines

	pci_walk_bus(udev->subordinate, pci_dev_set_disconnected, NULL);

before the list_for_each_entry_safe_reverse() iteration, instead of calling it for each device on the subordinate bus and for each device's children? Should be semantically identical, saves 3 LoC and saves wasted cycles of acquiring pci_bus_sem over and over again for each device on the subordinate bus.

Quoted 6 lines

Well this is borrowed code from DPC driver, hence I thought to keep the same.
but to me it looks like its taking care of PCIe switch where is goes through all the subordinates, and which could turn out to be more swicthes down the line, and son on...
it goes all the way down to the tree

... which is precisely what the one line I suggested above does.

You don't need to respin for this alone as far as I'm concerned, but please post a follow-up refactoring patch. I have a patch in the pipeline which makes the same change in pciehp, hence this caught my eye.

Thanks,

Lukas

This patch factors out error reporting callbacks, which are currently tightly coupled with AER.

DPC should be able to register callbacks and attempt recovery when DPC trigger event occurs.

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

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

+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) {
+			/*
+			 * 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.
+			 */
+			pci_printk(KERN_DEBUG, dev, "device has %s\n",
+				   dev->driver ?
+				   "no AER-aware driver" : "no driver");
+		}
+
+		/*
+		 * 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);
+#if defined(CONFIG_PCIEAER)
+		pci_uevent_ers(dev, PCI_ERS_RESULT_NONE);
+#endif
 
+static int report_slot_reset(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);
+	if (!dev->driver ||
+		!dev->driver->err_handler ||
+		!dev->driver->err_handler->slot_reset)
+		goto out;
+
+	err_handler = dev->driver->err_handler;
+	vote = err_handler->slot_reset(dev);
+	result_data->result = merge_result(result_data->result, vote);
+out:
+	device_unlock(&dev->dev);
+	return 0;
+}
+
+static int report_resume(struct pci_dev *dev, void *data)
+{
+	const struct pci_error_handlers *err_handler;
+
+	device_lock(&dev->dev);
+	dev->error_state = pci_channel_io_normal;
+
+	if (!dev->driver ||
+		!dev->driver->err_handler ||
+		!dev->driver->err_handler->resume)
+		goto out;
+
+	err_handler = dev->driver->err_handler;
+	err_handler->resume(dev);
+#if defined(CONFIG_PCIEAER)
+	pci_uevent_ers(dev, PCI_ERS_RESULT_RECOVERED);
+#endif
 
+void pcie_do_fatal_recovery(struct pci_dev *dev)
+{
+	struct pci_dev *udev;
+	struct pci_bus *parent;
+	struct pci_dev *pdev, *temp;
+	pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+	struct aer_broadcast_data result_data;
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		udev = dev;
+	else
+		udev = dev->bus->self;
+
+	parent = udev->subordinate;
+	pci_lock_rescan_remove();
+	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
+				 bus_list) {
+		pci_dev_get(pdev);
+		pci_dev_set_disconnected(pdev, NULL);
+		if (pci_has_subordinate(pdev))
+			pci_walk_bus(pdev->subordinate,
+				     pci_dev_set_disconnected, NULL);
+		pci_stop_and_remove_bus_device(pdev);
+		pci_dev_put(pdev);
+	}
+
+	result = reset_link(udev);
+
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+		/*
+		 * If the error is reported by a bridge, we think this error
+		 * is related to the downstream link of the bridge, so we
+		 * do error recovery on all subordinates of the bridge instead
+		 * of the bridge and clear the error status of the bridge.
+		 */
+		pci_walk_bus(dev->subordinate, report_resume, &result_data);
+		pci_cleanup_aer_uncorrect_error_status(dev);

DPC driver implements link_reset callback, and calls pci_do_fatal_recovery().

Which follows standard path of ERR_FATAL recovery.

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

drivers/pci/pci.h +1 -1

index 5e8857a..6af7595 100644
@@ -354,7 +354,7 @@ 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 pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
 void pcie_do_nonfatal_recovery(struct pci_dev *dev);
 

bool pcie_wait_for_link(struct pci_dev *pdev, bool active);

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

index fdfc474..36e622d 100644
@@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device 
 *aerdev,
 	} else if (info->severity == AER_NONFATAL)
 		pcie_do_nonfatal_recovery(dev);
 	else if (info->severity == AER_FATAL)
-		pcie_do_fatal_recovery(dev);
+		pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
 }
 

#ifdef CONFIG_ACPI_APEI_PCIEAER

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

@@ -321,7 +321,7 @@ static void aer_recover_work_func(struct 
 work_struct *work)
 		if (entry.severity == AER_NONFATAL)
 			pcie_do_nonfatal_recovery(pdev);
 		else if (entry.severity == AER_FATAL)
-			pcie_do_fatal_recovery(pdev);
+			pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
 		pci_dev_put(pdev);
 	}

}

drivers/pci/pcie/dpc.c +22 -20

index 80ec384..5680c13 100644
@@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev 
 *dpc)
 	pcie_wait_for_link(pdev, false);
 }
 
-static void dpc_work(struct work_struct *work)
+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;
-	u16 cap = dpc->cap_pos, ctl;
-
-	pci_lock_rescan_remove();
-	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
-					 bus_list) {
-		pci_dev_get(dev);
-		pci_dev_set_disconnected(dev, NULL);
-		if (pci_has_subordinate(dev))
-			pci_walk_bus(dev->subordinate,
-				     pci_dev_set_disconnected, NULL);
-		pci_stop_and_remove_bus_device(dev);
-		pci_dev_put(dev);
-	}
-	pci_unlock_rescan_remove();
-
+	struct dpc_dev *dpc;
+	struct pcie_device *pciedev;
+	struct device *devdpc;
+	u16 cap, ctl;
+
+	/*
+	 * DPC disables the Link automatically in hardware, so it has
+	 * already been reset by the time we get here.
+	 */
+
+	devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
+	pciedev = to_pcie_device(devdpc);
+	dpc = get_service_data(pciedev);
+	cap = dpc->cap_pos;
+
+	/*
+	 * Waiting until the link is inactive, then clearing DPC
+	 * trigger status to allow the port to leave DPC.
+	 */
 	dpc_wait_link_inactive(dpc);
+
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
-		return;
+		return PCI_ERS_RESULT_DISCONNECT;
 	if (dpc->rp_extensions && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,

drivers/pci/pcie/dpc.c +12 -0

 				       dpc->rp_pio_status);

@@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void dpc_work(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. */
+	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)

@@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = 
 {
 	.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/err.c +1 -1

index 33a16b1..29ff148 100644
@@ -185,7 +185,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, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	pci_ers_result_t status;

@@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev 
 *dev)
 	}
 
 	/* Use the aer driver of the component firstly */
-	driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
+	driver = pcie_port_find_service(udev, service);
 
 	if (driver && driver->reset_link) {

drivers/pci/pcie/err.c +1 -1

 		status = driver->reset_link(udev);

@@ -287,7 +287,7 @@ static pci_ers_result_t
 broadcast_error_message(struct pci_dev *dev,
  * followed by re-enumeration of devices.
  */
 
-void pcie_do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	struct pci_bus *parent;

@@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
 		pci_dev_put(pdev);
 	}
 
-	result = reset_link(udev);
+	result = reset_link(udev, service);
 
 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 		/*

include/linux/aer.h +0 -0

index 8f87bbe..0c506fe 100644

Quoted 170 lines · Oza Pawandeep

DPC driver implements link_reset callback, and calls pci_do_fatal_recovery().

Which follows standard path of ERR_FATAL recovery.

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

drivers/pci/pci.h +1 -1

index 5e8857a..6af7595 100644
@@ -354,7 +354,7 @@ 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 pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
 void pcie_do_nonfatal_recovery(struct pci_dev *dev);
 

bool pcie_wait_for_link(struct pci_dev *pdev, bool active);

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

index fdfc474..36e622d 100644
@@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
 *aerdev,
 	} else if (info->severity == AER_NONFATAL)
 		pcie_do_nonfatal_recovery(dev);
 	else if (info->severity == AER_FATAL)
-		pcie_do_fatal_recovery(dev);
+		pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
 }
 

#ifdef CONFIG_ACPI_APEI_PCIEAER

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

@@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
 *work)
 		if (entry.severity == AER_NONFATAL)
 			pcie_do_nonfatal_recovery(pdev);
 		else if (entry.severity == AER_FATAL)
-			pcie_do_fatal_recovery(pdev);
+			pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
 		pci_dev_put(pdev);
 	}

}

drivers/pci/pcie/dpc.c +22 -20

index 80ec384..5680c13 100644
@@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
 *dpc)
 	pcie_wait_for_link(pdev, false);
 }
 
-static void dpc_work(struct work_struct *work)
+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;
-	u16 cap = dpc->cap_pos, ctl;
-
-	pci_lock_rescan_remove();
-	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
-					 bus_list) {
-		pci_dev_get(dev);
-		pci_dev_set_disconnected(dev, NULL);
-		if (pci_has_subordinate(dev))
-			pci_walk_bus(dev->subordinate,
-				     pci_dev_set_disconnected, NULL);
-		pci_stop_and_remove_bus_device(dev);
-		pci_dev_put(dev);
-	}
-	pci_unlock_rescan_remove();
-
+	struct dpc_dev *dpc;
+	struct pcie_device *pciedev;
+	struct device *devdpc;
+	u16 cap, ctl;
+
+	/*
+	 * DPC disables the Link automatically in hardware, so it has
+	 * already been reset by the time we get here.
+	 */
+
+	devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
+	pciedev = to_pcie_device(devdpc);
+	dpc = get_service_data(pciedev);
+	cap = dpc->cap_pos;
+
+	/*
+	 * Waiting until the link is inactive, then clearing DPC
+	 * trigger status to allow the port to leave DPC.
+	 */
 	dpc_wait_link_inactive(dpc);
+
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
-		return;
+		return PCI_ERS_RESULT_DISCONNECT;
 	if (dpc->rp_extensions && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,

drivers/pci/pcie/dpc.c +12 -0

 				       dpc->rp_pio_status);

@@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void dpc_work(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. */
+	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)

@@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
 	.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/err.c +1 -1

index 33a16b1..29ff148 100644
@@ -185,7 +185,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, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	pci_ers_result_t status;

@@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
 *dev)
 	}
 
 	/* Use the aer driver of the component firstly */
-	driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
+	driver = pcie_port_find_service(udev, service);
 
 	if (driver && driver->reset_link) {

drivers/pci/pcie/err.c +1 -1

 		status = driver->reset_link(udev);

@@ -287,7 +287,7 @@ static pci_ers_result_t
 broadcast_error_message(struct pci_dev *dev,
  * followed by re-enumeration of devices.
  */
 
-void pcie_do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	struct pci_bus *parent;

@@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
 		pci_dev_put(pdev);
 	}
 
-	result = reset_link(udev);
+	result = reset_link(udev, service);
 

include/linux/aer.h line 17

Quoted 11 lines

drivers/pci/pcie/err.c +0 -0

 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 		/*

include/linux/aer.h +1 -0

index 8f87bbe..0c506fe 100644
@@ -14,6 +14,7 @@
 #define AER_NONFATAL			0
 #define AER_FATAL			1
 #define AER_CORRECTABLE			2
+#define DPC_FATAL			4

include/linux/aer.h line 19

Quoted 1 line

include/linux/aer.h +0 -0

 struct pci_dev;

Hi Bjorn,

I have addressed all the comments, and I hope we are heading towards closure.

I just figure that pcie_do_fatal_recovery (which is getting executed for DPC as well)

if (result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

I have to correct it to

if (service==AER  && result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

rest of the things look okay to me.
If you have any more comments,
I can fix them with this one and hopefully v17 will be the final one.

PS: I am going though the code more, and we can have some follow up patches (probably some cleanup)
for e.g. pcie_portdrv_slot_reset() checks

if (dev->error_state == pci_channel_io_frozen) {
		dev->state_saved = true;
		pci_restore_state(dev);
		pcie_portdrv_restore_config(dev);
		pci_enable_pcie_error_reporting(dev);
	}

but now since the ERR_FATAL path does not call pcie_portdrv_slot_reset the check is meaning less.

besides driver's shut_down callbacks might want to handle pci_channel_io_frozen, but that something left to be discussed later. So, I am not touching dev->error_state anywhere as of now in ERR_FATAL case.

Regards,
Oza.

Quoted 171 lines · On Fri, May 11, 2018 at 05:22:08PM +0530, [email protected] wrote:

Quoted 170 lines · Oza Pawandeep

DPC driver implements link_reset callback, and calls pci_do_fatal_recovery().

Which follows standard path of ERR_FATAL recovery.

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

drivers/pci/pci.h +1 -1

index 5e8857a..6af7595 100644
@@ -354,7 +354,7 @@ 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 pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
 void pcie_do_nonfatal_recovery(struct pci_dev *dev);
 

bool pcie_wait_for_link(struct pci_dev *pdev, bool active);

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

index fdfc474..36e622d 100644
@@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
 *aerdev,
 	} else if (info->severity == AER_NONFATAL)
 		pcie_do_nonfatal_recovery(dev);
 	else if (info->severity == AER_FATAL)
-		pcie_do_fatal_recovery(dev);
+		pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
 }
 

#ifdef CONFIG_ACPI_APEI_PCIEAER

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

@@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
 *work)
 		if (entry.severity == AER_NONFATAL)
 			pcie_do_nonfatal_recovery(pdev);
 		else if (entry.severity == AER_FATAL)
-			pcie_do_fatal_recovery(pdev);
+			pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
 		pci_dev_put(pdev);
 	}

}

drivers/pci/pcie/dpc.c +22 -20

index 80ec384..5680c13 100644
@@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
 *dpc)
 	pcie_wait_for_link(pdev, false);
 }
 
-static void dpc_work(struct work_struct *work)
+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;
-	u16 cap = dpc->cap_pos, ctl;
-
-	pci_lock_rescan_remove();
-	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
-					 bus_list) {
-		pci_dev_get(dev);
-		pci_dev_set_disconnected(dev, NULL);
-		if (pci_has_subordinate(dev))
-			pci_walk_bus(dev->subordinate,
-				     pci_dev_set_disconnected, NULL);
-		pci_stop_and_remove_bus_device(dev);
-		pci_dev_put(dev);
-	}
-	pci_unlock_rescan_remove();
-
+	struct dpc_dev *dpc;
+	struct pcie_device *pciedev;
+	struct device *devdpc;
+	u16 cap, ctl;
+
+	/*
+	 * DPC disables the Link automatically in hardware, so it has
+	 * already been reset by the time we get here.
+	 */
+
+	devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
+	pciedev = to_pcie_device(devdpc);
+	dpc = get_service_data(pciedev);
+	cap = dpc->cap_pos;
+
+	/*
+	 * Waiting until the link is inactive, then clearing DPC
+	 * trigger status to allow the port to leave DPC.
+	 */
 	dpc_wait_link_inactive(dpc);
+
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
-		return;
+		return PCI_ERS_RESULT_DISCONNECT;
 	if (dpc->rp_extensions && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,

drivers/pci/pcie/dpc.c +12 -0

 				       dpc->rp_pio_status);

@@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void dpc_work(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. */
+	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)

@@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
 	.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/err.c +1 -1

index 33a16b1..29ff148 100644
@@ -185,7 +185,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, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	pci_ers_result_t status;

@@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
 *dev)
 	}
 
 	/* Use the aer driver of the component firstly */
-	driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
+	driver = pcie_port_find_service(udev, service);
 
 	if (driver && driver->reset_link) {

drivers/pci/pcie/err.c +1 -1

 		status = driver->reset_link(udev);

@@ -287,7 +287,7 @@ static pci_ers_result_t
 broadcast_error_message(struct pci_dev *dev,
  * followed by re-enumeration of devices.
  */
 
-void pcie_do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	struct pci_bus *parent;

@@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
 		pci_dev_put(pdev);
 	}
 
-	result = reset_link(udev);
+	result = reset_link(udev, service);
 

Quoted 11 lines

include/linux/aer.h line 17

Quoted 11 lines

drivers/pci/pcie/err.c +0 -0

 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 		/*

include/linux/aer.h +1 -0

index 8f87bbe..0c506fe 100644
@@ -14,6 +14,7 @@
 #define AER_NONFATAL			0
 #define AER_FATAL			1
 #define AER_CORRECTABLE			2
+#define DPC_FATAL			4

I think DPC_FATAL can be 3, since these values are not used as a bit mask.

Quoted 18 lines

include/linux/aer.h line 19

Quoted 1 line

include/linux/aer.h +0 -0

 struct pci_dev;

Hi Bjorn,

I have addressed all the comments, and I hope we are heading towards closure.

I just figure that pcie_do_fatal_recovery (which is getting executed for DPC as well)

if (result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

Quoted 7 lines

I have to correct it to

if (service==AER  && result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

This patch is mostly a restructuring of DPC and doesn't really change its behavior. DPC didn't previously call pcie_wait_for_link() or pci_rescan_bus(), so I agree that adding the "service==AER" test will help preserve the existing DPC behavior.

However, the rescan should happen with DPC *somewhere* and we should clarify where that is. Maybe for now we only need a comment about where that happens. Ideally, we could eventually converge this so the same mechanism is used for AER and DPC, so we wouldn't need a test like "service=AER".

Quoted 3 lines

rest of the things look okay to me.
If you have any more comments,
I can fix them with this one and hopefully v17 will be the final one.

Quoted 62 lines

PS: I am going though the code more, and we can have some follow up patches (probably some cleanup)
for e.g. pcie_portdrv_slot_reset() checks

if (dev->error_state == pci_channel_io_frozen) {
		dev->state_saved = true;
		pci_restore_state(dev);
		pcie_portdrv_restore_config(dev);
		pci_enable_pcie_error_reporting(dev);
	}

but now since the ERR_FATAL path does not call pcie_portdrv_slot_reset the check is meaning less.

besides driver's shut_down callbacks might want to handle pci_channel_io_frozen, but that something left to be discussed later. So, I am not touching dev->error_state anywhere as of now in ERR_FATAL case.

Regards,
Oza.

Quoted 220 lines · Bjorn Helgaas

Quoted 171 lines · On Fri, May 11, 2018 at 05:22:08PM +0530, [email protected] wrote:

Quoted 170 lines · Oza Pawandeep

DPC driver implements link_reset callback, and calls pci_do_fatal_recovery().

Which follows standard path of ERR_FATAL recovery.

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

drivers/pci/pci.h +1 -1

index 5e8857a..6af7595 100644
@@ -354,7 +354,7 @@ 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 pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
 void pcie_do_nonfatal_recovery(struct pci_dev *dev);
 

bool pcie_wait_for_link(struct pci_dev *pdev, bool active);

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

index fdfc474..36e622d 100644
@@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
 *aerdev,
 	} else if (info->severity == AER_NONFATAL)
 		pcie_do_nonfatal_recovery(dev);
 	else if (info->severity == AER_FATAL)
-		pcie_do_fatal_recovery(dev);
+		pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
 }
 

#ifdef CONFIG_ACPI_APEI_PCIEAER

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

@@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
 *work)
 		if (entry.severity == AER_NONFATAL)
 			pcie_do_nonfatal_recovery(pdev);
 		else if (entry.severity == AER_FATAL)
-			pcie_do_fatal_recovery(pdev);
+			pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
 		pci_dev_put(pdev);
 	}

}

drivers/pci/pcie/dpc.c +22 -20

index 80ec384..5680c13 100644
@@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
 *dpc)
 	pcie_wait_for_link(pdev, false);
 }
 
-static void dpc_work(struct work_struct *work)
+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;
-	u16 cap = dpc->cap_pos, ctl;
-
-	pci_lock_rescan_remove();
-	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
-					 bus_list) {
-		pci_dev_get(dev);
-		pci_dev_set_disconnected(dev, NULL);
-		if (pci_has_subordinate(dev))
-			pci_walk_bus(dev->subordinate,
-				     pci_dev_set_disconnected, NULL);
-		pci_stop_and_remove_bus_device(dev);
-		pci_dev_put(dev);
-	}
-	pci_unlock_rescan_remove();
-
+	struct dpc_dev *dpc;
+	struct pcie_device *pciedev;
+	struct device *devdpc;
+	u16 cap, ctl;
+
+	/*
+	 * DPC disables the Link automatically in hardware, so it has
+	 * already been reset by the time we get here.
+	 */
+
+	devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
+	pciedev = to_pcie_device(devdpc);
+	dpc = get_service_data(pciedev);
+	cap = dpc->cap_pos;
+
+	/*
+	 * Waiting until the link is inactive, then clearing DPC
+	 * trigger status to allow the port to leave DPC.
+	 */
 	dpc_wait_link_inactive(dpc);
+
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
-		return;
+		return PCI_ERS_RESULT_DISCONNECT;
 	if (dpc->rp_extensions && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,

drivers/pci/pcie/dpc.c +12 -0

 				       dpc->rp_pio_status);

@@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void dpc_work(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. */
+	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)

@@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
 	.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/err.c +1 -1

index 33a16b1..29ff148 100644
@@ -185,7 +185,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, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	pci_ers_result_t status;

@@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
 *dev)
 	}
 
 	/* Use the aer driver of the component firstly */
-	driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
+	driver = pcie_port_find_service(udev, service);
 
 	if (driver && driver->reset_link) {

drivers/pci/pcie/err.c +1 -1

 		status = driver->reset_link(udev);

@@ -287,7 +287,7 @@ static pci_ers_result_t
 broadcast_error_message(struct pci_dev *dev,
  * followed by re-enumeration of devices.
  */
 
-void pcie_do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	struct pci_bus *parent;

@@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
 		pci_dev_put(pdev);
 	}
 
-	result = reset_link(udev);
+	result = reset_link(udev, service);
 

Quoted 11 lines

include/linux/aer.h line 17

Quoted 11 lines

drivers/pci/pcie/err.c +0 -0

 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 		/*

include/linux/aer.h +1 -0

index 8f87bbe..0c506fe 100644
@@ -14,6 +14,7 @@
 #define AER_NONFATAL			0
 #define AER_FATAL			1
 #define AER_CORRECTABLE			2
+#define DPC_FATAL			4

I think DPC_FATAL can be 3, since these values are not used as a bit mask.

Quoted 19 lines

include/linux/aer.h line 19

Quoted 1 line

include/linux/aer.h +0 -0

 struct pci_dev;

Hi Bjorn,

I have addressed all the comments, and I hope we are heading towards closure.

I just figure that pcie_do_fatal_recovery (which is getting executed for
DPC as well)

if (result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

Quoted 7 lines

I have to correct it to

if (service==AER  && result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

This patch is mostly a restructuring of DPC and doesn't really change its
behavior. DPC didn't previously call pcie_wait_for_link() or pci_rescan_bus(), so I agree that adding the "service==AER" test will help preserve the existing DPC behavior.

Quoted 5 lines

However, the rescan should happen with DPC *somewhere* and we should clarify where that is. Maybe for now we only need a comment about where that happens. Ideally, we could eventually converge this so the same mechanism is used for AER and DPC, so we wouldn't need a test like "service=AER".

I am sorry I pasted the wrong snippet.
following needs to be fixed in v17.
from:
if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
/*
* If the error is reported by a bridge, we think this error
* is related to the downstream link of the bridge, so we
* do error recovery on all subordinates of the bridge
instead

                 * of the bridge and clear the error status of the bridge.
                 */
                pci_walk_bus(dev->subordinate, report_resume, &result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

to

if (service==AER && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
/*
* If the error is reported by a bridge, we think this error
* is related to the downstream link of the bridge, so we
* do error recovery on all subordinates of the bridge
instead

                 * of the bridge and clear the error status of the bridge.
                 */
                pci_walk_bus(dev->subordinate, report_resume, &result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

this is only needed in case of AER.

by the way I can not find pci/oza-v16 branch when I cloned
git clone
https://kernel.googlesource.com/pub/scm/linux/kernel/git/helgaas/pci

Quoted 221 lines · On Wed, May 16, 2018 at 01:46:25PM +0530, [email protected] wrote:

Quoted 220 lines · Bjorn Helgaas

Quoted 171 lines · On Fri, May 11, 2018 at 05:22:08PM +0530, [email protected] wrote:

Quoted 170 lines · Oza Pawandeep

DPC driver implements link_reset callback, and calls pci_do_fatal_recovery().

Which follows standard path of ERR_FATAL recovery.

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

drivers/pci/pci.h +1 -1

index 5e8857a..6af7595 100644
@@ -354,7 +354,7 @@ 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 pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
 void pcie_do_nonfatal_recovery(struct pci_dev *dev);
 

bool pcie_wait_for_link(struct pci_dev *pdev, bool active);

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

index fdfc474..36e622d 100644
@@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
 *aerdev,
 	} else if (info->severity == AER_NONFATAL)
 		pcie_do_nonfatal_recovery(dev);
 	else if (info->severity == AER_FATAL)
-		pcie_do_fatal_recovery(dev);
+		pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
 }
 

#ifdef CONFIG_ACPI_APEI_PCIEAER

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

@@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
 *work)
 		if (entry.severity == AER_NONFATAL)
 			pcie_do_nonfatal_recovery(pdev);
 		else if (entry.severity == AER_FATAL)
-			pcie_do_fatal_recovery(pdev);
+			pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
 		pci_dev_put(pdev);
 	}

}

drivers/pci/pcie/dpc.c +22 -20

index 80ec384..5680c13 100644
@@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
 *dpc)
 	pcie_wait_for_link(pdev, false);
 }
 
-static void dpc_work(struct work_struct *work)
+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;
-	u16 cap = dpc->cap_pos, ctl;
-
-	pci_lock_rescan_remove();
-	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
-					 bus_list) {
-		pci_dev_get(dev);
-		pci_dev_set_disconnected(dev, NULL);
-		if (pci_has_subordinate(dev))
-			pci_walk_bus(dev->subordinate,
-				     pci_dev_set_disconnected, NULL);
-		pci_stop_and_remove_bus_device(dev);
-		pci_dev_put(dev);
-	}
-	pci_unlock_rescan_remove();
-
+	struct dpc_dev *dpc;
+	struct pcie_device *pciedev;
+	struct device *devdpc;
+	u16 cap, ctl;
+
+	/*
+	 * DPC disables the Link automatically in hardware, so it has
+	 * already been reset by the time we get here.
+	 */
+
+	devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
+	pciedev = to_pcie_device(devdpc);
+	dpc = get_service_data(pciedev);
+	cap = dpc->cap_pos;
+
+	/*
+	 * Waiting until the link is inactive, then clearing DPC
+	 * trigger status to allow the port to leave DPC.
+	 */
 	dpc_wait_link_inactive(dpc);
+
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
-		return;
+		return PCI_ERS_RESULT_DISCONNECT;
 	if (dpc->rp_extensions && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,

drivers/pci/pcie/dpc.c +12 -0

 				       dpc->rp_pio_status);

@@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void dpc_work(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. */
+	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)

@@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
 	.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/err.c +1 -1

index 33a16b1..29ff148 100644
@@ -185,7 +185,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, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	pci_ers_result_t status;

@@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
 *dev)
 	}
 
 	/* Use the aer driver of the component firstly */
-	driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
+	driver = pcie_port_find_service(udev, service);
 
 	if (driver && driver->reset_link) {

drivers/pci/pcie/err.c +1 -1

 		status = driver->reset_link(udev);

@@ -287,7 +287,7 @@ static pci_ers_result_t
 broadcast_error_message(struct pci_dev *dev,
  * followed by re-enumeration of devices.
  */
 
-void pcie_do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	struct pci_bus *parent;

@@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
 		pci_dev_put(pdev);
 	}
 
-	result = reset_link(udev);
+	result = reset_link(udev, service);
 

Quoted 11 lines

include/linux/aer.h line 17

Quoted 11 lines

drivers/pci/pcie/err.c +0 -0

 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 		/*

include/linux/aer.h +1 -0

index 8f87bbe..0c506fe 100644
@@ -14,6 +14,7 @@
 #define AER_NONFATAL			0
 #define AER_FATAL			1
 #define AER_CORRECTABLE			2
+#define DPC_FATAL			4

I think DPC_FATAL can be 3, since these values are not used as a bit mask.

Quoted 19 lines

include/linux/aer.h line 19

Quoted 1 line

include/linux/aer.h +0 -0

 struct pci_dev;

Hi Bjorn,

I have addressed all the comments, and I hope we are heading towards closure.

I just figure that pcie_do_fatal_recovery (which is getting executed for
DPC as well)

if (result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

Quoted 7 lines

I have to correct it to

if (service==AER  && result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

This patch is mostly a restructuring of DPC and doesn't really change its
behavior. DPC didn't previously call pcie_wait_for_link() or pci_rescan_bus(), so I agree that adding the "service==AER" test will help preserve the existing DPC behavior.

Quoted 5 lines

Quoted 5 lines

However, the rescan should happen with DPC *somewhere* and we should clarify where that is. Maybe for now we only need a comment about where that happens. Ideally, we could eventually converge this so the same mechanism is used for AER and DPC, so we wouldn't need a test like "service=AER".

Please still add a comment here about why the rescan behavior is different.

Quoted 32 lines

I am sorry I pasted the wrong snippet.
following needs to be fixed in v17.
from:

   if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think this 

error
* is related to the downstream link of the bridge, so
we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

                 */
                pci_walk_bus(dev->subordinate, report_resume, 
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

to

   if (service==AER  && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think this 

error
* is related to the downstream link of the bridge, so
we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

Quoted 7 lines

                 */
                pci_walk_bus(dev->subordinate, report_resume, 
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

this is only needed in case of AER.

Oh, I missed this before. It makes sense to clear the AER status here, but why do we need to call report_resume()? We just called all the driver .remove() methods and detached the drivers from the devices. So I don't think report_resume() will do anything ("dev->driver" should be NULL) except set the dev->error_state to pci_channel_io_normal. We probably don't need that because we didn't change error_state in this fatal error path.

Quoted 3 lines

by the way I can not find pci/oza-v16 branch when I cloned
git clone
https://kernel.googlesource.com/pub/scm/linux/kernel/git/helgaas/pci

Sorry, I must have forgotten to push it. It's there now at

https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/oza-v16

I don't know anything about the googlesource mirror, so I don't know when it will get there.

Quoted 271 lines · Bjorn Helgaas

Quoted 221 lines · On Wed, May 16, 2018 at 01:46:25PM +0530, [email protected] wrote:

Quoted 220 lines · Bjorn Helgaas

Quoted 171 lines · On Fri, May 11, 2018 at 05:22:08PM +0530, [email protected] wrote:

Quoted 170 lines · Oza Pawandeep

DPC driver implements link_reset callback, and calls pci_do_fatal_recovery().

Which follows standard path of ERR_FATAL recovery.

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

drivers/pci/pci.h +1 -1

index 5e8857a..6af7595 100644
@@ -354,7 +354,7 @@ 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 pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
 void pcie_do_nonfatal_recovery(struct pci_dev *dev);
 

bool pcie_wait_for_link(struct pci_dev *pdev, bool active);

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

index fdfc474..36e622d 100644
@@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
 *aerdev,
 	} else if (info->severity == AER_NONFATAL)
 		pcie_do_nonfatal_recovery(dev);
 	else if (info->severity == AER_FATAL)
-		pcie_do_fatal_recovery(dev);
+		pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
 }
 

#ifdef CONFIG_ACPI_APEI_PCIEAER

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

@@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
 *work)
 		if (entry.severity == AER_NONFATAL)
 			pcie_do_nonfatal_recovery(pdev);
 		else if (entry.severity == AER_FATAL)
-			pcie_do_fatal_recovery(pdev);
+			pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
 		pci_dev_put(pdev);
 	}

}

drivers/pci/pcie/dpc.c +22 -20

index 80ec384..5680c13 100644
@@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
 *dpc)
 	pcie_wait_for_link(pdev, false);
 }
 
-static void dpc_work(struct work_struct *work)
+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;
-	u16 cap = dpc->cap_pos, ctl;
-
-	pci_lock_rescan_remove();
-	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
-					 bus_list) {
-		pci_dev_get(dev);
-		pci_dev_set_disconnected(dev, NULL);
-		if (pci_has_subordinate(dev))
-			pci_walk_bus(dev->subordinate,
-				     pci_dev_set_disconnected, NULL);
-		pci_stop_and_remove_bus_device(dev);
-		pci_dev_put(dev);
-	}
-	pci_unlock_rescan_remove();
-
+	struct dpc_dev *dpc;
+	struct pcie_device *pciedev;
+	struct device *devdpc;
+	u16 cap, ctl;
+
+	/*
+	 * DPC disables the Link automatically in hardware, so it has
+	 * already been reset by the time we get here.
+	 */
+
+	devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
+	pciedev = to_pcie_device(devdpc);
+	dpc = get_service_data(pciedev);
+	cap = dpc->cap_pos;
+
+	/*
+	 * Waiting until the link is inactive, then clearing DPC
+	 * trigger status to allow the port to leave DPC.
+	 */
 	dpc_wait_link_inactive(dpc);
+
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
-		return;
+		return PCI_ERS_RESULT_DISCONNECT;
 	if (dpc->rp_extensions && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,

drivers/pci/pcie/dpc.c +12 -0

 				       dpc->rp_pio_status);

@@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void dpc_work(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. */
+	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)

@@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
 	.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/err.c +1 -1

index 33a16b1..29ff148 100644
@@ -185,7 +185,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, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	pci_ers_result_t status;

@@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
 *dev)
 	}
 
 	/* Use the aer driver of the component firstly */
-	driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
+	driver = pcie_port_find_service(udev, service);
 
 	if (driver && driver->reset_link) {

drivers/pci/pcie/err.c +1 -1

 		status = driver->reset_link(udev);

@@ -287,7 +287,7 @@ static pci_ers_result_t
 broadcast_error_message(struct pci_dev *dev,
  * followed by re-enumeration of devices.
  */
 
-void pcie_do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	struct pci_bus *parent;

@@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
 		pci_dev_put(pdev);
 	}
 
-	result = reset_link(udev);
+	result = reset_link(udev, service);
 

Quoted 11 lines

include/linux/aer.h line 17

Quoted 11 lines

drivers/pci/pcie/err.c +0 -0

 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 		/*

include/linux/aer.h +1 -0

index 8f87bbe..0c506fe 100644
@@ -14,6 +14,7 @@
 #define AER_NONFATAL			0
 #define AER_FATAL			1
 #define AER_CORRECTABLE			2
+#define DPC_FATAL			4

I think DPC_FATAL can be 3, since these values are not used as a bit mask.

Quoted 19 lines

include/linux/aer.h line 19

Quoted 1 line

include/linux/aer.h +0 -0

 struct pci_dev;

Hi Bjorn,

I have addressed all the comments, and I hope we are heading towards closure.

I just figure that pcie_do_fatal_recovery (which is getting executed for
DPC as well)

if (result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

Quoted 7 lines

I have to correct it to

if (service==AER  && result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

This patch is mostly a restructuring of DPC and doesn't really change its
behavior. DPC didn't previously call pcie_wait_for_link() or pci_rescan_bus(), so I agree that adding the "service==AER" test will help preserve the existing DPC behavior.

Quoted 5 lines

Quoted 5 lines

However, the rescan should happen with DPC *somewhere* and we should clarify where that is. Maybe for now we only need a comment about where that happens. Ideally, we could eventually converge this so the same mechanism is used for AER and DPC, so we wouldn't need a test like "service=AER".

Please still add a comment here about why the rescan behavior is different.

Quoted 32 lines

I am sorry I pasted the wrong snippet.
following needs to be fixed in v17.
from:

   if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think

this error
* is related to the downstream link of the bridge,
so we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

                 */
                pci_walk_bus(dev->subordinate, report_resume,
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

to

   if (service==AER  && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think

this error
* is related to the downstream link of the bridge,
so we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

Quoted 7 lines

                 */
                pci_walk_bus(dev->subordinate, report_resume,
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

this is only needed in case of AER.

Quoted 7 lines

Oh, I missed this before. It makes sense to clear the AER status here, but why do we need to call report_resume()? We just called all the driver .remove() methods and detached the drivers from the devices. So I don't think report_resume() will do anything ("dev->driver" should be NULL) except set the dev->error_state to pci_channel_io_normal. We probably don't need that because we didn't change error_state in this fatal error path.

if you remember, the path ends up calling
aer_error_resume

the existing code ends up calling aer_error_resume as follows.

do_recovery(pci_dev)

    broadcast_error_message(..., error_detected, ...)
    if (AER_FATAL)
      reset_link(pci_dev)
        udev = BRIDGE ? pci_dev : pci_dev->bus->self
        driver->reset_link(udev)
          aer_root_reset(udev)
    if (CAN_RECOVER)
      broadcast_error_message(..., mmio_enabled, ...)
    if (NEED_RESET)
      broadcast_error_message(..., slot_reset, ...)
    broadcast_error_message(dev, ..., report_resume, ...)
      if (BRIDGE)
        report_resume
          driver->resume
            pcie_portdrv_err_resume
              device_for_each_child(..., resume_iter)
                resume_iter
                  driver->error_resume
                    aer_error_resume
        pci_cleanup_aer_uncorrect_error_status(pci_dev)       # only if

BRIDGE
pci_write_config_dword(PCI_ERR_UNCOR_STATUS)

hence I think we have to call it in order to clear the root port PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA.
makes sense ?

Quoted 306 lines · On Wed, May 16, 2018 at 05:45:58PM +0530, [email protected] wrote:

Quoted 271 lines · Bjorn Helgaas

Quoted 221 lines · On Wed, May 16, 2018 at 01:46:25PM +0530, [email protected] wrote:

Quoted 220 lines · Bjorn Helgaas

Quoted 171 lines · On Fri, May 11, 2018 at 05:22:08PM +0530, [email protected] wrote:

Quoted 170 lines · Oza Pawandeep

DPC driver implements link_reset callback, and calls pci_do_fatal_recovery().

Which follows standard path of ERR_FATAL recovery.

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

drivers/pci/pci.h +1 -1

index 5e8857a..6af7595 100644
@@ -354,7 +354,7 @@ 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 pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
 void pcie_do_nonfatal_recovery(struct pci_dev *dev);
 

bool pcie_wait_for_link(struct pci_dev *pdev, bool active);

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

index fdfc474..36e622d 100644
@@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
 *aerdev,
 	} else if (info->severity == AER_NONFATAL)
 		pcie_do_nonfatal_recovery(dev);
 	else if (info->severity == AER_FATAL)
-		pcie_do_fatal_recovery(dev);
+		pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
 }
 

#ifdef CONFIG_ACPI_APEI_PCIEAER

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

@@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
 *work)
 		if (entry.severity == AER_NONFATAL)
 			pcie_do_nonfatal_recovery(pdev);
 		else if (entry.severity == AER_FATAL)
-			pcie_do_fatal_recovery(pdev);
+			pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
 		pci_dev_put(pdev);
 	}

}

drivers/pci/pcie/dpc.c +22 -20

index 80ec384..5680c13 100644
@@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
 *dpc)
 	pcie_wait_for_link(pdev, false);
 }
 
-static void dpc_work(struct work_struct *work)
+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;
-	u16 cap = dpc->cap_pos, ctl;
-
-	pci_lock_rescan_remove();
-	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
-					 bus_list) {
-		pci_dev_get(dev);
-		pci_dev_set_disconnected(dev, NULL);
-		if (pci_has_subordinate(dev))
-			pci_walk_bus(dev->subordinate,
-				     pci_dev_set_disconnected, NULL);
-		pci_stop_and_remove_bus_device(dev);
-		pci_dev_put(dev);
-	}
-	pci_unlock_rescan_remove();
-
+	struct dpc_dev *dpc;
+	struct pcie_device *pciedev;
+	struct device *devdpc;
+	u16 cap, ctl;
+
+	/*
+	 * DPC disables the Link automatically in hardware, so it has
+	 * already been reset by the time we get here.
+	 */
+
+	devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
+	pciedev = to_pcie_device(devdpc);
+	dpc = get_service_data(pciedev);
+	cap = dpc->cap_pos;
+
+	/*
+	 * Waiting until the link is inactive, then clearing DPC
+	 * trigger status to allow the port to leave DPC.
+	 */
 	dpc_wait_link_inactive(dpc);
+
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
-		return;
+		return PCI_ERS_RESULT_DISCONNECT;
 	if (dpc->rp_extensions && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,

drivers/pci/pcie/dpc.c +12 -0

 				       dpc->rp_pio_status);

@@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void dpc_work(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. */
+	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)

@@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
 	.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/err.c +1 -1

index 33a16b1..29ff148 100644
@@ -185,7 +185,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, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	pci_ers_result_t status;

@@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
 *dev)
 	}
 
 	/* Use the aer driver of the component firstly */
-	driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
+	driver = pcie_port_find_service(udev, service);
 
 	if (driver && driver->reset_link) {

drivers/pci/pcie/err.c +1 -1

 		status = driver->reset_link(udev);

@@ -287,7 +287,7 @@ static pci_ers_result_t
 broadcast_error_message(struct pci_dev *dev,
  * followed by re-enumeration of devices.
  */
 
-void pcie_do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	struct pci_bus *parent;

@@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
 		pci_dev_put(pdev);
 	}
 
-	result = reset_link(udev);
+	result = reset_link(udev, service);
 

Quoted 11 lines

include/linux/aer.h line 17

Quoted 11 lines

drivers/pci/pcie/err.c +0 -0

 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 		/*

include/linux/aer.h +1 -0

index 8f87bbe..0c506fe 100644
@@ -14,6 +14,7 @@
 #define AER_NONFATAL			0
 #define AER_FATAL			1
 #define AER_CORRECTABLE			2
+#define DPC_FATAL			4

I think DPC_FATAL can be 3, since these values are not used as a bit mask.

Quoted 19 lines

include/linux/aer.h line 18

Quoted 1 line

include/linux/aer.h +0 -0

 struct pci_dev;

Hi Bjorn,

I have addressed all the comments, and I hope we are heading towards closure.

I just figure that pcie_do_fatal_recovery (which is getting executed for
DPC as well)

if (result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

Quoted 7 lines

I have to correct it to

if (service==AER  && result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

This patch is mostly a restructuring of DPC and doesn't really change its
behavior. DPC didn't previously call pcie_wait_for_link() or pci_rescan_bus(), so I agree that adding the "service==AER" test will help preserve the existing DPC behavior.

Quoted 5 lines

Quoted 5 lines

However, the rescan should happen with DPC *somewhere* and we should clarify where that is. Maybe for now we only need a comment about where that happens. Ideally, we could eventually converge this so the same mechanism is used for AER and DPC, so we wouldn't need a test like "service=AER".

Please still add a comment here about why the rescan behavior is different.

Quoted 32 lines

I am sorry I pasted the wrong snippet.
following needs to be fixed in v17.
from:

   if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think

this error
* is related to the downstream link of the bridge,
so we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

                 */
                pci_walk_bus(dev->subordinate, report_resume,
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

to

   if (service==AER  && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think

this error
* is related to the downstream link of the bridge,
so we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

Quoted 7 lines

                 */
                pci_walk_bus(dev->subordinate, report_resume,
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

this is only needed in case of AER.

Quoted 7 lines

Oh, I missed this before. It makes sense to clear the AER status here, but why do we need to call report_resume()? We just called all the driver .remove() methods and detached the drivers from the devices. So I don't think report_resume() will do anything ("dev->driver" should be NULL) except set the dev->error_state to pci_channel_io_normal. We probably don't need that because we didn't change error_state in this fatal error path.

if you remember, the path ends up calling
aer_error_resume

the existing code ends up calling aer_error_resume as follows.

do_recovery(pci_dev)

    broadcast_error_message(..., error_detected, ...)
    if (AER_FATAL)
      reset_link(pci_dev)
        udev = BRIDGE ? pci_dev : pci_dev->bus->self
        driver->reset_link(udev)
          aer_root_reset(udev)
    if (CAN_RECOVER)
      broadcast_error_message(..., mmio_enabled, ...)
    if (NEED_RESET)
      broadcast_error_message(..., slot_reset, ...)
    broadcast_error_message(dev, ..., report_resume, ...)
      if (BRIDGE)
        report_resume
          driver->resume
            pcie_portdrv_err_resume
              device_for_each_child(..., resume_iter)
                resume_iter
                  driver->error_resume
                    aer_error_resume
        pci_cleanup_aer_uncorrect_error_status(pci_dev)       # only 

Quoted 7 lines

if
BRIDGE
pci_write_config_dword(PCI_ERR_UNCOR_STATUS)

hence I think we have to call it in order to clear the root port PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA.
makes sense ?

I know I sent you the call graph above, so you would think I might understand it, but you would be mistaken :) This still doesn't make sense to me.

I think your point is that we need to call aer_error_resume(). That is the aerdriver.error_resume() method. The AER driver only binds to root ports.

This path:

pcie_do_fatal_recovery

    pci_walk_bus(dev->subordinate, report_resume, &result_data)

calls report_resume() for every device on the dev->subordinate bus (and for anything below those devices). There are no root ports on that dev->subordinate bus, because root ports are always on a root bus, never on a subordinate bus.

So I don't see how report_resume() can ever get to aer_error_resume(). Can you instrument that path and verify that it actually does get there somehow?

Quoted 306 lines · On Wed, May 16, 2018 at 05:45:58PM +0530, [email protected] wrote:

Quoted 271 lines · Bjorn Helgaas

Quoted 221 lines · On Wed, May 16, 2018 at 01:46:25PM +0530, [email protected] wrote:

Quoted 220 lines · Bjorn Helgaas

Quoted 171 lines · On Fri, May 11, 2018 at 05:22:08PM +0530, [email protected] wrote:

Quoted 170 lines · Oza Pawandeep

DPC driver implements link_reset callback, and calls pci_do_fatal_recovery().

Which follows standard path of ERR_FATAL recovery.

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

drivers/pci/pci.h +1 -1

index 5e8857a..6af7595 100644
@@ -354,7 +354,7 @@ 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 pcie_do_fatal_recovery(struct pci_dev *dev);
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
 void pcie_do_nonfatal_recovery(struct pci_dev *dev);
 

bool pcie_wait_for_link(struct pci_dev *pdev, bool active);

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

index fdfc474..36e622d 100644
@@ -254,7 +254,7 @@ static void handle_error_source(struct pcie_device
 *aerdev,
 	} else if (info->severity == AER_NONFATAL)
 		pcie_do_nonfatal_recovery(dev);
 	else if (info->severity == AER_FATAL)
-		pcie_do_fatal_recovery(dev);
+		pcie_do_fatal_recovery(dev, PCIE_PORT_SERVICE_AER);
 }
 

#ifdef CONFIG_ACPI_APEI_PCIEAER

drivers/pci/pcie/aer/aerdrv_core.c +1 -1

@@ -321,7 +321,7 @@ static void aer_recover_work_func(struct work_struct
 *work)
 		if (entry.severity == AER_NONFATAL)
 			pcie_do_nonfatal_recovery(pdev);
 		else if (entry.severity == AER_FATAL)
-			pcie_do_fatal_recovery(pdev);
+			pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_AER);
 		pci_dev_put(pdev);
 	}

}

drivers/pci/pcie/dpc.c +22 -20

index 80ec384..5680c13 100644
@@ -73,29 +73,31 @@ static void dpc_wait_link_inactive(struct dpc_dev
 *dpc)
 	pcie_wait_for_link(pdev, false);
 }
 
-static void dpc_work(struct work_struct *work)
+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;
-	u16 cap = dpc->cap_pos, ctl;
-
-	pci_lock_rescan_remove();
-	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
-					 bus_list) {
-		pci_dev_get(dev);
-		pci_dev_set_disconnected(dev, NULL);
-		if (pci_has_subordinate(dev))
-			pci_walk_bus(dev->subordinate,
-				     pci_dev_set_disconnected, NULL);
-		pci_stop_and_remove_bus_device(dev);
-		pci_dev_put(dev);
-	}
-	pci_unlock_rescan_remove();
-
+	struct dpc_dev *dpc;
+	struct pcie_device *pciedev;
+	struct device *devdpc;
+	u16 cap, ctl;
+
+	/*
+	 * DPC disables the Link automatically in hardware, so it has
+	 * already been reset by the time we get here.
+	 */
+
+	devdpc = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_DPC);
+	pciedev = to_pcie_device(devdpc);
+	dpc = get_service_data(pciedev);
+	cap = dpc->cap_pos;
+
+	/*
+	 * Waiting until the link is inactive, then clearing DPC
+	 * trigger status to allow the port to leave DPC.
+	 */
 	dpc_wait_link_inactive(dpc);
+
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
-		return;
+		return PCI_ERS_RESULT_DISCONNECT;
 	if (dpc->rp_extensions && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,

drivers/pci/pcie/dpc.c +12 -0

 				       dpc->rp_pio_status);

@@ -108,6 +110,17 @@ static void dpc_work(struct work_struct *work)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void dpc_work(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. */
+	pcie_do_fatal_recovery(pdev, PCIE_PORT_SERVICE_DPC);
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)

@@ -288,6 +301,7 @@ static struct pcie_port_service_driver dpcdriver = {
 	.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/err.c +1 -1

index 33a16b1..29ff148 100644
@@ -185,7 +185,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, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	pci_ers_result_t status;

@@ -200,7 +200,7 @@ static pci_ers_result_t reset_link(struct pci_dev
 *dev)
 	}
 
 	/* Use the aer driver of the component firstly */
-	driver = pcie_port_find_service(udev, PCIE_PORT_SERVICE_AER);
+	driver = pcie_port_find_service(udev, service);
 
 	if (driver && driver->reset_link) {

drivers/pci/pcie/err.c +1 -1

 		status = driver->reset_link(udev);

@@ -287,7 +287,7 @@ static pci_ers_result_t
 broadcast_error_message(struct pci_dev *dev,
  * followed by re-enumeration of devices.
  */
 
-void pcie_do_fatal_recovery(struct pci_dev *dev)
+void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 {
 	struct pci_dev *udev;

drivers/pci/pcie/err.c +1 -1

 	struct pci_bus *parent;

@@ -313,7 +313,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev)
 		pci_dev_put(pdev);
 	}
 
-	result = reset_link(udev);
+	result = reset_link(udev, service);
 

Quoted 11 lines

include/linux/aer.h line 17

Quoted 11 lines

drivers/pci/pcie/err.c +0 -0

 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 		/*

include/linux/aer.h +1 -0

index 8f87bbe..0c506fe 100644
@@ -14,6 +14,7 @@
 #define AER_NONFATAL			0
 #define AER_FATAL			1
 #define AER_CORRECTABLE			2
+#define DPC_FATAL			4

I think DPC_FATAL can be 3, since these values are not used as a bit mask.

Quoted 19 lines

include/linux/aer.h line 18

Quoted 1 line

include/linux/aer.h +0 -0

 struct pci_dev;

Hi Bjorn,

I have addressed all the comments, and I hope we are heading towards closure.

I just figure that pcie_do_fatal_recovery (which is getting executed for
DPC as well)

if (result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

Quoted 7 lines

I have to correct it to

if (service==AER  && result == PCI_ERS_RESULT_RECOVERED) {
                if (pcie_wait_for_link(udev, true))
                        pci_rescan_bus(udev->bus);
                pci_info(dev, "Device recovery successful\n");
        }

This patch is mostly a restructuring of DPC and doesn't really change its
behavior. DPC didn't previously call pcie_wait_for_link() or pci_rescan_bus(), so I agree that adding the "service==AER" test will help preserve the existing DPC behavior.

Quoted 5 lines

Quoted 5 lines

However, the rescan should happen with DPC *somewhere* and we should clarify where that is. Maybe for now we only need a comment about where that happens. Ideally, we could eventually converge this so the same mechanism is used for AER and DPC, so we wouldn't need a test like "service=AER".

Please still add a comment here about why the rescan behavior is different.

Quoted 32 lines

I am sorry I pasted the wrong snippet.
following needs to be fixed in v17.
from:

   if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think

this error
* is related to the downstream link of the bridge,
so we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

                 */
                pci_walk_bus(dev->subordinate, report_resume,
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

to

   if (service==AER  && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think

this error
* is related to the downstream link of the bridge,
so we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

Quoted 7 lines

                 */
                pci_walk_bus(dev->subordinate, report_resume,
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

this is only needed in case of AER.

Quoted 7 lines

Oh, I missed this before. It makes sense to clear the AER status here, but why do we need to call report_resume()? We just called all the driver .remove() methods and detached the drivers from the devices. So I don't think report_resume() will do anything ("dev->driver" should be NULL) except set the dev->error_state to pci_channel_io_normal. We probably don't need that because we didn't change error_state in this fatal error path.

if you remember, the path ends up calling
aer_error_resume

the existing code ends up calling aer_error_resume as follows.

do_recovery(pci_dev)

    broadcast_error_message(..., error_detected, ...)
    if (AER_FATAL)
      reset_link(pci_dev)
        udev = BRIDGE ? pci_dev : pci_dev->bus->self
        driver->reset_link(udev)
          aer_root_reset(udev)
    if (CAN_RECOVER)
      broadcast_error_message(..., mmio_enabled, ...)
    if (NEED_RESET)
      broadcast_error_message(..., slot_reset, ...)
    broadcast_error_message(dev, ..., report_resume, ...)
      if (BRIDGE)
        report_resume
          driver->resume
            pcie_portdrv_err_resume
              device_for_each_child(..., resume_iter)
                resume_iter
                  driver->error_resume
                    aer_error_resume
        pci_cleanup_aer_uncorrect_error_status(pci_dev)       # only 

Quoted 7 lines

if
BRIDGE
pci_write_config_dword(PCI_ERR_UNCOR_STATUS)

hence I think we have to call it in order to clear the root port PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA.
makes sense ?

I know I sent you the call graph above, so you would think I might understand it, but you would be mistaken :) This still doesn't make sense to me.

I think your point is that we need to call aer_error_resume(). That is the aerdriver.error_resume() method. The AER driver only binds to root ports.

This path:

pcie_do_fatal_recovery

    pci_walk_bus(dev->subordinate, report_resume, &result_data)

calls report_resume() for every device on the dev->subordinate bus (and for anything below those devices). There are no root ports on that dev->subordinate bus, because root ports are always on a root bus, never on a subordinate bus.

So I don't see how report_resume() can ever get to aer_error_resume(). Can you instrument that path and verify that it actually does get there somehow?

Quoted 99 lines · Bjorn Helgaas

Quoted 77 lines · On Wed, May 16, 2018 at 05:45:58PM +0530, [email protected] wrote:

Quoted 42 lines · Bjorn Helgaas

Quoted 32 lines · On Wed, May 16, 2018 at 01:46:25PM +0530, [email protected] wrote:

I am sorry I pasted the wrong snippet.
following needs to be fixed in v17.
from:

   if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think

this error
* is related to the downstream link of the bridge,
so we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

                 */
                pci_walk_bus(dev->subordinate, report_resume,
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

to

   if (service==AER  && dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
                /*
                 * If the error is reported by a bridge, we think

this error
* is related to the downstream link of the bridge,
so we
* do error recovery on all subordinates of the bridge
instead
* of the bridge and clear the error status of the
bridge.

Quoted 7 lines

                 */
                pci_walk_bus(dev->subordinate, report_resume,
&result_data);
                pci_cleanup_aer_uncorrect_error_status(dev);
        }

this is only needed in case of AER.

Quoted 7 lines

Oh, I missed this before. It makes sense to clear the AER status here, but why do we need to call report_resume()? We just called all the driver .remove() methods and detached the drivers from the devices. So I don't think report_resume() will do anything ("dev->driver" should be NULL) except set the dev->error_state to pci_channel_io_normal. We probably don't need that because we didn't change error_state in this fatal error path.

if you remember, the path ends up calling
aer_error_resume

the existing code ends up calling aer_error_resume as follows.

do_recovery(pci_dev)

    broadcast_error_message(..., error_detected, ...)
    if (AER_FATAL)
      reset_link(pci_dev)
        udev = BRIDGE ? pci_dev : pci_dev->bus->self
        driver->reset_link(udev)
          aer_root_reset(udev)
    if (CAN_RECOVER)
      broadcast_error_message(..., mmio_enabled, ...)
    if (NEED_RESET)
      broadcast_error_message(..., slot_reset, ...)
    broadcast_error_message(dev, ..., report_resume, ...)
      if (BRIDGE)
        report_resume
          driver->resume
            pcie_portdrv_err_resume
              device_for_each_child(..., resume_iter)
                resume_iter
                  driver->error_resume
                    aer_error_resume
        pci_cleanup_aer_uncorrect_error_status(pci_dev)       # only

Quoted 7 lines

if
BRIDGE
pci_write_config_dword(PCI_ERR_UNCOR_STATUS)

hence I think we have to call it in order to clear the root port PCI_ERR_UNCOR_STATUS and PCI_EXP_DEVSTA.
makes sense ?

I know I sent you the call graph above, so you would think I might understand it, but you would be mistaken :) This still doesn't make sense to me.

I think your point is that we need to call aer_error_resume(). That is the aerdriver.error_resume() method. The AER driver only binds to root ports.

This path:

pcie_do_fatal_recovery

    pci_walk_bus(dev->subordinate, report_resume, &result_data)

Quoted 8 lines

calls report_resume() for every device on the dev->subordinate bus (and for anything below those devices). There are no root ports on that dev->subordinate bus, because root ports are always on a root bus, never on a subordinate bus.

So I don't see how report_resume() can ever get to aer_error_resume(). Can you instrument that path and verify that it actually does get there somehow?

you are right....the call pci_walk_bus(dev->subordinate, report_resume, &result_data); does not call aer_error_resume()

but
pci_walk_bus(udev->bus, report_resume, &result_data); does call aer_error_resume()

now if you look at the comment of the function:
/**
* aer_error_resume - clean up corresponding error status bits
* @dev: pointer to Root Port's pci_dev data structure *
* Invoked by Port Bus driver during nonfatal recovery. */

it is invoked during nonfatal recovery.
but the code handles both fatal and nonfatal clearing of error bits.

if (dev->error_state == pci_channel_io_normal)
status &= ~mask; /* Clear corresponding nonfatal bits */
else
status &= mask; /* Clear corresponding fatal bits */
pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);