Re: [PATCH v17 06/11] PCI: Establish common CXL Port protocol error flow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Mixed AER uncorrectable errors and Fatal errors bypass standard PCIe error recovery.
- [High] `cxl_forward_error()` unconditionally writes to `pdev->aer_cap + PCI_ERR_COR_STATUS`, which can corrupt BAR0 if `aer_cap` is 0.
- [Medium] AER Uncorrectable Error status is left uncleared if the CXL port driver is unbound.
--

commit 40776581efad3966047d440ec936219af21ff33c
Author: Terry Bowman <terry.bowman@amd.com>

PCI: Establish common CXL Port protocol error flow

This commit adds CXL Port protocol error handling callbacks to unify
detection, logging, and recovery across CXL Ports and Endpoints. It also
routes CXL protocol errors through the CXL RAS helpers and wires the AER
core to a kfifo for error processing.

> The correctable AER status is cleared by the producer in
> cxl_forward_error().

The patch 'PCI/CXL: Add RCH support to CXL handlers' updates
cxl_rch_handle_error_iter() to call cxl_forward_error() directly for
RCD Endpoints (RCiEPs).

If cxl_forward_error() unconditionally writes to
pdev->aer_cap + PCI_ERR_COR_STATUS to clear the correctable AER status,
what happens if an RCD Endpoint does not expose an AER capability to the
OS and dev->aer_cap is 0?

Would writing to 0 + PCI_ERR_COR_STATUS (offset 0x10) inadvertently
overwrite the PCI Base Address Register 0 (BAR0) and corrupt the device's
memory mapping? 

The original pci_aer_handle_error() explicitly guarded this write by
checking if the aer offset is non-zero.

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

[ ... ]

> @@ -1186,7 +1187,10 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info)
>  static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
>  {
>  	cxl_rch_handle_error(dev, info);
> -	pci_aer_handle_error(dev, info);
> +	if (is_cxl_error(dev, info))
> +		cxl_forward_error(dev, info);
> +	else
> +		pci_aer_handle_error(dev, info);
>  	pci_dev_put(dev);
>  }

Since is_cxl_error() returns true when the CXL Internal Error bit is set,
does this bypass standard PCIe error recovery if a device experiences a
concurrent CXL Internal Error and a standard PCIe Uncorrectable Error
(such as a Data Link Protocol Error)?

Additionally, if the error is fatal, the PCIe link is frozen. Standard
handling in pci_aer_handle_error() would normally call pcie_do_recovery()
to perform a link reset.

Because cxl_do_recovery() only clears the AER status and returns without
resetting the link, could the device be left permanently frozen?

> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c

[ ... ]

> +static int __cxl_proto_err_work_fn(struct cxl_proto_err_work_data *wd)
> +{
> +	struct cxl_dport *dport;
> +	struct cxl_port *port __free(put_cxl_port) =
> +		find_cxl_port_by_dev(&wd->pdev->dev, &dport);
> +
> +	if (!port) {
> +		dev_err_ratelimited(&wd->pdev->dev,
> +				    "Failed to find parent port device in CXL topology\n");
> +		return 0;
> +	}
> +
> +	/*
> +	 * Hold the port device lock and verify a driver is bound before
> +	 * handling errors. Protects against NULL deref if an error is
> +	 * dispatched before probe completion or after driver removal.
> +	 */
> +	guard(device)(&port->dev);
> +	if (!port->dev.driver) {
> +		dev_err_ratelimited(&port->dev,
> +				    "Port device is unbound, abort error handling\n");
> +		return 0;
> +	}
> +
> +	cxl_handle_proto_error(wd->pdev, port, dport, wd->severity);
> +
> +	return 0;
> +}

If the port is not found or the driver is unbound, this function aborts
and returns early before calling cxl_handle_proto_error().

Since handle_error_source() already bypassed the standard PCIe error
recovery, does returning early here leave the AER Uncorrectable Error
status uncleared on the device?

Standard PCIe handling normally ensures these registers are cleared even
if no driver is bound. Could leaving them uncleared block future error
reporting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505173029.2718246-1-terry.bowman@amd.com?part=6