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

[Bowman, Terry]

On 3/27/2025 12:08 PM, Bjorn Helgaas wrote:
> 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.
I sometimes struggle with too much parameter validation, especially in
new code. And there are often valid questions of "how can this happen
and does it need to be checked". Some of it borders on paranoid (pointing
back to initial development). Thanks for the feedback here, I will keep
this in mind.

>> +	}
>> +
>> +	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?
Got it "%x". Would you recommend the pci_warn_once() is removed?
>
>> +	}
>> +
>> +	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.
_pdev shadows pdev. In previous patchset review Dan asked to add reference count
incr because much of this logic is during error handling and devices can go away.
Long story to say I was using the following throughout where needed:


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);

>>  #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.
I'm surprised checkpatch() didn't catch this. Maybe it's cause the parameter
itself is not present. Thanks!
>> +#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.
Ok, got it. Without using typedefs, right ?
>> +{
>> +	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?
Protocol. As in CXL Protocol Error Information. I suppose it needs to be renamed
if it wasn't obvious.
>
> There's basically no error info here other than "severity".
Correct. It's more accurately "CXL Protocol Error Context" but I didn't
want to re-use 'context' because 'context' is used for thread/process
statefulness. Also, I followed the existing CPER parallel work that uses
a similar kfifo etc. Thoughts on rename?

> 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.
"pdev" represents a PCIe device: RP, USP, DSP, or EP.  "dev" is the same
device as "pdev" but "dev" is found in CXL topology. "dev" is discovered through
ACPI/platform enumeration and interconnected with other CXL "devs" using upstream
and downstream links. Moving back and forth between "pdev" and its CXL "dev"
requires a search unique to the device type and point beginning the search.

BTW, CXL "dev" devices discussed here are the underlying devices for 'struct cxl_port',
'struct cxl_dports', and CXL upstream ports.

The 'struct cxl_prot_error_info' could possibly be removed where only a 'pdev' or 'dev' and AER severity are cached. When I started implementing the redesign I found cacheing this information made it simpler to implement. This could be revisited to improve. But there are 2 caveats to consider: 1. Removing the cached data will require invoking more SW calls for "pdev"->"dev" conversions, converting "dev" to CXL port, etc. 2. Will require saving the AER severity somewhere at a minimum.
> I would have thought that "ras_base" would be a property of "dev" (the
> CXL device) and wouldn't need to be separate.
"ras_base" is a common member of the CXL Port, CXL Downstream Port, CXL Upstream Port,
and CXL EP. If one wants the "ras_base" for a given CXL "dev" then the "dev" must be
converted to CXL Port, Downstream Port, or Upstream Port.
> From above, I guess "ras_base" is a property of cxlds, not
> cxlds->cxlmd->dev.  Maybe we should be keeping &cxlds here instead and
> letting the consumer look up cxlds->cxlmd->dev?
Yes, at this "point" in the patchset. It is updated later when support is added
for CXL Port devices.

Terry
>> +};
>> +
>> +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
>>