Re: [PATCH v6 3/3] x86/earlyprintk: setup earlyprintk as early as possible

[Andy Shevchenko]

On Tue, 2015-05-12 at 22:26 +0600, Alexander Kuleshov wrote:
> 2015-05-12 17:19 GMT+06:00 Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>:
> >> +/* 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@xxxxxxxxx>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/