| Message ID | 20240220221146.399209-2-Benjamin.Cheatham@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl, EINJ: Update EINJ for CXL error types | expand |
Ben Cheatham wrote: > Change the EINJ module to install a platform device/driver on module > init and move the module init() and exit() functions to driver probe and > remove. This change allows the EINJ module to load regardless of whether > setting up EINJ succeeds, which allows dependent modules to still load > (i.e. the CXL core). > > Since EINJ may no longer be initialized when the module loads, any > functions that are called from dependent/external modules should check > the einj_initialized variable before calling any EINJ functions. > > Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com> > --- > drivers/acpi/apei/einj.c | 46 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 43 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c > index 89fb9331c611..6ea323b9d8ef 100644 > --- a/drivers/acpi/apei/einj.c > +++ b/drivers/acpi/apei/einj.c > @@ -21,6 +21,7 @@ > #include <linux/nmi.h> > #include <linux/delay.h> > #include <linux/mm.h> > +#include <linux/platform_device.h> > #include <asm/unaligned.h> > > #include "apei-internal.h" > @@ -137,6 +138,11 @@ static struct apei_exec_ins_type einj_ins_type[] = { > */ > static DEFINE_MUTEX(einj_mutex); > > +/* > + * Exported APIs use this flag to exit early if einj_probe() failed. > + */ > +static bool einj_initialized __ro_after_init; > + > static void *einj_param; > > static void einj_exec_ctx_init(struct apei_exec_context *ctx) > @@ -703,7 +709,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab) > return 0; > } > > -static int __init einj_init(void) > +static int __init einj_probe(struct platform_device *pdev) > { > int rc; > acpi_status status; > @@ -717,7 +723,7 @@ static int __init einj_init(void) > status = acpi_get_table(ACPI_SIG_EINJ, 0, > (struct acpi_table_header **)&einj_tab); > if (status == AE_NOT_FOUND) { > - pr_warn("EINJ table not found.\n"); > + pr_info("EINJ table not found.\n"); Per comment on cover letter this should be pr_debug() to hide it given that this module is no longer only loaded manually. > return -ENODEV; > } else if (ACPI_FAILURE(status)) { > pr_err("Failed to get EINJ table: %s\n", > @@ -805,7 +811,7 @@ static int __init einj_init(void) > return rc; > } > > -static void __exit einj_exit(void) > +static void __exit einj_remove(struct platform_device *pdev) > { > struct apei_exec_context ctx; > > @@ -826,6 +832,40 @@ static void __exit einj_exit(void) > acpi_put_table((struct acpi_table_header *)einj_tab); > } > > +static struct platform_device *einj_dev; > +struct platform_driver einj_driver = { This can be static. > + .remove_new = einj_remove, > + .driver = { > + .name = "acpi-einj", > + }, > +}; > + > +static int __init einj_init(void) > +{ > + struct platform_device_info einj_dev_info = { > + .name = "acpi-einj", > + .id = -1, > + }; > + int rc; > + > + einj_dev = platform_device_register_full(&einj_dev_info); > + if (IS_ERR_OR_NULL(einj_dev)) Given that platform_device_register_full() never returns NULL, this should be IS_ERR(). > + return PTR_ERR(einj_dev); > + > + rc = platform_driver_probe(&einj_driver, einj_probe); > + einj_initialized = rc == 0; > + > + return 0; > +} > + > +static void __exit einj_exit(void) > +{ > + if (einj_initialized) > + platform_driver_unregister(&einj_driver); > + > + platform_device_del(einj_dev); > +} > + > module_init(einj_init); > module_exit(einj_exit); > > -- > 2.34.1 >
Thanks for taking a look Dan, responses inline. On 2/21/24 12:18 AM, Dan Williams wrote: > Ben Cheatham wrote: >> Change the EINJ module to install a platform device/driver on module >> init and move the module init() and exit() functions to driver probe and >> remove. This change allows the EINJ module to load regardless of whether >> setting up EINJ succeeds, which allows dependent modules to still load >> (i.e. the CXL core). >> >> Since EINJ may no longer be initialized when the module loads, any >> functions that are called from dependent/external modules should check >> the einj_initialized variable before calling any EINJ functions. >> >> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com> >> --- >> drivers/acpi/apei/einj.c | 46 +++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 43 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c >> index 89fb9331c611..6ea323b9d8ef 100644 >> --- a/drivers/acpi/apei/einj.c >> +++ b/drivers/acpi/apei/einj.c >> @@ -21,6 +21,7 @@ >> #include <linux/nmi.h> >> #include <linux/delay.h> >> #include <linux/mm.h> >> +#include <linux/platform_device.h> >> #include <asm/unaligned.h> >> >> #include "apei-internal.h" >> @@ -137,6 +138,11 @@ static struct apei_exec_ins_type einj_ins_type[] = { >> */ >> static DEFINE_MUTEX(einj_mutex); >> >> +/* >> + * Exported APIs use this flag to exit early if einj_probe() failed. >> + */ >> +static bool einj_initialized __ro_after_init; >> + >> static void *einj_param; >> >> static void einj_exec_ctx_init(struct apei_exec_context *ctx) >> @@ -703,7 +709,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab) >> return 0; >> } >> >> -static int __init einj_init(void) >> +static int __init einj_probe(struct platform_device *pdev) >> { >> int rc; >> acpi_status status; >> @@ -717,7 +723,7 @@ static int __init einj_init(void) >> status = acpi_get_table(ACPI_SIG_EINJ, 0, >> (struct acpi_table_header **)&einj_tab); >> if (status == AE_NOT_FOUND) { >> - pr_warn("EINJ table not found.\n"); >> + pr_info("EINJ table not found.\n"); > > Per comment on cover letter this should be pr_debug() to hide it given > that this module is no longer only loaded manually. > Alright sounds good. >> return -ENODEV; >> } else if (ACPI_FAILURE(status)) { >> pr_err("Failed to get EINJ table: %s\n", >> @@ -805,7 +811,7 @@ static int __init einj_init(void) >> return rc; >> } >> >> -static void __exit einj_exit(void) >> +static void __exit einj_remove(struct platform_device *pdev) >> { >> struct apei_exec_context ctx; >> >> @@ -826,6 +832,40 @@ static void __exit einj_exit(void) >> acpi_put_table((struct acpi_table_header *)einj_tab); >> } >> >> +static struct platform_device *einj_dev; >> +struct platform_driver einj_driver = { > > This can be static. > Will change. >> + .remove_new = einj_remove, >> + .driver = { >> + .name = "acpi-einj", >> + }, >> +}; >> + >> +static int __init einj_init(void) >> +{ >> + struct platform_device_info einj_dev_info = { >> + .name = "acpi-einj", >> + .id = -1, >> + }; >> + int rc; >> + >> + einj_dev = platform_device_register_full(&einj_dev_info); >> + if (IS_ERR_OR_NULL(einj_dev)) > > Given that platform_device_register_full() never returns NULL, this > should be IS_ERR(). > Sure thing. >> + return PTR_ERR(einj_dev); >> + >> + rc = platform_driver_probe(&einj_driver, einj_probe); >> + einj_initialized = rc == 0; >> + >> + return 0; >> +} >> + >> +static void __exit einj_exit(void) >> +{ >> + if (einj_initialized) >> + platform_driver_unregister(&einj_driver); >> + >> + platform_device_del(einj_dev); >> +} >> + >> module_init(einj_init); >> module_exit(einj_exit); >> >> -- >> 2.34.1 >> > >
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c index 89fb9331c611..6ea323b9d8ef 100644 --- a/drivers/acpi/apei/einj.c +++ b/drivers/acpi/apei/einj.c @@ -21,6 +21,7 @@ #include <linux/nmi.h> #include <linux/delay.h> #include <linux/mm.h> +#include <linux/platform_device.h> #include <asm/unaligned.h> #include "apei-internal.h" @@ -137,6 +138,11 @@ static struct apei_exec_ins_type einj_ins_type[] = { */ static DEFINE_MUTEX(einj_mutex); +/* + * Exported APIs use this flag to exit early if einj_probe() failed. + */ +static bool einj_initialized __ro_after_init; + static void *einj_param; static void einj_exec_ctx_init(struct apei_exec_context *ctx) @@ -703,7 +709,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab) return 0; } -static int __init einj_init(void) +static int __init einj_probe(struct platform_device *pdev) { int rc; acpi_status status; @@ -717,7 +723,7 @@ static int __init einj_init(void) status = acpi_get_table(ACPI_SIG_EINJ, 0, (struct acpi_table_header **)&einj_tab); if (status == AE_NOT_FOUND) { - pr_warn("EINJ table not found.\n"); + pr_info("EINJ table not found.\n"); return -ENODEV; } else if (ACPI_FAILURE(status)) { pr_err("Failed to get EINJ table: %s\n", @@ -805,7 +811,7 @@ static int __init einj_init(void) return rc; } -static void __exit einj_exit(void) +static void __exit einj_remove(struct platform_device *pdev) { struct apei_exec_context ctx; @@ -826,6 +832,40 @@ static void __exit einj_exit(void) acpi_put_table((struct acpi_table_header *)einj_tab); } +static struct platform_device *einj_dev; +struct platform_driver einj_driver = { + .remove_new = einj_remove, + .driver = { + .name = "acpi-einj", + }, +}; + +static int __init einj_init(void) +{ + struct platform_device_info einj_dev_info = { + .name = "acpi-einj", + .id = -1, + }; + int rc; + + einj_dev = platform_device_register_full(&einj_dev_info); + if (IS_ERR_OR_NULL(einj_dev)) + return PTR_ERR(einj_dev); + + rc = platform_driver_probe(&einj_driver, einj_probe); + einj_initialized = rc == 0; + + return 0; +} + +static void __exit einj_exit(void) +{ + if (einj_initialized) + platform_driver_unregister(&einj_driver); + + platform_device_del(einj_dev); +} + module_init(einj_init); module_exit(einj_exit);
Change the EINJ module to install a platform device/driver on module init and move the module init() and exit() functions to driver probe and remove. This change allows the EINJ module to load regardless of whether setting up EINJ succeeds, which allows dependent modules to still load (i.e. the CXL core). Since EINJ may no longer be initialized when the module loads, any functions that are called from dependent/external modules should check the einj_initialized variable before calling any EINJ functions. Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com> --- drivers/acpi/apei/einj.c | 46 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-)