| Message ID | 20231213223702.543419-3-Benjamin.Cheatham@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CXL, ACPI, APEI, EINJ: Update EINJ for CXL error types | expand |
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>
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>
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.
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> >
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
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 --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) :