Messages in this thread | | | Date | Tue, 26 May 2015 05:56:41 +0200 | From | Heiko Schocher <> | Subject | Re: [RFC PATCH] video/logo: introduce new system state for checking if logos are freed |
| |
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@denx.de> >> >> --- >> 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
| |