[RFC PATCH] video/logo: introduce new system state for checking if logos are freed

commit 92b004d1aa9f ("video/logo: prevent use of logos after they have been freed")

added a late_initcall function to mark the logos as freed. In reality
the logos are freed later, and fbdev probe may be ran between this
late_initcall and the freeing of the logos. In that case the logos
would not be drawn. To prevent this introduced a new system_state
SYSTEM_FREEING_MEM and set this state before freeing memory. This
state could be checked now in fb_find_logo(). This system state
is maybe useful on other places too.

Signed-off-by: Heiko Schocher <redacted>

---
Found this issue on an imx6 based board with a display which needs
a spi initialization. With 3.18.2 I see a perfect logo, but with
current ml, bootlogo is missing, because drm gets probed before
spi display, which leads in drm probing is deferred until the
spi display is probed. After that drm is probed again ... but
this is too late for showing the bootlogo.
With this patch, bootlogo is drawn again. I am not sure, if it
is so easy to add a new system state ... but we should have a
possibility to detect if initdata is freed or not. this is maybe
also for other modules interesting. Maybe we add a
kernel_initdata_freed()
function instead of a new system state?

 drivers/video/logo/logo.c | 15 ++++-----------
 include/linux/kernel.h    |  1 +
 init/main.c               |  1 +
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/video/logo/logo.c b/drivers/video/logo/logo.c
index 10fbfd8..d798a9f 100644
--- a/drivers/video/logo/logo.c
+++ b/drivers/video/logo/logo.c

@@ -26,16 +26,6 @@ MODULE_PARM_DESC(nologo, "Disables startup logo");
  * Use late_init to mark the logos as freed to prevent any further use.
  */
 
-static bool logos_freed;
-
-static int __init fb_logo_late_init(void)
-{
-	logos_freed = true;
-	return 0;
-}
-
-late_initcall(fb_logo_late_init);
-
 /* logo's are marked __initdata. Use __init_refok to tell
  * modpost that it is intended that this function uses data
  * marked __initdata.

@@ -44,7 +34,10 @@ const struct linux_logo * __init_refok fb_find_logo(int depth)
 {
 	const struct linux_logo *logo = NULL;
 
-	if (nologo || logos_freed)
+	if (system_state >= SYSTEM_FREEING_MEM)
+		return NULL;
+
+	if (nologo)
 		return NULL;
 
 	if (depth >= 1) {

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3a5b48e..e5875bf 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h

@@ -462,6 +462,7 @@ extern bool early_boot_irqs_disabled;
 /* Values used for system_state */
 extern enum system_states {
 	SYSTEM_BOOTING,
+	SYSTEM_FREEING_MEM,
 	SYSTEM_RUNNING,
 	SYSTEM_HALT,
 	SYSTEM_POWER_OFF,

diff --git a/init/main.c b/init/main.c
index 2115055..4965ed0 100644
--- a/init/main.c
+++ b/init/main.c

@@ -931,6 +931,7 @@ static int __ref kernel_init(void *unused)
 	kernel_init_freeable();
 	/* need to finish all async __init code before freeing the memory */
 	async_synchronize_full();
+	system_state = SYSTEM_FREEING_MEM;
 	free_initmem();
 	mark_rodata_ro();
 	system_state = SYSTEM_RUNNING;

-- 
2.1.0

On 06/05/15 10:09, Heiko Schocher wrote:

Without locking, the initmem may be freed while fb_find_logo() is running.

 Tomi

Hello Tomi,

Am 25.05.2015 07:57, schrieb Tomi Valkeinen:

On 06/05/15 10:09, Heiko Schocher wrote:

quoted

commit 92b004d1aa9f ("video/logo: prevent use of logos after they have been freed")

added a late_initcall function to mark the logos as freed. In reality
the logos are freed later, and fbdev probe may be ran between this
late_initcall and the freeing of the logos. In that case the logos
would not be drawn. To prevent this introduced a new system_state
SYSTEM_FREEING_MEM and set this state before freeing memory. This
state could be checked now in fb_find_logo(). This system state
is maybe useful on other places too.

Signed-off-by: Heiko Schocher <redacted>

---
Found this issue on an imx6 based board with a display which needs
a spi initialization. With 3.18.2 I see a perfect logo, but with
current ml, bootlogo is missing, because drm gets probed before
spi display, which leads in drm probing is deferred until the
spi display is probed. After that drm is probed again ... but
this is too late for showing the bootlogo.
With this patch, bootlogo is drawn again. I am not sure, if it
is so easy to add a new system state ... but we should have a
possibility to detect if initdata is freed or not. this is maybe
also for other modules interesting. Maybe we add a
kernel_initdata_freed()
function instead of a new system state?

  drivers/video/logo/logo.c | 15 ++++-----------
  include/linux/kernel.h    |  1 +
  init/main.c               |  1 +
  3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/video/logo/logo.c b/drivers/video/logo/logo.c
index 10fbfd8..d798a9f 100644
--- a/drivers/video/logo/logo.c
+++ b/drivers/video/logo/logo.c

@@ -26,16 +26,6 @@ MODULE_PARM_DESC(nologo, "Disables startup logo");
   * Use late_init to mark the logos as freed to prevent any further use.
   */

-static bool logos_freed;
-
-static int __init fb_logo_late_init(void)
-{
-	logos_freed = true;
-	return 0;
-}
-
-late_initcall(fb_logo_late_init);
-
  /* logo's are marked __initdata. Use __init_refok to tell
   * modpost that it is intended that this function uses data
   * marked __initdata.

@@ -44,7 +34,10 @@ const struct linux_logo * __init_refok fb_find_logo(int depth)
  {
  	const struct linux_logo *logo = NULL;

-	if (nologo || logos_freed)
+	if (system_state >= SYSTEM_FREEING_MEM)
+		return NULL;
+
+	if (nologo)
  		return NULL;

  	if (depth >= 1) {

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3a5b48e..e5875bf 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h

@@ -462,6 +462,7 @@ extern bool early_boot_irqs_disabled;
  /* Values used for system_state */
  extern enum system_states {
  	SYSTEM_BOOTING,
+	SYSTEM_FREEING_MEM,
  	SYSTEM_RUNNING,
  	SYSTEM_HALT,
  	SYSTEM_POWER_OFF,

diff --git a/init/main.c b/init/main.c
index 2115055..4965ed0 100644
--- a/init/main.c
+++ b/init/main.c

@@ -931,6 +931,7 @@ static int __ref kernel_init(void *unused)
  	kernel_init_freeable();
  	/* need to finish all async __init code before freeing the memory */
  	async_synchronize_full();
+	system_state = SYSTEM_FREEING_MEM;
  	free_initmem();
  	mark_rodata_ro();
  	system_state = SYSTEM_RUNNING;

Without locking, the initmem may be freed while fb_find_logo() is running.

Yes, you are right, that must be added ... but has such a change a
chance to go in mainline?

BTW: Could this not be currently a problem on multicore systems?
If lets say core 2 just draws the logo, another core 1 calls
fb_logo_late_init() and later core 1 free_initmem(), while the core 2
still draws it?

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

On 26/05/15 06:56, Heiko Schocher wrote:

quoted

Without locking, the initmem may be freed while fb_find_logo() is
running.

Yes, you are right, that must be added ... but has such a change a
chance to go in mainline?

I don't know. To be honest, this whole thing feels a bit like hackery. I
think initdata should only be accessed from initcalls, never asynchronously.

BTW: Could this not be currently a problem on multicore systems?
If lets say core 2 just draws the logo, another core 1 calls
fb_logo_late_init() and later core 1 free_initmem(), while the core 2
still draws it?

Yes, I think so...

So, maybe it would be better to not even try to go forward with the
current approach. Two approaches come to my mind:

1) Keep the logos in the memory, and don't even try to free them. I
don't know many bytes they are in total, though.

2) Make a copy of the logos to a kmalloced area at some early boot
stage. Then manually free the logos at some point (after the first
access to the logos? after a certain time (urgh...)?).

 Tomi

On Tue, May 26, 2015 at 8:54 AM, Tomi Valkeinen [off-list ref] wrote:

On 26/05/15 06:56, Heiko Schocher wrote:

quoted

quoted

Without locking, the initmem may be freed while fb_find_logo() is
running.

Or afterwards. Drivers may keep the pointer around indefinitely.

quoted

Yes, you are right, that must be added ... but has such a change a
chance to go in mainline?

I don't know. To be honest, this whole thing feels a bit like hackery. I
think initdata should only be accessed from initcalls, never asynchronously.

quoted

BTW: Could this not be currently a problem on multicore systems?
If lets say core 2 just draws the logo, another core 1 calls
fb_logo_late_init() and later core 1 free_initmem(), while the core 2
still draws it?

Yes, I think so...

I don't think that can happen. All initcalls should complete before initmem
is freed.

So, maybe it would be better to not even try to go forward with the
current approach. Two approaches come to my mind:

1) Keep the logos in the memory, and don't even try to free them. I
don't know many bytes they are in total, though.

m68k/allmodconfig:

$ size drivers/video/logo/logo*o
   text   data    bss    dec    hex filename
     24   6961      0   6985   1b49 drivers/video/logo/logo_linux_clut224.o
     24    800      0    824    338 drivers/video/logo/logo_linux_mono.o
     24   3200      0   3224    c98 drivers/video/logo/logo_linux_vga16.o
     24   6955      0   6979   1b43 drivers/video/logo/logo_mac_clut224.o
    161      4      2    167     a7 drivers/video/logo/logo.o

Not that bad... Custom logos may be larger, though.

2) Make a copy of the logos to a kmalloced area at some early boot
stage. Then manually free the logos at some point (after the first
access to the logos? after a certain time (urgh...)?).

3) Draw the logos from an initcall on all frame buffers that exist at that
   point in time. Yes, this will destroy (part of) the content that's
   currently shown.

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 26/05/15 10:08, Geert Uytterhoeven wrote:

On Tue, May 26, 2015 at 8:54 AM, Tomi Valkeinen [off-list ref] wrote:

quoted

On 26/05/15 06:56, Heiko Schocher wrote:

quoted

quoted

Without locking, the initmem may be freed while fb_find_logo() is
running.

Or afterwards. Drivers may keep the pointer around indefinitely.

quoted

quoted

Yes, you are right, that must be added ... but has such a change a
chance to go in mainline?

I don't know. To be honest, this whole thing feels a bit like hackery. I
think initdata should only be accessed from initcalls, never asynchronously.

quoted

BTW: Could this not be currently a problem on multicore systems?
If lets say core 2 just draws the logo, another core 1 calls
fb_logo_late_init() and later core 1 free_initmem(), while the core 2
still draws it?

Yes, I think so...

I don't think that can happen. All initcalls should complete before initmem
is freed.

Ah, true, the question was only about the initcalls. I was answering to
what actually can happen with the logo code as a whole.

The whole problem started when I fixed an issue where the logos were
accessed from a workqueue. I don't remember the details, but I think drm
always (?) sets up some console stuff via workthread. In that case we
could have the workthread accessing the logos, while initmem is being freed.

quoted

So, maybe it would be better to not even try to go forward with the
current approach. Two approaches come to my mind:

1) Keep the logos in the memory, and don't even try to free them. I
don't know many bytes they are in total, though.

m68k/allmodconfig:

$ size drivers/video/logo/logo*o
   text   data    bss    dec    hex filename
     24   6961      0   6985   1b49 drivers/video/logo/logo_linux_clut224.o
     24    800      0    824    338 drivers/video/logo/logo_linux_mono.o
     24   3200      0   3224    c98 drivers/video/logo/logo_linux_vga16.o
     24   6955      0   6979   1b43 drivers/video/logo/logo_mac_clut224.o
    161      4      2    167     a7 drivers/video/logo/logo.o

Not that bad... Custom logos may be larger, though.

I wonder how much a simple RLE would cut down the sizes...

quoted

2) Make a copy of the logos to a kmalloced area at some early boot
stage. Then manually free the logos at some point (after the first
access to the logos? after a certain time (urgh...)?).

3) Draw the logos from an initcall on all frame buffers that exist at that
   point in time. Yes, this will destroy (part of) the content that's
   currently shown.

Isn't that almost the same as now? The problem is that the fb probes are
deferred to a very late stage, so we would not have the fbs when the
suggested initcall would be called.

 Tomi

Hello Tomi,

Am 26.05.2015 08:54, schrieb Tomi Valkeinen:

On 26/05/15 06:56, Heiko Schocher wrote:

quoted

quoted

Without locking, the initmem may be freed while fb_find_logo() is
running.

Yes, you are right, that must be added ... but has such a change a
chance to go in mainline?

I don't know. To be honest, this whole thing feels a bit like hackery. I

Full Ack ;-)

think initdata should only be accessed from initcalls, never asynchronously.

Yes.

quoted

BTW: Could this not be currently a problem on multicore systems?
If lets say core 2 just draws the logo, another core 1 calls
fb_logo_late_init() and later core 1 free_initmem(), while the core 2
still draws it?

Yes, I think so...

So, maybe it would be better to not even try to go forward with the
current approach. Two approaches come to my mind:

1) Keep the logos in the memory, and don't even try to free them. I
don't know many bytes they are in total, though.

That was my first thought too... but we waste memory ... not nice.

2) Make a copy of the logos to a kmalloced area at some early boot
stage. Then manually free the logos at some point (after the first
access to the logos? after a certain time (urgh...)?).

maybe yes ... or  ...

3) wait for all cores, until they reached SYSTEM_FREEING_MEM and
free init mem only than? But are all cores used/started when booting?

4) draw only one logo even on multicores ... why must every core draw
a logo? Currently each core draws the logo, and on a system with more
than 4 cores, I think this looks not really good ...

Hmm... I do not really have a lot experience in this area ... but why
is the logo only drawed when booting? Is there nothing like a "redraw"
of it?

Maybe others have here a good idea?

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Hi Tomi,

On Tue, May 26, 2015 at 9:15 AM, Tomi Valkeinen [off-list ref] wrote:

quoted

3) Draw the logos from an initcall on all frame buffers that exist at that
   point in time. Yes, this will destroy (part of) the content that's
   currently shown.

Isn't that almost the same as now? The problem is that the fb probes are
deferred to a very late stage, so we would not have the fbs when the
suggested initcall would be called.

Currently it's drawn from fbcon_switch().

Some drivers draw it independently from the fbcon code
(au1200fb_drv_probe() and mmpfb_probe()).

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 Tue, May 26, 2015 at 9:17 AM, Heiko Schocher [off-list ref] wrote:

4) draw only one logo even on multicores ... why must every core draw
a logo? Currently each core draws the logo, and on a system with more
than 4 cores, I think this looks not really good ...

I don't think each core draws a logo. They're all drawn from fbcon_switch().

Hmm... I do not really have a lot experience in this area ... but why
is the logo only drawed when booting? Is there nothing like a "redraw"
of it?

When do you want to redraw it? On VC switch (it's drawn on the first call
only, hence it disappears if you switch VC and back)?
Until when should it be redrawn?

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

Hello Tomi, Geert,

Am 26.05.2015 09:15, schrieb Tomi Valkeinen:

On 26/05/15 10:08, Geert Uytterhoeven wrote:

quoted

On Tue, May 26, 2015 at 8:54 AM, Tomi Valkeinen [off-list ref] wrote:

quoted

On 26/05/15 06:56, Heiko Schocher wrote:

quoted

quoted

Without locking, the initmem may be freed while fb_find_logo() is
running.

Or afterwards. Drivers may keep the pointer around indefinitely.

quoted

quoted

Yes, you are right, that must be added ... but has such a change a
chance to go in mainline?

I don't know. To be honest, this whole thing feels a bit like hackery. I
think initdata should only be accessed from initcalls, never asynchronously.

quoted

BTW: Could this not be currently a problem on multicore systems?
If lets say core 2 just draws the logo, another core 1 calls
fb_logo_late_init() and later core 1 free_initmem(), while the core 2
still draws it?

Yes, I think so...

I don't think that can happen. All initcalls should complete before initmem
is freed.

Ah, true, the question was only about the initcalls. I was answering to
what actually can happen with the logo code as a whole.

The whole problem started when I fixed an issue where the logos were
accessed from a workqueue. I don't remember the details, but I think drm
always (?) sets up some console stuff via workthread. In that case we
could have the workthread accessing the logos, while initmem is being freed.

quoted

quoted

So, maybe it would be better to not even try to go forward with the
current approach. Two approaches come to my mind:

1) Keep the logos in the memory, and don't even try to free them. I
don't know many bytes they are in total, though.

m68k/allmodconfig:

$ size drivers/video/logo/logo*o
    text   data    bss    dec    hex filename
      24   6961      0   6985   1b49 drivers/video/logo/logo_linux_clut224.o
      24    800      0    824    338 drivers/video/logo/logo_linux_mono.o
      24   3200      0   3224    c98 drivers/video/logo/logo_linux_vga16.o
      24   6955      0   6979   1b43 drivers/video/logo/logo_mac_clut224.o
     161      4      2    167     a7 drivers/video/logo/logo.o

Not that bad... Custom logos may be larger, though.

I wonder how much a simple RLE would cut down the sizes...

quoted

quoted

2) Make a copy of the logos to a kmalloced area at some early boot
stage. Then manually free the logos at some point (after the first
access to the logos? after a certain time (urgh...)?).

3) Draw the logos from an initcall on all frame buffers that exist at that
    point in time. Yes, this will destroy (part of) the content that's
    currently shown.

Isn't that almost the same as now? The problem is that the fb probes are
deferred to a very late stage, so we would not have the fbs when the
suggested initcall would be called.

Yes, exactly, this is my problem. DRM gets probed early and returns with
EPROBE_DEFER, as the display needs a spi init sequence, but spi is not
running yet ... later, when spi is up, DRM probes again, and all is
fine, but the logo is not drawn, as fb_logo_late_init() is called before,
which prevents drawing the logo.

We maybe could call fb_logo_late_init() directly from init/main.c
before calling free_initmem() ? But here again the question, could
it be possible that another core just draws the logo?
Or does async_synchronize_full() helps us here?

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Hello Geert,

Am 26.05.2015 09:25, schrieb Geert Uytterhoeven:

On Tue, May 26, 2015 at 9:17 AM, Heiko Schocher [off-list ref] wrote:

quoted

4) draw only one logo even on multicores ... why must every core draw
a logo? Currently each core draws the logo, and on a system with more
than 4 cores, I think this looks not really good ...

I don't think each core draws a logo. They're all drawn from fbcon_switch().

Hmm... I have here an imx6dl based system, and it draws two logos when booting...
I can prevent this two logos, if I do:

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0705d88..e907ab9 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c

@@ -664,8 +672,7 @@ int fb_show_logo(struct fb_info *info, int rotate)
  {
         int y;

-       y = fb_show_logo_line(info, rotate, fb_logo.logo, 0,
-                             num_online_cpus());
+       y = fb_show_logo_line(info, rotate, fb_logo.logo, 0, 1);
         y = fb_show_extra_logos(info, y, rotate);

         return y;

quoted

Hmm... I do not really have a lot experience in this area ... but why
is the logo only drawed when booting? Is there nothing like a "redraw"
of it?

When do you want to redraw it? On VC switch (it's drawn on the first call
only, hence it disappears if you switch VC and back)?
Until when should it be redrawn?

I don;t know, it was just a question ... If this is not possible, we
do not need to cover such a case, good!

bye,
Heiko

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Hi Heiko,

On Tue, May 26, 2015 at 9:35 AM, Heiko Schocher [off-list ref] wrote:

Am 26.05.2015 09:25, schrieb Geert Uytterhoeven:

quoted

On Tue, May 26, 2015 at 9:17 AM, Heiko Schocher [off-list ref] wrote:

quoted

4) draw only one logo even on multicores ... why must every core draw
a logo? Currently each core draws the logo, and on a system with more
than 4 cores, I think this looks not really good ...

I don't think each core draws a logo. They're all drawn from
fbcon_switch().

Hmm... I have here an imx6dl based system, and it draws two logos when
booting...

It draw one logo _for_ each cpu core, but each logo is not drawn _by_ each
cpu core.

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