Re: [PATCH v16 05/10] PCI: Establish common CXL Port protocol error flowUIRE

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

On Mon, 2 Mar 2026 14:36:43 -0600
Terry Bowman <terry.bowman@amd.com> wrote:

> Introduce CXL Port protocol error handling callbacks to unify detection,
> logging, and recovery across CXL Ports and Endpoints. Establish a consistent
> flow for correctable and uncorrectable CXL protocol errors. Support for RCH
> Downstream Port error handling will be added in a future patch.
> 
> 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>
> 
Hi Terry,

It's been long enough I've pretty much forgotten what this does, so
fresh review.  Might well be commenting on things that have come up
before!

Jonathan

> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 44791f6d7d50..1d4be2d78469 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c

> +static u64 cxl_serial_number(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct cxl_port *port __free(put_cxl_port) = get_cxl_port(pdev);
> +	struct device *port_dev = port ? port->uport_dev : NULL;

Maybe someone else asked for this, but to my eyes it's too complex.
Would be simpler to drop this local variable and

> +	struct cxl_memdev *cxlmd;
> +

	if (!port || !port->uport_dev || !is_cxl_memdev(dev))
		return 0;

	cxlmd = to_cxl_memdev(port->uport_dev);
..

> +	if (!port_dev || !is_cxl_memdev(dev))
> +		return 0;
> +
> +	cxlmd = to_cxl_memdev(port_dev);
> +	return cxlmd->cxlds->serial;
> +}

> +static void cxl_do_recovery(struct pci_dev *pdev)
> +{
> +	struct cxl_port *port __free(put_cxl_port) = get_cxl_port(pdev);
> +	struct device *dev = &pdev->dev;
> +	pci_ers_result_t status;
> +
> +	if (!port) {
> +		pci_err(pdev, "Failed to find the CXL device\n");
> +		return;
> +	}
> +
> +	status =  cxl_handle_ras(dev, cxl_serial_number(dev), cxl_get_ras_base(dev));
Extra space after = 

> +	if (status == PCI_ERS_RESULT_PANIC)
> +		panic("CXL cachemem error.");
> +
> +	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;
> @@ -327,3 +428,71 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,

> +
> +static void cxl_proto_err_work_fn(struct work_struct *work)
> +{
> +	struct cxl_proto_err_work_data wd;
> +
> +	/*
> +	 * Dequeue work forwarded from the AER driver
> +	 * See cxl_forward_error() for matching pci_dev_get()
> +	 */
> +	while (cxl_proto_err_kfifo_get(&wd)) {
> +		struct pci_dev *pdev __free(pci_dev_put) = wd.pdev;

I'm not particularly keen on the lack of constructor / destructor pairing this is giving
us but the alternatives are fiddly.  You could make cxl_proto_err_kfifo_get() return wd
/ ERR_PTR() and use a new DEFINE_FREE() for that structure. But it would require
memory allocation or a dance.


This would become something like
	struct cxl_proto_err_work_data scratch;

	whilst(true) {
		struct cxl_proto_err_work_data *wd __free(cxl_proto_err_work_d) =
			cxl_proto_err_kfifo_get(scratch);
		if (IS_ERR(wd))
			return;

		struct cxl_port *port __free(put_cxl_port) = get_cxl_port(wd->pdev);

...


	}

Hmm. Also not exactly elegant. Unless others have a better idea, let us stick
to what you have.

> +		struct cxl_port *port __free(put_cxl_port) = get_cxl_port(pdev);
> +
> +		if (!port) {
> +			pr_err_ratelimited("%s: Failed to find parent port device in CXL topology\n",
> +					   pci_name(pdev));
> +			continue;
> +		}
> +
> +		guard(device)(&port->dev);
> +		if (!port->dev.driver) {
> +			pr_err_ratelimited("%s: Port device is unbound, abort error handling\n",
> +					    dev_name(&port->dev));
> +			continue;
> +		}
> +
> +		cxl_handle_proto_error(pdev, wd.severity);
> +	}
> +}