Re: [PATCH v15 5/9] PCI: Establish common CXL Port protocol error flow

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

On 2/3/2026 11:08 PM, dan.j.williams@intel.com wrote:
> Terry Bowman wrote:
>> Introduce CXL Port protocol error handling callbacks to unify detection,
>> logging, and recovery across CXL Ports and Endpoints, including RCH
>> downstream ports. Establish a consistent flow for correctable and
>> uncorrectable CXL protocol errors.
>>
>> Provide the solution by adding cxl_port_cor_error_detected() and
>> cxl_port_error_detected() to handle correctable and uncorrectable handling
>> through CXL RAS helpers, coordinating uncorrectable recovery in
>> cxl_do_recovery(), and panicking when the handler returns PCI_ERS_RESULT_PANIC
>> to preserve fatal cachemem behavior. Gate endpoint handling on the endpoint
>> driver being bound to avoid processing errors on disabled devices.
>>
>> Centralize the RAS base lookup in cxl_get_ras_base(), selecting the
>> downstream-port dport->regs.ras for Root/Downstream Ports and port->regs.ras
>> for Upstream Ports/Endpoints.
>>
>> Export pcie_clear_device_status() and pci_aer_clear_fatal_status() to enable
>> cxl_core to clear PCIe/AER state in these flows.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> Reviewed-by: Dave Jiang dave.jiang@intel.com
>>
>> ---
>>
>> Changes in v14->v15:
>> - Update commit message and title. Added Bjorn's ack.
>> - Move CE and UCE handling logic here
>>
>> Changes in v13->v14:
>> - Add Dave Jiang's review-by
>> - Update commit message & headline (Bjorn)
>> - Refactor cxl_port_error_detected()/cxl_port_cor_error_detected() to
>>   one line (Jonathan)
>> - Remove cxl_walk_port() (Dan)
>> - Remove cxl_pci_drv_bound(). Check for 'is_cxl' parent port is
>>   sufficient (Dan)
>> - Remove device_lock_if()
>> - Combined CE and UCE here (Terry)
>>
>> Changes in v12->v13:
>> - Move get_pci_cxl_host_dev() and cxl_handle_proto_error() to Dequeue
>>   patch (Terry)
>> - Remove EP case in cxl_get_ras_base(), not used. (Terry)
>> - Remove check for dport->dport_dev (Dave)
>> - Remove whitespace (Terry)
>>
>> Changes in v11->v12:
>> - Add call to cxl_pci_drv_bound() in cxl_handle_proto_error() and
>>   pci_to_cxl_dev()
>> - Change cxl_error_detected() -> cxl_cor_error_detected()
>> - Remove NULL variable assignments
>> - Replace bus_find_device() with find_cxl_port_by_uport() for upstream
>>   port searches.
>>
>> Changes in v10->v11:
>> - None
>> ---
>>  drivers/cxl/core/ras.c | 134 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/pci/pci.c      |   1 +
>>  drivers/pci/pci.h      |   2 -
>>  drivers/pci/pcie/aer.c |   1 +
>>  include/linux/aer.h    |   2 +
>>  include/linux/pci.h    |   2 +
>>  6 files changed, 140 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
>> index a6c0bc6d7203..0216dafa6118 100644
>> --- a/drivers/cxl/core/ras.c
>> +++ b/drivers/cxl/core/ras.c
>> @@ -218,6 +218,68 @@ static struct cxl_port *get_cxl_port(struct pci_dev *pdev)
>>  	return NULL;
>>  }
>>  
>> +static void __iomem *cxl_get_ras_base(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	switch (pci_pcie_type(pdev)) {
>> +	case PCI_EXP_TYPE_ROOT_PORT:
>> +	case PCI_EXP_TYPE_DOWNSTREAM:
>> +	{
> 
> Nit, clang-format puts that { on the same line because coding style says
> only functions get newlines for open brackets.
> 

Hi Dan,

Thanks for the note. Would you like every switch-case to be upodated to match the clang 
recommended format?

>> +		struct cxl_dport *dport;
>> +		struct cxl_port *port __free(put_cxl_port) = find_cxl_port(&pdev->dev, &dport);
>> +
>> +		if (!dport) {
>> +			pci_err(pdev, "Failed to find the CXL device");
>> +			return NULL;
>> +		}
>> +		return dport->regs.ras;
>> +	}
>> +	case PCI_EXP_TYPE_UPSTREAM:
>> +	case PCI_EXP_TYPE_ENDPOINT:
>> +	{
>> +		struct cxl_port *port __free(put_cxl_port) = find_cxl_port_by_uport(&pdev->dev);
>> +
>> +		if (!port) {
>> +			pci_err(pdev, "Failed to find the CXL device");
>> +			return NULL;
>> +		}
>> +		return port->regs.ras;
>> +	}
>> +	}
>> +	dev_warn_once(dev, "Error: Unsupported device type (%#x)", pci_pcie_type(pdev));
>> +	return NULL;
>> +}
>> +
>> +static pci_ers_result_t cxl_port_error_detected(struct device *dev);
>> +
>> +static void cxl_do_recovery(struct pci_dev *pdev)
>> +{
>> +	struct cxl_port *port __free(put_cxl_port) = get_cxl_port(pdev);
>> +	pci_ers_result_t status;
>> +
>> +	if (!port) {
>> +		pci_err(pdev, "Failed to find the CXL device\n");
>> +		return;
>> +	}
>> +
>> +	status = cxl_port_error_detected(&pdev->dev);
>> +	if (status == PCI_ERS_RESULT_PANIC)
>> +		panic("CXL cachemem error.");
>> +
>> +	/*
>> +	 * If we have native control of AER, clear error status in the device
>> +	 * that detected the error.  If the platform retained control of AER,
>> +	 * it is responsible for clearing this status.  In that case, the
>> +	 * signaling device may not even be visible to the OS.
>> +	 */
> 
> This comment feels more appropriate as documentation for
> pcie_aer_is_native(). CXL is just using for the same purpose as all the
> other callers. You can maybe reference "See pcie_aer_is_native() for
> expecations on clearing errors", but I otherwise would not expect CXL to
> carry its own paragraph.
> 

Agreed. I’ll drop the local comment and rely on pcie_aer_is_native() semantics,
with a brief reference if needed.


>> +	if (pcie_aer_is_native(pdev)) {
>> +		pcie_clear_device_status(pdev);
>> +		pci_aer_clear_nonfatal_status(pdev);
>> +		pci_aer_clear_fatal_status(pdev);
>> +	}
>> +}
>> +
>>  void cxl_handle_cor_ras(struct device *dev, u64 serial, void __iomem *ras_base)
>>  {
>>  	void __iomem *addr;
>> @@ -288,6 +350,60 @@ bool cxl_handle_ras(struct device *dev, u64 serial, void __iomem *ras_base)
>>  	return true;
>>  }
>>  
>> +static void cxl_port_cor_error_detected(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct cxl_port *port __free(put_cxl_port) = get_cxl_port(pdev);
>> +
>> +	if (is_cxl_endpoint(port)) {
>> +		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>> +		struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +
>> +		guard(device)(&cxlmd->dev);
>> +
>> +		if (!dev->driver) {
>> +			dev_warn(&pdev->dev,
>> +				 "%s: memdev disabled, abort error handling\n",
>> +				 dev_name(dev));
>> +			return;
>> +		}
>> +
>> +		if (cxlds->rcd)
>> +			cxl_handle_rdport_errors(cxlds);
> 
> Isn't this dead code? Only VH topologies will ever get a forwarded CXL
> error, right? I realize it gets deleted in a future patch, but then why
> leave dead code in the git history?
> 
                            
Yes, agreed - I'll remove. Correct, only VH is forwarded. My understanding is the 
cxl_memdev guard and driver check are no longer required here. The memdev is only 
used to source the serial number, so I’ll refactor accordingly. Please correct
me if Im wrong.

I see an additional fix needed: cxl_rch_handle_error_iter() in pci/pcie/aer.c
also needs its callbacks updated. The RCH/RCD path previously invoked the EP
PCIe handlers, but with RAS now handled at the port level, those callbacks no
longer reach the correct logic.

I had a coupled ideas. One options is for the CXL logic to make a callback into a 
cxl_core exported function such as cxl_handle_rdport_errors(). BTW, the CXL logic in 
AER and the CXL driver's RAS are both built with the CONFIG_CXL_RAS config.

Another option is updating the CXL PCIe callbacks. The cxl_pci PCI error callbacks 
currently support only AER and could be updated to also support RCH/RCD (no VH) with 
something along the lines of below?

static bool cxl_pci_detected(struct pci_dev *pdev)                                                                                                      
{                                                                                                                                                     
 ...
     if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_EC &&
            is_aer_internal_error(info))
		cxl_handle_rdport_errors();

In this case we would also need a cxl_pci_cor_detected(). 


>> +
>> +		cxl_handle_cor_ras(dev, cxlds->serial, cxl_get_ras_base(dev));
>> +	} else {
>> +		cxl_handle_cor_ras(dev, 0, cxl_get_ras_base(dev));
>> +	}
>> +}
>> +
>> +static pci_ers_result_t cxl_port_error_detected(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct cxl_port *port __free(put_cxl_port) = get_cxl_port(pdev);
>> +
>> +	if (is_cxl_endpoint(port)) {
>> +		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>> +		struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +
>> +		guard(device)(&cxlmd->dev);
>> +
>> +		if (!dev->driver) {
>> +			dev_warn(&pdev->dev,
>> +				 "%s: memdev disabled, abort error handling\n",
>> +				 dev_name(dev));
>> +			return PCI_ERS_RESULT_NONE;
>> +		}
>> +
>> +		if (cxlds->rcd)
>> +			cxl_handle_rdport_errors(cxlds);
>> +
>> +		return cxl_handle_ras(dev, cxlds->serial, cxl_get_ras_base(dev));
>> +	} else {
>> +		return cxl_handle_ras(dev, 0, cxl_get_ras_base(dev));
>> +	}
>> +}
>> +
>>  void cxl_cor_error_detected(struct pci_dev *pdev)
>>  {
>>  	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>> @@ -363,6 +479,24 @@ EXPORT_SYMBOL_NS_GPL(cxl_error_detected, "CXL");
>>  
>>  static void cxl_handle_proto_error(struct cxl_proto_err_work_data *err_info)
>>  {
>> +	struct pci_dev *pdev = err_info->pdev;
>> +
>> +	if (err_info->severity == AER_CORRECTABLE) {
>> +
>> +		if (!pcie_aer_is_native(pdev))
>> +			return;
>> +
>> +		if (pdev->aer_cap)
>> +			pci_clear_and_set_config_dword(pdev,
>> +						       pdev->aer_cap + PCI_ERR_COR_STATUS,
>> +						       0, PCI_ERR_COR_INTERNAL);
>> +
>> +		cxl_port_cor_error_detected(&pdev->dev);
>> +
>> +		pcie_clear_device_status(pdev);
>> +	} else {
>> +		cxl_do_recovery(pdev);
>> +	}
>>  }
>>  
>>  static void cxl_proto_err_work_fn(struct work_struct *work)
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 13dbb405dc31..b7bfefdaf990 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -2248,6 +2248,7 @@ void pcie_clear_device_status(struct pci_dev *dev)
>>  	pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &sta);
>>  	pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
>>  }
>> +EXPORT_SYMBOL_GPL(pcie_clear_device_status);
> 
> No reason to open up this symbol to the world. Only cxl_core.ko needs
> this exported, and hopefully we never see another bus that abuses PCI
> like CXL does ever again.
> 
> [..]

Understood. I’ll switch this to a CXL‑scoped export.

>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index 7af10a74da34..4fc9de4c78f8 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -298,6 +298,7 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
>>  	if (status)
>>  		pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status);
>>  }
>> +EXPORT_SYMBOL_GPL(pci_aer_clear_fatal_status);
> 
> ditto, too wide of an export.
OK

-Terry