| Message ID | 20240329100203.540368-2-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | In Next |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | ACPI: APEI: EINJ: mark remove callback as __exit | expand |
Uwe Kleine-König wrote: > The einj_driver driver is registered using platform_driver_probe(). In > this case it cannot get unbound via sysfs and it's ok to put the remove > callback into an exit section. To prevent the modpost warning about > einj_driver referencing .exit.text, mark the driver struct with > __refdata and explain the situation in a comment. > > This is an improvement over commit a24118a8a687 ("ACPI: APEI: EINJ: mark > remove callback as non-__exit") which recently addressed the same issue, > but picked a less optimal variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks for the explanation Uwe, this makes sense. Rafael, do you want to pick this one up as well? Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On Fri, Mar 29, 2024, at 11:02, Uwe Kleine-König wrote: > The einj_driver driver is registered using platform_driver_probe(). In > this case it cannot get unbound via sysfs and it's ok to put the remove > callback into an exit section. To prevent the modpost warning about > einj_driver referencing .exit.text, mark the driver struct with > __refdata and explain the situation in a comment. > > This is an improvement over commit a24118a8a687 ("ACPI: APEI: EINJ: mark > remove callback as non-__exit") which recently addressed the same issue, > but picked a less optimal variant. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: Arnd Bergmann <arnd@arndb.de> I noticed another curiosity: > static struct platform_device *einj_dev; > -static struct platform_driver einj_driver = { > - .remove_new = einj_remove, > +/* > + * einj_remove() lives in .exit.text. For drivers registered via > + * platform_driver_probe() this is ok because they cannot get unbound at > + * runtime. So mark the driver struct with __refdata to prevent modpost > + * triggering a section mismatch warning. > + */ > +static struct platform_driver einj_driver __refdata = { > + .remove_new = __exit_p(einj_remove), > .driver = { > .name = "acpi-einj", > }, I was wondering why this doesn't cause an "unused function" warning for einj_remove(), given that __exit_p() turns the reference into NULL. As it turns out, the __exit annotation marks the function as "__attribute__((used))", so it still gets put in the object file but then dropped by the linker. The __used annotation seems to predate the introduction of "__attribute__((unused))", which would seem more appropriate here, which would allow more dead-code elimination. The patch below gets rid of the __used annotation completely, which in turn uncovers some interesting bugs with __exit functions in built-in code that are never called from anywhere, like drivers/video/fbdev/asiliantfb.c:627:20: error: 'asiliantfb_exit' defined but not used [-Werror=unused-function] Arnd diff --git a/include/linux/init.h b/include/linux/init.h index 58cef4c2e59a..d0e6354f3050 100644 --- a/include/linux/init.h +++ b/include/linux/init.h @@ -82,7 +82,7 @@ #define __exitused __used #endif -#define __exit __section(".exit.text") __exitused __cold notrace +#define __exit __section(".exit.text") __cold notrace /* Used for MEMORY_HOTPLUG */ #define __meminit __section(".meminit.text") __cold notrace \ @@ -394,7 +394,7 @@ void __init parse_early_options(char *cmdline); #ifdef MODULE #define __exit_p(x) x #else -#define __exit_p(x) NULL +#define __exit_p(x) (0 ? (x) : NULL) #endif #endif /* _LINUX_INIT_H */
On Sat, Mar 30, 2024 at 08:00:32AM +0100, Arnd Bergmann wrote: > On Fri, Mar 29, 2024, at 11:02, Uwe Kleine-König wrote: > > The einj_driver driver is registered using platform_driver_probe(). In > > this case it cannot get unbound via sysfs and it's ok to put the remove > > callback into an exit section. To prevent the modpost warning about > > einj_driver referencing .exit.text, mark the driver struct with > > __refdata and explain the situation in a comment. > > > > This is an improvement over commit a24118a8a687 ("ACPI: APEI: EINJ: mark > > remove callback as non-__exit") which recently addressed the same issue, > > but picked a less optimal variant. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > I noticed another curiosity: > > > static struct platform_device *einj_dev; > > -static struct platform_driver einj_driver = { > > - .remove_new = einj_remove, > > +/* > > + * einj_remove() lives in .exit.text. For drivers registered via > > + * platform_driver_probe() this is ok because they cannot get unbound at > > + * runtime. So mark the driver struct with __refdata to prevent modpost > > + * triggering a section mismatch warning. > > + */ > > +static struct platform_driver einj_driver __refdata = { > > + .remove_new = __exit_p(einj_remove), > > .driver = { > > .name = "acpi-einj", > > }, > > I was wondering why this doesn't cause an "unused function" > warning for einj_remove(), given that __exit_p() turns the > reference into NULL. > > As it turns out, the __exit annotation marks the function as > "__attribute__((used))", so it still gets put in the object > file but then dropped by the linker. The __used annotation > seems to predate the introduction of "__attribute__((unused))", > which would seem more appropriate here, which would allow > more dead-code elimination. > > The patch below gets rid of the __used annotation completely, > which in turn uncovers some interesting bugs with __exit > functions in built-in code that are never called from > anywhere, like > > drivers/video/fbdev/asiliantfb.c:627:20: error: 'asiliantfb_exit' defined but not used [-Werror=unused-function] Do you plan to follow up with the respective fixes? If not I can add it to my list of things to clean up. > diff --git a/include/linux/init.h b/include/linux/init.h > index 58cef4c2e59a..d0e6354f3050 100644 > --- a/include/linux/init.h > +++ b/include/linux/init.h > @@ -82,7 +82,7 @@ > #define __exitused __used > #endif > > -#define __exit __section(".exit.text") __exitused __cold notrace > +#define __exit __section(".exit.text") __cold notrace include/linux/init.h:82:1: error: '__exitused' defined but not used [-Werror=unused-macro] :-) Apart from that: nice find. > /* Used for MEMORY_HOTPLUG */ > #define __meminit __section(".meminit.text") __cold notrace \ > @@ -394,7 +394,7 @@ void __init parse_early_options(char *cmdline); > #ifdef MODULE > #define __exit_p(x) x > #else > -#define __exit_p(x) NULL > +#define __exit_p(x) (0 ? (x) : NULL) I remember wondering about __exit_p not using this idiom, but I didn't follow that thought. Best regards Uwe
On Fri, Mar 29, 2024 at 7:55 PM Dan Williams <dan.j.williams@intel.com> wrote: > > Uwe Kleine-König wrote: > > The einj_driver driver is registered using platform_driver_probe(). In > > this case it cannot get unbound via sysfs and it's ok to put the remove > > callback into an exit section. To prevent the modpost warning about > > einj_driver referencing .exit.text, mark the driver struct with > > __refdata and explain the situation in a comment. > > > > This is an improvement over commit a24118a8a687 ("ACPI: APEI: EINJ: mark > > remove callback as non-__exit") which recently addressed the same issue, > > but picked a less optimal variant. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Thanks for the explanation Uwe, this makes sense. > > Rafael, do you want to pick this one up as well? > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> Applied as 6.10 material, thanks!
diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c index 01faca3a238a..9515bcfe5e97 100644 --- a/drivers/acpi/apei/einj-core.c +++ b/drivers/acpi/apei/einj-core.c @@ -851,7 +851,7 @@ static int __init einj_probe(struct platform_device *pdev) return rc; } -static void einj_remove(struct platform_device *pdev) +static void __exit einj_remove(struct platform_device *pdev) { struct apei_exec_context ctx; @@ -873,8 +873,14 @@ static void einj_remove(struct platform_device *pdev) } static struct platform_device *einj_dev; -static struct platform_driver einj_driver = { - .remove_new = einj_remove, +/* + * einj_remove() lives in .exit.text. For drivers registered via + * platform_driver_probe() this is ok because they cannot get unbound at + * runtime. So mark the driver struct with __refdata to prevent modpost + * triggering a section mismatch warning. + */ +static struct platform_driver einj_driver __refdata = { + .remove_new = __exit_p(einj_remove), .driver = { .name = "acpi-einj", },
The einj_driver driver is registered using platform_driver_probe(). In this case it cannot get unbound via sysfs and it's ok to put the remove callback into an exit section. To prevent the modpost warning about einj_driver referencing .exit.text, mark the driver struct with __refdata and explain the situation in a comment. This is an improvement over commit a24118a8a687 ("ACPI: APEI: EINJ: mark remove callback as non-__exit") which recently addressed the same issue, but picked a less optimal variant. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/acpi/apei/einj-core.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)