LinuxLists.cc - [PATCH v13 0/4] x86/earlyprintk: setup serial earlyprintk as early as possible

2015-06-27 13:46:08

Subject: [PATCH v13 0/4] x86/earlyprintk: setup serial earlyprintk as early as possible

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
means that earlyprintk will be usable only after the 'parse_early_param'
will be executed or in another words earlyprintk is usable only during early
boot, kernel decompression and after call of the parse_early_param. But
there are many stuff after the kernel decompression and before the
parse_early_param will be called as memblock usage, early cpu initialization,
early ioremap initialization and etc... So earlyprintk allows us to see
what's going on there.

Changelog:

v13:
* do not setup pciserial from the arch/x86/kernel/head{32,64}.c, because
it uses ioremap and we can't do it really early;
* style fixes;
* patch for testing.

v12:
* all changes from the v11 are reverted
* setup_early_serial_console renamed to the setup_earlyprintk_console and
refactored. Now it checks 'earlyprintk=' in the kernel command line, set
earlyprintk_late variable to false. This variable allows to know, do we
can setup early console for the certain device.

v11:
* setup_log_buf moved to the arch/x86/kernel/head{32,64.c} from
the arch/x86/kernel/setup.c to setup early log_buf for the earlyprintl
* Update log_buf in the early_printk function
* Added additional patch for testing earlyprintk

v10:
* Removed style issues which are not related to the patchset.

v9:
* Add call of the lockdep_init to the arch/x86/kernel/head{32,64}.c
before the serial console initialization to prevent:

[ 0.000000] WARNING: lockdep init error! lock-(console_sem).lock was acquiredbefore lockdep_init
[ 0.000000] Call stack leading to lockdep invocation was:
[ 0.000000] [] save_stack_trace+0x2f/0x50
[ 0.000000] [] __lock_acquire+0xa2c/0xf00
[ 0.000000] [] lock_acquire+0xdb/0x2b0
[ 0.000000] [] _raw_spin_lock_irqsave+0x53/0x90
[ 0.000000] [] down+0x16/0x50
[ 0.000000] [] console_lock+0x19/0x60
[ 0.000000] [] register_console+0x116/0x350
[ 0.000000] [] setup_early_printk+0x165/0x467
[ 0.000000] [] setup_early_serial_console+0x56/0x58
[ 0.000000] [] x86_64_start_kernel+0xce/0x110
[ 0.000000] [] 0xffffffffffffffff
[ 0.000000] ------------------------

during early serial console initialization.

* Add additional check to the earlyprintk initialization to protect double initialization
of the early_serial_console.
* Fixed comment.

v8:
* Fixed warning with the definition of the setup_early_serial_console in the
arch/x86/include/asm/setup.h

v7:
* Move setup_early_serial_console to the the arch/x86/include/setup.h
* Add ifdefs to prevent setup_serial_console if CONFIG_EARLY_PRINTK is not set.

v6:
* Style fixes.
* Call of the suetp_builtin_cmdline moved to the separate patch.

v5:
* Call setup_builtin_cmdline instead of setup_cmdline

v4:
* Move setup_early_serial_console from the include/linux/printk.h to the
arch/x86/include/asm/serial.h, because this function is only for x86 now.

v3:
* Call setup_cmdline before setup_early_printk;
* setup_early_printk call wrapped with the setup_early_serial_console which
checks that 'serial' given to the earlyprintk command line option. This prevents
call of the setup_early_printk with the given pciserial/dbgp/efi, because they
are using early_ioremap.

v2:
* Comment added before the setup_early_printk call;
* Added information about testing to the commit message.

Alexander Kuleshov (4):
x86/setup: introduce setup_bultin_cmdline
x86/setup: handle builtin command line as early as possible
x86/earlyprintk: setup earlyprintk as early as possible
x86/earlyprintk: add some early_printk for tests

arch/x86/include/asm/setup.h | 8 +++++++-
arch/x86/kernel/early_printk.c | 42 +++++++++++++++++++++++++++++++++++-------
arch/x86/kernel/head32.c | 9 +++++++++
arch/x86/kernel/head64.c | 8 ++++++++
arch/x86/kernel/setup.c | 30 +++++++++++++++++-------------
init/main.c | 2 ++
6 files changed, 78 insertions(+), 21 deletions(-)

--
2.4.4.410.gc71d752

2015-06-27 13:46:25

by Alexander Kuleshov

[permalink] [raw]

Subject: [PATCH 1/4] x86/setup: introduce setup_bultin_cmdline

This patch introduces the setup_builtin_cmdline function which appends or
overrides boot_command_line with the builtin_cmdline if CONFIG_CMDLINE_BOOL
is set.

Previously this functional was in the setup_arch, but we need to move
it for getting actual command line as early as possible in the
arch/x86/kernel/head{32,64}.c for the earlyprintk setup.

Signed-off-by: Alexander Kuleshov <[email protected]>
---
arch/x86/include/asm/setup.h | 2 +-
arch/x86/kernel/setup.c | 29 +++++++++++++++++------------
2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
index 11af24e..70dfa61 100644
--- a/arch/x86/include/asm/setup.h
+++ b/arch/x86/include/asm/setup.h
@@ -124,8 +124,8 @@ asmlinkage void __init i386_start_kernel(void);
#else
asmlinkage void __init x86_64_start_kernel(char *real_mode);
asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
-
#endif /* __i386__ */
+void __init setup_builtin_cmdline(void);
#endif /* _SETUP */
#else
#define RESERVE_BRK(name,sz) \
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index d3b95b8..e528f12 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -847,6 +847,22 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
return 0;
}

+void __init setup_builtin_cmdline(void)
+{
+#ifdef CONFIG_CMDLINE_BOOL
+#ifdef CONFIG_CMDLINE_OVERRIDE
+ strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+#else
+ if (builtin_cmdline[0]) {
+ /* append boot loader cmdline to builtin */
+ strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
+ strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
+ strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+ }
+#endif
+#endif
+}
+
/*
* Determine if we were loaded by an EFI loader. If so, then we have also been
* passed the efi memmap, systab, etc., so we should use these data structures
@@ -975,2 +991,2 @@ void __init setup_arch(char **cmdline_p)
bss_resource.start = __pa_symbol(__bss_start);
bss_resource.end = __pa_symbol(__bss_stop)-1;

-#ifdef CONFIG_CMDLINE_BOOL
-#ifdef CONFIG_CMDLINE_OVERRIDE
- strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-#else
- if (builtin_cmdline[0]) {
- /* append boot loader cmdline to builtin */
- strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
- strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
- strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
- }
-#endif
-#endif
+ setup_builtin_cmdline();

strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
*cmdline_p = command_line;
--
2.4.4.410.gc71d752

2015-06-27 13:47:13

by Alexander Kuleshov

[permalink] [raw]

Subject: [PATCH 1/4 v13] x86/setup: introduce setup_bultin_cmdline

2015-06-27 13:48:12

by Alexander Kuleshov

[permalink] [raw]

Subject: [PATCH 2/4 v13] x86/setup: handle builtin command line as early as possible

This patch adds the call of the setup_builtin_cmdline to
handle builtin command line before we will setup earlyprintk.

Signed-off-by: Alexander Kuleshov <[email protected]>
---
arch/x86/kernel/head32.c | 1 +
arch/x86/kernel/head64.c | 1 +
arch/x86/kernel/setup.c | 2 --
3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
index 2911ef3..92e8b5f 100644
--- a/arch/x86/kernel/head32.c
+++ b/arch/x86/kernel/head32.c
@@ -31,6 +31,7 @@ static void __init i386_default_early_setup(void)

asmlinkage __visible void __init i386_start_kernel(void)
{
+ setup_builtin_cmdline();
cr4_init_shadow();
sanitize_boot_params(&boot_params);

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 5a46681..d255430 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -172,6 +172,7 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)

copy_bootdata(__va(real_mode_data));

+ setup_builtin_cmdline();
/*
* Load microcode early on BSP.
*/
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index e528f12..fc381ec 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -991,2 +991,2 @@ void __init setup_arch(char **cmdline_p)
bss_resource.start = __pa_symbol(__bss_start);
bss_resource.end = __pa_symbol(__bss_stop)-1;

- setup_builtin_cmdline();
-
strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
*cmdline_p = command_line;

--
2.4.4.410.gc71d752

2015-06-27 13:48:37

by Alexander Kuleshov

[permalink] [raw]

Subject: [PATCH 3/4 v13] x86/earlyprintk: setup earlyprintk as early as possible

The earlyprintk 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', but sometimes it is very useful to
know what occurs between. The earlyprintk can allow us to know what occurs after
kernel decompression and before parse_early_param will be called.

This patch provides following stuff:

1. Thi patch introduces the setup_earlyprintk_console function,
which called arch/x86/kernel/hhead{32,64}.c, parses kernel command line,
tries to find 'earlyprintk' option and calls setup_early_printk depending
on the result.

2. As setup_earlyprintk_console setups earlyprintk very early, we can't
use all console devices for now, but only serial and vga. There is
earlyprintk_late variable which determines ability to setup earlyprintk
for the certain device.

3. Call of the lockdep_init added to the arch/x86/kernel/head{32,64}.c
to prevent call of the register_console before the initialization of lockdep.
In other way there we will get:

[ 0.000000] WARNING: lockdep init error! lock-(console_sem).lock was acquiredbefore lockdep_init
[ 0.000000] Call stack leading to lockdep invocation was:
[ 0.000000] [] save_stack_trace+0x2f/0x50
[ 0.000000] [] __lock_acquire+0xa2c/0xf00
[ 0.000000] [] lock_acquire+0xdb/0x2b0
[ 0.000000] [] _raw_spin_lock_irqsave+0x53/0x90
[ 0.000000] [] down+0x16/0x50
[ 0.000000] [] console_lock+0x19/0x60
[ 0.000000] [] register_console+0x116/0x350
[ 0.000000] [] setup_early_printk+0x165/0x467
[ 0.000000] [] setup_early_serial_console+0x56/0x58
[ 0.000000] [] x86_64_start_kernel+0xce/0x110
[ 0.000000] [] 0xffffffffffffffff
[ 0.000000] ------------------------

This patch adds lockdep_init to the arch/x86/kernel/head{32,64}.c, but
not removed it from the init/main.c, because there is a couple of architectures
which have support of the lockdep, but do not call lockdep_init in their
architecture-dependent code.

4. As setup_earlyprintk_console can be called twice: from the
setup_earlyprintk_console and parse_early_param, additional
check added to the really early consoles.

Tested it with qemu, so early_printk() is usable and prints to serial
console right after setup_early_printk function called.

We will not see earlyprintk messages in the dmesg buffer, because
it is too early to initialized log_buf.

Signed-off-by: Alexander Kuleshov <[email protected]>
---
arch/x86/include/asm/setup.h | 6 ++++++
arch/x86/kernel/early_printk.c | 42 +++++++++++++++++++++++++++++++++++-------
arch/x86/kernel/head32.c | 8 ++++++++
arch/x86/kernel/head64.c | 7 +++++++
4 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
index 70dfa61..695f251 100644
--- a/arch/x86/include/asm/setup.h
+++ b/arch/x86/include/asm/setup.h
@@ -126,0 +126,0 @@ asmlinkage void __init x86_64_start_kernel(char *real_mode);
asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
#endif /* __i386__ */
void __init setup_builtin_cmdline(void);
+#ifdef CONFIG_EARLY_PRINTK
+/* used by arch/x86/kernel/head{32,64}.c */
+extern int __init setup_earlyprintk_console(void);
+#else
+static inline int __init setup_earlyprintk_console(void) { return 0; }
+#endif /* CONFIG_EARLY_PRINTK */
#endif /* _SETUP */
#else
#define RESERVE_BRK(name,sz) \
diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 89427d8..cc47bce 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -329,6 +329,15 @@ static inline void early_console_register(struct console *con, int keep_early)
register_console(early_console);
}

+/*
+ * Setup of earlyprintk is probably too early now. The setup_early_printk
+ * can be called from two places: from setup_earlyprintk_console and
+ * parse_early_param. In first case it is too early to setup earlyprintk
+ * for some devices as efi, pciserial and etc., but it can be set for
+ * vga and serial.
+ */
+static bool earlyprintk_late = 1;
+
static int __init setup_early_printk(char *buf)
{
int keep;
@@ -342,47 +351,66 @@ static int __init setup_early_printk(char *buf)
keep = (strstr(buf, "keep") != NULL);

while (*buf != '\0') {
- if (!strncmp(buf, "serial", 6)) {
+ if (!strncmp(buf, "serial", 6) &&
+ early_serial_console.index == -1) {
buf += 6;
early_serial_init(buf);
early_console_register(&early_serial_console, keep);
if (!strncmp(buf, ",ttyS", 5))
buf += 5;
}
- if (!strncmp(buf, "ttyS", 4)) {
+ if (!strncmp(buf, "ttyS", 4) &&
+ early_serial_console.index == -1) {
early_serial_init(buf + 4);
early_console_register(&early_serial_console, keep);
}
#ifdef CONFIG_PCI
- if (!strncmp(buf, "pciserial", 9)) {
+ if (!strncmp(buf, "pciserial", 9) && earlyprintk_late) {
early_pci_serial_init(buf + 9);
early_console_register(&early_serial_console, keep);
buf += 9; /* Keep from match the above "serial" */
}
#endif
if (!strncmp(buf, "vga", 3) &&
- boot_params.screen_info.orig_video_isVGA == 1) {
+ boot_params.screen_info.orig_video_isVGA == 1 &&
+ early_vga_console.index == -1) {
max_xpos = boot_params.screen_info.orig_video_cols;
max_ypos = boot_params.screen_info.orig_video_lines;
current_ypos = boot_params.screen_info.orig_y;
early_console_register(&early_vga_console, keep);
}
#ifdef CONFIG_EARLY_PRINTK_DBGP
- if (!strncmp(buf, "dbgp", 4) && !early_dbgp_init(buf + 4))
+ if (!strncmp(buf, "dbgp", 4) && !early_dbgp_init(buf + 4) &&
+ earlyprintk_late)
early_console_register(&early_dbgp_console, keep);
#endif
#ifdef CONFIG_HVC_XEN
- if (!strncmp(buf, "xen", 3))
+ if (!strncmp(buf, "xen", 3) && earlyprintk_late)
early_console_register(&xenboot_console, keep);
#endif
#ifdef CONFIG_EARLY_PRINTK_EFI
- if (!strncmp(buf, "efi", 3))
+ if (!strncmp(buf, "efi", 3) && earlyprintk_late)
early_console_register(&early_efi_console, keep);
#endif

buf++;
}
+
+ earlyprintk_late = 1;
return 0;
}

early_param("earlyprintk", setup_early_printk);
+
+int __init setup_earlyprintk_console(void)
+{
+ char *arg;
+
+ arg = strstr(boot_command_line, "earlyprintk=");
+ if (!arg)
+ return -1;
+
+ earlyprintk_late = 0;
+
+ return setup_early_printk(arg);
+}
diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
index 92e8b5f..d9189b1 100644
--- a/arch/x86/kernel/head32.c
+++ b/arch/x86/kernel/head32.c
@@ -32,0 +32,0 @@ static void __init i386_default_early_setup(void)
asmlinkage __visible void __init i386_start_kernel(void)
{
setup_builtin_cmdline();
+
+ setup_builtin_cmdline();
+
+ lockdep_init();
+
+ setup_earlyprintk_console();
+ early_printk("Early printk is initialized\n");
+
cr4_init_shadow();
sanitize_boot_params(&boot_params);

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index d255430..5386b3a 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -173,0 +173,0 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
copy_bootdata(__va(real_mode_data));

setup_builtin_cmdline();
+
+ lockdep_init();
+
+ setup_earlyprintk_console();
+
+ early_printk("Early printk is initialized\n");
+
/*
* Load microcode early on BSP.
*/
--
2.4.4.410.gc71d752

2015-06-27 13:48:53

by Alexander Kuleshov

[permalink] [raw]

Subject: [PATCH 4/4 v13] x86/earlyprintk: add some early_printk for tests

This patch is only for test of the full patch series. It provides
a couple calls of the early_printk function.

[Only for testing]

Signed-off-by: Alexander Kuleshov <[email protected]>
---
arch/x86/kernel/setup.c | 1 +
init/main.c | 2 ++
2 files changed, 3 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index fc381ec..488b1ba 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -878,6 +878,7 @@ void __init setup_builtin_cmdline(void)

void __init setup_arch(char **cmdline_p)
{
+ early_printk("Beginning of the x86 specific setup\n");
memblock_reserve(__pa_symbol(_text),
(unsigned long)__bss_stop - (unsigned long)_text);

diff --git a/init/main.c b/init/main.c
index c599aea..deeb2a2 100644
--- a/init/main.c
+++ b/init/main.c
@@ -499,6 +499,8 @@ asmlinkage __visible void __init start_kernel(void)
char *command_line;
char *after_dashes;

+ early_printk("start_kernel from init/main.c started to work\n");
+
/*
* Need to run as early as possible, to initialize the
* lockdep hash:
--
2.4.4.410.gc71d752

2015-06-27 13:54:48

by Alexander Kuleshov

[permalink] [raw]

Subject: Re: [PATCH 1/4] x86/setup: introduce setup_bultin_cmdline

sorry, forgot to add version to this patch, please skip this patch.

2015-06-27 19:46 GMT+06:00 Alexander Kuleshov <[email protected]>:
> This patch introduces the setup_builtin_cmdline function which appends or
> overrides boot_command_line with the builtin_cmdline if CONFIG_CMDLINE_BOOL
> is set.
>
> Previously this functional was in the setup_arch, but we need to move
> it for getting actual command line as early as possible in the
> arch/x86/kernel/head{32,64}.c for the earlyprintk setup.
>
> Signed-off-by: Alexander Kuleshov <[email protected]>
> ---
> arch/x86/include/asm/setup.h | 2 +-
> arch/x86/kernel/setup.c | 29 +++++++++++++++++------------
> 2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
> index 11af24e..70dfa61 100644
> --- a/arch/x86/include/asm/setup.h
> +++ b/arch/x86/include/asm/setup.h
> @@ -124,8 +124,8 @@ asmlinkage void __init i386_start_kernel(void);
> #else
> asmlinkage void __init x86_64_start_kernel(char *real_mode);
> asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
> -
> #endif /* __i386__ */
> +void __init setup_builtin_cmdline(void);
> #endif /* _SETUP */
> #else
> #define RESERVE_BRK(name,sz) \
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index d3b95b8..e528f12 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -847,6 +847,22 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
> return 0;
> }
>
> +void __init setup_builtin_cmdline(void)
> +{
> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_OVERRIDE
> + strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +#else
> + if (builtin_cmdline[0]) {
> + /* append boot loader cmdline to builtin */
> + strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> + strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> + strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> + }
> +#endif
> +#endif
> +}
> +
> /*
> * Determine if we were loaded by an EFI loader. If so, then we have also been
> * passed the efi memmap, systab, etc., so we should use these data structures
> @@ -975,2 +991,2 @@ void __init setup_arch(char **cmdline_p)
> bss_resource.start = __pa_symbol(__bss_start);
> bss_resource.end = __pa_symbol(__bss_stop)-1;
>
> -#ifdef CONFIG_CMDLINE_BOOL
> -#ifdef CONFIG_CMDLINE_OVERRIDE
> - strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> -#else
> - if (builtin_cmdline[0]) {
> - /* append boot loader cmdline to builtin */
> - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> - strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> - }
> -#endif
> -#endif
> + setup_builtin_cmdline();
>
> strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> *cmdline_p = command_line;
> --
> 2.4.4.410.gc71d752
>

2015-06-27 18:16:33

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH 1/4] x86/setup: introduce setup_bultin_cmdline

On Sat, Jun 27, 2015 at 4:54 PM, Alexander Kuleshov
<[email protected]> wrote:
> sorry, forgot to add version to this patch, please skip this patch.

You may, for example, supply --subject-prefix="PATCH v13" to git
format-patch command to create a nice version token.

One comment below.

>
> 2015-06-27 19:46 GMT+06:00 Alexander Kuleshov <[email protected]>:
>> This patch introduces the setup_builtin_cmdline function which appends or
>> overrides boot_command_line with the builtin_cmdline if CONFIG_CMDLINE_BOOL
>> is set.
>>
>> Previously this functional was in the setup_arch, but we need to move
>> it for getting actual command line as early as possible in the
>> arch/x86/kernel/head{32,64}.c for the earlyprintk setup.

I already once wrote you that commit message is not reflecting the
contents of the patch. Please, align, i.e. remove second paragraph and
add a line that there is no functional change.

Please, read carefully what others comment.

>>
>> Signed-off-by: Alexander Kuleshov <[email protected]>
>> ---
>> arch/x86/include/asm/setup.h | 2 +-
>> arch/x86/kernel/setup.c | 29 +++++++++++++++++------------
>> 2 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
>> index 11af24e..70dfa61 100644
>> --- a/arch/x86/include/asm/setup.h
>> +++ b/arch/x86/include/asm/setup.h
>> @@ -124,8 +124,8 @@ asmlinkage void __init i386_start_kernel(void);
>> #else
>> asmlinkage void __init x86_64_start_kernel(char *real_mode);
>> asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
>> -
>> #endif /* __i386__ */
>> +void __init setup_builtin_cmdline(void);
>> #endif /* _SETUP */
>> #else
>> #define RESERVE_BRK(name,sz) \
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index d3b95b8..e528f12 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -847,6 +847,22 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
>> return 0;
>> }
>>
>> +void __init setup_builtin_cmdline(void)
>> +{
>> +#ifdef CONFIG_CMDLINE_BOOL
>> +#ifdef CONFIG_CMDLINE_OVERRIDE
>> + strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> +#else
>> + if (builtin_cmdline[0]) {
>> + /* append boot loader cmdline to builtin */
>> + strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
>> + strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>> + strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> + }
>> +#endif
>> +#endif
>> +}
>> +
>> /*
>> * Determine if we were loaded by an EFI loader. If so, then we have also been
>> * passed the efi memmap, systab, etc., so we should use these data structures
>> @@ -975,2 +991,2 @@ void __init setup_arch(char **cmdline_p)
>> bss_resource.start = __pa_symbol(__bss_start);
>> bss_resource.end = __pa_symbol(__bss_stop)-1;
>>
>> -#ifdef CONFIG_CMDLINE_BOOL
>> -#ifdef CONFIG_CMDLINE_OVERRIDE
>> - strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> -#else
>> - if (builtin_cmdline[0]) {
>> - /* append boot loader cmdline to builtin */
>> - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
>> - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>> - strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> - }
>> -#endif
>> -#endif
>> + setup_builtin_cmdline();
>>
>> strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>> *cmdline_p = command_line;
>> --
>> 2.4.4.410.gc71d752
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
With Best Regards,
Andy Shevchenko

2015-06-27 18:39:17

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH 3/4 v13] x86/earlyprintk: setup earlyprintk as early as possible

On Sat, Jun 27, 2015 at 4:48 PM, Alexander Kuleshov
<[email protected]> wrote:
> The earlyprintk is usable only after the setup_early_printk will

You might use the standard form of the function representation in the
text, i.e. 'setup_early_printk()' (notice parens at the end of token).

> 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', but sometimes it is very useful to
> know what occurs between.

'in between'

> The earlyprintk can allow us to know what occurs after
> kernel decompression and before parse_early_param will be called.
>
> This patch provides following stuff:

'the following'

> 1. Thi patch introduces the setup_earlyprintk_console function,

'This'

> which called arch/x86/kernel/hhead{32,64}.c, parses kernel command line,

'which is called by …head…'

> tries to find 'earlyprintk' option and calls setup_early_printk depending
> on the result.
>
> 2. As setup_earlyprintk_console setups earlyprintk very early, we can't
> use all console devices for now, but only serial and vga. There is
> earlyprintk_late variable which determines ability to setup earlyprintk
> for the certain device.
>
> 3. Call of the lockdep_init added to the arch/x86/kernel/head{32,64}.c
> to prevent call of the register_console before the initialization of lockdep.
> In other way there we will get:
>
> [ 0.000000] WARNING: lockdep init error! lock-(console_sem).lock was acquiredbefore lockdep_init
> [ 0.000000] Call stack leading to lockdep invocation was:
> [ 0.000000] [] save_stack_trace+0x2f/0x50
> [ 0.000000] [] __lock_acquire+0xa2c/0xf00
> [ 0.000000] [] lock_acquire+0xdb/0x2b0
> [ 0.000000] [] _raw_spin_lock_irqsave+0x53/0x90
> [ 0.000000] [] down+0x16/0x50
> [ 0.000000] [] console_lock+0x19/0x60
> [ 0.000000] [] register_console+0x116/0x350
> [ 0.000000] [] setup_early_printk+0x165/0x467
> [ 0.000000] [] setup_early_serial_console+0x56/0x58
> [ 0.000000] [] x86_64_start_kernel+0xce/0x110
> [ 0.000000] [] 0xffffffffffffffff
> [ 0.000000] ------------------------
>
> This patch adds lockdep_init to the arch/x86/kernel/head{32,64}.c, but
> not removed it from the init/main.c, because there is a couple of architectures
> which have support of the lockdep, but do not call lockdep_init in their
> architecture-dependent code.
>
> 4. As setup_earlyprintk_console can be called twice: from the
> setup_earlyprintk_console and parse_early_param, additional

'from setup_earlyprintk_console()' (no need to have an article).

> check added to the really early consoles.

'is added'

>
> Tested it with qemu, so early_printk() is usable and prints to serial

'Tested with'

> console right after setup_early_printk function called.
>
> We will not see earlyprintk messages in the dmesg buffer, because
> it is too early to initialized log_buf.
>

I'm not a native speaker, so, my recommendation to you is to try to
find a guy who can check your messages before sending 'em out.

> Signed-off-by: Alexander Kuleshov <[email protected]>
> ---
> arch/x86/include/asm/setup.h | 6 ++++++
> arch/x86/kernel/early_printk.c | 42 +++++++++++++++++++++++++++++++++++-------
> arch/x86/kernel/head32.c | 8 ++++++++
> arch/x86/kernel/head64.c | 7 +++++++
> 4 files changed, 56 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
> index 70dfa61..695f251 100644
> --- a/arch/x86/include/asm/setup.h
> +++ b/arch/x86/include/asm/setup.h
> @@ -126,0 +126,0 @@ asmlinkage void __init x86_64_start_kernel(char *real_mode);
> asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
> #endif /* __i386__ */
> void __init setup_builtin_cmdline(void);
> +#ifdef CONFIG_EARLY_PRINTK
> +/* used by arch/x86/kernel/head{32,64}.c */
> +extern int __init setup_earlyprintk_console(void);
> +#else
> +static inline int __init setup_earlyprintk_console(void) { return 0; }
> +#endif /* CONFIG_EARLY_PRINTK */
> #endif /* _SETUP */
> #else
> #define RESERVE_BRK(name,sz) \
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index 89427d8..cc47bce 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -329,6 +329,15 @@ static inline void early_console_register(struct console *con, int keep_early)
> register_console(early_console);
> }
>
> +/*
> + * Setup of earlyprintk is probably too early now. The setup_early_printk
> + * can be called from two places:

> from setup_earlyprintk_console and
> + * parse_early_param.

-> funcname1() and funcname2()

> In first case it is too early to setup earlyprintk
> + * for some devices as efi, pciserial and etc., but it can be set for
> + * vga and serial.
> + */
> +static bool earlyprintk_late = 1;

It's a bool! So: true or false (nothing here in case of false).
Everywhere in your patch.

Why not to rename it and provide straight logic?

> +
> static int __init setup_early_printk(char *buf)
> {
> int keep;
> @@ -342,47 +351,66 @@ static int __init setup_early_printk(char *buf)
> keep = (strstr(buf, "keep") != NULL);
>
> while (*buf != '\0') {
> - if (!strncmp(buf, "serial", 6)) {
> + if (!strncmp(buf, "serial", 6) &&
> + early_serial_console.index == -1) {
> buf += 6;
> early_serial_init(buf);
> early_console_register(&early_serial_console, keep);
> if (!strncmp(buf, ",ttyS", 5))
> buf += 5;
> }
> - if (!strncmp(buf, "ttyS", 4)) {
> + if (!strncmp(buf, "ttyS", 4) &&
> + early_serial_console.index == -1) {
> early_serial_init(buf + 4);
> early_console_register(&early_serial_console, keep);
> }
> #ifdef CONFIG_PCI
> - if (!strncmp(buf, "pciserial", 9)) {
> + if (!strncmp(buf, "pciserial", 9) && earlyprintk_late) {
> early_pci_serial_init(buf + 9);
> early_console_register(&early_serial_console, keep);
> buf += 9; /* Keep from match the above "serial" */
> }
> #endif
> if (!strncmp(buf, "vga", 3) &&
> - boot_params.screen_info.orig_video_isVGA == 1) {
> + boot_params.screen_info.orig_video_isVGA == 1 &&
> + early_vga_console.index == -1) {
> max_xpos = boot_params.screen_info.orig_video_cols;
> max_ypos = boot_params.screen_info.orig_video_lines;
> current_ypos = boot_params.screen_info.orig_y;
> early_console_register(&early_vga_console, keep);
> }
> #ifdef CONFIG_EARLY_PRINTK_DBGP
> - if (!strncmp(buf, "dbgp", 4) && !early_dbgp_init(buf + 4))
> + if (!strncmp(buf, "dbgp", 4) && !early_dbgp_init(buf + 4) &&
> + earlyprintk_late)
> early_console_register(&early_dbgp_console, keep);
> #endif
> #ifdef CONFIG_HVC_XEN
> - if (!strncmp(buf, "xen", 3))
> + if (!strncmp(buf, "xen", 3) && earlyprintk_late)
> early_console_register(&xenboot_console, keep);
> #endif
> #ifdef CONFIG_EARLY_PRINTK_EFI
> - if (!strncmp(buf, "efi", 3))
> + if (!strncmp(buf, "efi", 3) && earlyprintk_late)
> early_console_register(&early_efi_console, keep);
> #endif
>
> buf++;
> }
> +
> + earlyprintk_late = 1;
> return 0;
> }
>
> early_param("earlyprintk", setup_early_printk);
> +
> +int __init setup_earlyprintk_console(void)
> +{
> + char *arg;
> +
> + arg = strstr(boot_command_line, "earlyprintk=");
> + if (!arg)
> + return -1;
> +
> + earlyprintk_late = 0;
> +
> + return setup_early_printk(arg);
> +}
> diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
> index 92e8b5f..d9189b1 100644
> --- a/arch/x86/kernel/head32.c
> +++ b/arch/x86/kernel/head32.c
> @@ -32,0 +32,0 @@ static void __init i386_default_early_setup(void)
> asmlinkage __visible void __init i386_start_kernel(void)
> {
> setup_builtin_cmdline();
> +
> + setup_builtin_cmdline();

Why two times?

> +
> + lockdep_init();
> +
> + setup_earlyprintk_console();
> + early_printk("Early printk is initialized\n");
> +
> cr4_init_shadow();
> sanitize_boot_params(&boot_params);
>
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index d255430..5386b3a 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -173,0 +173,0 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
> copy_bootdata(__va(real_mode_data));
>
> setup_builtin_cmdline();
> +
> + lockdep_init();
> +
> + setup_earlyprintk_console();
> +
> + early_printk("Early printk is initialized\n");
> +
> /*
> * Load microcode early on BSP.
> */

P.S. I guess you may try to submit first something a bit more trivial
that this to train your skills in open source community. You already
have 13 versions of the patch series with some stylistic issues. And
some of them might be due to you pay not much attention on them. This
makes your series quite unlikely to be reviewed and applied.

--
With Best Regards,
Andy Shevchenko

2015-06-27 19:09:45

by Alexander Kuleshov

[permalink] [raw]

Subject: Re: [PATCH 3/4 v13] x86/earlyprintk: setup earlyprintk as early as possible

Hello Andy,

2015-06-28 0:39 GMT+06:00 Andy Shevchenko <[email protected]>:
> P.S. I guess you may try to submit first something a bit more trivial
> that this to train your skills in open source community. You already
> have 13 versions of the patch series with some stylistic issues. And
> some of them might be due to you pay not much attention on them. This
> makes your series quite unlikely to be reviewed and applied.

First of all thank you for feedback and for you time. Yes, seems that main
problem in my attention. Will I think and reconsider the changes a
thousand times
before sending it in next time.