[PATCH] ignore ,keep for efi earlyprintk

[PATCH] ignore ,keep for efi earlyprintk

[Dave Young]

earlyprint=efi,keep results a kernel hang after "Freeing unused kernel memory"

Actually efi earlyprintk is using __init functions such as early_ioremap etc.
Thus efi earlyconsole can not be kept.

So just ignore it and give out a warning to user.

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 arch/x86/kernel/early_printk.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/kernel/early_printk.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/early_printk.c
+++ linux-2.6/arch/x86/kernel/early_printk.c
@@ -237,8 +237,11 @@ static int __init setup_early_printk(cha
 		}
 #endif
 #ifdef CONFIG_EARLY_PRINTK_EFI
-		if (!strncmp(buf, "efi", 3))
-			early_console_register(&early_efi_console, keep);
+		if (!strncmp(buf, "efi", 3)) {
+			if (keep)
+				pr_warn("Ignoring ,keep for earlyprintk=efi\n");
+			early_console_register(&early_efi_console, 0);
+		}
 #endif
 
 		buf++;

Re: [PATCH] ignore ,keep for efi earlyprintk

[Ingo Molnar]

* Dave Young <dyoung@redhat.com> wrote:

> earlyprint=efi,keep results a kernel hang after "Freeing unused kernel memory"

That's a bug.

> 
> Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> Thus efi earlyconsole can not be kept.
> 
> So just ignore it and give out a warning to user.

So, bugs should be fixed, not worked around.

earlyprintk=,keep is really useful for the occasional difficult low 
level bug in the printk code or similar, because earlyprintk is a 
completely separate code path from printk().

Thanks,

	Ingo

Re: [PATCH] ignore ,keep for efi earlyprintk

[Dave Young]

On 04/24/14 at 12:45pm, Ingo Molnar wrote:
> 
> * Dave Young <dyoung@redhat.com> wrote:
> 
> > earlyprint=efi,keep results a kernel hang after "Freeing unused kernel memory"
> 
> That's a bug.
> 
> > 
> > Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> > Thus efi earlyconsole can not be kept.
> > 
> > So just ignore it and give out a warning to user.
> 
> So, bugs should be fixed, not worked around.
> 
> earlyprintk=,keep is really useful for the occasional difficult low 
> level bug in the printk code or similar, because earlyprintk is a 
> completely separate code path from printk().

I thought that earlyprintk is just for debugging before real console is available.
But it does help debugging printk as you said.

Will think about how to fix it.

Thanks
Dave

Re: [PATCH] ignore ,keep for efi earlyprintk

[Matt Fleming]

On Thu, 24 Apr, at 05:36:58PM, Dave Young wrote:
> earlyprint=efi,keep results a kernel hang after "Freeing unused kernel memory"
 
Are you sure it hangs and it's not just that nothing is output to the
console after that point?

> Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> Thus efi earlyconsole can not be kept.
 
Urgh, good point, that's a bug. However I'm sure I've had this working
in the past and have in fact used it to debug stuff.

Could you try and fix this up so it works with 'keep'?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] ignore ,keep for efi earlyprintk

[Matt Fleming]

On Thu, 24 Apr, at 05:36:58PM, Dave Young wrote:
> earlyprint=efi,keep results a kernel hang after "Freeing unused kernel memory"
 
Are you sure it hangs and it's not just that nothing is output to the
console after that point?

> Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> Thus efi earlyconsole can not be kept.
 
Urgh, good point, that's a bug. However I'm sure I've had this working
in the past and have in fact used it to debug stuff.

Could you try and fix this up so it works with 'keep'?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] ignore ,keep for efi earlyprintk

[Dave Young]

On 04/25/14 at 12:53pm, Matt Fleming wrote:
> On Thu, 24 Apr, at 05:36:58PM, Dave Young wrote:
> > earlyprint=efi,keep results a kernel hang after "Freeing unused kernel memory"
>  
> Are you sure it hangs and it's not just that nothing is output to the
> console after that point?

There's other kernel messages without appending ,keep.

Also console=ttyS0 does not work with earlyprintk=efi,keep, removing ,keep it works well.

> 
> > Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> > Thus efi earlyconsole can not be kept.
>  
> Urgh, good point, that's a bug. However I'm sure I've had this working
> in the past and have in fact used it to debug stuff.
> 
> Could you try and fix this up so it works with 'keep'?

Will do. I want use ioremap when it is available but it's hard to decide
where to add the code.

I'm thinking to do below:
1. ioremap the whole framebuffer like: efi_fb = ioremap(lfb_base, lfb_size) in the
   function efi_enter_virtual_mode because ioremap should be usable then.

2. add two functions for earlyprintk use (scrollup, write, etc.) 
__init_refok void * get_vaddr(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);
}

__init_refok void put_vaddr(void *addr, unsigned long len)
{
	if (!efi_fb)
		early_iounmap(addr, len);
}

But it does not sound good to add the ioremap in enter virtual mode funtion.   

Please help if you have any other better idea.

Thanks
Dave

Re: [PATCH] ignore ,keep for efi earlyprintk

[Dave Young]

On 04/25/14 at 12:53pm, Matt Fleming wrote:
> On Thu, 24 Apr, at 05:36:58PM, Dave Young wrote:
> > earlyprint=efi,keep results a kernel hang after "Freeing unused kernel memory"
>  
> Are you sure it hangs and it's not just that nothing is output to the
> console after that point?

There's other kernel messages without appending ,keep.

Also console=ttyS0 does not work with earlyprintk=efi,keep, removing ,keep it works well.

> 
> > Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> > Thus efi earlyconsole can not be kept.
>  
> Urgh, good point, that's a bug. However I'm sure I've had this working
> in the past and have in fact used it to debug stuff.
> 
> Could you try and fix this up so it works with 'keep'?

Will do. I want use ioremap when it is available but it's hard to decide
where to add the code.

I'm thinking to do below:
1. ioremap the whole framebuffer like: efi_fb = ioremap(lfb_base, lfb_size) in the
   function efi_enter_virtual_mode because ioremap should be usable then.

2. add two functions for earlyprintk use (scrollup, write, etc.) 
__init_refok void * get_vaddr(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);
}

__init_refok void put_vaddr(void *addr, unsigned long len)
{
	if (!efi_fb)
		early_iounmap(addr, len);
}

But it does not sound good to add the ioremap in enter virtual mode funtion.   

Please help if you have any other better idea.

Thanks
Dave

Re: [PATCH] ignore ,keep for efi earlyprintk

[Matt Fleming]

On Sat, 26 Apr, at 03:28:21PM, Dave Young wrote:
> 
> There's other kernel messages without appending ,keep.
> 
> Also console=ttyS0 does not work with earlyprintk=efi,keep, removing
> ,keep it works well.

Try console=efi

> > > Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> > > Thus efi earlyconsole can not be kept.
> >  
> > Urgh, good point, that's a bug. However I'm sure I've had this working
> > in the past and have in fact used it to debug stuff.
> > 
> > Could you try and fix this up so it works with 'keep'?
> 
> Will do. I want use ioremap when it is available but it's hard to decide
> where to add the code.
> 
> I'm thinking to do below:
> 1. ioremap the whole framebuffer like: efi_fb = ioremap(lfb_base, lfb_size) in the
>    function efi_enter_virtual_mode because ioremap should be usable then.
> 
> 2. add two functions for earlyprintk use (scrollup, write, etc.) 
> __init_refok void * get_vaddr(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);
> }
> 
> __init_refok void put_vaddr(void *addr, unsigned long len)
> {
> 	if (!efi_fb)
> 		early_iounmap(addr, len);
> }
> 
> But it does not sound good to add the ioremap in enter virtual mode funtion.   
> 
> Please help if you have any other better idea.

I would just replace the existing calls to early_ioremap() with
efi_ioremap() and implement it like so (all in
arch/x86/platform/efi/early_printk.c),

static void *efi_ioremap(resource_size_t phys_addr, unsigned long size)
{
	if (system_state == SYSTEM_BOOTING)
		return early_ioremap(phys_addr, size);

	return ioremap(phys_addr, size);
}

At least I think SYSTEM_BOOTING is the correct way to figure out when to
use early_ioremap(), I'm open to other suggestions though.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] ignore ,keep for efi earlyprintk

[Matt Fleming]

On Sat, 26 Apr, at 03:28:21PM, Dave Young wrote:
> 
> There's other kernel messages without appending ,keep.
> 
> Also console=ttyS0 does not work with earlyprintk=efi,keep, removing
> ,keep it works well.

Try console=efi

> > > Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> > > Thus efi earlyconsole can not be kept.
> >  
> > Urgh, good point, that's a bug. However I'm sure I've had this working
> > in the past and have in fact used it to debug stuff.
> > 
> > Could you try and fix this up so it works with 'keep'?
> 
> Will do. I want use ioremap when it is available but it's hard to decide
> where to add the code.
> 
> I'm thinking to do below:
> 1. ioremap the whole framebuffer like: efi_fb = ioremap(lfb_base, lfb_size) in the
>    function efi_enter_virtual_mode because ioremap should be usable then.
> 
> 2. add two functions for earlyprintk use (scrollup, write, etc.) 
> __init_refok void * get_vaddr(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);
> }
> 
> __init_refok void put_vaddr(void *addr, unsigned long len)
> {
> 	if (!efi_fb)
> 		early_iounmap(addr, len);
> }
> 
> But it does not sound good to add the ioremap in enter virtual mode funtion.   
> 
> Please help if you have any other better idea.

I would just replace the existing calls to early_ioremap() with
efi_ioremap() and implement it like so (all in
arch/x86/platform/efi/early_printk.c),

static void *efi_ioremap(resource_size_t phys_addr, unsigned long size)
{
	if (system_state == SYSTEM_BOOTING)
		return early_ioremap(phys_addr, size);

	return ioremap(phys_addr, size);
}

At least I think SYSTEM_BOOTING is the correct way to figure out when to
use early_ioremap(), I'm open to other suggestions though.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] ignore ,keep for efi earlyprintk

[Dave Young]

On 04/29/14 at 10:45am, Matt Fleming wrote:
> On Sat, 26 Apr, at 03:28:21PM, Dave Young wrote:
> > 
> > There's other kernel messages without appending ,keep.
> > 
> > Also console=ttyS0 does not work with earlyprintk=efi,keep, removing
> > ,keep it works well.
> 
> Try console=efi

With console=efi, it also hangs the screen is not readable. When I add both console=efi
and console=ttyS0, in serial console output looks like it hangs while mounting rootfs. 
On screen it's same as console=efi only case.

I justed tested on qemu.

> 
> > > > Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> > > > Thus efi earlyconsole can not be kept.
> > >  
> > > Urgh, good point, that's a bug. However I'm sure I've had this working
> > > in the past and have in fact used it to debug stuff.
> > > 
> > > Could you try and fix this up so it works with 'keep'?
> > 
> > Will do. I want use ioremap when it is available but it's hard to decide
> > where to add the code.
> > 
> > I'm thinking to do below:
> > 1. ioremap the whole framebuffer like: efi_fb = ioremap(lfb_base, lfb_size) in the
> >    function efi_enter_virtual_mode because ioremap should be usable then.
> > 
> > 2. add two functions for earlyprintk use (scrollup, write, etc.) 
> > __init_refok void * get_vaddr(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);
> > }
> > 
> > __init_refok void put_vaddr(void *addr, unsigned long len)
> > {
> > 	if (!efi_fb)
> > 		early_iounmap(addr, len);
> > }
> > 
> > But it does not sound good to add the ioremap in enter virtual mode funtion.   
> > 
> > Please help if you have any other better idea.
> 
> I would just replace the existing calls to early_ioremap() with
> efi_ioremap() and implement it like so (all in
> arch/x86/platform/efi/early_printk.c),
> 
> static void *efi_ioremap(resource_size_t phys_addr, unsigned long size)
> {
> 	if (system_state == SYSTEM_BOOTING)
> 		return early_ioremap(phys_addr, size);
> 
> 	return ioremap(phys_addr, size);
> }
> 
> At least I think SYSTEM_BOOTING is the correct way to figure out when to
> use early_ioremap(), I'm open to other suggestions though.

I also thought about this before, but I remember it hangs before system state
changing.

The flow is like below, I added some debug printk, only the ones before free_initmem()
shows in log. Thus I guess this will not work. But I can have a try. 
        kernel_init_freeable();
        /* need to finish all async __init code before freeing the memory */
        async_synchronize_full();
        free_initmem();
        mark_rodata_ro();
        system_state = SYSTEM_RUNNING; 

Thanks
Dave

Re: [PATCH] ignore ,keep for efi earlyprintk

[Dave Young]

On 04/29/14 at 10:45am, Matt Fleming wrote:
> On Sat, 26 Apr, at 03:28:21PM, Dave Young wrote:
> > 
> > There's other kernel messages without appending ,keep.
> > 
> > Also console=ttyS0 does not work with earlyprintk=efi,keep, removing
> > ,keep it works well.
> 
> Try console=efi

With console=efi, it also hangs the screen is not readable. When I add both console=efi
and console=ttyS0, in serial console output looks like it hangs while mounting rootfs. 
On screen it's same as console=efi only case.

I justed tested on qemu.

> 
> > > > Actually efi earlyprintk is using __init functions such as early_ioremap etc.
> > > > Thus efi earlyconsole can not be kept.
> > >  
> > > Urgh, good point, that's a bug. However I'm sure I've had this working
> > > in the past and have in fact used it to debug stuff.
> > > 
> > > Could you try and fix this up so it works with 'keep'?
> > 
> > Will do. I want use ioremap when it is available but it's hard to decide
> > where to add the code.
> > 
> > I'm thinking to do below:
> > 1. ioremap the whole framebuffer like: efi_fb = ioremap(lfb_base, lfb_size) in the
> >    function efi_enter_virtual_mode because ioremap should be usable then.
> > 
> > 2. add two functions for earlyprintk use (scrollup, write, etc.) 
> > __init_refok void * get_vaddr(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);
> > }
> > 
> > __init_refok void put_vaddr(void *addr, unsigned long len)
> > {
> > 	if (!efi_fb)
> > 		early_iounmap(addr, len);
> > }
> > 
> > But it does not sound good to add the ioremap in enter virtual mode funtion.   
> > 
> > Please help if you have any other better idea.
> 
> I would just replace the existing calls to early_ioremap() with
> efi_ioremap() and implement it like so (all in
> arch/x86/platform/efi/early_printk.c),
> 
> static void *efi_ioremap(resource_size_t phys_addr, unsigned long size)
> {
> 	if (system_state == SYSTEM_BOOTING)
> 		return early_ioremap(phys_addr, size);
> 
> 	return ioremap(phys_addr, size);
> }
> 
> At least I think SYSTEM_BOOTING is the correct way to figure out when to
> use early_ioremap(), I'm open to other suggestions though.

I also thought about this before, but I remember it hangs before system state
changing.

The flow is like below, I added some debug printk, only the ones before free_initmem()
shows in log. Thus I guess this will not work. But I can have a try. 
        kernel_init_freeable();
        /* need to finish all async __init code before freeing the memory */
        async_synchronize_full();
        free_initmem();
        mark_rodata_ro();
        system_state = SYSTEM_RUNNING; 

Thanks
Dave

Re: [PATCH] ignore ,keep for efi earlyprintk

[Thomas Gleixner]

On Tue, 29 Apr 2014, Matt Fleming wrote:
> I would just replace the existing calls to early_ioremap() with
> efi_ioremap() and implement it like so (all in
> arch/x86/platform/efi/early_printk.c),
> 
> static void *efi_ioremap(resource_size_t phys_addr, unsigned long size)
> {
> 	if (system_state == SYSTEM_BOOTING)
> 		return early_ioremap(phys_addr, size);
> 
> 	return ioremap(phys_addr, size);
> }
> 
> At least I think SYSTEM_BOOTING is the correct way to figure out when to
> use early_ioremap(), I'm open to other suggestions though.

ioremap() is available after paging_init() has been called.

Just x86 has this extra bits so you can use early_ioremap/unmap
_AFTER_ paging_init(). No idea why, but probably to cope with some
brilliant init calls which run pre and post paging_init().

Thanks,

	tglx

Re: [PATCH] ignore ,keep for efi earlyprintk

[Thomas Gleixner]

On Tue, 29 Apr 2014, Matt Fleming wrote:
> I would just replace the existing calls to early_ioremap() with
> efi_ioremap() and implement it like so (all in
> arch/x86/platform/efi/early_printk.c),
> 
> static void *efi_ioremap(resource_size_t phys_addr, unsigned long size)
> {
> 	if (system_state == SYSTEM_BOOTING)
> 		return early_ioremap(phys_addr, size);
> 
> 	return ioremap(phys_addr, size);
> }
> 
> At least I think SYSTEM_BOOTING is the correct way to figure out when to
> use early_ioremap(), I'm open to other suggestions though.

ioremap() is available after paging_init() has been called.

Just x86 has this extra bits so you can use early_ioremap/unmap
_AFTER_ paging_init(). No idea why, but probably to cope with some
brilliant init calls which run pre and post paging_init().

Thanks,

	tglx

Re: [PATCH] ignore ,keep for efi earlyprintk

[Dave Young]

On 04/29/14 at 12:21pm, Thomas Gleixner wrote:
> On Tue, 29 Apr 2014, Matt Fleming wrote:
> > I would just replace the existing calls to early_ioremap() with
> > efi_ioremap() and implement it like so (all in
> > arch/x86/platform/efi/early_printk.c),
> > 
> > static void *efi_ioremap(resource_size_t phys_addr, unsigned long size)
> > {
> > 	if (system_state == SYSTEM_BOOTING)
> > 		return early_ioremap(phys_addr, size);
> > 
> > 	return ioremap(phys_addr, size);
> > }
> > 
> > At least I think SYSTEM_BOOTING is the correct way to figure out when to
> > use early_ioremap(), I'm open to other suggestions though.
> 
> ioremap() is available after paging_init() has been called.
> 
> Just x86 has this extra bits so you can use early_ioremap/unmap
> _AFTER_ paging_init(). No idea why, but probably to cope with some
> brilliant init calls which run pre and post paging_init().

Good idea, will use an initcall function for this purpose.
Thanks a lot.
Dave

Re: [PATCH] ignore ,keep for efi earlyprintk

[Dave Young]

On 04/29/14 at 12:21pm, Thomas Gleixner wrote:
> On Tue, 29 Apr 2014, Matt Fleming wrote:
> > I would just replace the existing calls to early_ioremap() with
> > efi_ioremap() and implement it like so (all in
> > arch/x86/platform/efi/early_printk.c),
> > 
> > static void *efi_ioremap(resource_size_t phys_addr, unsigned long size)
> > {
> > 	if (system_state == SYSTEM_BOOTING)
> > 		return early_ioremap(phys_addr, size);
> > 
> > 	return ioremap(phys_addr, size);
> > }
> > 
> > At least I think SYSTEM_BOOTING is the correct way to figure out when to
> > use early_ioremap(), I'm open to other suggestions though.
> 
> ioremap() is available after paging_init() has been called.
> 
> Just x86 has this extra bits so you can use early_ioremap/unmap
> _AFTER_ paging_init(). No idea why, but probably to cope with some
> brilliant init calls which run pre and post paging_init().

Good idea, will use an initcall function for this purpose.
Thanks a lot.
Dave