On Wed, Mar 26, 2025 at 08:47:04PM -0500, Terry Bowman wrote:
> CXL error handling will soon be moved from the AER driver into the CXL
> driver. This requires a notification mechanism for the AER driver to share
> the AER interrupt details with CXL driver. The notification is required for
> the CXL drivers to then handle CXL RAS errors.
>
> Add a kfifo work queue to be used by the AER driver and CXL driver. The AER
> driver will be the sole kfifo producer adding work. The cxl_core will be
> the sole kfifo consumer removing work. Add the boilerplate kfifo support.
>
> Add CXL work queue handler registration functions in the AER driver. Export
> the functions allowing CXL driver to access. Implement the registration
> functions for the CXL driver to assign or clear the work handler function.
>
> Create a work queue handler function, cxl_prot_err_work_fn(), as a stub for
> now. The CXL specific handling will be added in future patch.
>
> Introduce 'struct cxl_prot_err_info'. This structure caches CXL error
> details used in completing error handling. This avoid duplicating some
> function calls and allows the error to be treated generically when
> possible.
> ...
> +++ b/drivers/cxl/core/ras.c
> @@ -5,6 +5,7 @@
> #include <linux/aer.h>
> #include <cxl/event.h>
> #include <cxlmem.h>
> +#include <cxlpci.h>
> #include "trace.h"
>
> static void cxl_cper_trace_corr_port_prot_err(struct pci_dev *pdev,
> @@ -107,13 +108,64 @@ static void cxl_cper_prot_err_work_fn(struct work_struct *work)
> }
> static DECLARE_WORK(cxl_cper_prot_err_work, cxl_cper_prot_err_work_fn);
>
> +int cxl_create_prot_err_info(struct pci_dev *_pdev, int severity,
> + struct cxl_prot_error_info *err_info)
> +{
> + struct pci_dev *pdev __free(pci_dev_put) = pci_dev_get(_pdev);
> + struct cxl_dev_state *cxlds;
> +
> + if (!pdev || !err_info) {
> + pr_warn_once("Error: parameter is NULL");
> + return -ENODEV;
This is CXL code, so your call, but I'm always skeptical about testing
for NULL and basically ignoring a code defect that got us here with a
NULL pointer. I would rather take the NULL pointer dereference fault
and force a fix in the caller.
> + }
> +
> + if ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT) &&
> + (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)) {
> + pci_warn_once(pdev, "Error: Unsupported device type (%X)", pci_pcie_type(pdev));
> + return -ENODEV;
Similar. A pci_warn_once() here seems like a debugging aid during
development, not necessarily a production kind of thing.
Thanks for printing the type. I would use "%#x" to make it clear that
it's hex. There are about 1900 %X uses compared with 33K
%x uses, but maybe you have a reason to capitalize it?
> + }
> +
> + cxlds = pci_get_drvdata(pdev);
> + struct device *dev __free(put_device) = get_device(&cxlds->cxlmd->dev);
> +
> + if (!dev)
> + return -ENODEV;
> +
> + *err_info = (struct cxl_prot_error_info){ 0 };
Neat, I hadn't seen this idiom.
> + err_info->ras_base = cxlds->regs.ras;
> + err_info->severity = severity;
> + err_info->pdev = pdev;
> + err_info->dev = dev;
> +
> + return 0;
> +}
> +
> +struct work_struct cxl_prot_err_work;
> +
> int cxl_ras_init(void)
> {
> - return cxl_cper_register_prot_err_work(&cxl_cper_prot_err_work);
> + int rc;
> +
> + rc = cxl_cper_register_prot_err_work(&cxl_cper_prot_err_work);
> + if (rc) {
> + pr_err("Failed to register CPER kfifo with AER driver");
> + return rc;
> + }
> +
> + rc = cxl_register_prot_err_work(&cxl_prot_err_work, cxl_create_prot_err_info);
> + if (rc) {
> + pr_err("Failed to register kfifo with AER driver");
> + return rc;
> + }
> +
> + return rc;
> }
>
> void cxl_ras_exit(void)
> {
> cxl_cper_unregister_prot_err_work(&cxl_cper_prot_err_work);
> cancel_work_sync(&cxl_cper_prot_err_work);
> +
> + cxl_unregister_prot_err_work();
> + cancel_work_sync(&cxl_prot_err_work);
> }
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 54e219b0049e..92d72c0423ab 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -4,6 +4,7 @@
> #define __CXL_PCI_H__
> #include <linux/pci.h>
> #include "cxl.h"
> +#include "linux/aer.h"
>
> #define CXL_MEMORY_PROGIF 0x10
>
> @@ -135,4 +136,6 @@ void read_cdat_data(struct cxl_port *port);
> 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);
> +int cxl_create_prot_err_info(struct pci_dev *_pdev, int severity,
> + struct cxl_prot_error_info *err_info);
What does the "_" in "_pdev" signify? Looks unnecessarily different
than the decls above.
> #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 83f2069f111e..46123b70f496 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -110,6 +110,16 @@ struct aer_stats {
> static int pcie_aer_disable;
> static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
>
> +#if defined(CONFIG_PCIEAER_CXL)
> +#define CXL_ERROR_SOURCES_MAX 128
> +static DEFINE_KFIFO(cxl_prot_err_fifo, struct cxl_prot_err_work_data,
> + CXL_ERROR_SOURCES_MAX);
> +static DEFINE_SPINLOCK(cxl_prot_err_fifo_lock);
> +struct work_struct *cxl_prot_err_work;
> +static int (*cxl_create_prot_err_info)(struct pci_dev*, int severity,
> + struct cxl_prot_error_info*);
Space before "*" in the parameters.
> +#endif
> +
> void pci_no_aer(void)
> {
> pcie_aer_disable = 1;
> @@ -1577,6 +1587,35 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
> return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> }
>
> +
> +#if defined(CONFIG_PCIEAER_CXL)
> +int cxl_register_prot_err_work(struct work_struct *work,
> + int (*_cxl_create_prot_err_info)(struct pci_dev*, int,
> + struct cxl_prot_error_info*))
Ditto. Rewrap to fit in 80 columns, unindent this function pointer
decl to make it fit. Same below in aer.h.
> +{
> + guard(spinlock)(&cxl_prot_err_fifo_lock);
> + cxl_prot_err_work = work;
> + cxl_create_prot_err_info = _cxl_create_prot_err_info;
> + return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_register_prot_err_work, "CXL");
> +
> +int cxl_unregister_prot_err_work(void)
> +{
> + guard(spinlock)(&cxl_prot_err_fifo_lock);
> + cxl_prot_err_work = NULL;
> + cxl_create_prot_err_info = NULL;
> + return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_unregister_prot_err_work, "CXL");
> +
> +int cxl_prot_err_kfifo_get(struct cxl_prot_err_work_data *wd)
> +{
> + return kfifo_get(&cxl_prot_err_fifo, wd);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_prot_err_kfifo_get, "CXL");
> +#endif
> +
> static struct pcie_port_service_driver aerdriver = {
> .name = "aer",
> .port_type = PCIE_ANY_PORT,
> diff --git a/include/linux/aer.h b/include/linux/aer.h
> index 947b63091902..761d6f5cd792 100644
> --- a/include/linux/aer.h
> +++ b/include/linux/aer.h
> @@ -10,6 +10,7 @@
>
> #include <linux/errno.h>
> #include <linux/types.h>
> +#include <linux/workqueue_types.h>
>
> #define AER_NONFATAL 0
> #define AER_FATAL 1
> @@ -45,6 +46,24 @@ struct aer_capability_regs {
> u16 uncor_err_source;
> };
>
> +/**
> + * struct cxl_prot_err_info - Error information used in CXL error handling
> + * @pdev: PCI device with CXL error
> + * @dev: CXL device with error. From CXL topology using ACPI/platform discovery
> + * @ras_base: Mapped address of CXL RAS registers
> + * @severity: CXL AER/RAS severity: AER_CORRECTABLE, AER_FATAL, AER_NONFATAL
> + */
> +struct cxl_prot_error_info {
> + struct pci_dev *pdev;
> + struct device *dev;
> + void __iomem *ras_base;
> + int severity;
What does the "prot" in "cxl_prot_error_info" refer to?
There's basically no error info here other than "severity".
I guess "dev" and "pdev" are separate devices (otherwise you would
just use "&pdev->dev"), but I don't have any intuition about how they
might be related, which is a little disconcerting.
I would have thought that "ras_base" would be a property of "dev" (the
CXL device) and wouldn't need to be separate.
