Skip to contentSkip to conversation

PCI: pciehp: Ignore link events when there is a fatal error pending

PATCH In review Sinan Kaya 3 participants 9 messages Aug 19, 2018 – Aug 20, 2018

Subsystems (2)
lore
Share & tools
lore

Sinan Kaya

author
#0 lore raw

PATCH v8 1/2 PCI: pciehp: Ignore link events when there is a fatal error pending

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.

---
 drivers/pci/hotplug/pciehp_ctrl.c | 18 ++++++++++++++++
 drivers/pci/pci.h                 |  2 ++
 drivers/pci/pcie/err.c            | 34 +++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)
drivers/pci/hotplug/pciehp_ctrl.c +18 -0
index da7c72372ffc..22354b6850c3 100644
@@ -222,9 +222,27 @@ 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)) {
+		bool recovered;
+
+		recovered = pcie_wait_fatal_error_clear(pdev);
+
+		/* 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, trying hotplug reset\n",
+			  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..e2d98654630b 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);
+bool pcie_wait_fatal_error_clear(struct pci_dev *pdev);
 
 bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
 #ifdef CONFIG_PCIEASPM
drivers/pci/pcie/err.c +34 -0
index f7ce0cb0b0b7..b1b5604cb00b 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,36 @@ 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)
+{
+	u16 err_status = 0;
+	int rc;
+
+	if (!pci_is_pcie(pdev))
+		return false;
+
+	rc = pcie_capability_read_word(pdev, PCI_EXP_DEVSTA, &err_status);
+	if (rc)
+		return false;
+
+	return !!(err_status & PCI_EXP_DEVSTA_FED);
+}
+
+bool pcie_wait_fatal_error_clear(struct pci_dev *pdev)
+{
+	int timeout = 1000;
+	bool ret;
+
+	for (;;) {
+		ret = pcie_fatal_error_pending(pdev);
+		if (ret == false)
+			return true;
+		if (timeout <= 0)
+			break;
+		msleep(20);
+		timeout -= 20;
+	}
+
+	return false;
+}
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v8 2/2 PCI: pciehp: Mask AER surprise link down error if hotplug is enabled

PCIe Spec 3.0. 7.10.2. Uncorrectable Error Status Register (Offset 04h) defines link down errors as an AER error as bit 5 Surprise Down Error Status.

If hotplug is supported by a particular port, we want hotplug driver to handle the link down/up conditions via Data Link Layer Active interrupt rather than the AER error interrupt. Mask the Surprise Down Error during hotplug driver and re-enable it during remove.

---
 drivers/pci/hotplug/pciehp_core.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
drivers/pci/hotplug/pciehp_core.c +27 -0
index ec48c9433ae5..8322db8f369a 100644
@@ -229,6 +229,29 @@ static void pciehp_check_presence(struct controller *ctrl)
 	up_read(&ctrl->reset_lock);
 }
 
+static int pciehp_control_surprise_error(struct controller *ctrl, bool enable)
+{
+	struct pci_dev *pdev = ctrl->pcie->port;
+	u32 reg32;
+	int pos;
+
+	if (!pci_is_pcie(pdev))
+		return -ENODEV;
+
+	pos = pdev->aer_cap;
+	if (!pos)
+		return -ENODEV;
+
+	pci_read_config_dword(pdev, pos + PCI_ERR_UNCOR_MASK, &reg32);
+	if (enable)
+		reg32 &= ~PCI_ERR_UNC_SURPDN;
+	else
+		reg32 |= PCI_ERR_UNC_SURPDN;
+	pci_write_config_dword(pdev, pos + PCI_ERR_UNCOR_MASK, reg32);
+
+	return 0;
+}
+
 static int pciehp_probe(struct pcie_device *dev)
 {
 	int rc;
@@ -280,6 +303,9 @@ static int pciehp_probe(struct pcie_device *dev)
 
 	pciehp_check_presence(ctrl);
 
+	/* We want exclusive control of link down events in hotplug driver */
+	pciehp_control_surprise_error(ctrl, false);
+
 	return 0;
 
 err_out_shutdown_notification:
@@ -298,6 +324,7 @@ static void pciehp_remove(struct pcie_device *dev)
 	pci_hp_del(ctrl->slot->hotplug_slot);
 	pcie_shutdown_notification(ctrl);
 	cleanup_slot(ctrl);
+	pciehp_control_surprise_error(ctrl, true);
 	pciehp_release_ctrl(ctrl);
 }
 
Signature · 1 line
2.17.1
author
#2 lore raw

Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled

drivers/pci/hotplug/pciehp_core.c static void pciehp_remove(struct pcie_device *dev) · line 327

Quoted 2 lines · Sinan Kaya
  	cleanup_slot(ctrl);
drivers/pci/hotplug/pciehp_core.c +1 -0
+	pciehp_control_surprise_error(ctrl, true);

I think I need to move this one line up but I'd like to see some input here and also ask for some testing.

I don't have any hardware to test.

#3 lore raw

Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled

In reply to Sinan Kaya
I love your patch! Yet something to improve:

Hi Sinan,

I love your patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on next-20180817]
[cannot apply to v4.18]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sinan-Kaya/PCI-pciehp-Ignore-link-events-when-there-is-a-fatal-error-pending/20180820-074636
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-x075-201833 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

drivers/pci/hotplug/pciehp_core.c: In function 'pciehp_control_surprise_error':

>> drivers/pci/hotplug/pciehp_core.c:241:14: error: 'struct pci_dev' has no member named 'aer_cap'; did you mean 'ats_cap'?
     pos = pdev->aer_cap;
                 ^~~~~~~
                 ats_cap

vim +241 drivers/pci/hotplug/pciehp_core.c

231
232 static int pciehp_control_surprise_error(struct controller *ctrl, bool enable)
233 {
234 struct pci_dev *pdev = ctrl->pcie->port;
235 u32 reg32;
236 int pos;
237
238 if (!pci_is_pcie(pdev))
239 return -ENODEV;
240

 > 241		pos = pdev->aer_cap;

242 if (!pos)
243 return -ENODEV;
244
245 pci_read_config_dword(pdev, pos + PCI_ERR_UNCOR_MASK, &reg32);
246 if (enable)
247 reg32 &= ~PCI_ERR_UNC_SURPDN;
248 else
249 reg32 |= PCI_ERR_UNC_SURPDN;
250 pci_write_config_dword(pdev, pos + PCI_ERR_UNCOR_MASK, reg32);
251
252 return 0;
253 }
254

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#4 lore raw

Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled

In reply to Sinan Kaya
Quoted 1 line · Sinan Kaya

+static int pciehp_control_surprise_error(struct controller *ctrl, bool enable)

The return value isn't checked, so this could return void.

drivers/pci/hotplug/pciehp_core.c static int pciehp_probe(struct pcie_device *dev) · line 309

Quoted 8 lines
drivers/pci/hotplug/pciehp_core.c +3 -0
@@ -280,6 +303,9 @@ static int pciehp_probe(struct pcie_device *dev)
 
 	pciehp_check_presence(ctrl);
 
+	/* We want exclusive control of link down events in hotplug driver */
+	pciehp_control_surprise_error(ctrl, false);
+
 	return 0;

Hm, if the platform firmware hasn't granted native hotplug control to OSPM, or if some other hotplug driver than pciehp is used, shouldn't surprise down be ignored by error recovery as well? If yes, the mask would have to be set in generic code somewhere in drivers/pci/probe.c I guess, based on the is_hotplug_bridge bit in struct pci_dev.

(Interestingly, PCI_ERR_UNCOR_MASK is already changed in probe.c by program_hpp_type2(). That seems to be ACPI-specific code, which kind of begs the question why it's not in pci-acpi.c?)

Thanks,

Lukas

author
#5 lore raw

Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled

Quoted 5 lines · Lukas Wunner
Quoted 1 line · Sinan Kaya

+static int pciehp_control_surprise_error(struct controller *ctrl, bool enable)

The return value isn't checked, so this could return void.

Sure, I can do that.

Quoted 5 lines
Quoted 5 lines
drivers/pci/hotplug/pciehp_core.c +0 -0
@@ -280,6 +303,9 @@ static int pciehp_probe(struct pcie_device *dev)
 
  	pciehp_check_presence(ctrl);
 
Quoted 10 lines

drivers/pci/hotplug/pciehp_core.c static int pciehp_probe(struct pcie_device *dev) · line 309

Quoted 4 lines
drivers/pci/hotplug/pciehp_core.c +3 -0
+	/* We want exclusive control of link down events in hotplug driver */
+	pciehp_control_surprise_error(ctrl, false);
+
  	return 0;

Hm, if the platform firmware hasn't granted native hotplug control to OSPM, or if some other hotplug driver than pciehp is used, shouldn't surprise down be ignored by error recovery as well? If yes, the mask would have to be set in generic code somewhere in drivers/pci/probe.c I guess, based on the is_hotplug_bridge bit in struct pci_dev.

I could move this code if we know that is_hotplug_bridge flag is set regardless of OS hotplug driver control or not.

Quoted 4 lines

(Interestingly, PCI_ERR_UNCOR_MASK is already changed in probe.c by program_hpp_type2(). That seems to be ACPI-specific code, which kind of begs the question why it's not in pci-acpi.c?)

Yes, you can tell the OS what AER mask to set following hotplug insertion via ACPI HPP table especially if you remove a hotplug bridge. This is used during ACPI hotplug.

Quoted 5 lines

Thanks,

Lukas

#6 lore raw
In reply to Sinan Kaya
Quoted 17 lines · Sinan Kaya
drivers/pci/hotplug/pciehp_ctrl.c +8 -0
@@ -222,9 +222,27 @@ 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)) {
+		bool recovered;
+
+		recovered = pcie_wait_fatal_error_clear(pdev);
+
+		/* If the fatal error is gone and the link is up, return */

drivers/pci/hotplug/pciehp_ctrl.c void pciehp_handle_disable_request(struct slot *slot) · line 245

Quoted 10 lines
drivers/pci/hotplug/pciehp_ctrl.c +10 -0
+		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, trying hotplug reset\n",
+			  slot_name(slot));
+	}
+

This differs from v7 of the patch in that *any* fatal error, not just a Surprise Link Down, results in pciehp waiting for the error to clear.

I'm wondering if that's safe: Theoretically, the user might quickly swap the card in the slot during, say, a Completion Timeout Error, and with this patch pciehp would carry on as if nothing happened.

Thanks,

Lukas

author
#7 lore raw
Quoted 7 lines · Lukas Wunner
Quoted 1 line

+

This differs from v7 of the patch in that*any* fatal error, not just a Surprise Link Down, results in pciehp waiting for the error to clear.

I'm wondering if that's safe: Theoretically, the user might quickly swap the card in the slot during, say, a Completion Timeout Error, and with this patch pciehp would carry on as if nothing happened.

Functionally both patches are identical. The v7 was still allowing AER/DPC to handle all fatal error events except Surprise Link Down.

Now, second patch (v8 2/2) is masking the surprise link down event as we have talked before. Therefore, there is no need to filter out incoming errors by reading the status register and masking the unwanted bits.

Just to clarify something, this patch will wait for only the FATAL error events to be handled by the error handling services only.

Completion Timeout is a NONFATAL error event by default unless somebody tweaks the severity bits.

Anyhow, all FATAL errors cause one sort of link down either initiated by software (AER) or hardware (DPC). Therefore, hotplug driver will observe a link down event and AER/DPC needs to handle the event as usual.

#8 lore raw
Quoted 9 lines · Sinan Kaya
Quoted 7 lines · Lukas Wunner
Quoted 1 line

+

This differs from v7 of the patch in that*any* fatal error, not just a Surprise Link Down, results in pciehp waiting for the error to clear.

I'm wondering if that's safe: Theoretically, the user might quickly swap the card in the slot during, say, a Completion Timeout Error, and with this patch pciehp would carry on as if nothing happened.

Quoted 7 lines

Functionally both patches are identical. The v7 was still allowing AER/DPC to handle all fatal error events except Surprise Link Down.

Now, second patch (v8 2/2) is masking the surprise link down event as we have talked before. Therefore, there is no need to filter out incoming errors by reading the status register and masking the unwanted bits.

Ok, missed that.

Quoted 10 lines

Just to clarify something, this patch will wait for only the FATAL error events to be handled by the error handling services only.

Completion Timeout is a NONFATAL error event by default unless somebody tweaks the severity bits.

Anyhow, all FATAL errors cause one sort of link down either initiated by software (AER) or hardware (DPC). Therefore, hotplug driver will observe a link down event and AER/DPC needs to handle the event as usual.

Thanks for the clarification.

Lukas