Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754139AbbF0SjR (ORCPT ); Sat, 27 Jun 2015 14:39:17 -0400 Received: from mail-yk0-f173.google.com ([209.85.160.173]:33459 "EHLO mail-yk0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753151AbbF0SjK convert rfc822-to-8bit (ORCPT ); Sat, 27 Jun 2015 14:39:10 -0400 MIME-Version: 1.0 In-Reply-To: <1435412904-16730-1-git-send-email-kuleshovmail@gmail.com> References: <1435412739-15615-1-git-send-email-kuleshovmail@gmail.com> <1435412904-16730-1-git-send-email-kuleshovmail@gmail.com> Date: Sat, 27 Jun 2015 21:39:09 +0300 Message-ID: Subject: Re: [PATCH 3/4 v13] x86/earlyprintk: setup earlyprintk as early as possible From: Andy Shevchenko To: Alexander Kuleshov Cc: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Greg Kroah-Hartman , Andy Shevchenko , Borislav Petkov , David Cohen , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10070 Lines: 271 On Sat, Jun 27, 2015 at 4:48 PM, Alexander Kuleshov 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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/