[PATCH] earlyprintk=efi,keep fix

[PATCH] earlyprintk=efi,keep fix

[Dave Young]

earlyprintk=efi,keep will cause kernel hangs while freeing initmem like below:
[    2.826089] VFS: Mounted root (ext4 filesystem) readonly on device 254:2.
[    2.846592] devtmpfs: mounted
[    2.856974] Freeing unused kernel memory: 880K (ffffffff817d4000 - ffffffff818b0000)

It is caused by efi earlyprintk use __init function which will be freed later.
Such as early_efi_write is marked as __init, also it will use early_ioremap
which is init function as well.

To fix this issue, I added one early initcall efi_ioremap_fb which will map the whole
efi fb for later use. OTOH, adding a wrapper function efi_ioremap which will call
early_ioremap before ioremap is available.

With this patch applied efi boot ok with earlyprintk=efi,keep console=efi

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/early_printk.c |   54 +++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 12 deletions(-)

--- linux-2.6.orig/arch/x86/platform/efi/early_printk.c
+++ linux-2.6/arch/x86/platform/efi/early_printk.c
@@ -14,8 +14,38 @@
 
 static const struct font_desc *font;
 static u32 efi_x, efi_y;
+static void *efi_fb;
 
-static __init void early_efi_clear_scanline(unsigned int y)
+static __init int efi_ioremap_fb(void)
+{
+	unsigned long base, size;
+
+	base = boot_params.screen_info.lfb_base;
+	size = boot_params.screen_info.lfb_size;
+	efi_fb = ioremap(base, size);
+
+	return efi_fb ? 0 : -ENOMEM;
+}
+early_initcall(efi_ioremap_fb);
+
+static __init_refok void *efi_ioremap(unsigned long start, unsigned long len)
+{
+	unsigned long base;
+
+	base = boot_params.screen_info.lfb_base;
+	if (efi_fb)
+		return (efi_fb + start - base);
+	else
+		return early_ioremap(start, len);
+}
+
+static __init_refok void efi_iounmap(void *addr, unsigned long len)
+{
+	if (!efi_fb)
+		early_iounmap(addr, len);
+}
+
+static void early_efi_clear_scanline(unsigned int y)
 {
 	unsigned long base, *dst;
 	u16 len;
@@ -23,15 +53,15 @@ static __init void early_efi_clear_scanl
 	base = boot_params.screen_info.lfb_base;
 	len = boot_params.screen_info.lfb_linelength;
 
-	dst = early_ioremap(base + y*len, len);
+	dst = efi_ioremap(base + y*len, len);
 	if (!dst)
 		return;
 
 	memset(dst, 0, len);
-	early_iounmap(dst, len);
+	efi_iounmap(dst, len);
 }
 
-static __init void early_efi_scroll_up(void)
+static void early_efi_scroll_up(void)
 {
 	unsigned long base, *dst, *src;
 	u16 len;
@@ -42,20 +72,20 @@ static __init void early_efi_scroll_up(v
 	height = boot_params.screen_info.lfb_height;
 
 	for (i = 0; i < height - font->height; i++) {
-		dst = early_ioremap(base + i*len, len);
+		dst = efi_ioremap(base + i*len, len);
 		if (!dst)
 			return;
 
-		src = early_ioremap(base + (i + font->height) * len, len);
+		src = efi_ioremap(base + (i + font->height) * len, len);
 		if (!src) {
-			early_iounmap(dst, len);
+			efi_iounmap(dst, len);
 			return;
 		}
 
 		memmove(dst, src, len);
 
-		early_iounmap(src, len);
-		early_iounmap(dst, len);
+		efi_iounmap(src, len);
+		efi_iounmap(dst, len);
 	}
 }
 
@@ -79,7 +109,7 @@ static void early_efi_write_char(u32 *ds
 	}
 }
 
-static __init void
+static void
 early_efi_write(struct console *con, const char *str, unsigned int num)
 {
 	struct screen_info *si;
@@ -109,7 +139,7 @@ early_efi_write(struct console *con, con
 		for (h = 0; h < font->height; h++) {
 			unsigned int n, x;
 
-			dst = early_ioremap(base + (efi_y + h) * len, len);
+			dst = efi_ioremap(base + (efi_y + h) * len, len);
 			if (!dst)
 				return;
 
@@ -123,7 +153,7 @@ early_efi_write(struct console *con, con
 				s++;
 			}
 
-			early_iounmap(dst, len);
+			efi_iounmap(dst, len);
 		}
 
 		num -= count;

Re: [PATCH] earlyprintk=efi,keep fix

[Andrew Morton]

On Wed, 30 Apr 2014 10:13:03 +0800 Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> 
> earlyprintk=efi,keep will cause kernel hangs while freeing initmem like below:
> [    2.826089] VFS: Mounted root (ext4 filesystem) readonly on device 254:2.
> [    2.846592] devtmpfs: mounted
> [    2.856974] Freeing unused kernel memory: 880K (ffffffff817d4000 - ffffffff818b0000)
> 
> It is caused by efi earlyprintk use __init function which will be freed later.
> Such as early_efi_write is marked as __init, also it will use early_ioremap
> which is init function as well.
> 
> To fix this issue, I added one early initcall efi_ioremap_fb which will map the whole
> efi fb for later use. OTOH, adding a wrapper function efi_ioremap which will call
> early_ioremap before ioremap is available.
> 
> With this patch applied efi boot ok with earlyprintk=efi,keep console=efi
> 

CONFIG_DEBUG_SECTION_MISMATCH should have detected this, but
CONFIG_DEBUG_SECTION_MISMATCH doesn't seem to work any more. 
scripts/Makefile.modpost appears to be trying to pass -S to modpost if
CONFIG_DEBUG_SECTION_MISMATCH, but I'm not seeing any warnings.  The
CONFIG_DEBUG_SECTION_MISMATCH Kconfig help is not useful.  What did we
do?

Shudder.  I'll assume Matt will handle this patch?

Re: [PATCH] earlyprintk=efi,keep fix

[Matt Fleming]

On Wed, 30 Apr, at 12:52:11PM, Andrew Morton wrote:
> 
> Shudder.  I'll assume Matt will handle this patch?

Sure, I'll take a look tomorrow.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] earlyprintk=efi,keep fix

[Matt Fleming]

On Wed, 30 Apr, at 10:13:03AM, Dave Young wrote:
> 
> earlyprintk=efi,keep will cause kernel hangs while freeing initmem like below:
> [    2.826089] VFS: Mounted root (ext4 filesystem) readonly on device 254:2.
> [    2.846592] devtmpfs: mounted
> [    2.856974] Freeing unused kernel memory: 880K (ffffffff817d4000 - ffffffff818b0000)
> 
> It is caused by efi earlyprintk use __init function which will be freed later.
> Such as early_efi_write is marked as __init, also it will use early_ioremap
> which is init function as well.
> 
> To fix this issue, I added one early initcall efi_ioremap_fb which will map the whole
> efi fb for later use. OTOH, adding a wrapper function efi_ioremap which will call
> early_ioremap before ioremap is available.
> 
> With this patch applied efi boot ok with earlyprintk=efi,keep console=efi
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/platform/efi/early_printk.c |   54 +++++++++++++++++++++++++++--------
>  1 file changed, 42 insertions(+), 12 deletions(-)
 
Thanks Dave, good catch. I've got a few comments below.

> +static __init int efi_ioremap_fb(void)
> +{
> +	unsigned long base, size;
> +
> +	base = boot_params.screen_info.lfb_base;
> +	size = boot_params.screen_info.lfb_size;
> +	efi_fb = ioremap(base, size);
> +
> +	return efi_fb ? 0 : -ENOMEM;
> +}
> +early_initcall(efi_ioremap_fb);

So, here you're actually ioremap'ing the framebuffer irrespective of
whether anyone passed earlyprintk=efi,keep on the command line.
Furthermore, you'll perform the ioremap() even if some error was
encountered in early_efi_setup().

You need a way to figure out whether the ioremap() is actually needed.

Please document this function to explain why we need to ioremap() the
framebuffer from an early_initcall().

Oh and as a side note, please prefix functions in this file with
early_efi_*. Yeah, I realise that I suggested efi_ioremap() as a name -
my bad.

> +static __init_refok void *efi_ioremap(unsigned long start, unsigned long len)
> +{
> +	unsigned long base;
> +
> +	base = boot_params.screen_info.lfb_base;
> +	if (efi_fb)
> +		return (efi_fb + start - base);
> +	else
> +		return early_ioremap(start, len);
> +}

Adding .lfb_base in the caller and then subtracting here seems
needlessly complicated. Instead I'd suggest making early_efi_map() take
an offset and a length, and document when each framebuffer is used.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] earlyprintk=efi,keep fix

[Dave Young]

On 05/01/14 at 11:12am, Matt Fleming wrote:
> On Wed, 30 Apr, at 10:13:03AM, Dave Young wrote:
> > 
> > earlyprintk=efi,keep will cause kernel hangs while freeing initmem like below:
> > [    2.826089] VFS: Mounted root (ext4 filesystem) readonly on device 254:2.
> > [    2.846592] devtmpfs: mounted
> > [    2.856974] Freeing unused kernel memory: 880K (ffffffff817d4000 - ffffffff818b0000)
> > 
> > It is caused by efi earlyprintk use __init function which will be freed later.
> > Such as early_efi_write is marked as __init, also it will use early_ioremap
> > which is init function as well.
> > 
> > To fix this issue, I added one early initcall efi_ioremap_fb which will map the whole
> > efi fb for later use. OTOH, adding a wrapper function efi_ioremap which will call
> > early_ioremap before ioremap is available.
> > 
> > With this patch applied efi boot ok with earlyprintk=efi,keep console=efi
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/platform/efi/early_printk.c |   54 +++++++++++++++++++++++++++--------
> >  1 file changed, 42 insertions(+), 12 deletions(-)
>  
> Thanks Dave, good catch. I've got a few comments below.
> 
> > +static __init int efi_ioremap_fb(void)
> > +{
> > +	unsigned long base, size;
> > +
> > +	base = boot_params.screen_info.lfb_base;
> > +	size = boot_params.screen_info.lfb_size;
> > +	efi_fb = ioremap(base, size);
> > +
> > +	return efi_fb ? 0 : -ENOMEM;
> > +}
> > +early_initcall(efi_ioremap_fb);
> 
> So, here you're actually ioremap'ing the framebuffer irrespective of
> whether anyone passed earlyprintk=efi,keep on the command line.
> Furthermore, you'll perform the ioremap() even if some error was
> encountered in early_efi_setup().
> 
> You need a way to figure out whether the ioremap() is actually needed.
> 
> Please document this function to explain why we need to ioremap() the
> framebuffer from an early_initcall().
> 
> Oh and as a side note, please prefix functions in this file with
> early_efi_*. Yeah, I realise that I suggested efi_ioremap() as a name -
> my bad.
> 
> > +static __init_refok void *efi_ioremap(unsigned long start, unsigned long len)
> > +{
> > +	unsigned long base;
> > +
> > +	base = boot_params.screen_info.lfb_base;
> > +	if (efi_fb)
> > +		return (efi_fb + start - base);
> > +	else
> > +		return early_ioremap(start, len);
> > +}
> 
> Adding .lfb_base in the caller and then subtracting here seems
> needlessly complicated. Instead I'd suggest making early_efi_map() take
> an offset and a length, and document when each framebuffer is used.

Matt, thanks for the comments. I have sent out an update patch which addressed
the comments except the last one "and document when each framebuffer is used"
I added comment before function early_efi_map, not sure if it's what you want.

Thanks
Dave