Message-ID: <7c2bc818-5c91-42d0-93e8-1f38e68935aa@amd.com>
Date: Mon, 7 Apr 2025 08:43:00 -0500
From: "Bowman, Terry" <terry.bowman@....com>
To: Ira Weiny <ira.weiny@...el.com>, linux-cxl@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
 nifan.cxl@...il.com, dave@...olabs.net, jonathan.cameron@...wei.com,
 dave.jiang@...el.com, alison.schofield@...el.com, vishal.l.verma@...el.com,
 dan.j.williams@...el.com, bhelgaas@...gle.com, mahesh@...ux.ibm.com,
 oohall@...il.com, Benjamin.Cheatham@....com, rrichter@....com,
 nathan.fontenot@....com, Smita.KoralahalliChannabasappa@....com,
 lukas@...ner.de, ming.li@...omail.com, PradeepVineshReddy.Kodamati@....com
Subject: Re: [PATCH v8 03/16] CXL/AER: Introduce Kfifo for forwarding CXL
 errors



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@....com>
>> ---
>>  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
>>
>

Powered by blists - more mailing lists