[RFC] cxl: Device protocol AER injection

[RFC] cxl: Device protocol AER injection

[Terry Bowman]

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

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.

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 <Benjamin.Cheatham@amd.com>
Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 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
+	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.
+
 config CXL_ATL
 	def_bool y
 	depends on CXL_REGION
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
@@ -182,6 +182,9 @@ static inline struct device *dport_to_host(struct cxl_dport *dport)
 		return port->uport_dev;
 	return &port->dev;
 }
+
+extern struct dentry *cxl_debugfs;
+
 #ifdef CONFIG_CXL_RAS
 void cxl_ras_init(void);
 void cxl_ras_exit(void);
@@ -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
+
+#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
+
 #endif /* __CXL_CORE_H__ */
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index a76f3ee05cba8..79657e5fddaac 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2501,7 +2501,7 @@ const struct bus_type cxl_bus_type = {
 };
 EXPORT_SYMBOL_NS_GPL(cxl_bus_type, "CXL");
 
-static struct dentry *cxl_debugfs;
+struct dentry *cxl_debugfs;
 
 struct dentry *cxl_debugfs_create_dir(const char *dir)
 {
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)
+
+static DEFINE_MUTEX(cxl_aer_einj_mutex);
+
+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);
+	} 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)
+{
+	char sbdf[16], severity[4], topology[4] = "";
+	unsigned int domain, bus, dev, fn;
+	u32 aer_status, ras_status;
+	struct cxl_dport *dport;
+	char buf[128];
+	int nargs;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (count >= sizeof(buf)) {
+		pr_err("cxl-einj: input too long (%zu bytes, max %zu)\n", count, sizeof(buf) - 1);
+		return -EINVAL;
+	}
+
+	if (copy_from_user(buf, ubuf, count)) {
+		pr_err("cxl-einj: copy_from_user failed\n");
+		return -EFAULT;
+	}
+	buf[count] = '\0';
+
+	nargs = sscanf(buf, "%15s %3s %x %x %3s", sbdf, severity,
+		       &aer_status, &ras_status, topology);
+	if (nargs < 4) {
+		pr_err("cxl-einj: expected format: <SBDF> <UCE|CE> <aer_status> <ras_status>\n");
+		return -EINVAL;
+	}
+
+	if (nargs == 5 && strcmp(topology, "RCH") != 0)
+		return -EINVAL;
+
+	if (strcmp(severity, "UCE") != 0 && strcmp(severity, "CE") != 0) {
+		pr_err("cxl-einj: expected 'UCE' or 'CE', got '%s'\n", severity);
+		return -EINVAL;
+	}
+
+	if (sscanf(sbdf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn) != 4) {
+		pr_err("cxl-einj: invalid SBDF format '%s', expected DDDD:BB:DD.F\n", sbdf);
+		return -EINVAL;
+	}
+
+	struct pci_dev *pdev __free(pci_dev_put) =
+		pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(dev, fn));
+	if (!pdev) {
+		pr_err("cxl-einj: device %s not found\n", sbdf);
+		return -ENODEV;
+	}
+
+	guard(mutex)(cxl_aer_einj.lock);
+	cxl_aer_einj.dev = NULL;
+
+	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;
+	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,
+				       sizeof(cxl_aer_einj_usage) - 1);
+}
+
+static const struct file_operations cxl_ras_error_fops = {
+	.owner		= THIS_MODULE,
+	.read		= cxl_aer_einj_read,
+	.write		= cxl_aer_einj_write,
+	.llseek		= default_llseek,
+};
+
+static void cxl_ras_create_debugfs(struct dentry *dir)
+{
+	debugfs_create_file("aer_einj_inject", 0600, dir, NULL,
+			    &cxl_ras_error_fops);
+}
+
+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;
+			}
+		} else {
+			if (cxl_aer_einj.dev == dport->dport_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;
+			}
+		}
+	} 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)
+	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);
+#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)
+	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;
+	}
+#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)
+		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);
 
 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];
-- 
2.34.1

Re: [RFC] cxl: Device protocol AER injection

[Dave Jiang]

On 7/17/26 3:57 PM, Terry Bowman 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
> 
> 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.
> 
> 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 <Benjamin.Cheatham@amd.com>
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> ---
>  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
> +	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.
> +
>  config CXL_ATL
>  	def_bool y
>  	depends on CXL_REGION
> 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
> @@ -182,6 +182,9 @@ static inline struct device *dport_to_host(struct cxl_dport *dport)
>  		return port->uport_dev;
>  	return &port->dev;
>  }
> +
> +extern struct dentry *cxl_debugfs;
> +
>  #ifdef CONFIG_CXL_RAS
>  void cxl_ras_init(void);
>  void cxl_ras_exit(void);
> @@ -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
> +
> +#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
> +
>  #endif /* __CXL_CORE_H__ */
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index a76f3ee05cba8..79657e5fddaac 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2501,7 +2501,7 @@ const struct bus_type cxl_bus_type = {
>  };
>  EXPORT_SYMBOL_NS_GPL(cxl_bus_type, "CXL");
>  
> -static struct dentry *cxl_debugfs;
> +struct dentry *cxl_debugfs;
>  
>  struct dentry *cxl_debugfs_create_dir(const char *dir)
>  {
> 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)

Can this be moved to a header file? Prefer to not introduce #ifdef in the C source.

> +
> +static DEFINE_MUTEX(cxl_aer_einj_mutex);
> +
> +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);
> +	} 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)
> +{
> +	char sbdf[16], severity[4], topology[4] = "";
> +	unsigned int domain, bus, dev, fn;
> +	u32 aer_status, ras_status;
> +	struct cxl_dport *dport;
> +	char buf[128];
> +	int nargs;
> +	int ret;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (count >= sizeof(buf)) {
> +		pr_err("cxl-einj: input too long (%zu bytes, max %zu)\n", count, sizeof(buf) - 1);
> +		return -EINVAL;
> +	}
> +
> +	if (copy_from_user(buf, ubuf, count)) {
> +		pr_err("cxl-einj: copy_from_user failed\n");
> +		return -EFAULT;
> +	}
> +	buf[count] = '\0';
> +
> +	nargs = sscanf(buf, "%15s %3s %x %x %3s", sbdf, severity,
> +		       &aer_status, &ras_status, topology);
> +	if (nargs < 4) {
> +		pr_err("cxl-einj: expected format: <SBDF> <UCE|CE> <aer_status> <ras_status>\n");
> +		return -EINVAL;
> +	}
> +
> +	if (nargs == 5 && strcmp(topology, "RCH") != 0)
> +		return -EINVAL;
> +
> +	if (strcmp(severity, "UCE") != 0 && strcmp(severity, "CE") != 0) {
> +		pr_err("cxl-einj: expected 'UCE' or 'CE', got '%s'\n", severity);
> +		return -EINVAL;
> +	}
> +
> +	if (sscanf(sbdf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn) != 4) {
> +		pr_err("cxl-einj: invalid SBDF format '%s', expected DDDD:BB:DD.F\n", sbdf);
> +		return -EINVAL;
> +	}
> +
> +	struct pci_dev *pdev __free(pci_dev_put) =
> +		pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(dev, fn));
> +	if (!pdev) {
> +		pr_err("cxl-einj: device %s not found\n", sbdf);
> +		return -ENODEV;
> +	}
> +
> +	guard(mutex)(cxl_aer_einj.lock);
> +	cxl_aer_einj.dev = NULL;
> +
> +	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;
> +	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,
> +				       sizeof(cxl_aer_einj_usage) - 1);
> +}
> +
> +static const struct file_operations cxl_ras_error_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= cxl_aer_einj_read,
> +	.write		= cxl_aer_einj_write,
> +	.llseek		= default_llseek,
> +};
> +
> +static void cxl_ras_create_debugfs(struct dentry *dir)
> +{
> +	debugfs_create_file("aer_einj_inject", 0600, dir, NULL,
> +			    &cxl_ras_error_fops);
> +}
> +
> +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;
> +			}
> +		} else {
> +			if (cxl_aer_einj.dev == dport->dport_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;
> +			}
> +		}
> +	} 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)

I think you can do something like below instead:

	if (IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ))

DJ

 > +	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);
> +#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)
> +	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;
> +	}
> +#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)
> +		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);
>  
>  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];

Re: [RFC] cxl: Device protocol AER injection

[Bowman, Terry]

On 7/20/2026 12:38 PM, Dave Jiang wrote:
> 
> 
> On 7/17/26 3:57 PM, Terry Bowman 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
>>
>> 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.
>>
>> 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 <Benjamin.Cheatham@amd.com>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> ---
>>  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
>> +	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.
>> +
>>  config CXL_ATL
>>  	def_bool y
>>  	depends on CXL_REGION
>> 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
>> @@ -182,6 +182,9 @@ static inline struct device *dport_to_host(struct cxl_dport *dport)
>>  		return port->uport_dev;
>>  	return &port->dev;
>>  }
>> +
>> +extern struct dentry *cxl_debugfs;
>> +
>>  #ifdef CONFIG_CXL_RAS
>>  void cxl_ras_init(void);
>>  void cxl_ras_exit(void);
>> @@ -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
>> +
>> +#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
>> +
>>  #endif /* __CXL_CORE_H__ */
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index a76f3ee05cba8..79657e5fddaac 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -2501,7 +2501,7 @@ const struct bus_type cxl_bus_type = {
>>  };
>>  EXPORT_SYMBOL_NS_GPL(cxl_bus_type, "CXL");
>>  
>> -static struct dentry *cxl_debugfs;
>> +struct dentry *cxl_debugfs;
>>  
>>  struct dentry *cxl_debugfs_create_dir(const char *dir)
>>  {
>> 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)
> 
> Can this be moved to a header file? Prefer to not introduce #ifdef in the C source.
> 

Yes, I will move to core.h. 

>> +
>> +static DEFINE_MUTEX(cxl_aer_einj_mutex);
>> +
>> +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);
>> +	} 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)
>> +{
>> +	char sbdf[16], severity[4], topology[4] = "";
>> +	unsigned int domain, bus, dev, fn;
>> +	u32 aer_status, ras_status;
>> +	struct cxl_dport *dport;
>> +	char buf[128];
>> +	int nargs;
>> +	int ret;
>> +
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	if (count >= sizeof(buf)) {
>> +		pr_err("cxl-einj: input too long (%zu bytes, max %zu)\n", count, sizeof(buf) - 1);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (copy_from_user(buf, ubuf, count)) {
>> +		pr_err("cxl-einj: copy_from_user failed\n");
>> +		return -EFAULT;
>> +	}
>> +	buf[count] = '\0';
>> +
>> +	nargs = sscanf(buf, "%15s %3s %x %x %3s", sbdf, severity,
>> +		       &aer_status, &ras_status, topology);
>> +	if (nargs < 4) {
>> +		pr_err("cxl-einj: expected format: <SBDF> <UCE|CE> <aer_status> <ras_status>\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (nargs == 5 && strcmp(topology, "RCH") != 0)
>> +		return -EINVAL;
>> +
>> +	if (strcmp(severity, "UCE") != 0 && strcmp(severity, "CE") != 0) {
>> +		pr_err("cxl-einj: expected 'UCE' or 'CE', got '%s'\n", severity);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (sscanf(sbdf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn) != 4) {
>> +		pr_err("cxl-einj: invalid SBDF format '%s', expected DDDD:BB:DD.F\n", sbdf);
>> +		return -EINVAL;
>> +	}
>> +
>> +	struct pci_dev *pdev __free(pci_dev_put) =
>> +		pci_get_domain_bus_and_slot(domain, bus, PCI_DEVFN(dev, fn));
>> +	if (!pdev) {
>> +		pr_err("cxl-einj: device %s not found\n", sbdf);
>> +		return -ENODEV;
>> +	}
>> +
>> +	guard(mutex)(cxl_aer_einj.lock);
>> +	cxl_aer_einj.dev = NULL;
>> +
>> +	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;
>> +	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,
>> +				       sizeof(cxl_aer_einj_usage) - 1);
>> +}
>> +
>> +static const struct file_operations cxl_ras_error_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.read		= cxl_aer_einj_read,
>> +	.write		= cxl_aer_einj_write,
>> +	.llseek		= default_llseek,
>> +};
>> +
>> +static void cxl_ras_create_debugfs(struct dentry *dir)
>> +{
>> +	debugfs_create_file("aer_einj_inject", 0600, dir, NULL,
>> +			    &cxl_ras_error_fops);
>> +}
>> +
>> +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;
>> +			}
>> +		} else {
>> +			if (cxl_aer_einj.dev == dport->dport_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;
>> +			}
>> +		}
>> +	} 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)
> 
> I think you can do something like below instead:
> 
> 	if (IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ))
> 
> DJ
> 

Good idea. Thanks for pointing this out.

Terry

[snip]

Re: [RFC] cxl: Device protocol AER injection

[Jonathan Cameron]

On Fri, 17 Jul 2026 17:57:00 -0500
Terry Bowman <terry.bowman@amd.com> 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

Hi Terry,

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

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 <Benjamin.Cheatham@amd.com>
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
+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.

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

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


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

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

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

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

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

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

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.

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

Re: [RFC] cxl: Device protocol AER injection

[Bowman, Terry]

On 7/20/2026 4:04 PM, Jonathan Cameron wrote:
> On Fri, 17 Jul 2026 17:57:00 -0500
> Terry Bowman <terry.bowman@amd.com> 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
> 
> Hi Terry,
> 
> https://lore.kernel.org/linux-cxl/20260717222706.3540281-1-terry.bowman@amd.com
> 
> 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 <Benjamin.Cheatham@amd.com>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> +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];
> 

Re: [RFC] cxl: Device protocol AER injection

[Jonathan Cameron]

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

It is debugfs so rules are much more flexible than sysfs.




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

I hadn't really thought about the injection method.  Indeed raises
interesting questions of how it should be done.  Add an ABI doc
in Documentation/ABI for next version.

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

I just meant to replace those sizeof(u32) with something that indicates
where the size is coming from.  Applies equally below.


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

Just add minimal reference for that.

> 
> >> +				cxl_aer_einj.dev = NULL;
> >> +				return (__force void __iomem *)cxl_aer_einj.ras_registers;
> >> +			}
>
> >> +	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?

Sounds good.

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

>