| Message ID | 20240115172007.309547-4-Benjamin.Cheatham@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | CXL, ACPI, APEI, 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 | 43 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 41 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c > index 013eb621dc92..10d51cd73fa4 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" > @@ -136,6 +137,12 @@ static struct apei_exec_ins_type einj_ins_type[] = { > */ > static DEFINE_MUTEX(einj_mutex); > > +/* > + * Functions called from dependent modules should check this variable > + * before using any EINJ functionality. > + */ This reads slightly odd to me, is this clearer? "Exported APIs use this flag to exit early if einj_probe() failed." > +static bool einj_initialized; This can be marked __ro_after_init to make it clear that it is static for the lifetime of the module. > + > static void *einj_param; > > static void einj_exec_ctx_init(struct apei_exec_context *ctx) > @@ -684,7 +691,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab) > return 0; > } > > -static int __init einj_init(void) > +static int einj_probe(struct platform_device *pdev) This can remain __init since nothing will call this function port init(). > { > int rc; > acpi_status status; > @@ -782,7 +789,7 @@ static int __init einj_init(void) > return rc; > } > > -static void __exit einj_exit(void) > +static void einj_remove(struct platform_device *pdev) Similarly this can remain __exit. > { > struct apei_exec_context ctx; > > @@ -801,6 +808,38 @@ 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 = "einj", Perhaps call this acpi-einj just to preserve the namespace in case a cross-platform generic "einj" comes along. > + }, > +}; > + > +static int __init einj_init(void) > +{ > + struct platform_device_info einj_dev_info = { > + .name = "einj", Ditt "acpi-einj" > + .id = -1, > + }; > + > + einj_dev = platform_device_register_full(&einj_dev_info); Just return early here if this failed. > + einj_initialized = !platform_driver_probe(&einj_driver, einj_probe); Nit, but since platform_driver_probe() does not return bool, I would prefer this to be more explicit: err = platform_driver_probe(); einj_initialized = err == 0; I think it is ok for the platform-device to stick around if einj_probe() failures as userspace can see that the module is loaded but driver-init failed. Similarly it's probably also ok to fail the module load if platform_device_register_full() fails since something deeper is wrong with the system if it is starting to fail something so basic. > + if (!(einj_initialized || IS_ERR_OR_NULL(einj_dev))) > + platform_device_del(einj_dev); > + > + return 0; > +} > + > +static void __exit einj_exit(void) > +{ > + if (einj_initialized) { > + platform_driver_unregister(&einj_driver); > + platform_device_del(einj_dev); Per above, this device_del can move outside the conditional.
Hi Dan, thanks for the quick response! Comments inline. On 1/16/24 5:47 PM, 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 | 43 ++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 41 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c >> index 013eb621dc92..10d51cd73fa4 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" >> @@ -136,6 +137,12 @@ static struct apei_exec_ins_type einj_ins_type[] = { >> */ >> static DEFINE_MUTEX(einj_mutex); >> >> +/* >> + * Functions called from dependent modules should check this variable >> + * before using any EINJ functionality. >> + */ > > This reads slightly odd to me, is this clearer? > > "Exported APIs use this flag to exit early if einj_probe() failed." > That is clearer, I'll change it. >> +static bool einj_initialized; > > This can be marked __ro_after_init to make it clear that it is static > for the lifetime of the module. > Will do. >> + >> static void *einj_param; >> >> static void einj_exec_ctx_init(struct apei_exec_context *ctx) >> @@ -684,7 +691,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab) >> return 0; >> } >> >> -static int __init einj_init(void) >> +static int einj_probe(struct platform_device *pdev) > > This can remain __init since nothing will call this function port > init(). > >> { >> int rc; >> acpi_status status; >> @@ -782,7 +789,7 @@ static int __init einj_init(void) >> return rc; >> } >> >> -static void __exit einj_exit(void) >> +static void einj_remove(struct platform_device *pdev) > > Similarly this can remain __exit. > >> { >> struct apei_exec_context ctx; >> >> @@ -801,6 +808,38 @@ 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 = "einj", > > Perhaps call this acpi-einj just to preserve the namespace in case a > cross-platform generic "einj" comes along. > Gotcha, I'll make that change. >> + }, >> +}; >> + >> +static int __init einj_init(void) >> +{ >> + struct platform_device_info einj_dev_info = { >> + .name = "einj", > > Ditt "acpi-einj" > >> + .id = -1, >> + }; >> + >> + einj_dev = platform_device_register_full(&einj_dev_info); > > Just return early here if this failed. > Will do. >> + einj_initialized = !platform_driver_probe(&einj_driver, einj_probe); > > Nit, but since platform_driver_probe() does not return bool, I would > prefer this to be more explicit: > > err = platform_driver_probe(); > einj_initialized = err == 0; > I agree. I was trying to not have to use an extra variable for only one line but einj_initialized = platform_driver_probe() == 0; went over the column limit :/. > I think it is ok for the platform-device to stick around if einj_probe() > failures as userspace can see that the module is loaded but driver-init > failed. > My reasoning for this was since this is really a dummy device I didn't want to pollute the platform device in the case the driver failed, but I see the reasoning here and agree with you. > Similarly it's probably also ok to fail the module load if > platform_device_register_full() fails since something deeper is wrong > with the system if it is starting to fail something so basic. > Alright, will do. >> + if (!(einj_initialized || IS_ERR_OR_NULL(einj_dev))) >> + platform_device_del(einj_dev); >> + >> + return 0; >> +} >> + >> +static void __exit einj_exit(void) >> +{ >> + if (einj_initialized) { >> + platform_driver_unregister(&einj_driver); >> + platform_device_del(einj_dev); > > Per above, this device_del can move outside the conditional.
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c index 013eb621dc92..10d51cd73fa4 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" @@ -136,6 +137,12 @@ static struct apei_exec_ins_type einj_ins_type[] = { */ static DEFINE_MUTEX(einj_mutex); +/* + * Functions called from dependent modules should check this variable + * before using any EINJ functionality. + */ +static bool einj_initialized; + static void *einj_param; static void einj_exec_ctx_init(struct apei_exec_context *ctx) @@ -684,7 +691,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab) return 0; } -static int __init einj_init(void) +static int einj_probe(struct platform_device *pdev) { int rc; acpi_status status; @@ -782,7 +789,7 @@ static int __init einj_init(void) return rc; } -static void __exit einj_exit(void) +static void einj_remove(struct platform_device *pdev) { struct apei_exec_context ctx; @@ -801,6 +808,38 @@ 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 = "einj", + }, +}; + +static int __init einj_init(void) +{ + struct platform_device_info einj_dev_info = { + .name = "einj", + .id = -1, + }; + + einj_dev = platform_device_register_full(&einj_dev_info); + einj_initialized = !platform_driver_probe(&einj_driver, einj_probe); + + if (!(einj_initialized || IS_ERR_OR_NULL(einj_dev))) + platform_device_del(einj_dev); + + 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 | 43 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-)