Re: [PATCH v19 10/14] cxl: Update CXL Endpoint AER handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unconditional system panic on a frozen PCIe link for idle CXL devices due to an incorrect assumption about RAS register mapping.

Pre-existing issues:
- [Medium] Evaluating `BIT(63)` during CXL RAS handling on a dead link causes undefined behavior on 32-bit platforms and truncation on 64-bit platforms.
--

--- Patch [10]: [PATCH v19 10/14] cxl: Update CXL Endpoint AER handler ---
commit 4439ebdc3bdbec21997bf9c57f9c8a4e5e9901ea
Author: Terry Bowman <terry.bowman@amd.com>

cxl: Update CXL Endpoint AER handler

This commit updates the CXL Endpoint AER handler by renaming functions
and documenting the unconditional RAS read policy. It establishes that
reading a dead link returns 0xFFFFFFFF, which is interpreted as UCE
bits being set to trigger a panic.

> 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
[ ... ]
> @@ -343,9 +343,16 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>  		/*
>  		 * The CXL RAS read is unconditional regardless of channel
>  		 * state. Any uncorrectable error bit set in the CXL RAS
> -		 * status register triggers a panic below because CXL.mem
> -		 * cache coherency is already lost; continuing risks silent
> -		 * data corruption.
> +		 * status register triggers a panic because CXL.mem cache
> +		 * coherency is already lost; continuing risks silent data
> +		 * corruption.
> +		 *
> +		 * On a dead link readl() returns 0xFFFFFFFF which sets all
> +		 * UCE bits and also triggers the panic - this is intentional.
> +		 * If RAS registers are not mapped the read is skipped, the
> +		 * panic is not reached, and the frozen/perm_failure switch
> +		 * cases below handle AER recovery for devices without active
> +		 * CXL.mem traffic.
>  		 */
>  		ue = cxl_handle_ras(port, NULL, to_ras_base(port, NULL));
>  	}

[Severity: High]
The comment states that if RAS registers are not mapped, the read is skipped
and AER recovery handles devices without active CXL.mem traffic. Is this
assumption about unmapped RAS registers accurate?

It appears that cxl_endpoint_port_probe() maps RAS registers unconditionally
for all capable endpoints, regardless of CXL.mem activity. If a link freeze
occurs on an idle endpoint, won't this dead link read 0xFFFFFFFF and
trigger an unintended panic, effectively breaking standard PCIe AER recovery
for these devices?


[Severity: Medium]
This is a pre-existing issue, but since this patch intentionally relies on
dead links returning 0xFFFFFFFF during the RAS read, what happens inside
cxl_handle_ras() when we encounter one?

drivers/cxl/core/ras.c:cxl_handle_ras() {
    ...
    if (hweight32(status) > 1) {
        void __iomem *rcc_addr = ras_base + CXL_RAS_CAP_CONTROL_OFFSET;
        fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK, readl(rcc_addr)));
    ...
}

When readl() returns 0xFFFFFFFF on the dead link, FIELD_GET() extracts 63.
Will evaluating BIT(63) cause an out-of-bounds shift on 32-bit platforms,
and won't the 64-bit result be silently truncated to 0 when assigned to the
u32 fe variable?

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