diff mbox series

[v8,2/5] ACPI, APEI, EINJ: Add wrapper __init function

Message ID 20231213223702.543419-3-Benjamin.Cheatham@amd.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series CXL, ACPI, APEI, EINJ: Update EINJ for CXL error types | expand

Commit Message

Ben Cheatham Dec. 13, 2023, 10:36 p.m. UTC
The CXL core module should be able to load regardless of whether the
EINJ module initializes correctly. Instead of porting the EINJ module to
a library module, add a wrapper __init function around einj_init() to
pin the EINJ module even if it does not initialize correctly. This
should be fine since the EINJ module is only ever unloaded manually.

One note: since the CXL core will be calling into the EINJ module
directly, even though it may not have initialized, all CXL helper
functions *have* to check if the EINJ module is initialized before
doing any work.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
---
 drivers/acpi/apei/einj.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

Comments

Dan Williams Dec. 18, 2023, 11:59 p.m. UTC | #1
Ben Cheatham wrote:
> The CXL core module should be able to load regardless of whether the
> EINJ module initializes correctly. Instead of porting the EINJ module to
> a library module, add a wrapper __init function around einj_init() to

Small quibble with this wording... the larger EINJ module refactoring
would be separating module_init() from EINJ probe(). As is this simple
introduction of an einit_init() wrapper *is* refactoring this module to
be used as a module dependency.

> pin the EINJ module even if it does not initialize correctly. This
> should be fine since the EINJ module is only ever unloaded manually.
> 
> One note: since the CXL core will be calling into the EINJ module
> directly, even though it may not have initialized, all CXL helper
> functions *have* to check if the EINJ module is initialized before
> doing any work.

Another small quibble here, perhaps s/may not have initialized/may not
have successfully initialized/? Because initialization will have
definitely completed one way or the other, but callers need to abort if
it completed in error.

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Did Jonathan really get in and review this new patch in the series
before me? If yes, apologies I missed it, if no I think it is best
practice to not carry forward series Reviewed-by's if new patches appear
in the series between revisions.


> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>

With above fixups, feel free to add:

Co-developed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Jonathan Cameron Dec. 19, 2023, 3:39 p.m. UTC | #2
On Mon, 18 Dec 2023 15:59:12 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Ben Cheatham wrote:
> > The CXL core module should be able to load regardless of whether the
> > EINJ module initializes correctly. Instead of porting the EINJ module to
> > a library module, add a wrapper __init function around einj_init() to  
> 
> Small quibble with this wording... the larger EINJ module refactoring
> would be separating module_init() from EINJ probe(). As is this simple
> introduction of an einit_init() wrapper *is* refactoring this module to
> be used as a module dependency.
> 
> > pin the EINJ module even if it does not initialize correctly. This
> > should be fine since the EINJ module is only ever unloaded manually.
> > 
> > One note: since the CXL core will be calling into the EINJ module
> > directly, even though it may not have initialized, all CXL helper
> > functions *have* to check if the EINJ module is initialized before
> > doing any work.  
> 
> Another small quibble here, perhaps s/may not have initialized/may not
> have successfully initialized/? Because initialization will have
> definitely completed one way or the other, but callers need to abort if
> it completed in error.
> 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> Did Jonathan really get in and review this new patch in the series
> before me? If yes, apologies I missed it, if no I think it is best
> practice to not carry forward series Reviewed-by's if new patches appear
> in the series between revisions.

I'm not keen on the solution as it's esoteric and to me seems fragile.
I've looked at discussion on v7 and can see why you ended up with this
but I'd have preferred to see the 'violent' approach :)

I don't mind if there is consensus on this, but not reviewed by from
me for this one.

> 
> 
> > Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>  
> 
> With above fixups, feel free to add:
> 
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Dan Williams Dec. 19, 2023, 8:48 p.m. UTC | #3
Jonathan Cameron wrote:
> On Mon, 18 Dec 2023 15:59:12 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Ben Cheatham wrote:
> > > The CXL core module should be able to load regardless of whether the
> > > EINJ module initializes correctly. Instead of porting the EINJ module to
> > > a library module, add a wrapper __init function around einj_init() to  
> > 
> > Small quibble with this wording... the larger EINJ module refactoring
> > would be separating module_init() from EINJ probe(). As is this simple
> > introduction of an einit_init() wrapper *is* refactoring this module to
> > be used as a module dependency.
> > 
> > > pin the EINJ module even if it does not initialize correctly. This
> > > should be fine since the EINJ module is only ever unloaded manually.
> > > 
> > > One note: since the CXL core will be calling into the EINJ module
> > > directly, even though it may not have initialized, all CXL helper
> > > functions *have* to check if the EINJ module is initialized before
> > > doing any work.  
> > 
> > Another small quibble here, perhaps s/may not have initialized/may not
> > have successfully initialized/? Because initialization will have
> > definitely completed one way or the other, but callers need to abort if
> > it completed in error.
> > 
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> > 
> > Did Jonathan really get in and review this new patch in the series
> > before me? If yes, apologies I missed it, if no I think it is best
> > practice to not carry forward series Reviewed-by's if new patches appear
> > in the series between revisions.
> 
> I'm not keen on the solution as it's esoteric and to me seems fragile.
> I've looked at discussion on v7 and can see why you ended up with this
> but I'd have preferred to see the 'violent' approach :)

The issue though is similar to the argument for the creation of the
ACPI0017 device for CXL, there is not a great place to hang the einj
device-driver.

However, since einj has no legacy "auto-load" behavior, I think it is
not a lot of code to have einj's module_init() do something like this:

	einj_dev = platform_device_register_full(&einj_dev_info);
	platform_driver_register(&einj_driver);

Ben, you want to give that a shot? Jonathan is right that my proposed
hack is *a* solution but probably not *the* solution where this should
end up.
Ben Cheatham Dec. 20, 2023, 2:33 p.m. UTC | #4
On 12/19/23 9:39 AM, Jonathan Cameron wrote:
> On Mon, 18 Dec 2023 15:59:12 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
>> Ben Cheatham wrote:
>>> The CXL core module should be able to load regardless of whether the
>>> EINJ module initializes correctly. Instead of porting the EINJ module to
>>> a library module, add a wrapper __init function around einj_init() to  
>>
>> Small quibble with this wording... the larger EINJ module refactoring
>> would be separating module_init() from EINJ probe(). As is this simple
>> introduction of an einit_init() wrapper *is* refactoring this module to
>> be used as a module dependency.
>>
>>> pin the EINJ module even if it does not initialize correctly. This
>>> should be fine since the EINJ module is only ever unloaded manually.
>>>
>>> One note: since the CXL core will be calling into the EINJ module
>>> directly, even though it may not have initialized, all CXL helper
>>> functions *have* to check if the EINJ module is initialized before
>>> doing any work.  
>>
>> Another small quibble here, perhaps s/may not have initialized/may not
>> have successfully initialized/? Because initialization will have
>> definitely completed one way or the other, but callers need to abort if
>> it completed in error.
>>
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
>>
>> Did Jonathan really get in and review this new patch in the series
>> before me? If yes, apologies I missed it, if no I think it is best
>> practice to not carry forward series Reviewed-by's if new patches appear
>> in the series between revisions.
> 
> I'm not keen on the solution as it's esoteric and to me seems fragile.
> I've looked at discussion on v7 and can see why you ended up with this
> but I'd have preferred to see the 'violent' approach :)
> 
> I don't mind if there is consensus on this, but not reviewed by from
> me for this one.
> 

Gotcha, I'll go ahead on drop your reviewed-by.
>>
>>
>>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>  
>>
>> With above fixups, feel free to add:
>>
>> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
Ben Cheatham Dec. 20, 2023, 2:37 p.m. UTC | #5
On 12/19/23 2:48 PM, Dan Williams wrote:
> Jonathan Cameron wrote:
>> On Mon, 18 Dec 2023 15:59:12 -0800
>> Dan Williams <dan.j.williams@intel.com> wrote:
>>
>>> Ben Cheatham wrote:
>>>> The CXL core module should be able to load regardless of whether the
>>>> EINJ module initializes correctly. Instead of porting the EINJ module to
>>>> a library module, add a wrapper __init function around einj_init() to  
>>>
>>> Small quibble with this wording... the larger EINJ module refactoring
>>> would be separating module_init() from EINJ probe(). As is this simple
>>> introduction of an einit_init() wrapper *is* refactoring this module to
>>> be used as a module dependency.
>>>
>>>> pin the EINJ module even if it does not initialize correctly. This
>>>> should be fine since the EINJ module is only ever unloaded manually.
>>>>
>>>> One note: since the CXL core will be calling into the EINJ module
>>>> directly, even though it may not have initialized, all CXL helper
>>>> functions *have* to check if the EINJ module is initialized before
>>>> doing any work.  
>>>
>>> Another small quibble here, perhaps s/may not have initialized/may not
>>> have successfully initialized/? Because initialization will have
>>> definitely completed one way or the other, but callers need to abort if
>>> it completed in error.
>>>
>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
>>>
>>> Did Jonathan really get in and review this new patch in the series
>>> before me? If yes, apologies I missed it, if no I think it is best
>>> practice to not carry forward series Reviewed-by's if new patches appear
>>> in the series between revisions.
>>
>> I'm not keen on the solution as it's esoteric and to me seems fragile.
>> I've looked at discussion on v7 and can see why you ended up with this
>> but I'd have preferred to see the 'violent' approach :)
> 
> The issue though is similar to the argument for the creation of the
> ACPI0017 device for CXL, there is not a great place to hang the einj
> device-driver.
> 
> However, since einj has no legacy "auto-load" behavior, I think it is
> not a lot of code to have einj's module_init() do something like this:
> 
> 	einj_dev = platform_device_register_full(&einj_dev_info);
> 	platform_driver_register(&einj_driver);
> 
> Ben, you want to give that a shot? Jonathan is right that my proposed
> hack is *a* solution but probably not *the* solution where this should
> end up.

I can take a look. I won't be able to get to it until around the new year
since I'm vacation at the moment. I'll also respond take a look at the
rest of your review around then.

Thanks,
Ben
Dan Williams Dec. 20, 2023, 11:51 p.m. UTC | #6
Ben Cheatham wrote:
[..]
> > Ben, you want to give that a shot? Jonathan is right that my proposed
> > hack is *a* solution but probably not *the* solution where this should
> > end up.
> 
> I can take a look. I won't be able to get to it until around the new year
> since I'm vacation at the moment. I'll also respond take a look at the
> rest of your review around then.

Enjoy your vacation, and thanks for all the work on this. We'll get it
sorted, and I think small bit of EINJ modernization will be a nice New
Year's gift.
diff mbox series

Patch

diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 013eb621dc92..26a887d2a5cd 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -615,6 +615,8 @@  static int available_error_type_show(struct seq_file *m, void *v)
 
 DEFINE_SHOW_ATTRIBUTE(available_error_type);
 
+static bool einj_initialized;
+
 static int error_type_get(void *data, u64 *val)
 {
 	*val = error_type;
@@ -684,7 +686,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 +784,31 @@  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 EINJ helpers
+	 * regardless of whether the EINJ table is present and initialized
+	 * correctly. CXL helpers check @einj_initialized before
+	 * doing any work.
+	 */
+	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) :