Re: [PATCH v6 3/3] x86/earlyprintk: setup earlyprintk as early as possible

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, 2015-05-12 at 14:10 +0600, Alexander Kuleshov wrote:
> The early_printk function is usable only after the setup_early_printk will
> be executed. We pass 'earlyprintk' through the kernel command line, so it
> will be usable only after the 'parse_early_param' will be executed. This means
> that we have usable earlyprintk only during early boot, kernel decompression
> and after call of the 'parse_early_param'. This patchset makes earlyprintk
> usable before the call of the 'parse_early_param'.
> 
> This patch makes setup_early_printk visible for head{32,64}.c. So the
> 'early_printk' function will be usabable after decompression of the
> kernel and before parse_early_param will be called. It also must be
> safe if CONFIG_CMDLINE_BOOL and CONFIG_CMDLINE_OVERRIDE are set, because
> setup_cmdline function will be called before setup_early_printk.
> 

Few comments below.

> Tested it with qemu, so early_printk() is usable and prints to serial
> console right after setup_early_printk function called.
> 
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> ---
>  arch/x86/include/asm/serial.h  |  3 +++
>  arch/x86/kernel/early_printk.c | 25 +++++++++++++++++++++++++
>  arch/x86/kernel/head32.c       |  5 +++++
>  arch/x86/kernel/head64.c       |  5 +++++
>  4 files changed, 38 insertions(+)
> 
> diff --git a/arch/x86/include/asm/serial.h b/arch/x86/include/asm/serial.h
> index 8378b8c..198f041 100644
> --- a/arch/x86/include/asm/serial.h
> +++ b/arch/x86/include/asm/serial.h
> @@ -26,4 +26,7 @@
>  	{ .uart = 0,	BASE_BAUD,	0x3E8,	4,	STD_COMX_FLAGS	}, /* ttyS2 */	\
>  	{ .uart = 0,	BASE_BAUD,	0x2E8,	3,	STD_COM4_FLAGS	}, /* ttyS3 */
> 
> +/* used by arch/x86/kernel/head{32,64}.c */
> +int __init setup_early_serial_console(void);
> +

Actually, have you investigated how this works on the other supported
architectures? It might be better to align this with them.

>  #endif /* _ASM_X86_SERIAL_H */
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index 89427d8..96425c3 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -385,4 +385,29 @@ static int __init setup_early_printk(char *buf)
>  	return 0;
>  }
> 
> +int __init setup_early_serial_console(void)
> +{
> +#ifdef CONFIG_EARLY_PRINTK
> +	char *arg;
> +
> +	/*
> +	* make sure that we have:
> +	*      "serial,0x3f8,115200"
> +	*      "serial,ttyS0,115200"
> +	*      "ttyS0,115200"
> +	*/

Indentation and text styling problems.

> +	arg = strstr(boot_command_line, "earlyprintk=serial");
> +	if (!arg)
> +		arg = strstr(boot_command_line, "earlyprintk=ttyS");
> +	if (!arg)
> +		return -1;

What about other cases like that described in setup_early_printk()?

> +
> +	arg = strstr(boot_command_line, "earlyprintk=");
> +	/* += strlen("earlyprintk"); */
> +	arg += 12;
> +
> +	return setup_early_printk(arg);
> +#endif

#ifdef seems redundant here, you already build this module iff
EARLY_PRINTK is set.

> +}

So, the logic of this function seems broken. I don't get why you have to
check the contents of earlyprintk parameter.

> +
>  early_param("earlyprintk", setup_early_printk);
> diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
> index 92e8b5f..d325d0c 100644
> --- a/arch/x86/kernel/head32.c
> +++ b/arch/x86/kernel/head32.c
> @@ -32,6 +32,11 @@ static void __init i386_default_early_setup(void)
>  asmlinkage __visible void __init i386_start_kernel(void)
>  {
>  	setup_builtin_cmdline();
> +	setup_early_serial_console();
> +
> +	if (console_loglevel >= CONSOLE_LOGLEVEL_DEBUG)
> +		early_printk("Kernel alive\n");

This should be unified (like printed "Early printk is initialized" in
setup_early_printk() ), moreover what about other architectures?


> +
>  	cr4_init_shadow();
>  	sanitize_boot_params(&boot_params);
> 
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index 38da21c..06fcc1b 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -173,6 +173,11 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
>  	copy_bootdata(__va(real_mode_data));


>  	setup_builtin_cmdline();
> 
> +	setup_early_serial_console();

Those two can be grouped in the same way like in previous change (see
above).

> +
> +	if (console_loglevel >= CONSOLE_LOGLEVEL_DEBUG)
> +		early_printk("Kernel alive\n");

Same comment as in above chunk.

> +
>  	/*
>  	 * Load microcode early on BSP.
>  	 */
> --
> 2.4.0


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy