Re: [PATCH 3/7] PCI/ERR: Avoid stale error status bits on recovery failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: stale fatal AER status bits left uncleared on recovery failure path
--

commit f65c6a5d555f5001060d8335ce4ca73c0130b547
Author: Yury Murashka <yurypm@arista.com>
PCI/ERR: Avoid stale error status bits on recovery failure

This commit clears error status bits on recovery failure to prevent the
AER driver from falsely identifying unaffected devices as the source of
new errors. It does this by calling pcie_clear_device_status() and
pci_aer_clear_nonfatal_status() on the failure path in pcie_do_recovery().

Link: https://lore.kernel.org/r/CAPzpGcRCTCZtaX1EVaJNZ103THZKsoszZduY7=gwfYdcrMo-SQ@mail.gmail.com/

> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -284,6 +284,11 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  	return status;
>  
>  failed:
> +	if (host->native_aer || pcie_ports_native) {
> +		pcie_clear_device_status(dev);
> +		pci_aer_clear_nonfatal_status(dev);

[Severity: Medium]
When pcie_do_recovery() is invoked for a fatal error (pci_channel_io_frozen)
and recovery fails, will this leave the fatal error bits uncleared?

Looking at pci_aer_clear_nonfatal_status() in drivers/pci/pcie/aer.c, it
explicitly masks out fatal errors before writing back to the status register:

    pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &status);
    pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_SEVER, &sev);
    status &= ~sev;
    if (status)
        pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status);

If a fatal error occurred, the severity bit is 1, so the status bit is never
cleared. Could this leave stale fatal error bits that cause subsequent
is_error_source() checks to falsely identify this device?

Should this failure path use pci_aer_clear_status() or explicitly clear all
uncorrectable errors instead?

> +	}
> +
>  	pci_walk_bridge(bridge, pci_pm_runtime_put, NULL);
>  
>  	pci_walk_bridge(bridge, report_perm_failure_detected, NULL);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790531238.git.lukas@wunner.de?part=3