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

["Bowman, Terry" <terry.bowman@amd.com>]

On 9/2/2026 9:05 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 

The following 2 "new" issues are invalid with reasoning provided further below.

> 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
> 

The next issue is valid but "prior-existing" and I recommend this is addressed 
later in a separate follow-up submission.

> 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?
> 

I agree, the analysis is correct. On a dead link the First Error Pointer
field reads 0x3F (63), and BIT(63) is undefined on 32-bit and truncates to 0
when stored into the u32 fe on 64-bit.
                                                                                                                                                        
This is pre-existing: the fe = BIT(FIELD_GET(...)) encoding in
cxl_handle_ras() is unchanged by this patch, which only reworks the
surrounding comment. It's also diagnostic-only - fe feeds the trace
first_error field and has no bearing on the panic or recovery decision (that
is driven by the UCE status mask, not fe), so the dead-link path documented
here still behaves correctly.

I'd prefer to fix this properly in a follow-up submission rather than fold an
unrelated fix into this rename patch. Something like:

    u32 fe_bit = FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK, readl(rcc_addr));

    /* First Error Pointer is only valid within the UCE status field */
    fe = (fe_bit < 32) ? BIT(fe_bit) : 0;

>> -		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?
> 

No. For a CXL Endpoint, port->uport_dev is the CXL memory device (struct cxl_memdev),
not the PCI device the AER core has locked. 

The AER thread holds device_lock(&pdev->dev). The Endpoint Port is hosted on
a different device (a child of &pdev->dev), so port->uport_dev != &pdev->dev
and device_release_driver(port->uport_dev) acquires a different lock. This is
true for both VH Endpoints and RCDs.

>>  		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?
> 
To begin, RCD doesn't support hotplug, so the concurrent removal this
describes doesn't happen in practice.

Beyond that, I think this is looking at the wrong pair of locks. The concern
assumes an established parent->child device-core lock order, but the RCD
pci_dev is not a device-core child of port->uport_dev (the Host Bridge).
They're in different device hierarchies, PCI bus vs. CXL bus. So there is no
parent/child lock relationship between these two to invert on removal.

For a real AB-BA we'd need a path that holds the Host Bridge lock and then
blocks on the RCD's lock, and there isn't one. The Host Bridge lock is also
unavoidable here, since the RCH Downstream Port's RAS/AER registers live on
the Host Bridge, not the RCD.

-Terry