On 7/20/2026 4:04 PM, Jonathan Cameron wrote: > On Fri, 17 Jul 2026 17:57:00 -0500 > Terry Bowman <[email protected]> wrote: > >> This patch is intended to provide a method of testing the recently submitted >> cxl series "cxl: Enable CXL PCIe Port Protocol Error handling and logging" >> found >> here: >> >> https://lore.kernel.org/linux-cxl/[email protected]/T/#md90ec1fdd1b374bf1e32e7736e2b3e34b328c701 > > Hi Terry, > > https://lore.kernel.org/linux-cxl/[email protected] > > Works fine. Generally you can crop that end bit off the links. >
Good to know. I'll use that going forward. >> >> The changes in this patch will allow CXL RAS protocol testing by injecting >> AER errors using AER EINJ. The RAS register block status is updated >> using a central function to augment RAS register block returned by >> to_ras_base(). This supports all CXL devices including Root Ports, >> Upstream Switch Ports, Downstream Switch Ports, Endpoints, and RCH >> Downstream Ports. > > Why is this an RFC rather than a final proposal? There should always > be something to give the reviewer that info in the patch description. > I'd actually be tempted to throw a cover letter in to have somewhere > out of the way to put that information. > > Is it simply because it only makes sense once the other seris lands. > The immediate priority was providing a testing procedure for the v18 series. I wasn't sure how the design would be received. For instance, the to_einj_ras_base() could be moved into cxl-test as a to_ras_base() mock implemented function or it could remain in ras.c (or maybe even ras_einj.c) outside of cxl-mock. I'm looking forward to Alison's review and comments. My preference is to introduce core/ras_einj,c and add these changes. This would be to isolate all the changes except for: cxl_ras_einj_init(), cxl_ras_einj_exit(), and to_einj_ras_base(). I've started moving forward with making these changes knowing the direction could change once this receives more reviews. Also worth discussing is the commandline takes multiple parameters for a single sysfs file which I know isn't acceptable by everyone. I personally like the interface as-is because its simpler to use in comparison to multiple files that must be set individually. >> >> Add debugfs-based CXL protocol error injection for testing CXL RAS >> error handling paths. Injects CXL RAS protocol errors using AER internal >> error inject interface via /sys/kernel/debug/cxl/aer_einj_inject. >> >> RAS CXL status is set using to_ras_base() function override when kernel >> config >> CONFIG_CXL_PROTO_AER_EINJ is enabled. >> >> Usage: >> echo "DDDD:BB:DD.F [UCE|CE] AER_STATUS RAS_STATUS [RCH]" > \ >> /sys/kernel/debug/cxl/aer_einj_inject >> >> Move struct aer_error_inj and aer_inject() to linux/aer.h so CXL >> can invoke AER injection directly. Export aer_inject() with >> EXPORT_SYMBOL_GPL. >> >> Make cxl_debugfs non-static in port.c and declare it extern in >> core.h so the debugfs file can be created under the existing CXL >> debugfs root. >> >> Co-developed-by: Ben Cheatham <[email protected]> >> Signed-off-by: Ben Cheatham <[email protected]> >> Signed-off-by: Terry Bowman <[email protected]> > +CC Ashok, > > Various thing inline. Mostly this review is a bit superficial as I'd like > ideally to see a cleaner separation of this at level of files etc. > > Thanks, > > Jonathan >> --- >> drivers/cxl/Kconfig | 13 +++ >> drivers/cxl/core/core.h | 21 ++++ >> drivers/cxl/core/port.c | 2 +- >> drivers/cxl/core/ras.c | 208 ++++++++++++++++++++++++++++++++++ >> drivers/cxl/core/ras_rch.c | 12 ++ >> drivers/pci/pcie/aer_inject.c | 29 ++--- >> include/linux/aer.h | 15 +++ >> 7 files changed, 281 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig >> index 80aeb0d556bd7..ef449228b2549 100644 >> --- a/drivers/cxl/Kconfig >> +++ b/drivers/cxl/Kconfig >> @@ -238,6 +238,19 @@ config CXL_RAS >> def_bool y >> depends on ACPI_APEI_GHES && PCIEAER && CXL_BUS >> >> +config CXL_PROTO_AER_EINJ >> + bool "CXL: RAS Protocol Error Injection using AER EINJ" >> + depends on CXL_RAS >> + depends on PCIEAER_INJECT > > Do we think anyone who has CXL and PCIEAER_INJECT support will want > to carefully not build this? I'm just wondering if we can avoid asking > the question and base the built or not on the combination of those. > Good question: How do we incorporate this with the existing CXL EINJ functionality making them complementary and consistant? The existing EINJ is true ACPI injection but only supports RPs. The ACPI EINJ callouts are currrently in core/port.c. We should consider moving it into a common file such as core/ras_einj.c. With that move it will help force us merge the interfaces where/if possible and at least give central location for error RAS injection. I think folding the AER injection into PCIEAER_INJECT kernel config is reasonable but looking for others feedback. >> + help >> + Enable debugfs-based CXL protocol error injection. Writes to >> + /sys/kernel/debug/cxl/aer_einj_inject inject CXL RAS protocol >> + errors using the AER internal error inject interface. >> + >> + This is a debug/test facility. Say N for production kernels. >> + >> + If unsure say N. > >> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h >> index a55a4e409feda..91910d2bb5d39 100644 >> --- a/drivers/cxl/core/core.h >> +++ b/drivers/cxl/core/core.h > ... > >> @@ -244,4 +247,22 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox, const >> uuid_t *feat_uuid, >> >> resource_size_t cxl_rcd_component_reg_phys(struct device *dev, >> struct cxl_dport *dport); >> + >> +#ifdef CONFIG_CXL_PROTO_AER_EINJ > > Do we need the ifdefs? If the option isn't built none of this should get > used. So small benefit. > Youre right. You pointed that trick/pattern out before and results in less unneeded code. I'll make that change. >> + >> +#define AER_REGISTER_SIZE 5 >> +#define RAS_REGISTER_SIZE (CXL_RAS_CAPABILITY_LENGTH / sizeof(u32)) >> + >> +struct cxl_aer_einj { >> + int correctable; >> + bool is_rch; >> + struct mutex *lock; >> + struct device *dev; >> + u32 aer_registers[AER_REGISTER_SIZE]; >> + u32 ras_registers[RAS_REGISTER_SIZE]; >> +}; >> + >> +extern struct cxl_aer_einj cxl_aer_einj; >> +#endif > >> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c >> index d77208af41e03..d41deea899d30 100644 >> --- a/drivers/cxl/core/ras.c >> +++ b/drivers/cxl/core/ras.c >> @@ -3,6 +3,7 @@ >> >> #include <linux/pci.h> >> #include <linux/aer.h> >> +#include <linux/debugfs.h> >> #include <cxl/event.h> >> #include <cxlmem.h> >> #include <cxlpci.h> >> @@ -117,6 +118,195 @@ 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); >> >> +#if IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ) >> + > > Unless very strong reasons for it, generally don't do #if stuff in c files. > Just have a separate c file for this. Otherwise it hurts readability and > we tend to loose the clean separation over time. A file makes that less > likely. > Ok. > >> +static DEFINE_MUTEX(cxl_aer_einj_mutex); > > Needs a comment for what data it is protecting. > >> + >> +struct cxl_aer_einj cxl_aer_einj = { >> + .lock = &cxl_aer_einj_mutex, >> +}; >> + >> +static const char cxl_aer_einj_usage[] = >> + "ssss:bb:dd.f [UCE|CE] AER_STATUS RAS_STATUS [RCH]\n"; >> + >> +static int cxl_aer_inject_error(struct pci_dev *pdev, bool correctable, >> + u32 aer_status, u32 ras_status) >> +{ >> + /* RCD errors are signaled as internal errors on the associated RCEC */ >> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END) { >> + if (!pdev->rcec) >> + return -ENODEV; >> + pdev = pdev->rcec; >> + } >> + >> + struct aer_error_inj einj = { >> + .bus = pdev->bus->number, >> + .dev = PCI_SLOT(pdev->devfn), >> + .fn = PCI_FUNC(pdev->devfn), >> + .domain = pci_domain_nr(pdev->bus), >> + }; >> + int ret; >> + int aer_offset; >> + int ras_offset; >> + >> + if (correctable) { >> + einj.cor_status = aer_status | PCI_ERR_COR_INTERNAL; >> + aer_offset = PCI_ERR_COR_STATUS / sizeof(u32); >> + ras_offset = CXL_RAS_CORRECTABLE_STATUS_OFFSET / sizeof(u32); > > Given these are offsets into cxl_aer_einj.aer_registers / ras_registers > can we use sizeof(*cxl_aer_einj.aer_registers) etc > We could but that assumes the CEs are book ending the register blocks. And this would be inconsistent with UCE case below, right? Tell me if I misunderstaood the question. >> + } else { >> + einj.uncor_status = aer_status | PCI_ERR_UNC_INTN; >> + aer_offset = PCI_ERR_UNCOR_STATUS / sizeof(u32); >> + ras_offset = CXL_RAS_UNCORRECTABLE_STATUS_OFFSET / sizeof(u32); >> + } >> + >> + cxl_aer_einj.correctable = correctable; >> + cxl_aer_einj.aer_registers[aer_offset] = aer_status; >> + cxl_aer_einj.ras_registers[ras_offset] = ras_status; >> + >> + ret = aer_inject(&einj); >> + if (ret) { >> + pr_err("cxl-einj: aer_inject failed: %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static ssize_t cxl_aer_einj_write(struct file *file, >> + const char __user *ubuf, >> + size_t count, loff_t *ppos) > > Might as well wrap to 80 chars and save a line. > Ok >> +{ > > ... > >> + >> + struct cxl_port *port __free(put_cxl_port) = >> find_cxl_port_by_dev(&pdev->dev, &dport); >> + if (!port) { >> + dev_err(&pdev->dev, "cxl-einj: Failed to find CXL Port.\n"); >> + return -ENODEV; >> + } >> + >> + if (!to_ras_base(port, dport)) { >> + dev_err(&pdev->dev, "cxl-einj: RAS not initialized.\n"); >> + return -ENODEV; >> + } >> + >> + cxl_aer_einj.is_rch = (nargs == 5 && strcmp(topology, "RCH") == 0); >> + if (!cxl_aer_einj.is_rch) >> + pci_dev_get(pdev); >> + cxl_aer_einj.dev = cxl_aer_einj.is_rch ? pdev->dev.parent : &pdev->dev; > > cxl_aer_einj.dev = cxl_aer_einj.is_rch ? pdev->dev.parent : > pci_dev_get(&pdev->dev); > > Or use an if else for both is_rch based choices. > Lazy me wonders.... Can we just grab a reference to the dev.parent for is_rch > and > simplify the code? We don't really need it I think but it is harmless. > > Your change is cleaner and the parent ref increment doesnt hurt as you mentioned. >> + ret = cxl_aer_inject_error(pdev, strcmp(severity, "CE") == 0, >> + aer_status, ras_status); >> + if (ret) { >> + if (!cxl_aer_einj.is_rch) >> + pci_dev_put(pdev); >> + cxl_aer_einj.dev = NULL; >> + pr_err("cxl-einj: injection failed for %s: %d\n", sbdf, ret); >> + return ret; >> + } >> + >> + return count; >> +} >> + >> +static ssize_t cxl_aer_einj_read(struct file *file, char __user *ubuf, >> + size_t count, loff_t *ppos) >> +{ >> + return simple_read_from_buffer(ubuf, count, ppos, >> + cxl_aer_einj_usage, > > Probably wrap this to push that up a line. > Ok >> + sizeof(cxl_aer_einj_usage) - 1); >> +} > >> + >> +static void __iomem *to_einj_ras_base(struct cxl_port *port, struct >> cxl_dport *dport) >> +{ >> + if (dport) { >> + if (cxl_aer_einj.is_rch) { >> + if (cxl_aer_einj.dev == dport->dport_dev) { >> + cxl_aer_einj.dev = NULL; >> + return (__force void __iomem >> *)cxl_aer_einj.ras_registers; > > Given the output of this is always force cast, maybe move that force up to > the caller? > I think that works if it remains an internal helper and isn't made a mock function to to_ras_base(). If to_einj_ras_base() is used as a mock than it would require changing the to_ras_base() as well. >> + } >> + } else { >> + if (cxl_aer_einj.dev == dport->dport_dev) { >> + pci_dev_put(to_pci_dev(cxl_aer_einj.dev)); > > Not locally obvious why a thing called to_einj_ras_base should put anything > it didn't > get. I think this needs a restructure to more obviously be tidying up > references > that were held over the queue. At very leads needs a comment. > /* Reference held from X no longer needed so drop */ > The ref was incremented in cxl_aer_einj_write() on invoking injection. The ref is decremented here after its usage. RCHs are excluded because they dont have a SBDF. >> + cxl_aer_einj.dev = NULL; >> + return (__force void __iomem >> *)cxl_aer_einj.ras_registers; >> + } >> + } >> + } else if (!cxl_aer_einj.is_rch) { >> + struct device *dev = is_cxl_endpoint(port) ? >> + port->uport_dev->parent : port->uport_dev; >> + >> + if (dev_is_pci(dev) && cxl_aer_einj.dev == dev) { >> + pci_dev_put(to_pci_dev(cxl_aer_einj.dev)); >> + cxl_aer_einj.dev = NULL; >> + return (__force void __iomem >> *)cxl_aer_einj.ras_registers; >> + } >> + } >> + >> + return NULL; >> +} >> +#endif >> + >> static void cxl_unmask_proto_interrupts(struct device *dev) >> { >> struct pci_dev *pdev; >> @@ -238,6 +428,14 @@ void __iomem *to_ras_base(struct cxl_port *port, struct >> cxl_dport *dport) >> if (!port) >> return NULL; >> >> +#if IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ) > > Wrap with > if (IS_ENABLED()) to keep it visible to the compiler. > Ok >> + if (cxl_aer_einj.dev) { >> + void __iomem *einj = to_einj_ras_base(port, dport); >> + if (einj) >> + return einj; >> + } >> +#endif >> + >> if (dport) >> return dport->regs.ras; >> >> @@ -458,10 +656,20 @@ void cxl_ras_init(void) >> cxl_cper_register_prot_err_work(&cxl_cper_prot_err_work); >> cxl_register_proto_err_work(&cxl_proto_err_work, >> cxl_proto_err_do_flush); >> +#if IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ) >> + cxl_ras_create_debugfs(cxl_debugfs); > stub that in a header. > Actually, during rework today I moved it to cxl_ras_einj_init(), a new function. Thoughts? >> +#endif >> } >> >> void cxl_ras_exit(void) >> { >> cxl_unregister_proto_err_work(); >> cxl_cper_unregister_prot_err_work(); >> +#if IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ) > > As below. Use > if (IS_ENABLED()) > > and keep everything visible. > Ok >> + if (cxl_aer_einj.dev) { >> + if (!cxl_aer_einj.is_rch) >> + pci_dev_put(to_pci_dev(cxl_aer_einj.dev)); >> + cxl_aer_einj.dev = NULL; >> + } >> +#endif >> } >> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c >> index 14bb3bdb2d092..5071cf86e4a68 100644 >> --- a/drivers/cxl/core/ras_rch.c >> +++ b/drivers/cxl/core/ras_rch.c >> @@ -110,6 +110,14 @@ void cxl_handle_rdport_errors(struct pci_dev *pdev) >> if (!dport) >> return; >> >> +#if IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ) >> + if (cxl_aer_einj.is_rch && cxl_aer_einj.dev) { >> + severity = cxl_aer_einj.correctable ? >> + AER_CORRECTABLE : AER_FATAL; >> + goto handle_ras; >> + } > > Use instead > if (IS_ENABLED(CONFIG_CXL_PROTO_AR_EINJ)) > > then compiler can see the code (but remove it) which means > you don't need the dance around the label below. > > In general follow this pattern anyway rather than #if > when you can. > Got it. >> +#endif >> + >> if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs)) >> return; >> >> @@ -117,6 +125,10 @@ void cxl_handle_rdport_errors(struct pci_dev *pdev) >> return; >> >> pci_print_aer(pdev, severity, &aer_regs); >> + >> +#if IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ) >> +handle_ras: >> +#endif >> if (severity == AER_CORRECTABLE) >> cxl_handle_cor_ras(dport->port, dport, >> to_ras_base(port, dport), pdev->dsn); >> diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c >> index 09bfc7194ef31..b313adef680ae 100644 >> --- a/drivers/pci/pcie/aer_inject.c >> +++ b/drivers/pci/pcie/aer_inject.c >> @@ -14,6 +14,7 @@ >> >> #define dev_fmt(fmt) "aer_inject: " fmt >> >> +#include <linux/aer.h> >> #include <linux/module.h> >> #include <linux/init.h> >> #include <linux/interrupt.h> >> @@ -31,19 +32,6 @@ >> static bool aer_mask_override; >> module_param(aer_mask_override, bool, 0); >> >> -struct aer_error_inj { >> - u8 bus; >> - u8 dev; >> - u8 fn; >> - u32 uncor_status; >> - u32 cor_status; >> - u32 header_log0; >> - u32 header_log1; >> - u32 header_log2; >> - u32 header_log3; >> - u32 domain; >> -}; >> - >> struct aer_error { >> struct list_head list; >> u32 domain; >> @@ -316,7 +304,7 @@ static int pci_bus_set_aer_ops(struct pci_bus *bus) >> return 0; >> } >> >> -static int aer_inject(struct aer_error_inj *einj) >> +int aer_inject(struct aer_error_inj *einj) >> { >> struct aer_error *err, *rperr; >> struct aer_error *err_alloc = NULL, *rperr_alloc = NULL; >> @@ -332,10 +320,14 @@ static int aer_inject(struct aer_error_inj *einj) >> dev = pci_get_domain_bus_and_slot(einj->domain, einj->bus, devfn); >> if (!dev) >> return -ENODEV; >> - rpdev = pcie_find_root_port(dev); >> - /* If Root Port not found, try to find an RCEC */ >> - if (!rpdev) >> - rpdev = dev->rcec; >> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) > > { } > as the else is multiline (see coding standard) > Ok > Maybe need a comment for why it might be an RCEC for injection. > Is this an RCH specific path where there is nothing else to target? > >> + rpdev = dev; >> + else { >> + rpdev = pcie_find_root_port(dev); >> + /* If Root Port not found, try to find an RCEC */ >> + if (!rpdev) >> + rpdev = dev->rcec; >> + } >> if (!rpdev) { >> pci_err(dev, "Neither Root Port nor RCEC found\n"); >> ret = -ENODEV; >> @@ -482,6 +474,7 @@ static int aer_inject(struct aer_error_inj *einj) >> pci_dev_put(dev); >> return ret; >> } >> +EXPORT_SYMBOL_GPL(aer_inject); > I wonder if we want to restrict this to specific modules? > > One for Bjorn probably. > Because right now it injects AER to any PCI device. Also, worth discussing is the commandline currently uses multiple parameters for a single sysfs file. I mentioned this at the top. Thanks for reviewing Jonathan. -Terry >> >> static ssize_t aer_inject_write(struct file *filp, const char __user *ubuf, >> size_t usize, loff_t *off) >> diff --git a/include/linux/aer.h b/include/linux/aer.h >> index b3657b80564b9..65c22ba597657 100644 >> --- a/include/linux/aer.h >> +++ b/include/linux/aer.h >> @@ -27,6 +27,21 @@ >> struct pci_dev; >> struct work_struct; >> >> +struct aer_error_inj { >> + u8 bus; >> + u8 dev; >> + u8 fn; >> + u32 uncor_status; >> + u32 cor_status; >> + u32 header_log0; >> + u32 header_log1; >> + u32 header_log2; >> + u32 header_log3; >> + u32 domain; >> +}; >> + >> +int aer_inject(struct aer_error_inj *einj); >> + >> struct pcie_tlp_log { >> union { >> u32 dw[PCIE_STD_MAX_TLP_HEADERLOG]; >
