Re: [PATCH 7/7] PCI/AER: Enable baseline capability error reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci/aer: uncorrectable errors dropped for non-AER devices due to DEVSTA clear
- [Medium] pci/aer: leaked mask/first_error state to non-AER devices
--

commit 29da5e077bdaa4990a1072f7b27ccf2bb9cae518
Author: Lukas Wunner <lukas@wunner.de>

    PCI/AER: Enable baseline capability error reporting
    
    This commit enables PCIe baseline capability error reporting for devices
    that do not support AER. It relies on the error enable/status bits in the
    Device Control and Device Status registers. This completes the kernel's
    support for native error reporting.

Link: https://lore.kernel.org/r/20260826212619.GA1566339@bhelgaas/

> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 6bc843ab9b37f..62376aab4b6f1 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
[ ... ]
> @@ -1182,8 +1183,11 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
>  	if (!(reg16 & PCI_EXP_AER_FLAGS))
>  		return false;
>  
> -	if (!aer)
> -		return false;
> +	if (!aer) {
> +		pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &reg16);
> +		return reg16 & BIT(e_info->severity) &&
> +		       !PCI_POSSIBLE_ERROR(reg16);
> +	}

[Severity: High]
When a Root Port receives both a correctable and an uncorrectable error,
aer_isr_one_error() processes the correctable error first.

During this, pci_aer_handle_error() ends by unconditionally calling
pcie_clear_device_status():

pci_aer_handle_error() {
    ...
        if (pdrv && pdrv->err_handler &&
            pdrv->err_handler->cor_error_detected)
            pdrv->err_handler->cor_error_detected(dev);
        pcie_clear_device_status(dev);
}

This clears all error bits in PCI_EXP_DEVSTA (CED, NFED, FED, and URD).

Because this new logic in is_error_source() relies entirely on the NFED or
FED bit in PCI_EXP_DEVSTA to locate uncorrectable errors on non-AER devices,
won't this cause pending uncorrectable errors to be silently dropped?

[ ... ]
> @@ -1455,6 +1459,7 @@ int aer_get_device_error_info(struct aer_err_info *info, int i)
>  {
>  	struct pci_dev *dev;
>  	int type, aer;
> +	u16 devsta;
>  
>  	if (i >= AER_MAX_MULTI_ERR_DEVICES)
>  		return 0;
> @@ -1470,8 +1475,15 @@ int aer_get_device_error_info(struct aer_err_info *info, int i)
>  	info->is_cxl = pcie_is_cxl(dev);
>  
>  	/* The device might not support AER */
> -	if (!aer)
> -		return 0;
> +	if (!aer) {
> +		if (info->severity != AER_CORRECTABLE) {
> +			pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &devsta);
> +			if (devsta & PCI_EXP_DEVSTA_URD &&
> +			    !PCI_POSSIBLE_ERROR(devsta))
> +				info->status = PCI_ERR_UNC_UNSUP;
> +		}
> +		return 1;
> +	}

[Severity: Medium]
When aer_process_err_devices() processes multiple devices in a loop, it uses
a shared 'info' structure. Since this early return for non-AER devices
doesn't reset info->mask or info->first_error, will it leak state from the
previously processed AER device?

This leaked mask could be used by pci_dev_aer_stats_incr() to filter errors,
potentially dropping statistics for valid errors if the leaked mask covers
the PCI_ERR_UNC_UNSUP bit.

Should info->mask and info->first_error be explicitly cleared here?

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