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 ---
> Signed-off-by: Lukas Wunner <lu...@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).
> @@ -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
