PCI: Handle conflict between hotplug and error handling

AER/DPC reset is known as warm-resets. HP link recovery is known as cold-reset via power-off and power-on command to the PCI slot.

In the middle of a warm-reset operation (AER/DPC), we are:

1. turning off the slow power. Slot power needs to be kept on in order for recovery to succeed.

2. performing a cold reset causing Fatal Error recovery to fail.

If link goes down due to a DPC event, it should be recovered by DPC status trigger. Injecting a cold reset in the middle can cause a HW lockup as it is an undefined behavior.

Similarly, If link goes down due to an AER secondary bus reset issue, it should be recovered by HW. Injecting a cold reset in the middle of a secondary bus reset can cause a HW lockup as it is an undefined behavior.

1. HP ISR observes link down interrupt.
2. HP ISR checks that there is a fatal error pending, it doesn't touch the link.
3. HP ISR waits until link recovery happens.
4. HP ISR calls the read vendor id function.
5. If all fails, try the cold-reset approach.

If fatal error is pending and a fatal error service such as DPC or AER is running, it is the responsibility of the fatal error service to recover the link.

Signed-off-by Sinan Kaya <[email protected]>

---
 drivers/pci/hotplug/pciehp_ctrl.c | 19 ++++++++++
 drivers/pci/pci.h                 |  2 ++
 drivers/pci/pcie/err.c            | 60 +++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

drivers/pci/hotplug/pciehp_ctrl.c +16 -0

index da7c72372ffc..ba8dd51a3f0f 100644
@@ -222,9 +222,28 @@ void pciehp_handle_disable_request(struct slot 
 *slot)
 void pciehp_handle_presence_or_link_change(struct slot *slot, u32 
 events)
 {
 	struct controller *ctrl = slot->ctrl;
+	struct pci_dev *pdev = ctrl->pcie->port;
 	bool link_active;
 	u8 present;
 
+	/* If a fatal error is pending, wait for AER or DPC to handle it. */
+	if (pcie_fatal_error_pending(pdev, PCI_ERR_UNC_SURPDN)) {
+		bool recovered;
+
+		recovered = pcie_wait_fatal_error_clear(pdev,
+						PCI_ERR_UNC_SURPDN);
+
+		/* If the fatal error is gone and the link is up, return */
+		if (recovered && pcie_wait_for_link(pdev, true)) {
+			ctrl_info(ctrl, "Slot(%s): Ignoring Link event due to successful
 fatal error recovery\n",
+				  slot_name(slot));
+			return;
+		}
+
+		ctrl_info(ctrl, "Slot(%s): Fatal error recovery failed for Link

event, tryig hotplug reset\n",

drivers/pci/hotplug/pciehp_ctrl.c +3 -0

+			  slot_name(slot));
+	}
+
 	/*
 	 * If the slot is on and presence or link has changed, turn it off.
 	 * Even if it's occupied again, we cannot assume the card is the 

same.

drivers/pci/pci.h +2 -0

index c358e7a07f3f..2ebb05338368 100644
@@ -356,6 +356,8 @@ void pci_enable_acs(struct pci_dev *dev);
 /* PCI error reporting and recovery */
 void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service);
 void pcie_do_nonfatal_recovery(struct pci_dev *dev);
+bool pcie_fatal_error_pending(struct pci_dev *pdev, u32 usrmask);
+bool pcie_wait_fatal_error_clear(struct pci_dev *pdev, u32 usrmask);
 
 bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
 #ifdef CONFIG_PCIEASPM

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

index f7ce0cb0b0b7..8ea012dbf063 100644
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/aer.h>
+#include <linux/delay.h>
 #include "portdrv.h"
 #include "../pci.h"
 

@@ -386,3 +387,62 @@ void pcie_do_nonfatal_recovery(struct pci_dev 
 *dev)
 	/* TODO: Should kernel panic here? */
 	pci_info(dev, "AER: Device recovery failed\n");
 }
+
+bool pcie_fatal_error_pending(struct pci_dev *pdev, u32 usrmask)
+{
+	u16 err_status = 0;
+	u32 status, mask, severity;
+	int rc;
+
+	if (!pci_is_pcie(pdev))
+		return false;
+
+	rc = pcie_capability_read_word(pdev, PCI_EXP_DEVSTA, &err_status);
+	if (rc)
+		return false;
+
+	if (!(err_status & PCI_EXP_DEVSTA_FED))
+		return false;
+
+	if (!pdev->aer_cap)
+		return false;
+
+	rc = pci_read_config_dword(pdev, pdev->aer_cap + 

PCI_ERR_UNCOR_STATUS,

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

+				   &status);
+	if (rc)
+		return false;
+
+	rc = pci_read_config_dword(pdev, pdev->aer_cap + PCI_ERR_UNCOR_MASK,
+				   &mask);
+	if (rc)
+		return false;
+
+	rc = pci_read_config_dword(pdev, pdev->aer_cap + PCI_ERR_UNCOR_SEVER,
+				   &severity);
+	if (rc)
+		return false;
+
+	status &= mask;
+	status &= ~usrmask;
+	status &= severity;
+
+	return !!status;
+}
+
+bool pcie_wait_fatal_error_clear(struct pci_dev *pdev, u32 usrmask)
+{
+	int timeout = 1000;

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

+	}
+
+	return false;
+}

Quoted 9 lines · Sinan Kaya

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

+bool pcie_wait_fatal_error_clear(struct pci_dev *pdev, u32 usrmask)
+{
+	int timeout = 1000;
+	bool ret;
+
+	for (;;) {
+		ret = pcie_fatal_error_pending(pdev, usrmask);
+		if (ret == false)
+			return true;

drivers/pci/pcie/err.c void pcie_do_nonfatal_recovery(struct pci_dev *dev) · line 444

Quoted 4 lines

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

+		if (timeout <= 0)
+			break;
+		msleep(20);
+		timeout -= 20;

I assume that this timeout will come into effect if
1) AER/DPC takes longer time than 1 sec for recovery.
2) Lets us say both AER and DPC are disabled....are we going to wait for this timeout before HP can take over ?

Quoted 10 lines · On Mon, Aug 06, 2018 at 01:21:03PM +0530, [email protected] wrote:

Quoted 9 lines · Sinan Kaya

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

+bool pcie_wait_fatal_error_clear(struct pci_dev *pdev, u32 usrmask)
+{
+	int timeout = 1000;
+	bool ret;
+
+	for (;;) {
+		ret = pcie_fatal_error_pending(pdev, usrmask);
+		if (ret == false)
+			return true;

Quoted 8 lines

drivers/pci/pcie/err.c void pcie_do_nonfatal_recovery(struct pci_dev *dev) · line 444

Quoted 4 lines

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

+		if (timeout <= 0)
+			break;
+		msleep(20);
+		timeout -= 20;

I assume that this timeout will come into effect if
1) AER/DPC takes longer time than 1 sec for recovery.
2) Lets us say both AER and DPC are disabled....are we going to wait for this timeout before HP can take over ?

If CONFIG_PCIEAER is disabled, pdev->aer_cap will not be set because it is populated in pci_aer_init().

pcie_fatal_error_pending(), as introduced by this patch, returns false if pdev->aer_cap is not set. So pciehp will fall back to a cold reset if CONFIG_PCIEAER is disabled.

I'm not seeing a similar check for CONFIG_PCIE_DPC=n in this patch, but I'm not familiar enough with PCIe error recovery to know if such a check is at all needed.

Thanks,

Lukas

Quoted 1 line

Either AER or DPC would get triggered, not both.
in that case, if AER is disabled, then this code will return false thinking HP needs to handle it. but it might be that DPC would be triggering as well.
but I dont see DPC check anywhere, rather we are relying on PCI_EXP_DEVSTA
and following condition:
if (!pdev->aer_cap)
      return false;
so here we dont check anything with respect to DPC capability (although there is no such thing as dpc_cap) (except If I missed something)

Quoted 9 lines

Either AER or DPC would get triggered, not both.
in that case, if AER is disabled, then this code will return false thinking HP needs to handle it. but it might be that DPC would be triggering as well.
but I dont see DPC check anywhere, rather we are relying on PCI_EXP_DEVSTA
and following condition:
if (!pdev->aer_cap)
       return false;
so here we dont check anything with respect to DPC capability (although there is no such thing as dpc_cap) (except If I missed something)

That's true. We either need to go poll every single source (AER/DPC) for an error or rely on the DEVSTA. For ease of implementation, I suggest DEVSTA. Something like this:

Quoted 10 lines · Sinan Kaya

Quoted 9 lines

Either AER or DPC would get triggered, not both.
in that case, if AER is disabled, then this code will return false thinking HP needs to handle it. but it might be that DPC would be triggering as well.
but I dont see DPC check anywhere, rather we are relying on PCI_EXP_DEVSTA
and following condition:
if (!pdev->aer_cap)
       return false;
so here we dont check anything with respect to DPC capability (although there is no such thing as dpc_cap) (except If I missed something)

Quoted 3 lines

That's true. We either need to go poll every single source (AER/DPC) for an error or rely on the DEVSTA. For ease of implementation, I suggest DEVSTA. Something like this:

Hmm. Too quick...

Reduced set doesn't help. Surprise Link Down is also a fatal error. That's what I was trying to exclude. We'll have to poll the error source.

Quoted 4 lines

Quoted 3 lines

That's true. We either need to go poll every single source (AER/DPC) for an error or rely on the DEVSTA. For ease of implementation, I suggest DEVSTA. Something like this:

Quoted 5 lines

Hmm. Too quick...

Reduced set doesn't help. Surprise Link Down is also a fatal error. That's what I was trying to exclude. We'll have to poll the error source.

How about using pcie_port_find_service() ?

Surprise Link Down is also a fatal error.

Quoted 1 line · Sinan Kaya

Surprise Link Down is also a fatal error.

Seriously? On a Downstream Port which has the Hot-Plug Surprise bit in the Slot Capabilities register set, Surprise Link Down is a fatal error? That would seem somewhat contradictory. Do you have a section number in the PCIe Base Spec for this?

Thanks,

Lukas

Quoted 6 lines

Quoted 1 line

Surprise Link Down is also a fatal error.

Seriously?  On a Downstream Port which has the Hot-Plug Surprise bit in the Slot Capabilities register set, Surprise Link Down is a fatal error?  That would seem somewhat contradictory.  Do you have a section number in the PCIe Base Spec for this?

Spec 3.0. 7.10.2. Uncorrectable Error Status Register (Offset 04h) bit 5 Surprise Down Error Status (Optional).

Quoted 7 lines · Lukas Wunner

Quoted 1 line · Sinan Kaya

Surprise Link Down is also a fatal error.

Seriously? On a Downstream Port which has the Hot-Plug Surprise bit in the Slot Capabilities register set, Surprise Link Down is a fatal error? That would seem somewhat contradictory. Do you have a section number in the PCIe Base Spec for this?

Spec 3.0. 7.10.2. Uncorrectable Error Status Register (Offset 04h) bit 5 Surprise Down Error Status (Optional).

Quoted 7 lines · Sinan Kaya

Quoted 6 lines · Lukas Wunner

Quoted 1 line · Sinan Kaya

Surprise Link Down is also a fatal error.

Seriously? On a Downstream Port which has the Hot-Plug Surprise bit in the Slot Capabilities register set, Surprise Link Down is a fatal error? That would seem somewhat contradictory. Do you have a section number in the PCIe Base Spec for this?

Quoted 2 lines

Spec 3.0. 7.10.2. Uncorrectable Error Status Register (Offset 04h) bit 5 Surprise Down Error Status (Optional).

Okay, thanks. Can we always mask out this bit on a hotplug port with surprise removal capability? Doesn't make sense to me to let AER/DPC spring to action in that case.

drivers/pci/hotplug/pciehp_ctrl.c +6 -0

@@ -222,9 +222,28 @@ void pciehp_handle_disable_request(struct slot *slot)
 void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events)
 {
 	struct controller *ctrl = slot->ctrl;
+	struct pci_dev *pdev = ctrl->pcie->port;
 	bool link_active;
 	u8 present;
 
+	/* If a fatal error is pending, wait for AER or DPC to handle it. */
+	if (pcie_fatal_error_pending(pdev, PCI_ERR_UNC_SURPDN)) {
+		bool recovered;
+
+		recovered = pcie_wait_fatal_error_clear(pdev,

Lukas:
'Your patch "[PATCH v6 1/1] PCI: pciehp: Ignore link events when there is a fatal error pending" only checks once for a pending fatal error, it should poll until either the fatal error is gone or a timeout is hit. If the fatal error is gone and the link is up, you can just return from pciehp_handle_presence_or_link_change(). Else (in the timeout case) fall back to the normal handling of a Link Down, i.e. let it bring down the slot.'

Sinan Kaya (1):
PCI: pciehp: Ignore link events when there is a fatal error pending

 drivers/pci/hotplug/pciehp_ctrl.c | 19 ++++++++++
 drivers/pci/pci.h                 |  2 ++
 drivers/pci/pcie/err.c            | 60 +++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

Quoted 13 lines · Sinan Kaya

Lukas:
'Your patch "[PATCH v6 1/1] PCI: pciehp: Ignore link events when there is a fatal error pending" only checks once for a pending fatal error, it should poll until either the fatal error is gone or a timeout is hit. If the fatal error is gone and the link is up, you can just return from pciehp_handle_presence_or_link_change(). Else (in the timeout case) fall back to the normal handling of a Link Down, i.e. let it bring down the slot.'

Sinan Kaya (1):
PCI: pciehp: Ignore link events when there is a fatal error pending

Quoted 4 lines

  drivers/pci/hotplug/pciehp_ctrl.c | 19 ++++++++++
  drivers/pci/pci.h                 |  2 ++
  drivers/pci/pcie/err.c            | 60 +++++++++++++++++++++++++++++++
  3 files changed, 81 insertions(+)

Dropping for now because it sounds like you intend to mask out the error for hotplug ports.