Re: [RFC] cxl: Device protocol AER injection

On Fri, 17 Jul 2026 17:57:00 -0500
Terry Bowman <[email protected]> wrote:

> This patch is intended to provide a method of testing the recently submitted
> cxl series "cxl: Enable CXL PCIe Port Protocol Error handling and logging" found
> here:
> 
> https://lore.kernel.org/linux-cxl/[email protected]/T/#md90ec1fdd1b374bf1e32e7736e2b3e34b328c701

Hi Terry,

https://lore.kernel.org/linux-cxl/[email protected]

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

> 
> 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 <[email protected]>
> Signed-off-by: Ben Cheatham <[email protected]>
> Signed-off-by: Terry Bowman <[email protected]>
+CC Ashok,

Various thing inline.  Mostly this review is a bit superficial as I'd like
ideally to see a cleaner separation of this at level of files etc.

Thanks,

Jonathan
> ---
>  drivers/cxl/Kconfig           |  13 +++
>  drivers/cxl/core/core.h       |  21 ++++
>  drivers/cxl/core/port.c       |   2 +-
>  drivers/cxl/core/ras.c        | 208 ++++++++++++++++++++++++++++++++++
>  drivers/cxl/core/ras_rch.c    |  12 ++
>  drivers/pci/pcie/aer_inject.c |  29 ++---
>  include/linux/aer.h           |  15 +++
>  7 files changed, 281 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 80aeb0d556bd7..ef449228b2549 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -238,6 +238,19 @@ config CXL_RAS
>  	def_bool y
>  	depends on ACPI_APEI_GHES && PCIEAER && CXL_BUS
>  
> +config CXL_PROTO_AER_EINJ
> +	bool "CXL: RAS Protocol Error Injection using AER EINJ"
> +	depends on CXL_RAS
> +	depends on PCIEAER_INJECT

Do we think anyone who has CXL and PCIEAER_INJECT support will want
to carefully not build this?  I'm just wondering if we can avoid asking
the question and base the built or not on the combination of those.

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