Message-ID: <7d54aa5d-bef9-4611-b0ae-04279bb7dae2@amd.com>
Date: Tue, 30 Sep 2025 09:06:37 -0500
From: "Bowman, Terry" <terry.bowman@....com>
To: Dave Jiang <dave.jiang@...el.com>, dave@...olabs.net,
 jonathan.cameron@...wei.com, alison.schofield@...el.com,
 dan.j.williams@...el.com, bhelgaas@...gle.com, shiju.jose@...wei.com,
 ming.li@...omail.com, Smita.KoralahalliChannabasappa@....com,
 rrichter@....com, dan.carpenter@...aro.org,
 PradeepVineshReddy.Kodamati@....com, lukas@...ner.de,
 Benjamin.Cheatham@....com, sathyanarayanan.kuppuswamy@...ux.intel.com,
 linux-cxl@...r.kernel.org, alucerop@....com, ira.weiny@...el.com
Cc: linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v12 17/25] cxl/pci: Introduce CXL Endpoint protocol error
 handlers



On 9/26/2025 5:04 PM, Dave Jiang wrote:
> On 9/25/25 3:34 PM, Terry Bowman wrote:
>> CXL Endpoint protocol errors are currently handled using PCI error
>> handlers. The CXL Endpoint requires CXL specific handling in the case of
>> uncorrectable error (UCE) handling not provided by the PCI handlers.
>>
>> Add CXL specific handlers for CXL Endpoints. Rename the existing
>> cxl_error_handlers to be pci_error_handlers to more correctly indicate
>> the error type and follow naming consistency.
>>
>> The PCI handlers will be called if the CXL device is not trained for
>> alternate protocol (CXL). Update the CXL Endpoint PCI handlers to call the
>> CXL UCE handlers.
>>
>> The existing EP UCE handler includes checks for various results. These are
>> no longer needed because CXL UCE recovery will not be attempted. Implement
>> cxl_handle_ras() to return PCI_ERS_RESULT_NONE or PCI_ERS_RESULT_PANIC. The
>> CXL UCE handler is called by cxl_do_recovery() that acts on the return
>> value. In the case of the PCI handler path, call panic() if the result is
>> PCI_ERS_RESULT_PANIC.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@....com>
>> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>>
>> ---
>>
>> Changes in v11->v12:
>> - None
>>
>> Changes in v10->v11:
>> - cxl_error_detected() - Change handlers' scoped_guard() to guard() (Jonathan)
>> - cxl_error_detected() - Remove extra line (Shiju)
>> - Changes moved to core/ras.c (Terry)
>> - cxl_error_detected(), remove 'ue' and return with function call. (Jonathan)
>> - Remove extra space in documentation for PCI_ERS_RESULT_PANIC definition
>> - Move #include "pci.h from cxl.h to core.h (Terry)
>> - Remove unnecessary includes of cxl.h and core.h in mem.c (Terry)
>> ---
>>  drivers/cxl/core/core.h |  17 +++++++
>>  drivers/cxl/core/ras.c  | 110 +++++++++++++++++++---------------------
>>  drivers/cxl/cxlpci.h    |  15 ------
>>  drivers/cxl/pci.c       |   9 ++--
>>  4 files changed, 75 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index 8c51a2631716..74c64d458f12 100644
>> --- a/drivers/cxl/core/core.h
>> +++ b/drivers/cxl/core/core.h
>> @@ -6,6 +6,7 @@
>>  
>>  #include <cxl/mailbox.h>
>>  #include <linux/rwsem.h>
>> +#include <linux/pci.h>
>>  
>>  extern const struct device_type cxl_nvdimm_bridge_type;
>>  extern const struct device_type cxl_nvdimm_type;
>> @@ -150,6 +151,11 @@ void cxl_ras_exit(void);
>>  void cxl_switch_port_init_ras(struct cxl_port *port);
>>  void cxl_endpoint_port_init_ras(struct cxl_port *ep);
>>  void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host);
>> +pci_ers_result_t pci_error_detected(struct pci_dev *pdev,
>> +				    pci_channel_state_t error);
>> +void pci_cor_error_detected(struct pci_dev *pdev);
>> +void cxl_cor_error_detected(struct device *dev);
>> +pci_ers_result_t cxl_error_detected(struct device *dev);
>>  #else
>>  static inline int cxl_ras_init(void)
>>  {
>> @@ -163,6 +169,17 @@ static inline void cxl_switch_port_init_ras(struct cxl_port *port) { }
>>  static inline void cxl_endpoint_port_init_ras(struct cxl_port *ep) { }
>>  static inline void cxl_dport_init_ras_reporting(struct cxl_dport *dport,
>>  						struct device *host) { }
>> +static inline pci_ers_result_t pci_error_detected(struct pci_dev *pdev,
>> +						  pci_channel_state_t error)
>> +{
>> +	return PCI_ERS_RESULT_NONE;
>> +}
>> +static inline void pci_cor_error_detected(struct pci_dev *pdev) { }
>> +static inline void cxl_cor_error_detected(struct device *dev) { }
>> +static inline pci_ers_result_t cxl_error_detected(struct device *dev)
>> +{
>> +	return PCI_ERS_RESULT_NONE;
>> +}
>>  #endif // CONFIG_CXL_RAS
>>  
>>  int cxl_gpf_port_setup(struct cxl_dport *dport);
>> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
>> index 14a434bd68f0..39472d82d586 100644
>> --- a/drivers/cxl/core/ras.c
>> +++ b/drivers/cxl/core/ras.c
>> @@ -129,7 +129,7 @@ void cxl_ras_exit(void)
>>  }
>>  
>>  static void cxl_handle_cor_ras(struct device *dev, u64 serial, void __iomem *ras_base);
>> -static bool cxl_handle_ras(struct device *dev, u64 serial, void __iomem *ras_base);
>> +static pci_ers_result_t cxl_handle_ras(struct device *dev, u64 serial, void __iomem *ras_base);
>>  
>>  #ifdef CONFIG_CXL_RCH_RAS
>>  static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
>> @@ -371,7 +371,7 @@ static void header_log_copy(void __iomem *ras_base, u32 *log)
>>   * Log the state of the RAS status registers and prepare them to log the
>>   * next error status. Return 1 if reset needed.
>>   */
>> -static bool cxl_handle_ras(struct device *dev, u64 serial, void __iomem *ras_base)
>> +static pci_ers_result_t cxl_handle_ras(struct device *dev, u64 serial, void __iomem *ras_base)
>>  {
>>  	u32 hl[CXL_HEADERLOG_SIZE_U32];
>>  	void __iomem *addr;
>> @@ -380,13 +380,13 @@ static bool cxl_handle_ras(struct device *dev, u64 serial, void __iomem *ras_bas
>>  
>>  	if (!ras_base) {
>>  		dev_warn_once(dev, "CXL RAS register block is not mapped");
>> -		return false;
>> +		return PCI_ERS_RESULT_NONE;
>>  	}
>>  
>>  	addr = ras_base + CXL_RAS_UNCORRECTABLE_STATUS_OFFSET;
>>  	status = readl(addr);
>>  	if (!(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK))
>> -		return false;
>> +		return PCI_ERS_RESULT_NONE;
>>  
>>  	/* If multiple errors, log header points to first error from ctrl reg */
>>  	if (hweight32(status) > 1) {
>> @@ -403,76 +403,72 @@ static bool cxl_handle_ras(struct device *dev, u64 serial, void __iomem *ras_bas
>>  	trace_cxl_aer_uncorrectable_error(dev, status, fe, hl, serial);
>>  	writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
>>  
>> -	return true;
>> +	return PCI_ERS_RESULT_PANIC;
>>  }
>>  
>> -void cxl_cor_error_detected(struct pci_dev *pdev)
>> +void cxl_cor_error_detected(struct device *dev)
> Why change the input parameter to 'struct device' to just convert it back in the first parameter? I understand that later on cxl_handle_proto_error() will pass in a 'dev', but since it's going to be a pci_dev anyways, can you just pass in a pci_dev instead of doing all this back and forth?

Dan made a point in previous revision that handling functions should work on 
devices (to include the parameter). This is to be consistent with CXL device/port error 
handling rather than PCIe error handling. Let me know how to proceed.

Terry
>>  {
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>>  	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>> -	struct device *dev = &cxlds->cxlmd->dev;
>> +	struct device *cxlmd_dev = &cxlds->cxlmd->dev;
>>  
>> -	scoped_guard(device, dev) {
>> -		if (!dev->driver) {
>> -			dev_warn(&pdev->dev,
>> -				 "%s: memdev disabled, abort error handling\n",
>> -				 dev_name(dev));
>> -			return;
>> -		}
>> +	guard(device)(cxlmd_dev);
>>  
>> -		if (cxlds->rcd)
>> -			cxl_handle_rdport_errors(cxlds);
>> -
>> -		cxl_handle_cor_ras(&cxlds->cxlmd->dev, cxlds->serial, cxlds->regs.ras);
>> +	if (!cxlmd_dev->driver) {
>> +		dev_warn(&pdev->dev, "%s: memdev disabled, abort error handling", dev_name(dev));
>> +		return;
>>  	}
>> +
>> +	if (cxlds->rcd)
>> +		cxl_handle_rdport_errors(cxlds);
>> +
>> +	cxl_handle_cor_ras(&cxlds->cxlmd->dev, cxlds->serial, cxlds->regs.ras);
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_cor_error_detected, "CXL");
>>  
>> -pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>> -				    pci_channel_state_t state)
>> +void pci_cor_error_detected(struct pci_dev *pdev)
>>  {
>> +	cxl_cor_error_detected(&pdev->dev);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(pci_cor_error_detected, "CXL");
>> +
>> +pci_ers_result_t cxl_error_detected(struct device *dev)
> Same comment as above.
>
> DJ
>
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>>  	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>> -	struct cxl_memdev *cxlmd = cxlds->cxlmd;
>> -	struct device *dev = &cxlmd->dev;
>> -	bool ue;
>> +	struct device *cxlmd_dev = &cxlds->cxlmd->dev;
>>  
>> -	scoped_guard(device, dev) {
>> -		if (!dev->driver) {
>> -			dev_warn(&pdev->dev,
>> -				 "%s: memdev disabled, abort error handling\n",
>> -				 dev_name(dev));
>> -			return PCI_ERS_RESULT_DISCONNECT;
>> -		}
>> +	guard(device)(cxlmd_dev);
>>  
>> -		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.
>> -		 */
>> -		ue = cxl_handle_ras(&cxlds->cxlmd->dev, cxlds->serial, cxlds->regs.ras);
>> -	}
>> -
>> -
>> -	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:
>> +	if (!dev->driver) {
>>  		dev_warn(&pdev->dev,
>> -			 "%s: frozen state error detected, disable CXL.mem\n",
>> +			 "%s: memdev disabled, abort error handling\n",
>>  			 dev_name(dev));
>> -		device_release_driver(dev);
>> -		return PCI_ERS_RESULT_NEED_RESET;
>> -	case pci_channel_io_perm_failure:
>> -		dev_warn(&pdev->dev,
>> -			 "failure state error detected, request disconnect\n");
>>  		return PCI_ERS_RESULT_DISCONNECT;
>>  	}
>> -	return PCI_ERS_RESULT_NEED_RESET;
>> +
>> +	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.
>> +	 */
>> +	return cxl_handle_ras(&cxlds->cxlmd->dev, cxlds->serial, cxlds->regs.ras);
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_error_detected, "CXL");
>> +
>> +pci_ers_result_t pci_error_detected(struct pci_dev *pdev,
>> +				    pci_channel_state_t error)
>> +{
>> +	pci_ers_result_t rc;
>> +
>> +	rc = cxl_error_detected(&pdev->dev);
>> +	if (rc == PCI_ERS_RESULT_PANIC)
>> +		panic("CXL cachemem error.");
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(pci_error_detected, "CXL");
>> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
>> index 3882a089ae77..189cd8fabc2c 100644
>> --- a/drivers/cxl/cxlpci.h
>> +++ b/drivers/cxl/cxlpci.h
>> @@ -77,19 +77,4 @@ static inline bool cxl_pci_flit_256(struct pci_dev *pdev)
>>  int devm_cxl_port_enumerate_dports(struct cxl_port *port);
>>  struct cxl_dev_state;
>>  void read_cdat_data(struct cxl_port *port);
>> -
>> -#ifdef CONFIG_CXL_RAS
>> -void cxl_cor_error_detected(struct pci_dev *pdev);
>> -pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>> -				    pci_channel_state_t state);
>> -#else
>> -static inline void cxl_cor_error_detected(struct pci_dev *pdev) { }
>> -
>> -static inline pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>> -						  pci_channel_state_t state)
>> -{
>> -	return PCI_ERS_RESULT_NONE;
>> -}
>> -#endif
>> -
>>  #endif /* __CXL_PCI_H__ */
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index bd95be1f3d5c..71fb8709081e 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -16,6 +16,7 @@
>>  #include "cxlpci.h"
>>  #include "cxl.h"
>>  #include "pmu.h"
>> +#include "core/core.h"
>>  
>>  /**
>>   * DOC: cxl pci
>> @@ -1112,11 +1113,11 @@ static void cxl_reset_done(struct pci_dev *pdev)
>>  	}
>>  }
>>  
>> -static const struct pci_error_handlers cxl_error_handlers = {
>> -	.error_detected	= cxl_error_detected,
>> +static const struct pci_error_handlers pci_error_handlers = {
>> +	.error_detected	= pci_error_detected,
>>  	.slot_reset	= cxl_slot_reset,
>>  	.resume		= cxl_error_resume,
>> -	.cor_error_detected	= cxl_cor_error_detected,
>> +	.cor_error_detected	= pci_cor_error_detected,
>>  	.reset_done	= cxl_reset_done,
>>  };
>>  
>> @@ -1124,7 +1125,7 @@ static struct pci_driver cxl_pci_driver = {
>>  	.name			= KBUILD_MODNAME,
>>  	.id_table		= cxl_mem_pci_tbl,
>>  	.probe			= cxl_pci_probe,
>> -	.err_handler		= &cxl_error_handlers,
>> +	.err_handler		= &pci_error_handlers,
>>  	.dev_groups		= cxl_rcd_groups,
>>  	.driver	= {
>>  		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,

Powered by blists - more mailing lists