| Message ID | 20231107091740.3924258-2-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | fb: handle remove callbacks in .exit.text and convert to .remove_new | expand |
Hi Uwe, On Tue, Nov 7, 2023 at 10:20 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On today's platforms the benefit of platform_driver_probe() isn't that > relevant any more. It allows to drop some code after booting (or module > loading) for .probe() and discard the .remove() function completely if > the driver is built-in. This typically saves a few 100k. Which is a lot on platforms with only a few MiBs of RAM... > The downside of platform_driver_probe() is that the driver cannot be > bound and unbound at runtime which is ancient and also slightly > complicates testing. There are also thoughts to deprecate > platform_driver_probe() because it adds some complexity in the driver > core for little gain. Also many drivers don't use it correctly. This > driver for example misses to mark the driver struct with __refdata which > is needed to suppress a (W=1) modpost warning: > > WARNING: modpost: drivers/video/fbdev/amifb: section mismatch in reference: amifb_driver+0x4 (section: .data) -> amifb_remove (section: .exit.text) > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks for your patch! > --- a/drivers/video/fbdev/amifb.c > +++ b/drivers/video/fbdev/amifb.c > @@ -3530,7 +3530,7 @@ static inline void chipfree(void) > * Initialisation > */ > > -static int __init amifb_probe(struct platform_device *pdev) > +static int amifb_probe(struct platform_device *pdev) noreply@ellerman.id.au reported the following build failure for e.g. m68k-defconfig in next-20231108: WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x15c (section: .text) -> ami_modedb (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x17a (section: .text) -> amifb_hfmin (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x188 (section: .text) -> amifb_hfmax (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x190 (section: .text) -> amifb_vfmin (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x198 (section: .text) -> amifb_vfmax (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x1ba (section: .text) -> ami_modedb (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x1c4 (section: .text) -> ami_modedb (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x1ca (section: .text) -> mode_option (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x1ee (section: .text) -> ami_modedb (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x398 (section: .text) -> amifb_hfmin (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x39e (section: .text) -> amifb_hfmax (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x3a4 (section: .text) -> amifb_vfmin (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x3aa (section: .text) -> amifb_vfmax (section: .init.data) WARNING: modpost: vmlinux: section mismatch in reference: amifb_probe+0x3f0 (section: .text) -> mode_option (section: .init.data) ERROR: modpost: Section mismatches detected. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 11/8/23 22:06, Geert Uytterhoeven wrote: > On Tue, Nov 7, 2023 at 10:20 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: >> On today's platforms the benefit of platform_driver_probe() isn't that >> relevant any more. It allows to drop some code after booting (or module >> loading) for .probe() and discard the .remove() function completely if >> the driver is built-in. This typically saves a few 100k. > > Which is a lot on platforms with only a few MiBs of RAM... True. Given the warnings below, what is your suggestion? Better to drop the amifb patch ? Helge ... > WARNING: modpost: vmlinux: section mismatch in reference: > amifb_probe+0x15c (section: .text) -> ami_modedb (section: .init.data) > ...
Hi Helge, On Wed, Nov 8, 2023 at 10:32 PM Helge Deller <deller@gmx.de> wrote: > On 11/8/23 22:06, Geert Uytterhoeven wrote: > > On Tue, Nov 7, 2023 at 10:20 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > >> On today's platforms the benefit of platform_driver_probe() isn't that > >> relevant any more. It allows to drop some code after booting (or module > >> loading) for .probe() and discard the .remove() function completely if > >> the driver is built-in. This typically saves a few 100k. > > > > Which is a lot on platforms with only a few MiBs of RAM... > > True. > Given the warnings below, what is your suggestion? > Better to drop the amifb patch ? I think so. There is a reason these drivers use platform_driver_probe(). Gr{oetje,eeting}s, Geert
On 11/8/23 22:34, Geert Uytterhoeven wrote: > Hi Helge, > > On Wed, Nov 8, 2023 at 10:32 PM Helge Deller <deller@gmx.de> wrote: >> On 11/8/23 22:06, Geert Uytterhoeven wrote: >>> On Tue, Nov 7, 2023 at 10:20 AM Uwe Kleine-König >>> <u.kleine-koenig@pengutronix.de> wrote: >>>> On today's platforms the benefit of platform_driver_probe() isn't that >>>> relevant any more. It allows to drop some code after booting (or module >>>> loading) for .probe() and discard the .remove() function completely if >>>> the driver is built-in. This typically saves a few 100k. >>> >>> Which is a lot on platforms with only a few MiBs of RAM... >> >> True. >> Given the warnings below, what is your suggestion? >> Better to drop the amifb patch ? > > I think so. There is a reason these drivers use platform_driver_probe(). Ok, I've dropped both amifb patches. Helge
diff --git a/drivers/video/fbdev/amifb.c b/drivers/video/fbdev/amifb.c index b18c6b4f129a..85da43034166 100644 --- a/drivers/video/fbdev/amifb.c +++ b/drivers/video/fbdev/amifb.c @@ -3530,7 +3530,7 @@ static inline void chipfree(void) * Initialisation */ -static int __init amifb_probe(struct platform_device *pdev) +static int amifb_probe(struct platform_device *pdev) { struct fb_info *info; int tag, i, err = 0; @@ -3752,7 +3752,7 @@ static int __init amifb_probe(struct platform_device *pdev) } -static int __exit amifb_remove(struct platform_device *pdev) +static int amifb_remove(struct platform_device *pdev) { struct fb_info *info = platform_get_drvdata(pdev); @@ -3769,13 +3769,13 @@ static int __exit amifb_remove(struct platform_device *pdev) } static struct platform_driver amifb_driver = { - .remove = __exit_p(amifb_remove), - .driver = { + .probe = amifb_probe, + .remove = amifb_remove, + .driver = { .name = "amiga-video", }, }; - -module_platform_driver_probe(amifb_driver, amifb_probe); +module_platform_driver(amifb_driver); MODULE_LICENSE("GPL"); MODULE_ALIAS("platform:amiga-video");
On today's platforms the benefit of platform_driver_probe() isn't that relevant any more. It allows to drop some code after booting (or module loading) for .probe() and discard the .remove() function completely if the driver is built-in. This typically saves a few 100k. The downside of platform_driver_probe() is that the driver cannot be bound and unbound at runtime which is ancient and also slightly complicates testing. There are also thoughts to deprecate platform_driver_probe() because it adds some complexity in the driver core for little gain. Also many drivers don't use it correctly. This driver for example misses to mark the driver struct with __refdata which is needed to suppress a (W=1) modpost warning: WARNING: modpost: drivers/video/fbdev/amifb: section mismatch in reference: amifb_driver+0x4 (section: .data) -> amifb_remove (section: .exit.text) Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/video/fbdev/amifb.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)