Re: [PATCH 1/5] PCI/AER: Allow drivers to opt in to Bus Reset on Non-Fatal Errors
From: Niklas Schnelle <schnelle@linux.ibm.com>
Date: 2025-08-14 07:56:30
Also in:
linux-pci
On Wed, 2025-08-13 at 07:11 +0200, Lukas Wunner wrote:
When Advanced Error Reporting was introduced in September 2006 by commit
6c2b374d7485 ("PCI-Express AER implemetation: AER core and aerdriver"), it
sought to adhere to the recovery flow and callbacks specified in
Documentation/PCI/pci-error-recovery.rst.--- snip ---quoted hunk ↗ jump to hunk
Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/pcie/err.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index de6381c690f5..e795e5ae6b03 100644 --- a/drivers/pci/pcie/err.c +++ b/drivers/pci/pcie/err.c@@ -217,15 +217,10 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, pci_walk_bridge(bridge, pci_pm_runtime_get_sync, NULL); pci_dbg(bridge, "broadcast error_detected message\n"); - if (state == pci_channel_io_frozen) { + if (state == pci_channel_io_frozen) pci_walk_bridge(bridge, report_frozen_detected, &status); - if (reset_subordinates(bridge) != PCI_ERS_RESULT_RECOVERED) { - pci_warn(bridge, "subordinate device reset failed\n"); - goto failed; - } - } else { + else pci_walk_bridge(bridge, report_normal_detected, &status); - } if (status == PCI_ERS_RESULT_CAN_RECOVER) { status = PCI_ERS_RESULT_RECOVERED;
On s390 PCI errors leave the device with MMIO blocked until either the error state is cleared or we reset via the firmware interface. With this change and the pci_channel_io_frozen case AER would now do the report_mmio_enabled() before the reset with nothing happening between report_frozen_detected() and report_mmio_enabled() is MMIO enabled at this point? I think this callback really only makes sense if you have an equivalent to s390's clearing of the error state that enables MMIO but doesn't otherwise reset. Similarly EEH has eeh_pci_enable(pe, EEH_OPT_THAW_MMIO).
quoted hunk ↗ jump to hunk
@@ -233,6 +228,14 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, pci_walk_bridge(bridge, report_mmio_enabled, &status); } + if (status == PCI_ERS_RESULT_NEED_RESET || + state == pci_channel_io_frozen) { + if (reset_subordinates(bridge) != PCI_ERS_RESULT_RECOVERED) { + pci_warn(bridge, "subordinate device reset failed\n"); + goto failed; + } + } + if (status == PCI_ERS_RESULT_NEED_RESET) { /* * TODO: Should call platform-specific
I wonder if it might make sense to merge the reset into the above existing if. That would also work well with Sathyanarayanan's suggestion to have state == pci_channel_io_frozen upgrade to PCI_ERS_RESULT_NEED_RESET. I'm a bit confused by that TODO comment and anyway, it asks for a platform-specific reset to be added bu reset_subordinate() already seems to allow platform specific behavior, so maybe the moved reset should replace the TODO? Also I think for the kind of broken case where the state is pci_channel_io_frozen but the drivers just reports PCI_ERS_RESULT_CAN_RECOVER it looks like there would be a reset but no calls to ->slot_reset(). Thanks, Niklas