Re: [PATCH 1/5] PCI/AER: Allow drivers to opt in to Bus Reset on Non-Fatal Errors

[Niklas Schnelle <schnelle@linux.ibm.com>]

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 <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).

> @@ -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