Re: [PATCH v7 1/5] cxl/port: Add EINJ debugfs files and callback support

[Dan Williams <dan.j.williams@intel.com>]

Ben Cheatham wrote:
> 
> 
> On 12/8/23 2:35 PM, Ben Cheatham wrote:
> > 
> > 
> > On 12/8/23 12:01 PM, Dan Williams wrote:
> >> Ben Cheatham wrote:
> >> [..]
> >>>> This can be simplified. Have something like:
> >>>>
> >>>> config CXL_EINJ
> >>>> 	default CXL_BUS
> >>>> 	depends on ACPI_APEI_EINJ && ACPI_APEI_EINJ=CXL_BUS
> >>>> 	...
> >>>>
> >>>> Then the documentation moves to Kconfig for how to enable this and the
> >>>> CXL code can directly call into the EINJ module without worry.
> >>>>
> >>>> It would of course need stubs like these in a shared header:
> >>>>
> >>>> #ifdef CONFIG_CXL_EINJ
> >>>> int cxl_einj_available_error_type(struct seq_file *m, void *v);
> >>>> int cxl_einj_inject_error(struct cxl_dport *dport, u64 type);
> >>>> #else
> >>>> static inline int cxl_einj_available_error_type(struct seq_file *m, void *v)
> >>>> {
> >>>> 	return -ENXIO;
> >>>> }
> >>>>
> >>>> int cxl_einj_inject_error(struct cxl_dport *dport, u64 type)
> >>>> {
> >>>> 	return -ENXIO;
> >>>> }
> >>>> #endif
> >>>>
> >>>
> >>> I had to go back and take a look, but I had a shared header in v5
> >>> (link:
> >>> https://lore.kernel.org/linux-cxl/20230926120418.0000575d@Huawei.com/).
> >>> Jonathan recommended that I instead include cxl.h directly, but that
> >>> was pretty much a completely different patch set at the time (and the
> >>> header was under include/linux/). That being said, I agree that a
> >>> header under drivers/cxl would make much more sense here.
> >>
> >> I agree with Jonathan that there are still cases where it makes sense to
> >> do the relative include thing, but cxl_pmu is an intimate extenstion of
> >> the CXL subsystem that just happens to live in a another directory. This
> >> EINJ support is a handful of functions to communicate between modules
> >> with no need for EINJ to understand all the gory details in cxl.h.
> >>
> > 
> > All right that makes sense. Thanks for the clarification.
> > 
> 
> Ok so I took a look at implementing this. I don't think this solution ends up having
> the intended behavior. Using a shared header and the Kconfig rules above introduces
> a dependency on the EINJ module, which I was trying to avoid by using the callbacks
> since I don't think the CXL core should fail to load if the EINJ module fails.

Good looking out for that. However, if EINJ is going to be offering
services to other parts of the kernel it needs to be better behaved as
library module that can export symbols independent of the platform
specific, not software, error conditions that can make einj_init() fail.

> So, unless you have any other suggestions, I'll use the Kconfig rules above but keep
> the callbacks (and also change the EINJ module to use IS_REACHABLE(CONFIG_CXL_EINJ) instead
> of IS_REACHABLE(CONFIG_CXL_ACPI) || IS_REACHABLE(CONFIG_CXL_PORT)). I may also just
> be doing something wrong (most likely due to late Friday brain fog), so let me know if
> I got something wrong here.

I think the dependency is ok, it's the myriad of failure cases in
einj_init() that are at issue.

If einj.ko was loaded relative to a platform device then it would
already be the case that all of those platform specific setup details
would move to something like einj_probe() insted of einj_init().
Unfortuantely, a full refactor along those lines would probably yield
more violence than benefit.

However, something like that can be approximated since einj.ko, before
your changes, is only ever loaded manually. Just require it to be
unloaded manually unless CXL has it pinned, something like the below.

Open to suggestions from other linux-acpi@ denizens.

diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 013eb621dc92..9c179766af88 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -137,6 +137,7 @@ static struct apei_exec_ins_type einj_ins_type[] = {
 static DEFINE_MUTEX(einj_mutex);
 
 static void *einj_param;
+static bool einj_initialized;
 
 static void einj_exec_ctx_init(struct apei_exec_context *ctx)
 {
@@ -684,7 +685,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab)
 	return 0;
 }
 
-static int __init einj_init(void)
+static int __init __einj_init(void)
 {
 	int rc;
 	acpi_status status;
@@ -782,10 +783,30 @@ static int __init einj_init(void)
 	return rc;
 }
 
+static int __init einj_init(void)
+{
+	int rc = __einj_init();
+
+	einj_initialized = (rc == 0);
+
+	/*
+	 * CXL needs to be able to link and call its error injection
+	 * helpers regardless of whether the EINJ table is present and
+	 * initialized correctly. Helpers check @einj_initialized.
+	 */
+	if (IS_ENABLED(CONFIG_CXL_EINJ))
+		return 0;
+
+	return rc;
+}
+
 static void __exit einj_exit(void)
 {
 	struct apei_exec_context ctx;
 
+	if (!einj_initialized)
+		return;
+
 	if (einj_param) {
 		acpi_size size = (acpi5) ?
 			sizeof(struct set_error_type_with_address) :