Address error and recovery for AER and DPC

This patch renames error recovery to generic name with pcie prefix

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

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]>

This patch implements generic pcie_port_find_service() routine.

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

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

+static int find_dpc_dev_iter(struct device *device, void *data)
+{
+	struct pcie_port_service_driver *service_driver;
+	struct device **dev;
+
+	dev = (struct device **) data;
+
+	if (device->bus == &pcie_port_bus_type && device->driver) {
+		service_driver = to_service_driver(device->driver);
+		if (service_driver->service == PCIE_PORT_SERVICE_DPC) {
+			*dev = device;
+			return 1;
+		}
+	}
+
+	return 0;

Quoted 21 lines · Oza Pawandeep

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

+static int find_dpc_dev_iter(struct device *device, void *data)
+{
+	struct pcie_port_service_driver *service_driver;
+	struct device **dev;
+
+	dev = (struct device **) data;
+
+	if (device->bus == &pcie_port_bus_type && device->driver) {
+		service_driver = to_service_driver(device->driver);
+		if (service_driver->service == PCIE_PORT_SERVICE_DPC) {
+			*dev = device;
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+static struct device *pci_find_dpc_dev(struct pci_dev *pdev)
+{
+	struct device *dev = NULL;

drivers/pci/pcie/pcie-dpc.c static const char * const rp_pio_error_string[] = { · line 74

Quoted 5 lines

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

+
+	device_for_each_child(&pdev->dev, &dev, find_dpc_dev_iter);
+
+	return dev;
+}

The only caller of this doesn't seem to care to use struct device. This should probably just extract struct dpc_dev directly from in here.

Quoted 22 lines · Keith Busch

Quoted 21 lines · Oza Pawandeep

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

+static int find_dpc_dev_iter(struct device *device, void *data)
+{
+	struct pcie_port_service_driver *service_driver;
+	struct device **dev;
+
+	dev = (struct device **) data;
+
+	if (device->bus == &pcie_port_bus_type && device->driver) {
+		service_driver = to_service_driver(device->driver);
+		if (service_driver->service == PCIE_PORT_SERVICE_DPC) {
+			*dev = device;
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+static struct device *pci_find_dpc_dev(struct pci_dev *pdev)
+{
+	struct device *dev = NULL;

Quoted 9 lines

drivers/pci/pcie/pcie-dpc.c static const char * const rp_pio_error_string[] = { · line 74

Quoted 5 lines

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

+
+	device_for_each_child(&pdev->dev, &dev, find_dpc_dev_iter);
+
+	return dev;
+}

The only caller of this doesn't seem to care to use struct device. This should probably just extract struct dpc_dev directly from in here.

Bjorn wants to kill the port service driver infrastructure but that is a much bigger task.

How do we obtain the DPC object from the parent object directly? Each port service driver object is a children.

drivers/pci/pci.c +12 -0

+/**
+ * pcie_wait_for_link - Wait for link till it's active/inactive
+ * @pdev: Bridge device
+ * @active: waiting for active or inactive ?
+ *
+ * Use this to wait till link becomes active or inactive.
+ */
+bool pcie_wait_for_link(struct pci_dev *pdev, bool active)
+{
+	int timeout = 1000;
+	bool ret;
+	u16 lnk_status;

drivers/pci/pci.c +6 -0

+
+	pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n",
+		 active ? "set" : "cleared");
+
+	return false;
+}

Quoted 16 lines · Oza Pawandeep

drivers/pci/pci.c +16 -0

+/**
+ * pcie_wait_for_link - Wait for link till it's active/inactive
+ * @pdev: Bridge device
+ * @active: waiting for active or inactive ?
+ *
+ * Use this to wait till link becomes active or inactive.
+ */
+bool pcie_wait_for_link(struct pci_dev *pdev, bool active)
+{
+	int timeout = 1000;
+	bool ret;
+	u16 lnk_status;
+
+	for (;;) {
+		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
+		ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);

drivers/pci/pci.c static int pci_pm_reset(struct pci_dev *dev, int probe) · line 4200

Quoted 6 lines

drivers/pci/pci.c +6 -0

+		if (ret == active)
+			return true;
+		if (timeout <= 0)
+			break;
+		timeout -= 10;
+	}

This is missing an msleep(10) at each iteration.

Quoted 7 lines

drivers/pci/pci.c +6 -0

+
+	pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n",
+		 active ? "set" : "cleared");
+
+	return false;
+}

DPC and AER should attempt recovery in the same way, except the cases where system is with hotplug enabled.

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

drivers/pci/pcie/pcie-dpc.c +25 -16

index 8e1553b..6d9a841 100644
@@ -108,8 +108,6 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
  */
 static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 {
-	struct pci_bus *parent = pdev->subordinate;
-	struct pci_dev *dev, *temp;
 	struct dpc_dev *dpc;
 	struct pcie_device *pciedev;
 	struct device *devdpc;

@@ -120,19 +118,6 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 	dpc = get_service_data(pciedev);
 	cap = dpc->cap_pos;
 
-	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();
-
 	dpc_wait_link_inactive(dpc);
 	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
 		return PCI_ERS_RESULT_DISCONNECT;

@@ -152,13 +137,37 @@ static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 	return PCI_ERS_RESULT_RECOVERED;
 }
 
+static void dpc_reset_link_remove_dev(struct pci_dev *pdev)
+{
+	struct pci_bus *parent = pdev->subordinate;
+	struct pci_dev *dev, *temp;
+
+	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();
+
+	dpc_reset_link(pdev);
+}
+
 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_recovery(pdev, DPC_FATAL);
+	if (!pdev->is_hotplug_bridge)
+		pcie_do_recovery(pdev, DPC_FATAL);
+	else
+		dpc_reset_link_remove_dev(pdev);
 }
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 {

Signature · 1 line

2.7.4

Quoted 2 lines

DPC and AER should attempt recovery in the same way, except the cases where system is with hotplug enabled.

What's the connection with hotplug? I see from the patch that for hotplug bridges you remove the tree below the bridge, and otherwise you just reset the secondary link (I think).

The changelog should explain why we need the difference.

I'm a little skeptical to begin with, because I'm not sure why we should handle a DPC event differently just because a bridge has the *capability* of hotplug. Even if a hotplug bridge reports a DPC event, that doesn't necessarily mean a hotplug has occurred.

Quoted 5 lines

Quoted 5 lines

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;

Quoted 7 lines

drivers/pci/pcie/pcie-dpc.c static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev) · line 171

Quoted 7 lines

        /* From DPC point of view error is always FATAL. */

drivers/pci/pcie/pcie-dpc.c +4 -1

-       pcie_do_recovery(pdev, DPC_FATAL);
+       if (!pdev->is_hotplug_bridge)
+               pcie_do_recovery(pdev, DPC_FATAL);
+       else
+               dpc_reset_link_remove_dev(pdev);
 }

Quoted 6 lines · Bjorn Helgaas

Quoted 2 lines

DPC and AER should attempt recovery in the same way, except the cases where system is with hotplug enabled.

What's the connection with hotplug? I see from the patch that for hotplug bridges you remove the tree below the bridge, and otherwise you just reset the secondary link (I think).

Quoted 6 lines

The changelog should explain why we need the difference.

I'm a little skeptical to begin with, because I'm not sure why we should handle a DPC event differently just because a bridge has the *capability* of hotplug. Even if a hotplug bridge reports a DPC event, that doesn't necessarily mean a hotplug has occurred.

Let's do a recap on what we have discussed about this until now.

There are two conflicting error recovery mechanisms for PCIe.

If a system supports both hotplug and DPC, endpoint can be removed and inserted safely. DPC driver shuts down the driver on link down. When link comes back up, hotplug driver takes over and initiates an enumeration process. Keith mentioned the stop and re-enumerate design was chosen because someone could remove a drive and insert an unrelated drive back to the system. We can't really save and restore state as we do in the AER path.

Now, let's assume a system without hotplug capability. Second mechanism is to go through DPC/AER path and do an automatic link down recovery via DPC retrain/secondary bus reset including register save and restore. Second mechanism is more suitable for handling "surprise link down" event. The goal is to retrain the link and continue driver operation.

The goal of this patch to separate these two cases from each other as the DPC driver needs to work on both contexts. Current DPC code doesn't handle the second use case.

[+cc Alex because of his interest in device reset]

For context, here's the part of the patch we're discussing:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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;

Quoted 7 lines

Quoted 7 lines

drivers/pci/pcie/pcie-dpc.c static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev) · line 171

Quoted 7 lines

        /* From DPC point of view error is always FATAL. */

drivers/pci/pcie/pcie-dpc.c +4 -1

-       pcie_do_recovery(pdev, DPC_FATAL);
+       if (!pdev->is_hotplug_bridge)
+               pcie_do_recovery(pdev, DPC_FATAL);
+       else
+               dpc_reset_link_remove_dev(pdev);
 }

This is at the point where DPC has been triggered in the hardware and the DPC driver is starting recovery, and I'm wondering why we need to handle the "!pdev->is_hotplug_bridge" case differently.

Quoted 33 lines · Sinan Kaya

Quoted 6 lines · Bjorn Helgaas

Quoted 2 lines

DPC and AER should attempt recovery in the same way, except the cases where system is with hotplug enabled.

What's the connection with hotplug? I see from the patch that for hotplug bridges you remove the tree below the bridge, and otherwise you just reset the secondary link (I think).

Quoted 6 lines

The changelog should explain why we need the difference.

I'm a little skeptical to begin with, because I'm not sure why we should handle a DPC event differently just because a bridge has the *capability* of hotplug. Even if a hotplug bridge reports a DPC event, that doesn't necessarily mean a hotplug has occurred.

Let's do a recap on what we have discussed about this until now.

There are two conflicting error recovery mechanisms for PCIe.

If a system supports both hotplug and DPC, endpoint can be removed and inserted safely. DPC driver shuts down the driver on link down. When link comes back up, hotplug driver takes over and initiates an enumeration process. Keith mentioned the stop and re-enumerate design was chosen because someone could remove a drive and insert an unrelated drive back to the system. We can't really save and restore state as we do in the AER path.

Now, let's assume a system without hotplug capability. Second mechanism is to go through DPC/AER path and do an automatic link down recovery via DPC retrain/secondary bus reset including register save and restore. Second mechanism is more suitable for handling "surprise link down" event. The goal is to retrain the link and continue driver operation.

Quoted 3 lines

The goal of this patch to separate these two cases from each other as the DPC driver needs to work on both contexts. Current DPC code doesn't handle the second use case.

I think the scenario you are describing is two systems that are identical except that in the first, the endpoint is below a hotplug bridge, while in the second, it's below a non-hotplug bridge. There's no physical hotplug (no drive removed or inserted), and DPC is triggered in both systems.

I suggest that DPC should be handled identically in both systems:

- The PCI core should have the same view of the endpoint: it should be removed and re-added in both cases (or in neither case).

- The endpoint itself should not be able to tell the difference: it should see a link down event, followed by a link retrain, followed by the same sequence of config accesses, etc.

- The endpoint driver should not be able to tell the difference, i.e., we should be calling the same pci_error_handlers callbacks in both cases.

It's true that in the non-hotplug system, pciehp probably won't start re-enumeration, so we might need an alternate path to trigger that.

But that's not what we're doing in this patch. In this patch we're adding a much bigger difference: for hotplug bridges, we stop and remove the hierarchy below the bridge; for non-hotplug bridges, we do the AER-style flow of calling pci_error_handlers callbacks.

Bjorn

Quoted 20 lines · Bjorn Helgaas

I think the scenario you are describing is two systems that are identical except that in the first, the endpoint is below a hotplug bridge, while in the second, it's below a non-hotplug bridge. There's no physical hotplug (no drive removed or inserted), and DPC is triggered in both systems.

I suggest that DPC should be handled identically in both systems:

- The PCI core should have the same view of the endpoint: it should be removed and re-added in both cases (or in neither case).

- The endpoint itself should not be able to tell the difference: it should see a link down event, followed by a link retrain, followed by the same sequence of config accesses, etc.

- The endpoint driver should not be able to tell the difference, i.e., we should be calling the same pci_error_handlers callbacks in both cases.

Quoted 7 lines

It's true that in the non-hotplug system, pciehp probably won't start re-enumeration, so we might need an alternate path to trigger that.

But that's not what we're doing in this patch. In this patch we're adding a much bigger difference: for hotplug bridges, we stop and remove the hierarchy below the bridge; for non-hotplug bridges, we do the AER-style flow of calling pci_error_handlers callbacks.

Our approach on V12 was to go to AER style recovery for all DPC events regardless of hotplug support or not.

Keith was not comfortable with this approach. That's why, we special cased hotplug.

If we drop 6/6 on this patch on v13, we achieve this. We still have to take care of Keith's inputs on individual patches.

we have been struggling with the direction for a while.

Keith, what do you think?

Quoted 38 lines · Sinan Kaya

Quoted 20 lines · Bjorn Helgaas

I think the scenario you are describing is two systems that are identical except that in the first, the endpoint is below a hotplug bridge, while in the second, it's below a non-hotplug bridge. There's no physical hotplug (no drive removed or inserted), and DPC is triggered in both systems.

I suggest that DPC should be handled identically in both systems:

- The PCI core should have the same view of the endpoint: it should be removed and re-added in both cases (or in neither case).

- The endpoint itself should not be able to tell the difference: it should see a link down event, followed by a link retrain, followed by the same sequence of config accesses, etc.

- The endpoint driver should not be able to tell the difference, i.e., we should be calling the same pci_error_handlers callbacks in both cases.

Quoted 7 lines

It's true that in the non-hotplug system, pciehp probably won't start re-enumeration, so we might need an alternate path to trigger that.

But that's not what we're doing in this patch. In this patch we're adding a much bigger difference: for hotplug bridges, we stop and remove the hierarchy below the bridge; for non-hotplug bridges, we do the AER-style flow of calling pci_error_handlers callbacks.

Our approach on V12 was to go to AER style recovery for all DPC events regardless of hotplug support or not.

Keith was not comfortable with this approach. That's why, we special cased hotplug.

If we drop 6/6 on this patch on v13, we achieve this. We still have to take care of Keith's inputs on individual patches.

Quoted 3 lines

we have been struggling with the direction for a while.

Keith, what do you think?

My only concern was for existing production environments that use DPC for handling surprise removal, and I don't wish to break the existing uses.

Quoted 43 lines · Keith Busch

Quoted 41 lines · Sinan Kaya

Quoted 20 lines · Bjorn Helgaas

I think the scenario you are describing is two systems that are identical except that in the first, the endpoint is below a hotplug bridge, while in the second, it's below a non-hotplug bridge. There's no physical hotplug (no drive removed or inserted), and DPC is triggered in both systems.

I suggest that DPC should be handled identically in both systems:

- The PCI core should have the same view of the endpoint: it should be removed and re-added in both cases (or in neither case).

- The endpoint itself should not be able to tell the difference: it should see a link down event, followed by a link retrain, followed by the same sequence of config accesses, etc.

- The endpoint driver should not be able to tell the difference, i.e., we should be calling the same pci_error_handlers callbacks in both cases.

Quoted 7 lines

It's true that in the non-hotplug system, pciehp probably won't start re-enumeration, so we might need an alternate path to trigger that.

But that's not what we're doing in this patch. In this patch we're adding a much bigger difference: for hotplug bridges, we stop and remove the hierarchy below the bridge; for non-hotplug bridges, we do the AER-style flow of calling pci_error_handlers callbacks.

Our approach on V12 was to go to AER style recovery for all DPC events regardless of hotplug support or not.

Keith was not comfortable with this approach. That's why, we special cased hotplug.

If we drop 6/6 on this patch on v13, we achieve this. We still have to take care of Keith's inputs on individual patches.

we have been struggling with the direction for a while.

Keith, what do you think?

Quoted 3 lines

My only concern was for existing production environments that use DPC for handling surprise removal, and I don't wish to break the existing uses.

Also, I thought the plan was to keep hotplug and non-hotplug the same, except for the very end: if not a hotplug bridge, initiate the rescan automatically after releasing from containment, otherwise let pciehp handle it when the link reactivates.

Quoted 6 lines · Keith Busch

Also, I thought the plan was to keep hotplug and non-hotplug the same, except for the very end: if not a hotplug bridge, initiate the rescan automatically after releasing from containment, otherwise let pciehp handle it when the link reactivates.

Hmm...

AER driver doesn't do stop and rescan approach for fatal errors. AER driver makes an error callback followed by secondary bus reset and finally driver the resume callback on the endpoint only if link recovery is successful. Otherwise, AER driver bails out with recovery unsuccessful message.

Why do we need an additional rescan in the DPC driver if the link is up and driver resumes operation?

Quoted 10 lines · Sinan Kaya

Quoted 6 lines · Keith Busch

Also, I thought the plan was to keep hotplug and non-hotplug the same, except for the very end: if not a hotplug bridge, initiate the rescan automatically after releasing from containment, otherwise let pciehp handle it when the link reactivates.

Hmm...

Quoted 4 lines

AER driver doesn't do stop and rescan approach for fatal errors. AER driver makes an error callback followed by secondary bus reset and finally driver the resume callback on the endpoint only if link recovery is successful. Otherwise, AER driver bails out with recovery unsuccessful message.

I'm not sure if that's necessarily true. People have reported AER handling triggers PCIe hotplug events, and creates some interesting race conditions:

https://marc.info/?l=linux-pci&m=152336615707640&w=2

https://www.spinics.net/lists/linux-pci/msg70614.html

Quoted 2 lines

Why do we need an additional rescan in the DPC driver if the link is up and driver resumes operation?

I thought the plan was to have DPC always go through the removal path to ensure all devices are properly configured when containment is released. In order to reconfigure those, you'll need to initiate the rescan from somewhere.

Quoted 11 lines · Keith Busch

Quoted 10 lines · Sinan Kaya

Quoted 6 lines · Keith Busch

Also, I thought the plan was to keep hotplug and non-hotplug the same, except for the very end: if not a hotplug bridge, initiate the rescan automatically after releasing from containment, otherwise let pciehp handle it when the link reactivates.

Hmm...

Quoted 8 lines

Quoted 4 lines

AER driver doesn't do stop and rescan approach for fatal errors. AER driver makes an error callback followed by secondary bus reset and finally driver the resume callback on the endpoint only if link recovery is successful. Otherwise, AER driver bails out with recovery unsuccessful message.

I'm not sure if that's necessarily true. People have reported AER handling triggers PCIe hotplug events, and creates some interesting race conditions:

By reading the code, I don't see a stop and rescan in the AER error recovery path.

As both logs indicate, stop and rescan is initiated in response to link down and link up interrupts triggered by the secondary bus reset.
The SW entity handling these is not AER driver. It is the hotplug driver running asynchronous to the AER driver.

AER driver should have tried a slot reset before attempting to do a secondary bus reset.

/**
* pci_reset_slot - reset a PCI slot
* @slot: PCI slot to reset
*
* A PCI bus may host multiple slots, each slot may support a reset mechanism
* independent of other slots. For instance, some slots may support slot power
* control. In the case of a 1:1 bus to slot architecture, this function may
* wrap the bus reset to avoid spurious slot related events such as hotplug.
* Generally a slot reset should be attempted before a bus reset. All of the
* function of the slot and any subordinate buses behind the slot are reset
* through this function. PCI config space of all devices in the slot and
* behind the slot is saved before and restored after reset.
*
* Return 0 on success, non-zero on error.
*/
int pci_reset_slot(struct pci_slot *slot)

Slot reset is there to mask hotplug interrupts before the reset and unmask them after reset.

Quoted 5 lines

https://marc.info/?l=linux-pci&m=152336615707640&w=2

https://www.spinics.net/lists/linux-pci/msg70614.html

Quoted 8 lines

Quoted 2 lines

Why do we need an additional rescan in the DPC driver if the link is up and driver resumes operation?

I thought the plan was to have DPC always go through the removal path to ensure all devices are properly configured when containment is released. In order to reconfigure those, you'll need to initiate the rescan from somewhere.

This is where the contradiction is.

Bjorn is asking for a unified error handling for both AER and DPC.

Current AER error recovery framework is error callback + secondary bus reset + resume callback.

How does this stop + rescan model fit?

Do we want to change the error recovery framework? I suppose this will become a bigger conversation as there are more customers of this.

You indicated that you want to unify the AER and DPC behavior. Let's settle on what we want to do one more time. We have been going forth and back on the direction.

Quoted 3 lines · Sinan Kaya

You indicated that you want to unify the AER and DPC behavior. Let's settle on what we want to do one more time. We have been going forth and back on the direction.

My thinking is that as much as possible, similar events should be handled similarly, whether the mechanism is AER, DPC, EEH, etc. Ideally, drivers shouldn't have to be aware of which mechanism is in use.

Error recovery includes conventional PCI as well, but right now I think we're only concerned with PCIe. The following error types are from PCIe r4.0, sec 6.2.2:

ERR_COR
Corrected by hardware with no software intervention. Software involved for logging only.

Handled by AER via pci_error_handlers; DPC is never involved.

Link is unaffected.

ERR_NONFATAL
A transaction is unreliable but the link is fully functional.

If DPC is not supported, handled by AER via pci_error_handlers and the link is unaffected.

If DPC supported, handled by DPC (because we set PCI_EXP_DPC_CTL_EN_NONFATAL) via remove/re-enumerate.

ERR_FATAL
The link is unreliable.

If DPC is not supported, handled by AER via pci_error_handlers and the link is reset.

If DPC supported, handled by DPC via remove/re-enumerate.

It doesn't seem right to me that we handle both ERR_NONFATAL and ERR_FATAL events differently if we happen to have DPC support in a switch.

Maybe we should consider triggering DPC only on ERR_FATAL? That would keep DPC out of the ERR_NONFATAL cases.

For ERR_FATAL, maybe we should bite the bullet and use remove/re-enumerate for AER as well as for DPC. That would be painful for higher-level software, but if we're willing to accept that pain for new systems that support DPC, maybe life would be better overall if it worked the same way on systems without DPC?

Bjorn

Quoted 47 lines · Bjorn Helgaas

Quoted 3 lines · Sinan Kaya

You indicated that you want to unify the AER and DPC behavior. Let's settle on what we want to do one more time. We have been going forth and back on the direction.

My thinking is that as much as possible, similar events should be handled similarly, whether the mechanism is AER, DPC, EEH, etc. Ideally, drivers shouldn't have to be aware of which mechanism is in use.

Error recovery includes conventional PCI as well, but right now I think we're only concerned with PCIe. The following error types are from PCIe r4.0, sec 6.2.2:

ERR_COR
Corrected by hardware with no software intervention. Software involved for logging only.

Handled by AER via pci_error_handlers; DPC is never involved.

Link is unaffected.

ERR_NONFATAL
A transaction is unreliable but the link is fully functional.

If DPC is not supported, handled by AER via pci_error_handlers and the link is unaffected.

If DPC supported, handled by DPC (because we set PCI_EXP_DPC_CTL_EN_NONFATAL) via remove/re-enumerate.

ERR_FATAL
The link is unreliable.

If DPC is not supported, handled by AER via pci_error_handlers and the link is reset.

If DPC supported, handled by DPC via remove/re-enumerate.

It doesn't seem right to me that we handle both ERR_NONFATAL and ERR_FATAL events differently if we happen to have DPC support in a switch.

Maybe we should consider triggering DPC only on ERR_FATAL? That would keep DPC out of the ERR_NONFATAL cases.

Quoted 7 lines

For ERR_FATAL, maybe we should bite the bullet and use remove/re-enumerate for AER as well as for DPC. That would be painful for higher-level software, but if we're willing to accept that pain for new systems that support DPC, maybe life would be better overall if it worked the same way on systems without DPC?

Bjorn

This had crossed my mind when I first looked at the code.
DPC is getting triggered for both ERR_NONFATAL and ERR_FATAL case. I thought the primary purpose of DPC to recover fatal errors, by triggering HW recovery.
but what if some platform wants to handle both FATAL and NON_FATAL with DPC ?

As you said AER FATAL cases and DPC FATAL cases should be handled similarly. e.g. remove/re-enumerate the devices.

while NON_FATAL case; only AER would come into picture.
if some platform would like to handle DPC NON_FATAL then it should follow AER NON_FATAL path (where it does not do remove/re-enumerate)

And the case where hotplug is enabled, remove/re-enumerate more sense in case of ERR_FATAL.
And the case where hotplug is disabled, only re-enumeration is required. (no need to remove the devices)
but then do we need to handle this case specifically, what is the harm in removing the devices in all the cases followed by re-enumerate ?

Regards,
Oza.

Quoted 54 lines · On 2018-04-16 11:03, [email protected] wrote:

Quoted 53 lines · Bjorn Helgaas

Quoted 3 lines · Sinan Kaya

You indicated that you want to unify the AER and DPC behavior. Let's settle on what we want to do one more time. We have been going forth and back on the direction.

My thinking is that as much as possible, similar events should be handled similarly, whether the mechanism is AER, DPC, EEH, etc. Ideally, drivers shouldn't have to be aware of which mechanism is in use.

Error recovery includes conventional PCI as well, but right now I think we're only concerned with PCIe. The following error types are from PCIe r4.0, sec 6.2.2:

ERR_COR
Corrected by hardware with no software intervention. Software involved for logging only.

Handled by AER via pci_error_handlers; DPC is never involved.

Link is unaffected.

ERR_NONFATAL
A transaction is unreliable but the link is fully functional.

If DPC is not supported, handled by AER via pci_error_handlers and the link is unaffected.

If DPC supported, handled by DPC (because we set PCI_EXP_DPC_CTL_EN_NONFATAL) via remove/re-enumerate.

ERR_FATAL
The link is unreliable.

If DPC is not supported, handled by AER via pci_error_handlers and the link is reset.

If DPC supported, handled by DPC via remove/re-enumerate.

It doesn't seem right to me that we handle both ERR_NONFATAL and ERR_FATAL events differently if we happen to have DPC support in a switch.

Maybe we should consider triggering DPC only on ERR_FATAL? That would keep DPC out of the ERR_NONFATAL cases.

For ERR_FATAL, maybe we should bite the bullet and use remove/re-enumerate for AER as well as for DPC. That would be painful for higher-level software, but if we're willing to accept that pain for new systems that support DPC, maybe life would be better overall if it worked the same way on systems without DPC?

Quoted 8 lines

Quoted 1 line

Bjorn

This had crossed my mind when I first looked at the code. DPC is getting triggered for both ERR_NONFATAL and ERR_FATAL case. I thought the primary purpose of DPC to recover fatal errors, by triggering HW recovery.
but what if some platform wants to handle both FATAL and NON_FATAL with DPC ?

Quoted 7 lines

As you said AER FATAL cases and DPC FATAL cases should be handled similarly. e.g. remove/re-enumerate the devices.

while NON_FATAL case; only AER would come into picture. if some platform would like to handle DPC NON_FATAL then it should follow AER NON_FATAL path (where it does not do remove/re-enumerate)

Quoted 6 lines

And the case where hotplug is enabled, remove/re-enumerate more sense in case of ERR_FATAL. And the case where hotplug is disabled, only re-enumeration is required. (no need to remove the devices) but then do we need to handle this case specifically, what is the harm in removing the devices in all the cases followed by re-enumerate ?

To Clarify the last line, what I meant here was, in case of ERR_FATAL we can always remove/re-enumerate the devices irrespective of hotplug is enabled or not.

and in case of ERR_NONFATAL, DPC will follow AER path (where it just tries to recover) although I am not very sure that how to handle ERR_NONFATAL case if hotplug is enabled. Because as Keith suggested device might have been changed run-time.

It doesn't seem right to me that we handle both ERR_NONFATAL and ERR_FATAL events differently if we happen to have DPC support in a switch.

Maybe we should consider triggering DPC only on ERR_FATAL? That would keep DPC out of the ERR_NONFATAL cases.

From reliability perspective, it makes sense. DPC handles NONFATAL errors

For ERR_FATAL, maybe we should bite the bullet and use remove/re-enumerate for AER as well as for DPC. That would be painful for higher-level software, but if we're willing to accept that pain for new systems that support DPC, maybe life would be better overall if it worked the same way on systems without DPC?

Quoted 7 lines · Bjorn Helgaas

It doesn't seem right to me that we handle both ERR_NONFATAL and ERR_FATAL events differently if we happen to have DPC support in a switch.

Maybe we should consider triggering DPC only on ERR_FATAL? That would keep DPC out of the ERR_NONFATAL cases.

From reliability perspective, it makes sense. DPC handles NONFATAL errors by bringing down the link. If error happened behind a switch and root port is handling DPC, we are impacting a lot of devices operation because of one faulty device.

Keith, do you have any preference on this direction?

Quoted 5 lines

For ERR_FATAL, maybe we should bite the bullet and use remove/re-enumerate for AER as well as for DPC. That would be painful for higher-level software, but if we're willing to accept that pain for new systems that support DPC, maybe life would be better overall if it worked the same way on systems without DPC?

Sure, we can go to this route as well.

This patch set brings in error handling support for DPC

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

DPC should behave identical to AER as far as error handling is concerned. DPC should remove the devices and not to do recovery for hotplug enabled system.

Quoted 8 lines · Oza Pawandeep

This patch set brings in error handling support for DPC

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

Quoted 2 lines

DPC should behave identical to AER as far as error handling is concerned. DPC should remove the devices and not to do recovery for hotplug enabled system.

Is there a specific bug that's fixed by these patches? I didn't see one mentioned in the changelogs.

Quoted 9 lines · Bjorn Helgaas

Quoted 8 lines · Oza Pawandeep

This patch set brings in error handling support for DPC

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

Quoted 6 lines

Quoted 2 lines

DPC should behave identical to AER as far as error handling is concerned. DPC should remove the devices and not to do recovery for hotplug enabled system.

Is there a specific bug that's fixed by these patches? I didn't see one mentioned in the changelogs.

There is no actual bug.

We realized that DPC and hotplug is heavily integrated today. We have use cases for systems without hotplug support but still support DPC. That's the problem we are trying to solve with this patchset.

Quoted 21 lines · Sinan Kaya

Quoted 19 lines · Bjorn Helgaas

Quoted 10 lines · Oza Pawandeep

This patch set brings in error handling support for DPC

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

Quoted 4 lines

DPC should behave identical to AER as far as error handling is concerned.
DPC should remove the devices and not to do recovery for hotplug enabled system.

Is there a specific bug that's fixed by these patches? I didn't see one mentioned in the changelogs.

Quoted 6 lines

There is no actual bug.

We realized that DPC and hotplug is heavily integrated today. We have use cases for systems without hotplug support but still support DPC. That's the problem we are trying to solve with this patchset.

Adding to what Sinan said;

DPC should handle the error handling and recovery similar to AER, because finally both are attempting recovery in some or the other way, and for that error handling and recovery framework has to be loosely coupled. It achieves uniformity and transparency to the error handling agents such as AER, DPC, with respect to recovery and error handling.

So, this patch-set tries to unify lot of things between error agents and make them behave in a well defined way. (be it error (FATAL, NON_FATAL) handling or recovery).

Quoted 22 lines · On Mon, Apr 16, 2018 at 11:33:13AM +0530, [email protected] wrote:

Quoted 21 lines · Sinan Kaya

Quoted 19 lines · Bjorn Helgaas

Quoted 10 lines · Oza Pawandeep

This patch set brings in error handling support for DPC

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

Quoted 4 lines

DPC should behave identical to AER as far as error handling is concerned.
DPC should remove the devices and not to do recovery for hotplug enabled system.

Is there a specific bug that's fixed by these patches? I didn't see one mentioned in the changelogs.

Quoted 6 lines

Quoted 6 lines

There is no actual bug.

We realized that DPC and hotplug is heavily integrated today. We have use cases for systems without hotplug support but still support DPC. That's the problem we are trying to solve with this patchset.

Apparently there's a problem with systems that have DPC but not hotplug. It will be extremely helpful if you can articulate what that problem is and include it in the appropriate changelog.

Quoted 9 lines

Adding to what Sinan said;

DPC should handle the error handling and recovery similar to AER, because finally both are attempting recovery in some or the other way, and for that error handling and recovery framework has to be loosely coupled. It achieves uniformity and transparency to the error handling agents such as AER, DPC, with respect to recovery and error handling.

Quoted 3 lines

So, this patch-set tries to unify lot of things between error agents and make them behave in a well defined way. (be it error (FATAL, NON_FATAL) handling or recovery).

I totally support this objective.

Bjorn

Quoted 4 lines

Quoted 4 lines

We realized that DPC and hotplug is heavily integrated today. We have use cases for systems without hotplug support but still support DPC. That's the problem we are trying to solve with this patchset.

Apparently there's a problem with systems that have DPC but not hotplug. It will be extremely helpful if you can articulate what that problem is and include it in the appropriate changelog.