On 3/28/2025 12:01 PM, Ira Weiny wrote:
> 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.
Yes, I'll remove that.
>> 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
The problem is cxlds only works for CXL EPs and doesn't scale for CXL Port devices.
Port devices are not associated with a cxlds.
We should consider adding a cached dereferenced status in 'struct err_info' as well. This was something I wanted to bring up in the cover sheet for discussion.
Terry
>> +
>> 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
>>
>
