On 6/6/25 7:27 AM, Bowman, Terry wrote:
>
>
> On 6/5/2025 7:27 PM, Dave Jiang wrote:
>>
>> On 6/3/25 10:22 AM, 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 with the CXL driver. The notification will be used
>>> as an indication for the CXL drivers to handle and log the 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 and 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 registration
>>> functions for the CXL driver to assign or clear the work handler function.
>>>
>>> Introduce function cxl_create_prot_err_info() and 'struct cxl_prot_err_info'.
>>> Implement cxl_create_prot_err_info() to populate a 'struct cxl_prot_err_info'
>>> instance with the AER severity and the erring device's PCI SBDF. The SBDF
>>> details will be used to rediscover the erring device after the CXL driver
>>> dequeues the kfifo work. The device rediscovery will be introduced along
>>> with the CXL handling in future patches.
>>>
>>> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
>>> ---
>>> drivers/cxl/core/ras.c | 31 +++++++++-
>>> drivers/cxl/cxlpci.h | 1 +
>>> drivers/pci/pcie/aer.c | 132 ++++++++++++++++++++++++++++-------------
>>> include/linux/aer.h | 36 +++++++++++
>>> 4 files changed, 157 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
>>> index 485a831695c7..d35525e79e04 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,41 @@ 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);
>>>
>>> +#ifdef CONFIG_PCIEAER_CXL
>>> +
>>> +static void cxl_prot_err_work_fn(struct work_struct *work)
>>> +{
>>> +}
>>> +
>>> +#else
>>> +static void cxl_prot_err_work_fn(struct work_struct *work) { }
>>> +#endif /* CONFIG_PCIEAER_CXL */
>> I wonder instead of the ifdef block we can just do:
>>
>> static void cxl_prot_err_work_fn(...)
>> {
>> if (!IS_ENABLED(CONFIG_PCIEAER_CXL))
>> return;
>>
>> ....
>> }
> I have a TODO request from Jonathan Cameron in the previous series iteration to address the
> same #ifdef cleanup. Jonathan recommended introducing drivers/cxl/core/aer.c and moving the
> CXL related AER logic to the new file. Are you OK with that solution?
Yes that works. Thanks!
DJ
>
>> In general we want to avoid ifdefs in C files.
>>
>> Also, where is CONFIG_PCIEAER_CXL defined? I'm having trouble finding the Kconfig that declares it.
>>
>> $ git grep CONFIG_PCIEAER_CXL
>> drivers/cxl/core/pci.c:#ifdef CONFIG_PCIEAER_CXL
>> drivers/cxl/core/ras.c:#ifdef CONFIG_PCIEAER_CXL
>> drivers/cxl/core/ras.c:#endif /* CONFIG_PCIEAER_CXL */
>> drivers/cxl/cxl.h:#ifdef CONFIG_PCIEAER_CXL
>> drivers/cxl/port.c:#ifdef CONFIG_PCIEAER_CXL
>> drivers/cxl/port.c:#endif /* CONFIG_PCIEAER_CXL */
>> drivers/pci/pcie/aer.c:#if defined(CONFIG_PCIEAER_CXL)
>> drivers/pci/pcie/aer.c:#ifdef CONFIG_PCIEAER_CXL
>> drivers/pci/pcie/aer.c:#if defined(CONFIG_PCIEAER_CXL)
>> include/linux/aer.h:#if defined(CONFIG_PCIEAER_CXL)
>>
> CONFIG_PCIEAER_CXL is a Kconfig dependent on CONFIG_PCIEAER. When enabled the
> #define is found in include/generated/autoconf.h
>
>>> +
>>> +static struct work_struct cxl_prot_err_work;
>>> +static DECLARE_WORK(cxl_prot_err_work, cxl_prot_err_work_fn);
>>> +
>>> 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 AER kfifo (%x)", rc);
>>> +
>>> + rc = cxl_register_prot_err_work(&cxl_prot_err_work);
>>> + if (rc) {
>>> + pr_err("Failed to register native AER kfifo (%x)", rc);
>>> + return rc;
>>> + }
>>> +
>>> + return 0;
>>> }
>>>
>>> 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..6f1396ef7b77 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
>>>
>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>> index adb4b1123b9b..5350fa5be784 100644
>>> --- a/drivers/pci/pcie/aer.c
>>> +++ b/drivers/pci/pcie/aer.c
>>> @@ -114,6 +114,14 @@ 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)
>> Would it make sense to move all the CXL bits to a cxl_aer.c instead of all the ifdefs in this C file?
>>
>> DJ
>
> Yes, this is a good idea. I'll make the AER driver related change to separate the CXL logic.
>
> Terry
>
>>> +#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;
>>> +#endif
>>> +
>>> void pci_no_aer(void)
>>> {
>>> pcie_aer_disable = 1;
>>> @@ -1004,45 +1012,17 @@ static bool is_internal_error(struct aer_err_info *info)
>>> return info->status & PCI_ERR_UNC_INTN;
>>> }
>>>
>>> -static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
>>> +static bool is_cxl_error(struct pci_dev *pdev, struct aer_err_info *info)
>>> {
>>> - struct aer_err_info *info = (struct aer_err_info *)data;
>>> - const struct pci_error_handlers *err_handler;
>>> + if (!info || !info->is_cxl)
>>> + return false;
>>>
>>> - if (!is_cxl_mem_dev(dev) || !cxl_error_is_native(dev))
>>> - return 0;
>>> + /* Only CXL Endpoints are currently supported */
>>> + if ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT) &&
>>> + (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_EC))
>>> + return false;
>>>
>>> - /* Protect dev->driver */
>>> - device_lock(&dev->dev);
>>> -
>>> - err_handler = dev->driver ? dev->driver->err_handler : NULL;
>>> - if (!err_handler)
>>> - goto out;
>>> -
>>> - if (info->severity == AER_CORRECTABLE) {
>>> - if (err_handler->cor_error_detected)
>>> - err_handler->cor_error_detected(dev);
>>> - } else if (err_handler->error_detected) {
>>> - if (info->severity == AER_NONFATAL)
>>> - err_handler->error_detected(dev, pci_channel_io_normal);
>>> - else if (info->severity == AER_FATAL)
>>> - err_handler->error_detected(dev, pci_channel_io_frozen);
>>> - }
>>> -out:
>>> - device_unlock(&dev->dev);
>>> - return 0;
>>> -}
>>> -
>>> -static void cxl_rch_handle_error(struct pci_dev *dev, struct aer_err_info *info)
>>> -{
>>> - /*
>>> - * 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(dev) == PCI_EXP_TYPE_RC_EC &&
>>> - is_internal_error(info))
>>> - pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info);
>>> + return is_internal_error(info);
>>> }
>>>
>>> static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
>>> @@ -1056,13 +1036,17 @@ static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
>>> return *handles_cxl;
>>> }
>>>
>>> -static bool handles_cxl_errors(struct pci_dev *rcec)
>>> +static bool handles_cxl_errors(struct pci_dev *dev)
>>> {
>>> bool handles_cxl = false;
>>>
>>> - if (pci_pcie_type(rcec) == PCI_EXP_TYPE_RC_EC &&
>>> - pcie_aer_is_native(rcec))
>>> - pcie_walk_rcec(rcec, handles_cxl_error_iter, &handles_cxl);
>>> + if (!pcie_aer_is_native(dev))
>>> + return false;
>>> +
>>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
>>> + pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl);
>>> + else
>>> + handles_cxl = pcie_is_cxl(dev);
>>>
>>> return handles_cxl;
>>> }
>>> @@ -1076,10 +1060,46 @@ static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>>> pci_info(rcec, "CXL: Internal errors unmasked");
>>> }
>>>
>>> +static int cxl_create_prot_error_info(struct pci_dev *pdev,
>>> + struct aer_err_info *aer_err_info,
>>> + struct cxl_prot_error_info *cxl_err_info)
>>> +{
>>> + cxl_err_info->severity = aer_err_info->severity;
>>> +
>>> + cxl_err_info->function = PCI_FUNC(pdev->devfn);
>>> + cxl_err_info->device = PCI_SLOT(pdev->devfn);
>>> + cxl_err_info->bus = pdev->bus->number;
>>> + cxl_err_info->segment = pci_domain_nr(pdev->bus);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void forward_cxl_error(struct pci_dev *pdev, struct aer_err_info *aer_err_info)
>>> +{
>>> + struct cxl_prot_err_work_data wd;
>>> + struct cxl_prot_error_info *cxl_err_info = &wd.err_info;
>>> +
>>> + cxl_create_prot_error_info(pdev, aer_err_info, cxl_err_info);
>>> +
>>> + if (!kfifo_put(&cxl_prot_err_fifo, wd)) {
>>> + dev_err_ratelimited(&pdev->dev, "CXL kfifo overflow\n");
>>> + return;
>>> + }
>>> +
>>> + schedule_work(cxl_prot_err_work);
>>> +}
>>> +
>>> #else
>>> static inline void cxl_rch_enable_rcec(struct pci_dev *dev) { }
>>> static inline void cxl_rch_handle_error(struct pci_dev *dev,
>>> struct aer_err_info *info) { }
>>> +static inline void forward_cxl_error(struct pci_dev *dev,
>>> + struct aer_err_info *info) { }
>>> +static inline bool handles_cxl_errors(struct pci_dev *dev)
>>> +{
>>> + return false;
>>> +}
>>> +static bool is_cxl_error(struct pci_dev *pdev, struct aer_err_info *info) { return 0; };
>>> #endif
>>>
>>> /**
>>> @@ -1117,8 +1137,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info)
>>>
>>> static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
>>> {
>>> - cxl_rch_handle_error(dev, info);
>>> - pci_aer_handle_error(dev, info);
>>> + if (is_cxl_error(dev, info))
>>> + forward_cxl_error(dev, info);
>>> + else
>>> + pci_aer_handle_error(dev, info);
>>> +
>>> pci_dev_put(dev);
>>> }
>>>
>>> @@ -1582,6 +1605,31 @@ 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)
>>> +{
>>> + guard(spinlock)(&cxl_prot_err_fifo_lock);
>>> + cxl_prot_err_work = work;
>>> + 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;
>>> + 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 02940be66324..550407240ab5 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
>>> @@ -53,6 +54,27 @@ struct aer_capability_regs {
>>> u16 uncor_err_source;
>>> };
>>>
>>> +/**
>>> + * struct cxl_prot_err_info - Error information used in CXL error handling
>>> + * @severity: AER severity
>>> + * @function: Device's PCI function
>>> + * @device: Device's PCI device
>>> + * @bus: Device's PCI bus
>>> + * @segment: Device's PCI segment
>>> + */
>>> +struct cxl_prot_error_info {
>>> + int severity;
>>> +
>>> + u8 function;
>>> + u8 device;
>>> + u8 bus;
>>> + u16 segment;
>>> +};
>>> +
>>> +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);
>>> @@ -64,6 +86,20 @@ 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_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)
>>> +{
>>> + 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);
>
