On Tue, 2015-05-12 at 22:26 +0600, Alexander Kuleshov wrote:
> 2015-05-12 17:19 GMT+06:00 Andy Shevchenko
> <andriy.shevche...@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.shevche...@intel.com>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
