| Message ID | 1430896145-8887-1-git-send-email-hs@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Tomi
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
On 26/05/15 06:56, Heiko Schocher wrote: >> 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 <tomi.valkeinen@ti.com> wrote: > On 26/05/15 06:56, Heiko Schocher wrote: >>> Without locking, the initmem may be freed while fb_find_logo() is >>> running. Or afterwards. Drivers may keep the pointer around indefinitely. >> 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... 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/05/15 10:08, Geert Uytterhoeven wrote: > On Tue, May 26, 2015 at 8:54 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >> On 26/05/15 06:56, Heiko Schocher wrote: >>>> Without locking, the initmem may be freed while fb_find_logo() is >>>> running. > > Or afterwards. Drivers may keep the pointer around indefinitely. > >>> 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... > > 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. >> 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... >> 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: > >>> 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. >> 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
Hi Tomi, On Tue, May 26, 2015 at 9:15 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 26, 2015 at 9:17 AM, Heiko Schocher <hs@denx.de> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Tomi, Geert, Am 26.05.2015 09:15, schrieb Tomi Valkeinen: > > > On 26/05/15 10:08, Geert Uytterhoeven wrote: >> On Tue, May 26, 2015 at 8:54 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >>> On 26/05/15 06:56, Heiko Schocher wrote: >>>>> Without locking, the initmem may be freed while fb_find_logo() is >>>>> running. >> >> Or afterwards. Drivers may keep the pointer around indefinitely. >> >>>> 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... >> >> 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. > >>> 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... > >>> 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
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;
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(-)