| Message ID | 20250327014717.2988633-4-terry.bowman@amd.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Enable CXL PCIe port protocol error handling and logging | expand |
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. > + } > + > + 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? > + } > + > + 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. > #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. > +#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. > +{ > + 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? There's basically no error info here other than "severity". 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. I would have thought that "ras_base" would be a property of "dev" (the CXL device) and wouldn't need to be separate. 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? > +}; > + > +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 >
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 >>
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@amd.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 > + > 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 >
What does this series apply to? I default to the current -rc1 (v6.14-rc1), but this doesn't apply there, and I don't have the base-commit: aae0594a7053c60b82621136257c8b648c67b512 mentioned in the cover letter. Sometimes things make more sense when I can see everything as applied. On Thu, Mar 27, 2025 at 01:12:30PM -0500, Bowman, Terry wrote: > 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. > >> +int cxl_create_prot_err_info(struct pci_dev *_pdev, int severity, > >> + struct cxl_prot_error_info *err_info) > >> +{ > ... > >> + 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? The dependency on pdev being an endpoint is not clear here, so I would just remove the check altogether or move it to the place that breaks if pdev is not an endpoint. > >> +#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 ? A typedef would be fine with me. > >> +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. Unless there are CXL non-protocol errors that need to be distinguished, I would just omit "prot" altogether. > > 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? What's the name of the corresponding CPER struct? > > 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. It seems weird to me to have two device pointers here. Seems like we should use a single pointer to identify the device, and if we need to get from PCI to CXL or vice versa, there should be a pointer somewhere so we don't have to search all the time. > > 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. Passing around ras_base seems dodgy to me. I think it's better to pass around a real entity like a pci_dev or cxl_port or cxl_dport or whatever. Code that needs to deal with ras_base presumably should know about the internals of the device ras_base belongs to. Bjorn
On 3/28/2025 12:02 PM, Bjorn Helgaas wrote: > What does this series apply to? I default to the current -rc1 > (v6.14-rc1), but this doesn't apply there, and I don't have the > base-commit: aae0594a7053c60b82621136257c8b648c67b512 mentioned in the > cover letter. > > Sometimes things make more sense when I can see everything as applied. This base commit is from cxl/next and can be found here: https://web.git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=next Terry > On Thu, Mar 27, 2025 at 01:12:30PM -0500, Bowman, Terry wrote: >> 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. >>>> +int cxl_create_prot_err_info(struct pci_dev *_pdev, int severity, >>>> + struct cxl_prot_error_info *err_info) >>>> +{ >> ... >>>> + 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? > The dependency on pdev being an endpoint is not clear here, so I would > just remove the check altogether or move it to the place that breaks > if pdev is not an endpoint. > >>>> +#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 ? > A typedef would be fine with me. > >>>> +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. > Unless there are CXL non-protocol errors that need to be > distinguished, I would just omit "prot" altogether. > >>> 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? > What's the name of the corresponding CPER struct? > >>> 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. > It seems weird to me to have two device pointers here. Seems like we > should use a single pointer to identify the device, and if we need to > get from PCI to CXL or vice versa, there should be a pointer somewhere > so we don't have to search all the time. > >>> 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. > Passing around ras_base seems dodgy to me. I think it's better to > pass around a real entity like a pci_dev or cxl_port or cxl_dport or > whatever. Code that needs to deal with ras_base presumably should > know about the internals of the device ras_base belongs to. > > Bjorn
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; + + 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); #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);
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. Signed-off-by: Terry Bowman <terry.bowman@amd.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(-)