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

[sashiko-bot@kernel.org]

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