Re: [PATCH v5] cxl/core: Create einj_types regardless of EINJ init order

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

On 11/1/2026 9:24 PM, Yongxin Yang wrote:
> [You don't often get email from yangyongxin@kylinos.cn. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> The einj_types file under the CXL debugfs directory is never created when
> cxl_core is built in.  cxl_core_init() runs at subsys_initcall level, while
> einj_init() registers EINJ at device_initcall level, so the
> einj_cxl_is_initialized() check in cxl_core_init() always sees EINJ as not
> up yet and the debugfs_create_file() below it is never reached.  With
> cxl_core as a module, module dependencies make sure it is loaded after EINJ
> is up and the file is created.
> 
> Create the file unconditionally instead, gated only on the CXL error
> injection support being compiled in, and have
> einj_cxl_available_error_type_show() return -ENODEV until EINJ is up.  This
> single check in cxl_core_init() covers every combination of built-in and
> modular cxl_core, cxl_acpi and EINJ, with no extra hooks.

The first sentence makes no sense, it says it is created unconditionally and
then lists a condition for it to be created XD. Second sentence in unnecessary
as well, you already mentioned the module case is fine above. I'd change to
something like: "Change the file creation condition from EINJ being initialized
to a compile time check for ACPI_APEI_EINJ_CXL and return -ENODEV until EINJ
is up."
> 
> The check in the show callback is required, not cosmetic: einj_tab is
> NULL until einj_init() runs, so einj_get_available_error_type() would
> dereference a NULL pointer.
> 

This is pretty self-explanatory in the code, I'd remove this part.

> This restores the discovery of the supported error types only.  On a fully
> built-in RCH/RCD hierarchy the inject files below the dport directory are
> still not created: cxl_debugfs_create_dport_dir() has the same early return
> and the only dport there comes from cxl_acpi_probe() at
> subsys_initcall_sync level, before EINJ is up.

I'm looking at this now and this doesn't need to be the case. We can do the same
thing in cxl_debugfs_create_dport_dir() and replace the "if (!einj_cxl_is_initialized())"
with "if (IS_ENABLED(CONFIG_ACPI_APEI_EINJ_CXL))", then modify cxl_einj_inject() to do
check einj_cxl_is_initialized() before doing anything.

Also, this function isn't broken for RCH ports specifically, it's all downstream ports...
> 
> Fixes: 8039804cfa73 ("cxl/core: Add CXL EINJ debugfs files")
> Cc: <stable@vger.kernel.org>

I don't think this is big enough to be backported to stable. This functionality isn't very
heavily used, and the config where it's broken isn't what most people use AFAIK.
> Suggested-by: Dave Jiang <dave.jiang@intel.com>
> Suggested-by: Ben Cheatham <benjamin.cheatham@amd.com>

Suggested-by tags are for when the original change was suggested by the party, not
when they have review suggestions.

> Link: https://lore.kernel.org/all/de14c4ef-e3d8-432f-9ce8-87c468d6d2ad@amd.com/
> Link: https://lore.kernel.org/all/7a22cc6a-deba-40e6-8033-97d1234420ba@amd.com/

These probably shouldn't be here, you can put links to the past revisions under the git marker ("---")
below though if you feel so inclined.
> Signed-off-by: Yongxin Yang <yangyongxin@kylinos.cn>
> ---
> Changes in v5:
> - Rework per Dave's suggestion, agreed by Ben: create einj_types from
>   cxl_core_init() without looking at the runtime EINJ state, and return
>   -ENODEV from einj_cxl_available_error_type_show() while EINJ is not
>   initialized, instead of creating the file from three separate hooks.
>   This drops the mutex, the exported helper and the cached dentry.
> - Drop the cxl_acpi_probe() hook: host bridge dports, RCH included, already
>   go through __devm_cxl_add_dport() -> cxl_debugfs_create_dport_dir() in the
>   same probe, so the hook added nothing.
> - Gate the creation on CONFIG_ACPI_APEI_EINJ_CXL rather than on the runtime
>   EINJ state.
> - Dropped the Reviewed-by tag as the approach changed significantly.
> 
>  drivers/acpi/apei/einj-cxl.c | 4 ++++
>  drivers/cxl/core/port.c      | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/apei/einj-cxl.c b/drivers/acpi/apei/einj-cxl.c
> index e70a416ec925..3090c7649ed2 100644
> --- a/drivers/acpi/apei/einj-cxl.c
> +++ b/drivers/acpi/apei/einj-cxl.c
> @@ -30,6 +30,10 @@ int einj_cxl_available_error_type_show(struct seq_file *m, void *v)
>         int cxl_err, rc;
>         u32 available_error_type = 0;
> 
> +       /* einj_tab stays NULL until EINJ is initialized, don't read it. */

Remove this comment, it's redundant in this situation.

Thanks,
Ben

> +       if (!einj_cxl_is_initialized())
> +               return -ENODEV;
> +
>         rc = einj_get_available_error_type(&available_error_type, ACPI_EINJ_GET_ERROR_TYPE);
>         if (rc)
>                 return rc;
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 625e4aa427db..d4ed1cc7c480 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2515,7 +2515,7 @@ static __init int cxl_core_init(void)
> 
>         cxl_debugfs = debugfs_create_dir("cxl", NULL);
> 
> -       if (einj_cxl_is_initialized())
> +       if (IS_ENABLED(CONFIG_ACPI_APEI_EINJ_CXL))
>                 debugfs_create_file("einj_types", 0400, cxl_debugfs, NULL,
>                                     &einj_cxl_available_error_type_fops);
> 
> --
> 2.25.1
>