Re: [PATCH v20 5/9] cxl: Update CXL Endpoint AER handler

[sashiko-bot@kernel.org]

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

New issues:
- [High] Self-deadlock with PCI AER core via device_release_driver()
- [High] AB-BA lock inversion between PCI AER core and CXL port uport_dev

Pre-existing issues:
- [High] Undefined behavior and truncation on BIT(63) during dead link RAS handling
--

commit 2f0ed965e221155153683db0b8960c5aeb8afd4e
Author: Terry Bowman <terry.bowman@amd.com>

cxl: Update CXL Endpoint AER handler
    
This patch renames cxl_error_detected() to cxl_pci_error_detected() and aligns
error handler names. It also modifies the RAS read policy to be unconditional
and documents the behavior on dead links.

> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index e307361bb39e4..bf479fac08565 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c

[ ... ]

> -pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> -				    pci_channel_state_t state)
> -{
> -	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> -	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> -	struct device *dev = &cxlmd->dev;
> -	bool ue;
> -
> -	scoped_guard(device, dev) {
> -		if (!dev->driver) {
> -			dev_warn(&pdev->dev,
> -				 "%s: memdev disabled, abort error handling\n",
> -				 dev_name(dev));
> +				 "%s: port disabled, abort error handling\n",
> +				 dev_name(&port->dev));
>  			return PCI_ERS_RESULT_DISCONNECT;
>  		}
>  
> -		if (cxlds->rcd)
> -			cxl_handle_rdport_errors(cxlds);
>  		/*
> -		 * A frozen channel indicates an impending reset which is fatal to
> -		 * CXL.mem operation, and will likely crash the system. On the off
> -		 * chance the situation is recoverable dump the status of the RAS
> -		 * capability registers and bounce the active state of the memdev.
> +		 * The CXL RAS uncorrectable status is the only signal here
> +		 * that the error is a CXL internal (protocol) error. A set
> +		 * UCE bit confirms it and triggers the panic below. On a dead
> +		 * link readl() returns 0xFFFFFFFF, which sets all UCE bits and
> +		 * triggers the panic intentionally.
> +		 *
> +		 * If RAS is not mapped the read is skipped. Unlike
> +		 * cxl_do_recovery(), which is reached only after
> +		 * is_aer_internal_error() has already confirmed a CXL internal
> +		 * UCE, this path has no such confirmation, so an unmapped RAS
> +		 * block cannot attribute the error to CXL and must not panic.
> +		 * The switch cases below then handle AER recovery.
>  		 */

[Severity: High]
This is a pre-existing issue, but this patch explicitly documents that
reading the RAS registers on a dead link returning 0xFFFFFFFF is
intentional.

If readl() returns 0xFFFFFFFF, inside cxl_handle_ras(), hweight32(status) > 1
is true, so it evaluates:

    fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK,
                       readl(rcc_addr)));

Since the dead link causes readl() to return 0xFFFFFFFF, FIELD_GET will
extract bits 0-5 and yield 63. Will evaluating BIT(63) trigger undefined
behavior on 32-bit platforms and silent truncation on 64-bit platforms?

> -		ue = cxl_handle_ras(&cxlds->cxlmd->dev, cxlmd->endpoint->regs.ras);
> +		ue = cxl_handle_ras(port, NULL, to_ras_base(port, NULL));
>  	}
>  
> +	/*
> +	 * CXL.mem UCE means cache coherency is lost. Continuing risks
> +	 * silent data corruption.
> +	 */
> +	if (ue)
> +		panic("CXL cachemem error");
> +
>  	switch (state) {
>  	case pci_channel_io_normal:
> -		if (ue) {
> -			device_release_driver(dev);
> -			return PCI_ERS_RESULT_NEED_RESET;
> -		}
>  		return PCI_ERS_RESULT_CAN_RECOVER;
>  	case pci_channel_io_frozen:
>  		dev_warn(&pdev->dev,
>  			 "%s: frozen state error detected, disable CXL.mem\n",
> -			 dev_name(dev));
> -		device_release_driver(dev);
> +			 dev_name(port->uport_dev));
> +		device_release_driver(port->uport_dev);

[Severity: High]
The AER core calls report_error_detected() which acquires
device_lock(&pdev->dev) before invoking this driver callback.

Because port is resolved via find_cxl_port_by_uport(&pdev->dev),
port->uport_dev is identical to &pdev->dev. Will calling
device_release_driver(port->uport_dev) here unconditionally attempt to
acquire the exact lock that the AER thread is already holding, resulting in
a self-deadlock?

>  		return PCI_ERS_RESULT_NEED_RESET;

[ ... ]

> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
> index e0e01aa5eba6c..bc6cf50fcc926 100644
> --- a/drivers/cxl/core/ras_rch.c
> +++ b/drivers/cxl/core/ras_rch.c

[ ... ]

> -void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
> -{
> -	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	struct aer_capability_regs aer_regs;
>  	struct cxl_dport *dport;
> -	int severity;
>  
> -	struct cxl_port *port __free(put_cxl_port) =
> -		cxl_pci_find_port(pdev, &dport);
> +	struct cxl_port *port __free(put_cxl_port) = cxl_pci_find_port(pdev, NULL);
>  	if (!port)
>  		return;
>  
> +	guard(device)(port->uport_dev);

[Severity: High]
The AER core executes cxl_pci_error_detected() with device_lock(&pdev->dev)
already held, which synchronously calls this function for an RCD.

For an RCD, port->uport_dev resolves to the ACPI root or host bridge, which
is an ancestor of pdev. Will locking this ancestor device while holding a
descendant's lock create an AB-BA deadlock with concurrent device removal
operations that lock from parent to child?

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