Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751263AbbEZD5n (ORCPT ); Mon, 25 May 2015 23:57:43 -0400 Received: from [212.18.0.9] ([212.18.0.9]:41836 "EHLO mail-out.m-online.net" rhost-flags-FAIL-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751005AbbEZD5k (ORCPT ); Mon, 25 May 2015 23:57:40 -0400 X-Auth-Info: UjV02cmltK+zWcAbdwoT/GXF6m0mM+TcMR/F3NvQbw4= Message-ID: <5563EEF9.3080901@denx.de> Date: Tue, 26 May 2015 05:56:41 +0200 From: Heiko Schocher Reply-To: hs@denx.de Organization: DENX Software Engineering User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Tomi Valkeinen CC: linux-kernel@vger.kernel.org, Geert Uytterhoeven , linux-fbdev@vger.kernel.org, Jean-Christophe Plagniol-Villard Subject: Re: [RFC PATCH] video/logo: introduce new system state for checking if logos are freed References: <1430896145-8887-1-git-send-email-hs@denx.de> <5562B9CE.7050807@ti.com> In-Reply-To: <5562B9CE.7050807@ti.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4216 Lines: 115 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 >> >> --- >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/