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 | 18 ++++++++++++++++
drivers/pci/pci.h | 2 ++
drivers/pci/pcie/err.c | 34 +++++++++++++++++++++++++++++++
3 files changed, 54 insertions(+)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index da7c72372ffc..22354b6850c3 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -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.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index c358e7a07f3f..e2d98654630b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index f7ce0cb0b0b7..b1b5604cb00b 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -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;
+}
--
2.17.1
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.
Signed-off-by: Sinan Kaya <[email protected]>
---
drivers/pci/hotplug/pciehp_core.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index ec48c9433ae5..8322db8f369a 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -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, ®32);
+ 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);
}
--
2.17.1
On 8/18/2018 2:51 AM, Sinan Kaya wrote:
> cleanup_slot(ctrl);
> + 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.
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, ®32);
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
On Fri, Aug 17, 2018 at 11:51:10PM -0700, Sinan Kaya wrote:
> +static int pciehp_control_surprise_error(struct controller *ctrl, bool enable)
The return value isn't checked, so this could return void.
> @@ -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
On Fri, Aug 17, 2018 at 11:51:09PM -0700, Sinan Kaya wrote:
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -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));
> + }
> +
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
On 8/20/2018 5:22 AM, Lukas Wunner wrote:
>> +
> 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.
On Mon, Aug 20, 2018 at 12:59:05PM -0400, Sinan Kaya wrote:
> On 8/20/2018 5:22 AM, Lukas Wunner wrote:
> > > +
> > 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.
Ok, missed that.
> 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
On 8/20/2018 4:21 AM, Lukas Wunner wrote:
> On Fri, Aug 17, 2018 at 11:51:10PM -0700, Sinan Kaya wrote:
>> +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.
>
>> @@ -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.
I could move this code if we know that is_hotplug_bridge flag is set
regardless of OS hotplug driver control or not.
>
> (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.
>
> Thanks,
>
> Lukas
>