[PATCH] cxl/core: Create einj_types when EINJ becomes available

[PATCH] cxl/core: Create einj_types when EINJ becomes available

[Yongxin Yang]

From: Yang Yongxin <yangyongxin@kylinos.cn>

The einj_types file under the CXL debugfs directory is never created.

cxl_core_init() runs at subsys_initcall level, while EINJ registers itself
from einj_init() at module_init level. When cxl_core_init() evaluates
einj_cxl_is_initialized() the EINJ driver has not been initialized yet, so
the value is always false and the debugfs_create_file("einj_types") call
below it is never reached. Users have no way to discover which CXL error
types the platform supports from debugfs.

Create the file lazily, from the first place where EINJ is known to be up.
Note that this is a partial fix: if no injectable dport is ever added, the
file still does not appear. Fixing the initcall ordering itself would
require moving cxl_core_init() after EINJ is probed, which risks the CXL
bus registration order, so it is left out of scope here.

Fixes: 8039804cfa73 ("cxl/core: Add CXL EINJ debugfs files")
Cc: <stable@vger.kernel.org>
Signed-off-by: Yang Yongxin <yangyongxin@kylinos.cn>
---
 drivers/cxl/core/port.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 625e4aa427db..7de08063469f 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -802,6 +802,27 @@ static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
 
 DEFINE_SHOW_ATTRIBUTE(einj_cxl_available_error_type);
 
+static struct dentry *cxl_debugfs;
+static struct dentry *einj_types_dentry;
+
+/*
+ * einj_types cannot be created from cxl_core_init(): that runs at
+ * subsys_initcall level, before einj_init() has set einj_initialized.
+ * Create it lazily, from the first place where EINJ is known to be up.
+ */
+static void cxl_einj_debugfs_add_types(void)
+{
+	if (einj_types_dentry)
+		return;
+
+	if (!einj_cxl_is_initialized())
+		return;
+
+	einj_types_dentry = debugfs_create_file("einj_types", 0400, cxl_debugfs,
+						NULL,
+						&einj_cxl_available_error_type_fops);
+}
+
 static int cxl_einj_inject(void *data, u64 type)
 {
 	struct cxl_dport *dport = data;
@@ -819,6 +840,8 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport)
 	struct cxl_port *parent = parent_port_of(dport->port);
 	struct dentry *dir;
 
+	cxl_einj_debugfs_add_types();
+
 	if (!einj_cxl_is_initialized())
 		return;
 
@@ -2501,8 +2524,6 @@ const struct bus_type cxl_bus_type = {
 };
 EXPORT_SYMBOL_NS_GPL(cxl_bus_type, "CXL");
 
-static struct dentry *cxl_debugfs;
-
 struct dentry *cxl_debugfs_create_dir(const char *dir)
 {
 	return debugfs_create_dir(dir, cxl_debugfs);
@@ -2515,10 +2536,6 @@ static __init int cxl_core_init(void)
 
 	cxl_debugfs = debugfs_create_dir("cxl", NULL);
 
-	if (einj_cxl_is_initialized())
-		debugfs_create_file("einj_types", 0400, cxl_debugfs, NULL,
-				    &einj_cxl_available_error_type_fops);
-
 	cxl_mbox_init();
 
 	rc = cxl_memdev_init();
-- 
2.25.1

Re: [PATCH] cxl/core: Create einj_types when EINJ becomes available

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] cxl/core: data race on einj_types_dentry in cxl_einj_debugfs_add_types()
- [Medium] cxl/core: incomplete einj_types init ordering and modular regression
--

commit 8c31c56682570f7a66710a4b59dab14f80b09a59
Author: Yang Yongxin <yangyongxin@kylinos.cn>

cxl/core: Create einj_types when EINJ becomes available

This commit moves the creation of the einj_types debugfs file out of
cxl_core_init() into a helper called from cxl_debugfs_create_dport_dir() so
that the file can be created lazily after the einj driver initializes.

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index f3705ea44db77..f80dd8f4221a7 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -811,6 +811,27 @@ static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
>  
>  DEFINE_SHOW_ATTRIBUTE(einj_cxl_available_error_type);
>  
> +static struct dentry *cxl_debugfs;
> +static struct dentry *einj_types_dentry;
> +
> +/*
> + * einj_types cannot be created from cxl_core_init(): that runs at
> + * subsys_initcall level, before einj_init() has set einj_initialized.
> + * Create it lazily, from the first place where EINJ is known to be up.
> + */
> +static void cxl_einj_debugfs_add_types(void)
> +{
> +	if (einj_types_dentry)
> +		return;
> +
> +	if (!einj_cxl_is_initialized())
> +		return;
> +
> +	einj_types_dentry = debugfs_create_file("einj_types", 0400, cxl_debugfs,
> +						NULL,
> +						&einj_cxl_available_error_type_fops);
> +}

[Severity: Medium]
Can concurrent dport enumeration race on einj_types_dentry in
cxl_einj_debugfs_add_types()?

Previously, debugfs_create_file("einj_types", ...) was only called once from
cxl_core_init() in single-threaded initcall context. With this change, it is
called via:

  cxl_pci_probe()
    -> devm_cxl_add_memdev()
      -> cxl_mem_probe()
        -> devm_cxl_enumerate_ports()
          -> add_port_attach_ep()
            -> probe_dport()
              -> cxl_port_add_dport()
                -> devm_cxl_add_dport_by_dev()
                  -> __devm_cxl_add_dport()
                    -> cxl_debugfs_create_dport_dir()
                      -> cxl_einj_debugfs_add_types()

Because cxl_pci_driver enables asynchronous probing in drivers/cxl/pci.c:

	.driver	= {
		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
	},

and add_port_attach_ep() in drivers/cxl/core/port.c only serializes on the
per-port device lock:

	scoped_guard(device, &parent_port->dev) {
		parent_dport = cxl_find_dport_by_dev(parent_port, dparent);
		if (!parent_dport) {
			parent_dport = probe_dport(parent_port, dparent);

two worker threads probing endpoints under different parent ports can enter
cxl_einj_debugfs_add_types() simultaneously, both observe einj_types_dentry
as null, and both call debugfs_create_file("einj_types", ...).

In fs/debugfs/inode.c:debugfs_start_creating(), the losing caller will fail
with:

	dentry = simple_start_creating(parent, name);
	if (IS_ERR(dentry)) {
		if (dentry == ERR_PTR(-EEXIST))
			pr_err("'%s' already exists in '%pd'\n", name, parent);
		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
	}

logging a spurious error and racing to overwrite einj_types_dentry with the
error pointer. Should einj_types_dentry creation be synchronized with a lock
or once-only helper?

> +
>  static int cxl_einj_inject(void *data, u64 type)
>  {
>  	struct cxl_dport *dport = data;
> @@ -833,6 +854,8 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport)
>  	struct cxl_port *parent = parent_port_of(dport->port);
>  	struct dentry *dir;
>  
> +	cxl_einj_debugfs_add_types();
> +
>  	if (!einj_cxl_is_initialized())
>  		return;
>  
> @@ -2589,8 +2612,6 @@ const struct bus_type cxl_bus_type = {
>  };
>  EXPORT_SYMBOL_NS_GPL(cxl_bus_type, "CXL");
>  
> -static struct dentry *cxl_debugfs;
> -
>  struct dentry *cxl_debugfs_create_dir(const char *dir)
>  {
>  	return debugfs_create_dir(dir, cxl_debugfs);
> @@ -2603,10 +2624,6 @@ static __init int cxl_core_init(void)
>  
>  	cxl_debugfs = debugfs_create_dir("cxl", NULL);
>  
> -	if (einj_cxl_is_initialized())
> -		debugfs_create_file("einj_types", 0400, cxl_debugfs, NULL,
> -				    &einj_cxl_available_error_type_fops);
> -

[Severity: Medium]
Does removing this check from cxl_core_init() introduce a regression for
modular builds, while still missing built-in restricted host topologies?

First, when cxl_core is built as a module, subsys_initcall(cxl_core_init)
compiles to module_init(cxl_core_init). Because cxl_core directly references
einj symbols (einj_cxl_is_initialized and
einj_cxl_available_error_type_show), the einj module is already initialized
before cxl_core_init() runs. Removing the creation from cxl_core_init()
rather than calling cxl_einj_debugfs_add_types() here means einj_types is no
longer created when cxl_core.ko is loaded before or without dport
enumeration.

Second, when both cxl and einj are built-in, cxl_acpi_init() runs at
subsys_initcall_sync in drivers/cxl/acpi.c:

  subsys_initcall_sync(cxl_acpi_init);

which still executes before einj_init() at device_initcall in
drivers/acpi/apei/einj-core.c:

  module_init(einj_init);

During cxl_acpi_probe(), add_host_bridge_dport() in drivers/cxl/acpi.c adds
the host bridge and restricted host dports:

	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
		dev_dbg(match, "RCRB found for UID %lld: %pa\n", ctx.uid,
			&ctx.base);
		dport = devm_cxl_add_rch_dport(root_port, bridge, ctx.uid,
					       ctx.base);
	} else {
		dport = devm_cxl_add_dport(root_port, bridge, ctx.uid,
					   CXL_RESOURCE_NONE);
	}

At that point einj_cxl_is_initialized() is still false in
cxl_debugfs_create_dport_dir(). Later, when restricted host endpoints probe
at device_initcall, devm_cxl_enumerate_ports() in drivers/cxl/core/port.c
returns early without adding any further dports:

	if (cxlmd->cxlds->rcd)
		return 0;

As a result, on built-in restricted host platforms,
cxl_debugfs_create_dport_dir() is never called after einj_init(), so neither
einj_types nor the dport's einj_inject file is created despite an injectable
dport being present.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028055401.1626640-1-yangyongxin@kylinos.cn?part=1

[PATCH v2] cxl/core: Create einj_types when EINJ becomes available

[Yongxin Yang]

From: Yang Yongxin <yangyongxin@kylinos.cn>

The einj_types file under the CXL debugfs directory is never created.

cxl_core_init() runs at subsys_initcall level, while EINJ registers itself
from einj_init() at module_init level. When cxl_core_init() evaluates
einj_cxl_is_initialized() the built-in EINJ driver has not been initialized
yet, so the value is always false and the debugfs_create_file("einj_types")
call below it is never reached. Users have no way to discover which CXL
error types the platform supports from debugfs.

Create the file from both places that can know EINJ is up: cxl_core_init(),
which covers the case where cxl_core is a module and einj has already been
initialized by the time the module is loaded, and the dport probe path,
which covers the built-in case. The dport probe path can run concurrently
for different parent ports, each holding only its own device lock, so
serialize the check and the create to avoid a duplicate
debugfs_create_file() attempt that would only fail with -EEXIST.

Note that this is still a partial fix: an RCH platform returns early from
devm_cxl_enumerate_ports() before any dport is added, so einj_types does
not appear there. Fixing that requires moving cxl_core_init() after EINJ
is probed, or creating the file from cxl_acpi_probe(), both of which are
out of scope here.

Fixes: 8039804cfa73 ("cxl/core: Add CXL EINJ debugfs files")
Cc: <stable@vger.kernel.org>
Signed-off-by: Yang Yongxin <yangyongxin@kylinos.cn>
---
Changes in v2:
- Create einj_types from cxl_core_init() as well, so that the module case,
  where einj is already initialized when cxl_core is loaded, keeps working
  and is not regressed by the lazy path alone.
- Serialize the check and the create with a mutex, as the dport probe path
  can run concurrently for different parent ports.
- Spell out that RCH platforms are still not covered, and why.
- Rebase onto mainline, so the diff no longer carries the CXL fixes that
  are queued in linux-next.

 drivers/cxl/core/port.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 625e4aa427db..c1736a00f22f 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -802,6 +802,30 @@ static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
 
 DEFINE_SHOW_ATTRIBUTE(einj_cxl_available_error_type);
 
+static struct dentry *cxl_debugfs;
+static struct dentry *einj_types_dentry;
+
+/*
+ * einj_types is created from two places: cxl_core_init() catches the case
+ * where cxl_core is a module and einj is already up, while the dport probe
+ * path catches the built-in case, where cxl_core_init() runs at
+ * subsys_initcall level before einj_init() has set einj_initialized.
+ *
+ * The dport probe path can run concurrently for different parent ports, each
+ * holding only its own device lock, so the check and the create need to be
+ * serialized to avoid a duplicate debugfs_create_file() attempt.
+ */
+static DEFINE_MUTEX(einj_types_lock);
+static void cxl_einj_debugfs_add_types(void)
+{
+	mutex_lock(&einj_types_lock);
+	if (!einj_types_dentry && einj_cxl_is_initialized())
+		einj_types_dentry = debugfs_create_file("einj_types", 0400,
+							cxl_debugfs, NULL,
+							&einj_cxl_available_error_type_fops);
+	mutex_unlock(&einj_types_lock);
+}
+
 static int cxl_einj_inject(void *data, u64 type)
 {
 	struct cxl_dport *dport = data;
@@ -819,6 +843,8 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport)
 	struct cxl_port *parent = parent_port_of(dport->port);
 	struct dentry *dir;
 
+	cxl_einj_debugfs_add_types();
+
 	if (!einj_cxl_is_initialized())
 		return;
 
@@ -2501,8 +2527,6 @@ const struct bus_type cxl_bus_type = {
 };
 EXPORT_SYMBOL_NS_GPL(cxl_bus_type, "CXL");
 
-static struct dentry *cxl_debugfs;
-
 struct dentry *cxl_debugfs_create_dir(const char *dir)
 {
 	return debugfs_create_dir(dir, cxl_debugfs);
@@ -2515,10 +2539,6 @@ static __init int cxl_core_init(void)
 
 	cxl_debugfs = debugfs_create_dir("cxl", NULL);
 
-	if (einj_cxl_is_initialized())
-		debugfs_create_file("einj_types", 0400, cxl_debugfs, NULL,
-				    &einj_cxl_available_error_type_fops);
-
 	cxl_mbox_init();
 
 	rc = cxl_memdev_init();
@@ -2541,6 +2561,8 @@ static __init int cxl_core_init(void)
 
 	cxl_ras_init();
 
+	cxl_einj_debugfs_add_types();
+
 	return 0;
 
 err_region:
-- 
2.25.1