| Message ID | 20231107091740.3924258-3-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 |
On Tue, Nov 07, 2023 at 10:17:43AM +0100, Uwe Kleine-König 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. > > 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/atmel_lcdfb: section mismatch in reference: atmel_lcdfb_driver+0x4 (section: .data) -> atmel_lcdfb_remove (section: .exit.text) > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/video/fbdev/atmel_lcdfb.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c > index a908db233409..b218731ef732 100644 > --- a/drivers/video/fbdev/atmel_lcdfb.c > +++ b/drivers/video/fbdev/atmel_lcdfb.c > @@ -1017,7 +1017,7 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo) > return ret; > } > > -static int __init atmel_lcdfb_probe(struct platform_device *pdev) > +static int atmel_lcdfb_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct fb_info *info; > @@ -1223,7 +1223,7 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev) > return ret; > } > > -static int __exit atmel_lcdfb_remove(struct platform_device *pdev) > +static int atmel_lcdfb_remove(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct fb_info *info = dev_get_drvdata(dev); > @@ -1301,7 +1301,8 @@ static int atmel_lcdfb_resume(struct platform_device *pdev) > #endif > > static struct platform_driver atmel_lcdfb_driver = { > - .remove = __exit_p(atmel_lcdfb_remove), > + .probe = atmel_lcdfb_probe, > + .remove = atmel_lcdfb_remove, > .suspend = atmel_lcdfb_suspend, > .resume = atmel_lcdfb_resume, > .driver = { > @@ -1310,7 +1311,7 @@ static struct platform_driver atmel_lcdfb_driver = { > }, > }; > > -module_platform_driver_probe(atmel_lcdfb_driver, atmel_lcdfb_probe); > +module_platform_driver(atmel_lcdfb_driver, ); Argh, the , must be removed. I had this in my working copy but forgot to squash it into this commit. Sorry! Can you squash in the following please?: diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c index 0531d6f6dcc5..88c75ae7d315 100644 --- a/drivers/video/fbdev/atmel_lcdfb.c +++ b/drivers/video/fbdev/atmel_lcdfb.c @@ -1308,8 +1308,7 @@ static struct platform_driver atmel_lcdfb_driver = { .of_match_table = atmel_lcdfb_dt_ids, }, }; - -module_platform_driver(atmel_lcdfb_driver, ); +module_platform_driver(atmel_lcdfb_driver); MODULE_DESCRIPTION("AT91 LCD Controller framebuffer driver"); MODULE_AUTHOR("Nicolas Ferre <nicolas.ferre@atmel.com>"); Best regards Uwe
On 11/7/23 21:01, Uwe Kleine-König wrote: > On Tue, Nov 07, 2023 at 10:17:43AM +0100, Uwe Kleine-König 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. >> >> 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/atmel_lcdfb: section mismatch in reference: atmel_lcdfb_driver+0x4 (section: .data) -> atmel_lcdfb_remove (section: .exit.text) >> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> --- >> drivers/video/fbdev/atmel_lcdfb.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c >> index a908db233409..b218731ef732 100644 >> --- a/drivers/video/fbdev/atmel_lcdfb.c >> +++ b/drivers/video/fbdev/atmel_lcdfb.c >> @@ -1017,7 +1017,7 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo) >> return ret; >> } >> >> -static int __init atmel_lcdfb_probe(struct platform_device *pdev) >> +static int atmel_lcdfb_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct fb_info *info; >> @@ -1223,7 +1223,7 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev) >> return ret; >> } >> >> -static int __exit atmel_lcdfb_remove(struct platform_device *pdev) >> +static int atmel_lcdfb_remove(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct fb_info *info = dev_get_drvdata(dev); >> @@ -1301,7 +1301,8 @@ static int atmel_lcdfb_resume(struct platform_device *pdev) >> #endif >> >> static struct platform_driver atmel_lcdfb_driver = { >> - .remove = __exit_p(atmel_lcdfb_remove), >> + .probe = atmel_lcdfb_probe, >> + .remove = atmel_lcdfb_remove, >> .suspend = atmel_lcdfb_suspend, >> .resume = atmel_lcdfb_resume, >> .driver = { >> @@ -1310,7 +1311,7 @@ static struct platform_driver atmel_lcdfb_driver = { >> }, >> }; >> >> -module_platform_driver_probe(atmel_lcdfb_driver, atmel_lcdfb_probe); >> +module_platform_driver(atmel_lcdfb_driver, ); > > Argh, the , must be removed. I had this in my working copy but forgot to > squash it into this commit. Sorry! > > Can you squash in the following please?: Sure. I fixed it up in the git tree. Thanks! Helge > diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c > index 0531d6f6dcc5..88c75ae7d315 100644 > --- a/drivers/video/fbdev/atmel_lcdfb.c > +++ b/drivers/video/fbdev/atmel_lcdfb.c > @@ -1308,8 +1308,7 @@ static struct platform_driver atmel_lcdfb_driver = { > .of_match_table = atmel_lcdfb_dt_ids, > }, > }; > - > -module_platform_driver(atmel_lcdfb_driver, ); > +module_platform_driver(atmel_lcdfb_driver); > > MODULE_DESCRIPTION("AT91 LCD Controller framebuffer driver"); > MODULE_AUTHOR("Nicolas Ferre <nicolas.ferre@atmel.com>"); > > Best regards > Uwe >
On Tue, Nov 07, 2023 at 10:17:43AM +0100, Uwe Kleine-König 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. > > 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/atmel_lcdfb: section mismatch in reference: atmel_lcdfb_driver+0x4 (section: .data) -> atmel_lcdfb_remove (section: .exit.text) > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/video/fbdev/atmel_lcdfb.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c > index a908db233409..b218731ef732 100644 > --- a/drivers/video/fbdev/atmel_lcdfb.c > +++ b/drivers/video/fbdev/atmel_lcdfb.c > @@ -1017,7 +1017,7 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo) > return ret; > } > > -static int __init atmel_lcdfb_probe(struct platform_device *pdev) > +static int atmel_lcdfb_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct fb_info *info; > @@ -1223,7 +1223,7 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev) > return ret; > } > > -static int __exit atmel_lcdfb_remove(struct platform_device *pdev) > +static int atmel_lcdfb_remove(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct fb_info *info = dev_get_drvdata(dev); > @@ -1301,7 +1301,8 @@ static int atmel_lcdfb_resume(struct platform_device *pdev) > #endif > > static struct platform_driver atmel_lcdfb_driver = { > - .remove = __exit_p(atmel_lcdfb_remove), > + .probe = atmel_lcdfb_probe, > + .remove = atmel_lcdfb_remove, > .suspend = atmel_lcdfb_suspend, > .resume = atmel_lcdfb_resume, > .driver = { > @@ -1310,7 +1311,7 @@ static struct platform_driver atmel_lcdfb_driver = { > }, > }; > > -module_platform_driver_probe(atmel_lcdfb_driver, atmel_lcdfb_probe); > +module_platform_driver(atmel_lcdfb_driver, ); > > MODULE_DESCRIPTION("AT91 LCD Controller framebuffer driver"); > MODULE_AUTHOR("Nicolas Ferre <nicolas.ferre@atmel.com>"); > -- > 2.42.0 > For what it's worth, this introduces a warning when building certain configurations (such as ARCH=arm multi_v5_defconfig) with clang: WARNING: modpost: vmlinux: section mismatch in reference: atmel_lcdfb_probe+0x6c4 (section: .text) -> atmel_lcdfb_init_fbinfo (section: .init.text) WARNING: modpost: vmlinux: section mismatch in reference: atmel_lcdfb_probe+0x858 (section: .text) -> atmel_lcdfb_fix (section: .init.rodata) This appears to be legitimate to me? GCC did not warn but I assume that is due to differences in inlining. The following clears it up for me, should I send a standalone patch or should this be squashed in? Cheers, Nathan diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c index 88c75ae7d315..9e391e5eaf9d 100644 --- a/drivers/video/fbdev/atmel_lcdfb.c +++ b/drivers/video/fbdev/atmel_lcdfb.c @@ -220,7 +220,7 @@ static inline void atmel_lcdfb_power_control(struct atmel_lcdfb_info *sinfo, int } } -static const struct fb_fix_screeninfo atmel_lcdfb_fix __initconst = { +static const struct fb_fix_screeninfo atmel_lcdfb_fix = { .type = FB_TYPE_PACKED_PIXELS, .visual = FB_VISUAL_TRUECOLOR, .xpanstep = 0, @@ -841,7 +841,7 @@ static void atmel_lcdfb_task(struct work_struct *work) atmel_lcdfb_reset(sinfo); } -static int __init atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo) +static int atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo) { struct fb_info *info = sinfo->info; int ret = 0;
On 11/8/23 19:48, Nathan Chancellor wrote: > On Tue, Nov 07, 2023 at 10:17:43AM +0100, Uwe Kleine-König 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. >> >> 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/atmel_lcdfb: section mismatch in reference: atmel_lcdfb_driver+0x4 (section: .data) -> atmel_lcdfb_remove (section: .exit.text) >> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> --- >> drivers/video/fbdev/atmel_lcdfb.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c >> index a908db233409..b218731ef732 100644 >> --- a/drivers/video/fbdev/atmel_lcdfb.c >> +++ b/drivers/video/fbdev/atmel_lcdfb.c >> @@ -1017,7 +1017,7 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo) >> return ret; >> } >> >> -static int __init atmel_lcdfb_probe(struct platform_device *pdev) >> +static int atmel_lcdfb_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct fb_info *info; >> @@ -1223,7 +1223,7 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev) >> return ret; >> } >> >> -static int __exit atmel_lcdfb_remove(struct platform_device *pdev) >> +static int atmel_lcdfb_remove(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct fb_info *info = dev_get_drvdata(dev); >> @@ -1301,7 +1301,8 @@ static int atmel_lcdfb_resume(struct platform_device *pdev) >> #endif >> >> static struct platform_driver atmel_lcdfb_driver = { >> - .remove = __exit_p(atmel_lcdfb_remove), >> + .probe = atmel_lcdfb_probe, >> + .remove = atmel_lcdfb_remove, >> .suspend = atmel_lcdfb_suspend, >> .resume = atmel_lcdfb_resume, >> .driver = { >> @@ -1310,7 +1311,7 @@ static struct platform_driver atmel_lcdfb_driver = { >> }, >> }; >> >> -module_platform_driver_probe(atmel_lcdfb_driver, atmel_lcdfb_probe); >> +module_platform_driver(atmel_lcdfb_driver, ); >> >> MODULE_DESCRIPTION("AT91 LCD Controller framebuffer driver"); >> MODULE_AUTHOR("Nicolas Ferre <nicolas.ferre@atmel.com>"); >> -- >> 2.42.0 >> > > For what it's worth, this introduces a warning when building certain > configurations (such as ARCH=arm multi_v5_defconfig) with clang: > > WARNING: modpost: vmlinux: section mismatch in reference: atmel_lcdfb_probe+0x6c4 (section: .text) -> atmel_lcdfb_init_fbinfo (section: .init.text) > WARNING: modpost: vmlinux: section mismatch in reference: atmel_lcdfb_probe+0x858 (section: .text) -> atmel_lcdfb_fix (section: .init.rodata) > > This appears to be legitimate to me? GCC did not warn but I assume that > is due to differences in inlining. The following clears it up for me, > should I send a standalone patch or should this be squashed in? I've squashed it into the original patch. Thank you! Helge > Cheers, > Nathan > > diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c > index 88c75ae7d315..9e391e5eaf9d 100644 > --- a/drivers/video/fbdev/atmel_lcdfb.c > +++ b/drivers/video/fbdev/atmel_lcdfb.c > @@ -220,7 +220,7 @@ static inline void atmel_lcdfb_power_control(struct atmel_lcdfb_info *sinfo, int > } > } > > -static const struct fb_fix_screeninfo atmel_lcdfb_fix __initconst = { > +static const struct fb_fix_screeninfo atmel_lcdfb_fix = { > .type = FB_TYPE_PACKED_PIXELS, > .visual = FB_VISUAL_TRUECOLOR, > .xpanstep = 0, > @@ -841,7 +841,7 @@ static void atmel_lcdfb_task(struct work_struct *work) > atmel_lcdfb_reset(sinfo); > } > > -static int __init atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo) > +static int atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo) > { > struct fb_info *info = sinfo->info; > int ret = 0;
Hello, On Wed, Nov 08, 2023 at 11:48:05AM -0700, Nathan Chancellor wrote: > diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c > index 88c75ae7d315..9e391e5eaf9d 100644 > --- a/drivers/video/fbdev/atmel_lcdfb.c > +++ b/drivers/video/fbdev/atmel_lcdfb.c > @@ -220,7 +220,7 @@ static inline void atmel_lcdfb_power_control(struct atmel_lcdfb_info *sinfo, int > } > } > > -static const struct fb_fix_screeninfo atmel_lcdfb_fix __initconst = { > +static const struct fb_fix_screeninfo atmel_lcdfb_fix = { > .type = FB_TYPE_PACKED_PIXELS, > .visual = FB_VISUAL_TRUECOLOR, > .xpanstep = 0, I wonder if this was broken already before my patch. atmel_lcdfb_probe() does info->fix = atmel_lcdfb_fix; and unless I miss something (this is well possible) that is used e.g. in atmel_lcdfb_set_par() -> atmel_lcdfb_update_dma(). So atmel_lcdfb_fix should better not live in .init memory?! Someone with more knowledge about fbdev might want to take a look and decide if this justifies a separate fix that should then be backported to stable, too?! > @@ -841,7 +841,7 @@ static void atmel_lcdfb_task(struct work_struct *work) > atmel_lcdfb_reset(sinfo); > } > > -static int __init atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo) > +static int atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo) > { > struct fb_info *info = sinfo->info; > int ret = 0; This is only a problem since my patch. Thanks for your report and patch. Best regards Uwe
On 11/8/23 22:00, Uwe Kleine-König wrote: > On Wed, Nov 08, 2023 at 11:48:05AM -0700, Nathan Chancellor wrote: >> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c >> index 88c75ae7d315..9e391e5eaf9d 100644 >> --- a/drivers/video/fbdev/atmel_lcdfb.c >> +++ b/drivers/video/fbdev/atmel_lcdfb.c >> @@ -220,7 +220,7 @@ static inline void atmel_lcdfb_power_control(struct atmel_lcdfb_info *sinfo, int >> } >> } >> >> -static const struct fb_fix_screeninfo atmel_lcdfb_fix __initconst = { >> +static const struct fb_fix_screeninfo atmel_lcdfb_fix = { >> .type = FB_TYPE_PACKED_PIXELS, >> .visual = FB_VISUAL_TRUECOLOR, >> .xpanstep = 0, > > I wonder if this was broken already before my patch. atmel_lcdfb_probe() > does > > info->fix = atmel_lcdfb_fix; > > and unless I miss something (this is well possible) that is used e.g. in > atmel_lcdfb_set_par() -> atmel_lcdfb_update_dma(). So atmel_lcdfb_fix > should better not live in .init memory?! Someone with more knowledge > about fbdev might want to take a look and decide if this justifies a > separate fix that should then be backported to stable, too?! I don't think a backport this is necessary. The "__initconst" atmel_lcdfb_fix struct was only copied in the "__init" atmel_lcdfb_probe() function. So, both were dropped at the same time in older kernels. Since your patch dropped the "__init" from atmel_lcdfb_probe(), the __initconst from atmel_lcdfb_fix has to be removed too. So, I believe folding in Nathan's patch is OK and we don't need a seperate (or backport) patch. Helge
On Wed, Nov 08, 2023 at 10:24:09PM +0100, Helge Deller wrote: > On 11/8/23 22:00, Uwe Kleine-König wrote: > > On Wed, Nov 08, 2023 at 11:48:05AM -0700, Nathan Chancellor wrote: > > > diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c > > > index 88c75ae7d315..9e391e5eaf9d 100644 > > > --- a/drivers/video/fbdev/atmel_lcdfb.c > > > +++ b/drivers/video/fbdev/atmel_lcdfb.c > > > @@ -220,7 +220,7 @@ static inline void atmel_lcdfb_power_control(struct atmel_lcdfb_info *sinfo, int > > > } > > > } > > > > > > -static const struct fb_fix_screeninfo atmel_lcdfb_fix __initconst = { > > > +static const struct fb_fix_screeninfo atmel_lcdfb_fix = { > > > .type = FB_TYPE_PACKED_PIXELS, > > > .visual = FB_VISUAL_TRUECOLOR, > > > .xpanstep = 0, > > > > I wonder if this was broken already before my patch. atmel_lcdfb_probe() > > does > > > > info->fix = atmel_lcdfb_fix; > > > > and unless I miss something (this is well possible) that is used e.g. in > > atmel_lcdfb_set_par() -> atmel_lcdfb_update_dma(). So atmel_lcdfb_fix > > should better not live in .init memory?! Someone with more knowledge > > about fbdev might want to take a look and decide if this justifies a > > separate fix that should then be backported to stable, too?! > > I don't think a backport this is necessary. > The "__initconst" atmel_lcdfb_fix struct was only copied in the > "__init" atmel_lcdfb_probe() function. > So, both were dropped at the same time in older kernels. But info and so info->fix live longer than the probe function, don't they? So a call to atmel_lcdfb_update_dma() should better not happen when .init is already discarded, right? Best regards Uwe
On 11/8/23 22:52, Uwe Kleine-König wrote: > On Wed, Nov 08, 2023 at 10:24:09PM +0100, Helge Deller wrote: >> On 11/8/23 22:00, Uwe Kleine-König wrote: >>> On Wed, Nov 08, 2023 at 11:48:05AM -0700, Nathan Chancellor wrote: >>>> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c >>>> index 88c75ae7d315..9e391e5eaf9d 100644 >>>> --- a/drivers/video/fbdev/atmel_lcdfb.c >>>> +++ b/drivers/video/fbdev/atmel_lcdfb.c >>>> @@ -220,7 +220,7 @@ static inline void atmel_lcdfb_power_control(struct atmel_lcdfb_info *sinfo, int >>>> } >>>> } >>>> >>>> -static const struct fb_fix_screeninfo atmel_lcdfb_fix __initconst = { >>>> +static const struct fb_fix_screeninfo atmel_lcdfb_fix = { >>>> .type = FB_TYPE_PACKED_PIXELS, >>>> .visual = FB_VISUAL_TRUECOLOR, >>>> .xpanstep = 0, >>> >>> I wonder if this was broken already before my patch. atmel_lcdfb_probe() >>> does >>> >>> info->fix = atmel_lcdfb_fix; >>> >>> and unless I miss something (this is well possible) that is used e.g. in >>> atmel_lcdfb_set_par() -> atmel_lcdfb_update_dma(). So atmel_lcdfb_fix >>> should better not live in .init memory?! Someone with more knowledge >>> about fbdev might want to take a look and decide if this justifies a >>> separate fix that should then be backported to stable, too?! >> >> I don't think a backport this is necessary. >> The "__initconst" atmel_lcdfb_fix struct was only copied in the >> "__init" atmel_lcdfb_probe() function. >> So, both were dropped at the same time in older kernels. > > But info and so info->fix live longer than the probe function, don't > they? Yes, they do. But AFAICS info->fix contains a *copy* of the initial atmel_lcdfb_fix struct (and not a pointer to it). So that should be ok. Helge
Hello, On Wed, Nov 08, 2023 at 10:57:00PM +0100, Helge Deller wrote: > On 11/8/23 22:52, Uwe Kleine-König wrote: > > But info and so info->fix live longer than the probe function, don't > > they? > > Yes, they do. > But AFAICS info->fix contains a *copy* of the initial atmel_lcdfb_fix struct > (and not a pointer to it). So that should be ok. If you say so that's good. I grepped a bit around and didn't find a place where a copy is made. But that's probably me and I'll consider the case closed. Thanks Uwe
On 11/9/23 07:24, Uwe Kleine-König wrote: > Hello, > > On Wed, Nov 08, 2023 at 10:57:00PM +0100, Helge Deller wrote: >> On 11/8/23 22:52, Uwe Kleine-König wrote: >>> But info and so info->fix live longer than the probe function, don't >>> they? >> >> Yes, they do. >> But AFAICS info->fix contains a *copy* of the initial atmel_lcdfb_fix struct >> (and not a pointer to it). So that should be ok. > > If you say so that's good. I grepped a bit around and didn't find a > place where a copy is made. But that's probably me and I'll consider the > case closed. It's not directly obvious, but the copy happens in the line you pointed out previously. In include/linux/fb.h: struct fb_info { ... struct fb_var_screeninfo var; /* Current var */ struct fb_fix_screeninfo fix; /* Current fix */ so, "fb_info.fix" is a struct, and not a pointer. In drivers/video/fbdev/atmel_lcdfb.c: static int atmel_lcdfb_probe(struct platform_device *pdev) { ... info->fix = atmel_lcdfb_fix; // (line 1065) this becomes effectively a: memcpy(&info->fix, &atmel_lcdfb_fix, sizeof(struct fb_fix_screeninfo)); so, the compiler copies the "__initconst" data over to the info->fix struct. Helge
On 09/11/2023 at 10:55, Helge Deller wrote: > On 11/9/23 07:24, Uwe Kleine-König wrote: >> Hello, >> >> On Wed, Nov 08, 2023 at 10:57:00PM +0100, Helge Deller wrote: >>> On 11/8/23 22:52, Uwe Kleine-König wrote: >>>> But info and so info->fix live longer than the probe function, don't >>>> they? >>> >>> Yes, they do. >>> But AFAICS info->fix contains a *copy* of the initial atmel_lcdfb_fix struct >>> (and not a pointer to it). So that should be ok. >> >> If you say so that's good. I grepped a bit around and didn't find a >> place where a copy is made. But that's probably me and I'll consider the >> case closed. > > It's not directly obvious, but the copy happens in the line you pointed > out previously. > > In include/linux/fb.h: > > struct fb_info { > ... > struct fb_var_screeninfo var; /* Current var */ > struct fb_fix_screeninfo fix; /* Current fix */ > > so, "fb_info.fix" is a struct, and not a pointer. > > In drivers/video/fbdev/atmel_lcdfb.c: > static int atmel_lcdfb_probe(struct platform_device *pdev) > { > ... > info->fix = atmel_lcdfb_fix; // (line 1065) > > this becomes effectively a: > memcpy(&info->fix, &atmel_lcdfb_fix, sizeof(struct fb_fix_screeninfo)); > > so, the compiler copies the "__initconst" data over to the info->fix struct. Helge, Uwe and Nathan, Thanks a lot for making this move, patch and detailed explanation. Best regards, Nicolas
Hello Helge, On Thu, Nov 09, 2023 at 10:55:41AM +0100, Helge Deller wrote: > On 11/9/23 07:24, Uwe Kleine-König wrote: > > Hello, > > > > On Wed, Nov 08, 2023 at 10:57:00PM +0100, Helge Deller wrote: > > > On 11/8/23 22:52, Uwe Kleine-König wrote: > > > > But info and so info->fix live longer than the probe function, don't > > > > they? > > > > > > Yes, they do. > > > But AFAICS info->fix contains a *copy* of the initial atmel_lcdfb_fix struct > > > (and not a pointer to it). So that should be ok. > > > > If you say so that's good. I grepped a bit around and didn't find a > > place where a copy is made. But that's probably me and I'll consider the > > case closed. > > It's not directly obvious, but the copy happens in the line you pointed > out previously. > > In include/linux/fb.h: > > struct fb_info { > ... > struct fb_var_screeninfo var; /* Current var */ > struct fb_fix_screeninfo fix; /* Current fix */ > > so, "fb_info.fix" is a struct, and not a pointer. > > In drivers/video/fbdev/atmel_lcdfb.c: > static int atmel_lcdfb_probe(struct platform_device *pdev) > { > ... > info->fix = atmel_lcdfb_fix; // (line 1065) > > this becomes effectively a: > memcpy(&info->fix, &atmel_lcdfb_fix, sizeof(struct fb_fix_screeninfo)); Ah right. Thanks for that hint. I didn't spot this and grepped for memcpy and memdup. Uwe
diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c index a908db233409..b218731ef732 100644 --- a/drivers/video/fbdev/atmel_lcdfb.c +++ b/drivers/video/fbdev/atmel_lcdfb.c @@ -1017,7 +1017,7 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo) return ret; } -static int __init atmel_lcdfb_probe(struct platform_device *pdev) +static int atmel_lcdfb_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct fb_info *info; @@ -1223,7 +1223,7 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev) return ret; } -static int __exit atmel_lcdfb_remove(struct platform_device *pdev) +static int atmel_lcdfb_remove(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct fb_info *info = dev_get_drvdata(dev); @@ -1301,7 +1301,8 @@ static int atmel_lcdfb_resume(struct platform_device *pdev) #endif static struct platform_driver atmel_lcdfb_driver = { - .remove = __exit_p(atmel_lcdfb_remove), + .probe = atmel_lcdfb_probe, + .remove = atmel_lcdfb_remove, .suspend = atmel_lcdfb_suspend, .resume = atmel_lcdfb_resume, .driver = { @@ -1310,7 +1311,7 @@ static struct platform_driver atmel_lcdfb_driver = { }, }; -module_platform_driver_probe(atmel_lcdfb_driver, atmel_lcdfb_probe); +module_platform_driver(atmel_lcdfb_driver, ); MODULE_DESCRIPTION("AT91 LCD Controller framebuffer driver"); MODULE_AUTHOR("Nicolas Ferre <nicolas.ferre@atmel.com>");
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/atmel_lcdfb: section mismatch in reference: atmel_lcdfb_driver+0x4 (section: .data) -> atmel_lcdfb_remove (section: .exit.text) Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/video/fbdev/atmel_lcdfb.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)