Re: [RFC] cxl: Device protocol AER injection

On Fri, 17 Jul 2026 17:57:00 -0500
Terry Bowman [off-list ref] 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/20260717222706.3540281-1-terry.bowman@amd.com/T/#md90ec1fdd1b374bf1e32e7736e2b3e34b328c701 (local)

Hi Terry,

https://lore.kernel.org/linux-cxl/20260717222706.3540281-1-terry.bowman@amd.com (local)

Works fine.  Generally you can crop that end bit off the links.

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. 

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 <redacted>
Signed-off-by: Ben Cheatham <redacted>
Signed-off-by: Terry Bowman <redacted>

+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

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.

+	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.

...

Do we need the ifdefs?  If the option isn't built none of this should get
used.  So small benefit.

+
+#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

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.

+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

+	} 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.

+{

...

+
+	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.

+	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.

+				       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?

+			}
+		} 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 */

Wrap with
	if (IS_ENABLED()) to keep it visible to the compiler.

stub that in a header.

+#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.

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.

{ }
as the else is multiline (see coding standard)

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?

I wonder if we want to restrict this to specific modules?

One for Bjorn probably.