Re: [RESEND v13 19/25] cxl/pci: Introduce CXL protocol error handlers for Endpoints

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Tue, 4 Nov 2025 11:02:59 -0600
Terry Bowman <terry.bowman@amd.com> wrote:

> CXL Endpoint protocol errors are currently handled by generic PCI error
> handlers. However, uncorrectable errors (UCEs) require CXL.mem protocol-
> specific handling logic that the PCI handlers cannot provide.
> 
> Add dedicated CXL protocol error handlers for CXL Endpoints. Rename the
> existing cxl_error_handlers to pci_error_handlers to better reflect their
> purpose and maintain naming consistency. Update the PCI error handlers to
> invoke the new CXL protocol handlers when the endpoint is operating in
> CXL.mem mode.
> 
> Implement cxl_handle_ras() to return PCI_ERS_RESULT_NONE or
> PCI_ERS_RESULT_PANIC. Remove unnecessary result checks from the previous
> endpoint UCE handler since CXL UCE recovery is not implemented in this
> patch.
> 
> Add device lock assertions to protect against concurrent device or RAS
> register removal during error handling. Two devices require locking for
> CXL endpoints:
> 
> 1. The PCI device (pdev->dev) - RAS registers are allocated and mapped
>    using devm_* functions with this device as the host. Locking prevents
>    the RAS registers from being unmapped until after error handling
>    completes.
> 
> 2. The CXL memory device (cxlmd->dev) - Holds a reference to the RAS
>    registers accessed during error handling. Locking prevents the memory
>    device and its RAS register references from being removed during error
>    handling.
> 
> The lock assertions added here will be satisfied by device locks
> introduced in a subsequent patch. A future patch will extend the CXL UCE
> handler to support full UCE recovery.
> 
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
Hi Terry,

A few comments inline.

Thanks,

Jonathan


> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index cb712772de5c..beb142054bda 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -128,6 +128,11 @@ void cxl_ras_exit(void)
>  	cancel_work_sync(&cxl_cper_prot_err_work);
>  }
>  
> +static bool is_pcie_endpoint(struct pci_dev *pdev)
> +{
> +	return pci_pcie_type(pdev) == PCI_EXP_TYPE_ENDPOINT;
> +}

Not used that I can see. Maybe should be in a different patch?


>  }
>  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)
> +{
> +	struct cxl_dev_state *cxlds;
> +
> +	device_lock_assert(&pdev->dev);
> +	if (!cxl_pci_drv_bound(pdev))
> +		return;
> +
> +	cxlds = pci_get_drvdata(pdev);
> +	guard(device)(&cxlds->cxlmd->dev);
> +
> +	cxl_cor_error_detected(&pdev->dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(pci_cor_error_detected, "CXL");

Similarly to below.  I'm not keen on exporting such generic PCI
sounding functions even in the CXL namespace.

> +
> +pci_ers_result_t cxl_error_detected(struct device *dev)
>  {
> -	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> -	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> -	struct device *dev = &cxlmd->dev;
> -	bool ue;
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  
> -	guard(device)(dev);
> +	device_lock_assert(cxlds->dev);
> +	device_lock_assert(&cxlmd->dev);
>  
>  	if (!dev->driver) {
> -		dev_warn(&pdev->dev,
> +		dev_warn(cxlds->dev,
>  			 "%s: memdev disabled, abort error handling\n",
>  			 dev_name(dev));
>  		return PCI_ERS_RESULT_DISCONNECT;
> @@ -289,32 +308,34 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>  
>  	if (cxlds->rcd)
>  		cxl_handle_rdport_errors(cxlds);
> +
I'd drop this blank line addition as it doesn't matter much and it does
add noise to the patch.

>  	/*
>  	 * 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.
>  	 */

Mind you - I think this comment wants to go away as it's talking about code
that is no longer here.


> -	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:
> -		dev_warn(&pdev->dev,
> -			 "%s: frozen state error detected, disable CXL.mem\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;
> +	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)
> +{
> +	struct cxl_dev_state *cxlds;
> +	pci_ers_result_t rc;
> +
> +	device_lock_assert(&pdev->dev);
> +	if (!cxl_pci_drv_bound(pdev))
> +		return PCI_ERS_RESULT_NONE;
> +
> +	cxlds = pci_get_drvdata(pdev);
> +	guard(device)(&cxlds->cxlmd->dev);
> +
> +	rc = cxl_error_detected(&cxlds->cxlmd->dev);
> +	if (rc == PCI_ERS_RESULT_PANIC)
> +		panic("CXL cachemem error.");
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(pci_error_detected, "CXL");

Whilst the symbol is namespaced, I'm not sure I want to see
an exported CXL specific function that sounds so generic pci.

Maybe cxl_pci_error_detected() or something like that?

> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index a0a491e7b5b9..3526e6d75f79 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -79,21 +79,10 @@ 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);
>  void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host);
>  void cxl_uport_init_ras_reporting(struct cxl_port *port,
>  				  struct device *host);
>  #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;
> -}
> -
>  static inline void cxl_dport_init_ras_reporting(struct cxl_dport *dport,
>  						struct device *host) { }
>  static inline void cxl_uport_init_ras_reporting(struct cxl_port *port,