Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933391AbbELR6x (ORCPT ); Tue, 12 May 2015 13:58:53 -0400 Received: from mga03.intel.com ([134.134.136.65]:21453 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932788AbbELR6w (ORCPT ); Tue, 12 May 2015 13:58:52 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,416,1427785200"; d="scan'208";a="492692007" Message-ID: <1431453527.28073.73.camel@linux.intel.com> Subject: Re: [PATCH v6 3/3] x86/earlyprintk: setup earlyprintk as early as possible From: Andy Shevchenko To: Alexander Kuleshov Cc: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , LKML , Greg Kroah-Hartman , Borislav Petkov , Mark Rustad , Yinghai Lu Date: Tue, 12 May 2015 20:58:47 +0300 In-Reply-To: References: <1431418137-32320-1-git-send-email-kuleshovmail@gmail.com> <1431418208-2274-1-git-send-email-kuleshovmail@gmail.com> <1431429553.28073.63.camel@linux.intel.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2777 Lines: 85 On Tue, 2015-05-12 at 22:26 +0600, Alexander Kuleshov wrote: > 2015-05-12 17:19 GMT+06:00 Andy Shevchenko : > >> +/* 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. > > Yes. In other architetures, earlyprintk setup occurs through 'early_param', > as it in the x86 now. So, which means that instead of this proposal (in a hackish way, since it half-working solution) maybe better to reconsider how you may handle early_param? > > > > > 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 > > > > So, the logic of this function seems broken. I don't get why you have to > > check the contents of earlyprintk parameter. > > > > Because for now we can setup only serial console, for other we need ioremap > which is not enabled for this moment. Here we just try to find serial console > and setup it, if another argument passed to the 'earlyprintk', it will > be parsed in the > 'setup_arch'. Even for EFI case? So, you might redesign that to somehow test if the setup_early_printk() is called in early_param() context or even earlier and depending on that do initializations regarding to possibilities, though I have no idea how to this in clean way. Currently you have two places where you check the content of the parameter, which is not okay from my p.o.v. > > >> > >> 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). > > > > I'm not sure that I understand this. Can you please, explain what did > you mean here. It's about style. Just make empty line before setup_builtin_cmdline() instead of doing this in between two setup_ functions. > > Thank you. -- Andy Shevchenko Intel Finland Oy -- 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/