Messages in this thread | | | Subject | Re: [PATCH v6 3/3] x86/earlyprintk: setup earlyprintk as early as possible | From | Andy Shevchenko <> | Date | Tue, 12 May 2015 20:58:47 +0300 |
| |
On Tue, 2015-05-12 at 22:26 +0600, Alexander Kuleshov wrote: > 2015-05-12 17:19 GMT+06:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > >> +/* 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 <andriy.shevchenko@intel.com> Intel Finland Oy
| |