Re: [PATCH v2 2/2] PCI/AER: Check validity of Error Source ID register

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] PCI/AER: false Root Port match in is_error_source() skips
  downstream search
--

commit 96874e2f4a0f4f66402c752cd7b61e3904f79ff3
Author: Lukas Wunner <lukas@wunner.de>

PCI/AER: Check validity of Error Source ID register

This commit replaces the bus-zero heuristic for PCI_ERR_ROOT_ERR_SRC with a
check that the Error Source ID matches the Root Port or RCEC itself or falls
within its subordinate or associated buses and devices.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=222095

> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index d492c7ce01e15..6f24602314225 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
[ ... ]
> @@ -1146,6 +1147,24 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>  	return 0;
>  }
>  
> +static bool source_id_valid(struct pci_dev *root, u16 id)
> +{
> +	/* The Root Port or RCEC can itself be a valid Error Source */
> +	if (pci_dev_id(root) == id)
> +		return true;
> +
> +	/* For RCECs, @id shall be in range of its associated buses/devices */
> +	if (pci_pcie_type(root) == PCI_EXP_TYPE_RC_EC)
> +		return pci_rcec_associated(root, id);
> +
> +	/* For Root Ports, @id shall be in range of its subordinate buses */
> +	if (!root->subordinate)
> +		return false;
> +
> +	return root->subordinate->busn_res.start <= PCI_BUS_NUM(id) &&
> +	       PCI_BUS_NUM(id) <= root->subordinate->busn_res.end;
> +}
> +
>  /**
>   * is_error_source - check whether the device is source of reported error
>   * @dev: pointer to pci_dev to be checked
> @@ -1157,11 +1176,8 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
>  	u32 status, mask;
>  	u16 reg16;
>  
> -	/*
> -	 * When bus ID is equal to 0, it might be a bad ID
> -	 * reported by Root Port.
> -	 */
> -	if ((PCI_BUS_NUM(e_info->id) != 0) &&
> +	/* Some Root Ports report an invalid Error Source ID */
> +	if (e_info->id_valid &&
>  	    !(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
>  		/* Device ID match? */
>  		if (e_info->id == pci_dev_id(dev))

[Severity: Medium]
As reported by Ilpo Järvinen on v1 [1], can treating pci_dev_id(root) == id
as unconditionally valid cause a false-positive match on the Root Port and
skip searching the subordinate hierarchy when a Root Port at 00:00.0 (such
as on DesignWare, Cadence, or Aardvark SoC host bridges) reports 0x0000 for
a downstream device error, or when a Root Port at 00:xx.y loses the bus
number of a downstream error source at bus:xx.y?

Previously, PCI_BUS_NUM(e_info->id) != 0 prevented an Error Source ID on bus
0 from matching the Root Port by ID alone, so is_error_source() fell through
to check the Root Port's PCI_ERR_COR_STATUS or PCI_ERR_UNCOR_STATUS
registers.  With e_info->id_valid set to true, find_source_device() ->
find_device_iter() -> is_error_source() matches the Root Port on ID alone
and returns before walking parent->subordinate (for single errors).

When aer_process_err_devices() then calls aer_get_device_error_info() on the
Root Port, it finds no unmasked error bits set in the Root Port's own AER
status register and returns 0:

drivers/pci/pcie/aer.c:aer_get_device_error_info() {
    ...
	if (info->severity == AER_CORRECTABLE) {
		pci_read_config_dword(dev, aer + PCI_ERR_COR_STATUS,
			&info->status);
		pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK,
			&info->mask);
		if (!(info->status & ~info->mask))
			return 0;
	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
		   type == PCI_EXP_TYPE_RC_EC ||
		   type == PCI_EXP_TYPE_DOWNSTREAM ||
		   info->severity == AER_NONFATAL) {

		/* Link is still healthy for IO reads */
		pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,
			&info->status);
		pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
			&info->mask);
		if (!(info->status & ~info->mask))
			return 0;
    ...
}

As a result, the downstream device's AER error goes unlogged, its AER status
register is not cleared, and pcie_do_recovery() is skipped for non-fatal and
fatal errors.  Note that find_source_device() already checks the Root Port
first via find_device_iter() and returns before walking parent->subordinate
whenever the Root Port has an unmasked error bit set in its own AER status
register.

[1] https://lore.kernel.org/r/97e38070-1ad1-74d3-3b60-7fd2ead00c42@linux.intel.com

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