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

From: Heiko Schocher
Date: Tue May 26 2015 - 00:03:47 EST


Hello Tomi,

Am 25.05.2015 07:57, schrieb Tomi Valkeinen:


On 06/05/15 10:09, Heiko Schocher wrote:
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 <hs@xxxxxxx>

---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/