Re: [RFC PATCH v2 2/2] PCI/CXL: Enable usage of RDPAS to shortcut error device discovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect fall-through logic in `cxl_rch_get_err_src_id()` extracts and returns the wrong error source ID for correctable errors.
- [High] The RDPAS list is built concurrently without locking and traversed locklessly in the AER error handler, exposing readers to partially built lists or list corruption.
- [Low] The function `cxl_forward_error` is invoked but is neither declared nor defined anywhere in the kernel.
--

commit e9dabb60b82f98b2295a3d242546827da2279bbf
Author: Dave Jiang <dave.jiang@intel.com>

PCI/CXL: Enable usage of RDPAS to shortcut error device discovery

This commit changes the CXL RCH error handler to probe the RCRB first to
see if the error device can be discovered quickly, falling back to iterating
through RCiEPs if needed. It introduces RDPAS parsing to directly locate the
RCRB and determine the BDF of the error device.

diff --git a/drivers/pci/pcie/aer_cxl_rch.c b/drivers/pci/pcie/aer_cxl_rch.c
--- a/drivers/pci/pcie/aer_cxl_rch.c
+++ b/drivers/pci/pcie/aer_cxl_rch.c

[ ... ]

> +static u16 cxl_rch_get_err_src_id(u64 rcrb_base, struct aer_err_info *info)
> +{
> +	u32 root_status, err_src;

[ ... ]

> +	if (info->severity == AER_CORRECTABLE &&
> +	    root_status & PCI_ERR_ROOT_COR_RCV)
> +		return FIELD_GET(GENMASK(15, 0), err_src);
> +
> +	/* Assume at this point the info->severity points to UNCOR */
> +	if (root_status & PCI_ERR_ROOT_UNCOR_RCV)
> +		return FIELD_GET(GENMASK(31, 16), err_src);
> +
> +	return 0;
> +}

[Severity: High]
If this is handling a correctable error but PCI_ERR_ROOT_COR_RCV is not set,
is it intentional to fall through to the uncorrectable error check?

If an uncorrectable error is concurrently logged in the RCRB status, this
would return the source ID of the uncorrectable error instead of 0, which
might misroute the correctable error notification.

Should these severity checks be mutually exclusive using an else block?

> +static bool cxl_rch_forward_error_by_dsp(struct pci_dev *rcec, u64 rcrb_base,
> +					 struct aer_err_info *info)
> +{

[ ... ]

> +	if (!is_cxl_mem_dev(pdev) || !cxl_error_is_native(pdev))
> +		return false;
> +
> +	cxl_forward_error(pdev, info);

[Severity: Low]
Does cxl_forward_error() exist in the kernel tree?

I'm having trouble finding its declaration or definition in the codebase,
which seems like it might cause an implicit function declaration build failure.

> +	return true;
> +}
> +
> +static bool cxl_rch_handled_error_by_rdpas(struct pci_dev *rcec,
> +					   struct aer_err_info *info)
> +{
> +	struct cxl_rdpas_rcec *rdpas_rcec;
> +	struct cxl_rdpas_entry *entry;
> +	bool handled = false;
> +
> +	rdpas_rcec = cxl_get_rdpas_by_rcec(rcec);
> +	if (!rdpas_rcec)
> +		return false;
> +
> +	/*
> +	 * The RCEC aggregates multiple downstream ports. Each CXL.io
> +	 * downstream port associated with this RCEC exposes the RCRB at its
> +	 * base address; walk them all and forward the error from every port
> +	 * that reports a valid error source.
> +	 */
> +	list_for_each_entry(entry, &rdpas_rcec->ports, list) {

[Severity: High]
Could this lockless list traversal race with the initialization in
__cxl_parse_rdpas()?

If an asynchronous AER hardware event triggers cxl_rch_handle_error() while
__cxl_parse_rdpas() is still running, it seems we might walk rdpas_rcec->ports
using list_for_each_entry() while it is concurrently being mutated by
list_add_tail().

Since the rdpas_rcec container is published to the cxl_rdpas xarray before
the list is fully populated, would this expose readers to partially built
lists or use-after-free conditions? Additionally, would the lockless check
of rdpas_parsed in cxl_rdpas_init() allow concurrent probes to race and
corrupt the list?

> +		if (entry->protocol != ACPI_CEDT_RDPAS_PROTOCOL_IO)
> +			continue;
> +
> +		if (cxl_rch_forward_error_by_dsp(rcec, entry->address, info))
> +			handled = true;
> +	}
> +
> +	return handled;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618170723.2010490-1-dave.jiang@intel.com?part=2