[PATCH] PCI/AER: Add a kernel parameter to skip the reset during AER recovery

[PATCH] PCI/AER: Add a kernel parameter to skip the reset during AER recovery

[Sunny Feng]

After an uncorrectable error, AER recovery resets the hierarchy below
the Root Port in aer_root_reset() (a Secondary Bus Reset, or an FLR for
an RCEC or RCiEP). Some devices keep diagnostic state, in the device
itself or in the retimers on its link, that is lost by that reset. To
root cause the error, that state needs to be collected before the
device is reset.

Add a "pci=error_reset_disable=<pci_dev>[; ...]" kernel parameter,
using the same device format as "disable_acs_redir". When a device
below the Root Port matches, aer_root_reset() skips the reset, leaves
the devices in the state the error left them in and reports the
recovery as failed. The Root Error Status is still cleared and the
Root Port interrupts re-enabled, so later errors are still reported.
The diagnostics can then be collected before the devices are recovered.

Signed-off-by: Sunny Feng <sunnfeng@amazon.com>
---
 .../admin-guide/kernel-parameters.txt         | 10 +++++
 drivers/pci/pci.c                             | 41 ++++++++++++++++++-
 drivers/pci/pci.h                             |  1 +
 drivers/pci/pcie/aer.c                        | 27 +++++++++++-
 4 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 68647ff4bdd2..eac6f72dcc2a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5329,6 +5329,16 @@ Kernel parameters
 
 				Note: this may remove isolation between devices
 				and may put more devices in an IOMMU group.
+		error_reset_disable=<pci_dev>[; ...]
+				Specify one or more PCI devices (in the format
+				specified above) separated by semicolons.
+				AER recovery does not reset a hierarchy that
+				contains one of these devices after an
+				uncorrectable error. Recovery is not attempted:
+				the devices are left in the state the error
+				left them in and are reported as failed, so
+				their diagnostics can be collected before they
+				are recovered.
 		force_floating	[S390] Force usage of floating interrupts.
 		nomio		[S390] Do not use MIO instructions.
 		norid		[S390] ignore the RID field and force use of
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c5ed5da9cdc8..2b4a06f3a38e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -901,6 +901,41 @@ void pci_request_acs(void)
 
 static const char *disable_acs_redir_param;
 static const char *config_acs_param;
+static const char *error_reset_disable_param;
+
+/**
+ * pci_error_reset_disabled - should AER recovery leave a device unreset
+ * @dev: the PCI device to test
+ *
+ * Returns true if @dev is listed in the "pci=error_reset_disable=" kernel
+ * parameter, in which case AER recovery must not reset the hierarchy
+ * that contains it.
+ */
+bool pci_error_reset_disabled(struct pci_dev *dev)
+{
+	const char *p = error_reset_disable_param;
+	int ret;
+
+	if (!p)
+		return false;
+
+	while (*p) {
+		ret = pci_dev_str_match(dev, p, &p);
+		if (ret < 0) {
+			pr_info_once("PCI: Can't parse error_reset_disable parameter: %s\n",
+				     error_reset_disable_param);
+			return false;
+		} else if (ret == 1) {
+			return true;
+		}
+
+		if (*p != ';' && *p != ',')
+			return false;
+		p++;
+	}
+
+	return false;
+}
 
 struct pci_acs {
 	u16 ctrl;
@@ -6883,6 +6918,8 @@ static int __init pci_setup(char *str)
 				disable_acs_redir_param = str + 18;
 			} else if (!strncmp(str, "config_acs=", 11)) {
 				config_acs_param = str + 11;
+			} else if (!strncmp(str, "error_reset_disable=", 20)) {
+				error_reset_disable_param = str + 20;
 			} else {
 				pr_err("PCI: Unknown option `%s'\n", str);
 			}
@@ -6894,7 +6931,8 @@ static int __init pci_setup(char *str)
 early_param("pci", pci_setup);
 
 /*
- * 'resource_alignment_param' and 'disable_acs_redir_param' are initialized
+ * 'resource_alignment_param', 'disable_acs_redir_param' and
+ * 'error_reset_disable_param' are initialized
  * in pci_setup(), above, to point to data in the __initdata section which
  * will be freed after the init sequence is complete. We can't allocate memory
  * in pci_setup() because some architectures do not have any memory allocation
@@ -6908,6 +6946,7 @@ static int __init pci_realloc_setup_params(void)
 					   GFP_KERNEL);
 	disable_acs_redir_param = kstrdup(disable_acs_redir_param, GFP_KERNEL);
 	config_acs_param = kstrdup(config_acs_param, GFP_KERNEL);
+	error_reset_disable_param = kstrdup(error_reset_disable_param, GFP_KERNEL);
 
 	return 0;
 }
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a002fe1a3f4c..364c99e1a6a1 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -244,6 +244,7 @@ bool pci_reset_supported(struct pci_dev *dev);
 void pci_init_reset_methods(struct pci_dev *dev);
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
 int pci_bus_error_reset(struct pci_dev *dev);
+bool pci_error_reset_disabled(struct pci_dev *dev);
 int pci_try_reset_bridge(struct pci_dev *bridge);
 
 struct pci_cap_saved_data {
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index c376c3adbefd..1f403f0f5d25 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1837,6 +1837,16 @@ static int aer_resume(struct pcie_device *dev)
 	return 0;
 }
 
+/* pci_walk_bus() callback; returns 0 so the walk visits every device */
+static int aer_check_error_reset_disabled(struct pci_dev *dev, void *data)
+{
+	bool *disabled = data;
+
+	if (pci_error_reset_disabled(dev))
+		*disabled = true;
+	return 0;
+}
+
 /**
  * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
  * @dev: pointer to Root Port, RCEC, or RCiEP
@@ -1849,6 +1859,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 	struct pci_dev *root;
 	int aer;
 	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
+	bool reset_disabled = false;
 	u32 reg32;
 	int rc;
 
@@ -1872,7 +1883,21 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 	if (host->native_aer && aer)
 		aer_disable_irq(root);
 
-	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
+	/*
+	 * Leave the devices in the state the error left them in if one of
+	 * them is listed in the error_reset_disable parameter, so their
+	 * diagnostics can be collected before they are recovered.
+	 */
+	if (dev->subordinate)
+		pci_walk_bus(dev->subordinate, aer_check_error_reset_disabled,
+			     &reset_disabled);
+	else
+		aer_check_error_reset_disabled(dev, &reset_disabled);
+
+	if (reset_disabled) {
+		rc = -ECANCELED;
+		pci_info(dev, "not reset, error_reset_disable matches a device below\n");
+	} else if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
 		rc = pcie_reset_flr(dev, PCI_RESET_DO_RESET);
 		if (!rc)
 			pci_info(dev, "has been reset\n");

base-commit: 573bbee68354c1a3566b6451d5477416c3be9462
-- 
2.47.3

Re: [PATCH] PCI/AER: Add a kernel parameter to skip the reset during AER recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci/aer: interrupt storm when skipping AER reset
--

commit fe869440b3515f272e9e500d4db9fdb37085a98f
Author: Sunny Feng <sunnfeng@amazon.com>

PCI/AER: Add a kernel parameter to skip the reset during AER recovery

This patch introduces a "pci=error_reset_disable=" kernel parameter to skip
resetting specific PCI devices during AER recovery. This allows the devices
to remain in their errored state so diagnostics can be collected, while
still re-enabling Root Port interrupts.

> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index c376c3adbefd2..1f403f0f5d258 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c

[ ... ]

> @@ -1872,7 +1883,21 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>  	if (host->native_aer && aer)
>  		aer_disable_irq(root);
>  
> -	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
> +	/*
> +	 * Leave the devices in the state the error left them in if one of
> +	 * them is listed in the error_reset_disable parameter, so their
> +	 * diagnostics can be collected before they are recovered.
> +	 */
> +	if (dev->subordinate)
> +		pci_walk_bus(dev->subordinate, aer_check_error_reset_disabled,
> +			     &reset_disabled);
> +	else
> +		aer_check_error_reset_disabled(dev, &reset_disabled);
> +
> +	if (reset_disabled) {
> +		rc = -ECANCELED;
> +		pci_info(dev, "not reset, error_reset_disable matches a device below\n");

[Severity: High]
Could this cause an interrupt storm if a device is skipped from AER reset?

When a device is skipped via the error_reset_disable parameter, it is left
in a broken state with its Uncorrectable Error Status asserted.

Later in aer_root_reset(), the code unconditionally clears the Root Error
Status and re-enables Root Port AER interrupts via aer_enable_irq(root).

According to the PCIe Base Specification Rev 6.0, Section 6.2.4.2
(Multiple Error Handling), a device is allowed to generate additional Error
Messages for subsequent errors of the same type even if the status bit is
already set.

If the un-reset device continues generating errors (for example, due to a
degraded link or ongoing background DMA), the Root Port will continually
receive messages and fire interrupts.

Since the aer_irq() ISR clears the Root Error Status and returns even when
the event queue is full:

drivers/pci/pcie/aer.c:aer_irq() {
    ...
    pci_write_config_dword(rp, aer + PCI_ERR_ROOT_STATUS, e_src.status);

    if (!kfifo_put(&rpc->aer_fifo, e_src))
        return IRQ_HANDLED;
    ...
}

Will this lead to an infinite hard interrupt storm loop, potentially
causing a denial of service?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013201045.33806-1-sunnfeng@amazon.com?part=1