Re: [PATCH v8 05/16] PCI/AER: CXL driver dequeues CXL error forwarded from AER service driver

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

On Wed, 26 Mar 2025 20:47:06 -0500
Terry Bowman <terry.bowman@amd.com> wrote:

> The AER driver is now designed to forward CXL protocol errors to the CXL
> driver. Update the CXL driver with functionality to dequeue the forwarded
> CXL error from the kfifo. Also, update the CXL driver to process the CXL
> protocol errors using CXL protocol error handlers.
> 
> First, move cxl_rch_handle_error_iter() from aer.c to cxl/core/ras.c.
> Remove and drop the cxl_rch_handle_error() in aer.c as it is not needed.
> 
> Introduce function cxl_prot_err_work_fn() to dequeue work forwarded by the
> AER service driver. This will begin the CXL protocol error processing
> with the call to cxl_handle_prot_error().
> 
> Introduce cxl_handle_prot_error() to differntiate between Restricted CXL
> Host (RCH) protocol errors and CXL virtual host (VH) protocol errors.
> RCH errors will be processed with a call to walk the associated Root
> Complex Event Collector's (RCEC) secondary bus looking for the Root Complex
> Integrated Endpoint (RCiEP) to handle the RCH error. Export pcie_walk_rcec()
> so the CXL driver can walk the RCEC's downstream bus, searching for
> the RCiEP.
> 
> VH correctable error (CE) processing will call the CXL CE handler if
> present. VH uncorrectable errors (UCE) will call cxl_do_recovery(),
> implemented as a stub for now and to be updated in future patch. Export
> pci_aer_clean_fatal_status() and pci_clean_device_status() used to clean up
> AER status after handling.
> 
> Create cxl_driver::error_handler structure similar to
> pci_driver::error_handlers. Add handlers for CE and UCE CXL.io errors. Add
> 'struct cxl_prot_error_info' as a parameter to the CXL CE and UCE error
> handlers.
> 
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> ---
>  drivers/cxl/core/ras.c  | 102 +++++++++++++++++++++++++++++++++++++++-
>  drivers/cxl/cxl.h       |  17 +++++++
>  drivers/pci/pci.c       |   1 +
>  drivers/pci/pci.h       |   6 ---
>  drivers/pci/pcie/aer.c  |  42 +----------------
>  drivers/pci/pcie/rcec.c |   1 +
>  include/linux/aer.h     |   2 +
>  include/linux/pci.h     |  10 ++++
>  8 files changed, 133 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index ecb60a5962de..eca8f11a05d9 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -139,8 +139,108 @@ int cxl_create_prot_err_info(struct pci_dev *_pdev, int severity,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_create_prot_err_info, "CXL");
>  
> -struct work_struct cxl_prot_err_work;
> +static void cxl_do_recovery(struct pci_dev *pdev) { }
> +
> +static int cxl_rch_handle_error_iter(struct pci_dev *pdev, void *data)
> +{
> +	struct cxl_prot_error_info *err_info = data;
> +	const struct cxl_error_handlers *err_handler;
> +	struct device *dev = err_info->dev;
> +	struct cxl_driver *pdrv;
> +
> +	/*
> +	 * The capability, status, and control fields in Device 0,
> +	 * Function 0 DVSEC control the CXL functionality of the
> +	 * entire device (CXL 3.0, 8.1.3).
> +	 */
> +	if (pdev->devfn != PCI_DEVFN(0, 0))
> +		return 0;
> +
> +	/*
> +	 * CXL Memory Devices must have the 502h class code set (CXL
> +	 * 3.0, 8.1.12.1).
> +	 */
> +	if ((pdev->class >> 8) != PCI_CLASS_MEMORY_CXL)
> +		return 0;
> +
> +	if (!is_cxl_memdev(dev) || !dev->driver)
> +		return 0;
> +
> +	pdrv = to_cxl_drv(dev->driver);
> +	if (!pdrv || !pdrv->err_handler)
> +		return 0;
> +
> +	err_handler = pdrv->err_handler;
> +	if (err_info->severity == AER_CORRECTABLE) {
> +		if (err_handler->cor_error_detected)
> +			err_handler->cor_error_detected(dev, err_info);
> +	} else if (err_handler->error_detected) {
> +		cxl_do_recovery(pdev);
> +	}
> +
> +	return 0;
> +}
> +
> +static void cxl_handle_prot_error(struct pci_dev *pdev, struct cxl_prot_error_info *err_info)
> +{
> +	if (!pdev || !err_info)

Are these real potential conditions?  If so can we have a comment on why.
If this is defensive only, do we need it? 
Looks like the caller below checked pdev already.

> +		return;
> +
> +	/*
> +	 * Internal errors of an RCEC indicate an AER error in an
> +	 * RCH's downstream port. Check and handle them in the CXL.mem
> +	 * device driver.
> +	 */
> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_EC)
> +		return pcie_walk_rcec(pdev, cxl_rch_handle_error_iter, err_info);
> +
> +	if (err_info->severity == AER_CORRECTABLE) {
> +		struct device *dev __free(put_device) = get_device(err_info->dev);

Similar question around lifetimes. The caller already got this. Why again?

> +		struct cxl_driver *pdrv;

calling a cxl driver pdrv seems odd.  cdrv maybe?

> +		int aer = pdev->aer_cap;
> +
> +		if (!dev || !dev->driver)
> +			return;
> +
> +		if (aer) {
> +			int ras_status;
> +
> +			pci_read_config_dword(pdev, aer + PCI_ERR_COR_STATUS, &ras_status);

If we get multiple bits set in this register, can this wipe out ones we haven't noticed
anywhere else in the handling?  Bad tlp etc.  Maybe we need to ensure this only clears
the internal error bit?

> +			pci_write_config_dword(pdev, aer + PCI_ERR_COR_STATUS,
> +					       ras_status);
> +		}
> +
> +		pdrv = to_cxl_drv(dev->driver);
> +		if (!pdrv || !pdrv->err_handler ||
> +		    !pdrv->err_handler->cor_error_detected)
> +			return;
> +
> +		pdrv->err_handler->cor_error_detected(dev, err_info);
> +		pcie_clear_device_status(pdev);
> +	} else {
> +		cxl_do_recovery(pdev);
> +	}
> +}
> +
> +static void cxl_prot_err_work_fn(struct work_struct *work)
> +{
> +	struct cxl_prot_err_work_data wd;
> +
> +	while (cxl_prot_err_kfifo_get(&wd)) {
> +		struct cxl_prot_error_info *err_info = &wd.err_info;
> +		struct device *dev __free(put_device) = get_device(err_info->dev);
> +		struct pci_dev *pdev __free(pci_dev_put) = pci_dev_get(err_info->pdev);
> +
> +		if (!dev || !pdev)
> +			continue;
> +
> +		cxl_handle_prot_error(pdev, err_info);
> +	}
> +}
> +
> +static DECLARE_WORK(cxl_prot_err_work, cxl_prot_err_work_fn);

Ah! Here it is... I think this can be in patch 3. With a stub of the function
(which is what the patch 3 description claims is there).

>  

Jonathan