Re: [PATCH v8 03/16] CXL/AER: Introduce Kfifo for forwarding CXL errors

From: Ira Weiny
Date: Fri Mar 28 2025 - 13:01:27 EST

Next message: Bjorn Helgaas: "Re: [PATCH v8 03/16] CXL/AER: Introduce Kfifo for forwarding CXL errors"
Previous message: Oleg Nesterov: "Re: [syzbot] [netfs?] INFO: task hung in netfs_unbuffered_write_iter"
In reply to: Bowman, Terry: "Re: [PATCH v8 03/16] CXL/AER: Introduce Kfifo for forwarding CXL errors"
Next in thread: Terry Bowman: "[PATCH v8 04/16] cxl/aer: AER service driver forwards CXL error to CXL driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

This part of the message is no longer valid.

>
> Introduce 'struct cxl_prot_err_info'. This structure caches CXL error

cxl_prot_error_info

> details used in completing error handling. This avoid duplicating some
> function calls and allows the error to be treated generically when
> possible.
>
> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> ---
> drivers/cxl/core/ras.c | 54 +++++++++++++++++++++++++++++++++++++++++-
> drivers/cxl/cxlpci.h | 3 +++
> drivers/pci/pcie/aer.c | 39 ++++++++++++++++++++++++++++++
> include/linux/aer.h | 37 +++++++++++++++++++++++++++++
> 4 files changed, 132 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 485a831695c7..ecb60a5962de 100644
> --- a/drivers/cxl/core/ras.c
> +++ 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;
> + }
> +
> + 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;
> + }
> +
> + 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 };
> + err_info->ras_base = cxlds->regs.ras;
> + err_info->severity = severity;
> + err_info->pdev = pdev;
> + err_info->dev = dev;

How are pdev and dev guaranteed to be valid after the put_device() and
pci_dev_put() free functions are called on return?

> +
> + return 0;
> +}
> +
> +struct work_struct cxl_prot_err_work;

Why is this not declared with DECLARE_WORK()?

Without that I don't know what cancel_work_sync() will do with this in the
!CONFIG_PCIEAER_CXL case.

Ah... ok looks like that comes in 5/16. :-/

I got side tracked looking at the rest of the series after I found this
change in 5/16.

I'll send these questions out but I'm thinking Bjorn is correct that
passing cxlds or something might work better than stashing pdev/dev. But
even then I'm not sure about the object lifetimes.

Ira

> +
> 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);
> #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*);
> +#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*))
> +{
> + 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;
> +};
> +
> +struct cxl_prot_err_work_data {
> + struct cxl_prot_error_info err_info;
> +};
> +
> #if defined(CONFIG_PCIEAER)
> int pci_aer_clear_nonfatal_status(struct pci_dev *dev);
> int pcie_aer_is_native(struct pci_dev *dev);
> @@ -56,6 +75,24 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev)
> static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; }
> #endif
>
> +#if defined(CONFIG_PCIEAER_CXL)
> +int cxl_register_prot_err_work(struct work_struct *work,
> + int (*_cxl_create_proto_err_info)(struct pci_dev*, int,
> + struct cxl_prot_error_info*));
> +int cxl_unregister_prot_err_work(void);
> +int cxl_prot_err_kfifo_get(struct cxl_prot_err_work_data *wd);
> +#else
> +static inline int
> +cxl_register_prot_err_work(struct work_struct *work,
> + int (*_cxl_create_proto_err_info)(struct pci_dev*, int,
> + struct cxl_prot_error_info*))
> +{
> + return 0;
> +}
> +static inline int cxl_unregister_prot_err_work(void) { return 0; }
> +static inline int cxl_prot_err_kfifo_get(struct cxl_prot_err_work_data *wd) { return 0; }
> +#endif
> +
> void pci_print_aer(struct pci_dev *dev, int aer_severity,
> struct aer_capability_regs *aer);
> int cper_severity_to_aer(int cper_severity);
> --
> 2.34.1
>

Next message: Bjorn Helgaas: "Re: [PATCH v8 03/16] CXL/AER: Introduce Kfifo for forwarding CXL errors"
Previous message: Oleg Nesterov: "Re: [syzbot] [netfs?] INFO: task hung in netfs_unbuffered_write_iter"
In reply to: Bowman, Terry: "Re: [PATCH v8 03/16] CXL/AER: Introduce Kfifo for forwarding CXL errors"
Next in thread: Terry Bowman: "[PATCH v8 04/16] cxl/aer: AER service driver forwards CXL error to CXL driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]