On 2024-08-28 19:42:51+0000, Helge Deller wrote:
> On 8/27/24 17:25, Thomas Weißschuh wrote:
> > These variables are only used inside efifb_probe().
> > Afterwards they are using memory unnecessarily.
>
> Did you check if this change really saves some memory?
Nope...
> With your change, the compiler will either create a hidden
> structure which it uses then, or it generates assembly
> instructions to fill the struct at runtime.
> Both options may not actually reduce the memory footprint...
Thanks for the explanation, it makes sense.
On advantage of the on-stack data would be future-proofing.
Efi efifb_probe() overrides some fields in these structs only in certain
codepaths then the globally shared data could become inconsistent.
But that's not the case today.
> Another option might be to mark the static struct __initdata
> if you expect another card to take over before the memory is
> freed at runtime. But I'm not sure if it's worth possible
> implications.
Agreed.
> Helge
>
> > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > ---
> > drivers/video/fbdev/efifb.c | 36 ++++++++++++++++++------------------
> > 1 file changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > index 8dd82afb3452..8bfe0ccbc67a 100644
> > --- a/drivers/video/fbdev/efifb.c
> > +++ b/drivers/video/fbdev/efifb.c
> > @@ -52,24 +52,6 @@ struct efifb_par {
> > resource_size_t size;
> > };
> >
> > -static struct fb_var_screeninfo efifb_defined = {
> > - .activate = FB_ACTIVATE_NOW,
> > - .height = -1,
> > - .width = -1,
> > - .right_margin = 32,
> > - .upper_margin = 16,
> > - .lower_margin = 4,
> > - .vsync_len = 4,
> > - .vmode = FB_VMODE_NONINTERLACED,
> > -};
> > -
> > -static struct fb_fix_screeninfo efifb_fix = {
> > - .id = "EFI VGA",
> > - .type = FB_TYPE_PACKED_PIXELS,
> > - .accel = FB_ACCEL_NONE,
> > - .visual = FB_VISUAL_TRUECOLOR,
> > -};
> > -
> > static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
> > unsigned blue, unsigned transp,
> > struct fb_info *info)
> > @@ -357,6 +339,24 @@ static int efifb_probe(struct platform_device *dev)
> > char *option = NULL;
> > efi_memory_desc_t md;
> >
> > + struct fb_var_screeninfo efifb_defined = {
> > + .activate = FB_ACTIVATE_NOW,
> > + .height = -1,
> > + .width = -1,
> > + .right_margin = 32,
> > + .upper_margin = 16,
> > + .lower_margin = 4,
> > + .vsync_len = 4,
> > + .vmode = FB_VMODE_NONINTERLACED,
> > + };
> > +
> > + struct fb_fix_screeninfo efifb_fix = {
> > + .id = "EFI VGA",
> > + .type = FB_TYPE_PACKED_PIXELS,
> > + .accel = FB_ACCEL_NONE,
> > + .visual = FB_VISUAL_TRUECOLOR,
> > + };
> > +
> > /*
> > * If we fail probing the device, the kernel might try a different
> > * driver. We get a copy of the attached screen_info, so that we can
> >
>
