On Wed, Jun 08, 2016 at 10:06:22AM -0700, Peter Hurley wrote:
> On 06/03/2016 08:19 AM, Mark Rutland wrote:
> > Commit cda64e6824026575 ("serial: earlycon: Fixup earlycon console name
> > and index") added code to decompose an earlycon driver name into a
> > string prefix and numeric suffix, and place this into console::name and
> > console::index, such that we'd get a pretty name out of the core console
> > code, which requires a name and index. This is simply to give a
> > pretty/useful earlycon driver name in the log messages, and other than
> > logging the name and index hold no significance.
>
> The idea was to keep the console_cmdline[] array consistent with the
> registered consoles, even though earlycon uses its own matching scheme.
> Least surprise and all that.
>
> > However, this is broken for drivers such as "pl011", where the "011"
> > suffix gets stripped off, then subsequently restored, printed as a
> > decimal, erroneously giving "pl11" in log messages.
>
> Yeah, I saw that.
>
> > Additionally, for
> > earlycon drivers without a numeric suffix, "0" is added regardless. Thus
> > the code is broken in some cases, and generally inconsistent.
>
> I would like to continue with some kind of naming scheme that preserves
> both the command line parameters (uart,uart8250,pl011,etc.) and uniquely
> identifies which uart driver is the earlycon.
I understand that we wish to know which driver is the earlycon. However,
I would argue that logging that at earlycon_init time (as this patch
does) should be sufficient.
> The current scheme could be fixed easily enough (by just using a single
> digit).
> Or using a separator, ie. uart/0, pl011/0, etc.
That would prevent the mangling issue, certainly.
The only issue I would have with this approach is that as with the
drivers that simply get a '0' appended, I suspect the index may be
confused with the usual index for the device (e.g. pl011/0 may be
assumed to be ttyAMA0, which isn't necessarily the case).
Regardless, it's definitely less confusing than "pl11".
Thanks,
Mark.
>
> Regards,
> Peter Hurley
>
>
> > Instead, this patch changes the earlycon code to consistently register
> > "earlycon0", but ensures that the earlycon driver name is logged at
> > earlycon_init time. This is obvious, consistent, and sufficient to
> > provide the required information to the user.
> >
> > With this in place, amongst the first messages from the kernel, the user
> > will see something like:
> >
> > [ 0.000000] earlycon: earlycon0 (pl011) at MMIO 0x000000007ff80000
> > (options '')
> > [ 0.000000] bootconsole [earlycon0] enabled
> >
> > Signed-off-by: Mark Rutland <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Jiri Slaby <[email protected]>
> > Cc: Leif Lindholm <[email protected]>
> > Cc: Peter Hurley <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > drivers/tty/serial/earlycon.c | 19 ++++---------------
> > 1 file changed, 4 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> > index 067783f..2b6622a 100644
> > --- a/drivers/tty/serial/earlycon.c
> > +++ b/drivers/tty/serial/earlycon.c
> > @@ -29,7 +29,7 @@
> > #include <asm/serial.h>
> >
> > static struct console early_con = {
> > - .name = "uart", /* fixed up at earlycon registration */
> > + .name = "earlycon",
> > .flags = CON_PRINTBUFFER | CON_BOOT,
> > .index = 0,
> > };
> > @@ -60,24 +60,13 @@ static void __init earlycon_init(struct earlycon_device
> > *device,
> > {
> > struct console *earlycon = device->con;
> > struct uart_port *port = &device->port;
> > - const char *s;
> > - size_t len;
> > -
> > - /* scan backwards from end of string for first non-numeral */
> > - for (s = name + strlen(name);
> > - s > name && s[-1] >= '0' && s[-1] <= '9';
> > - s--)
> > - ;
> > - if (*s)
> > - earlycon->index = simple_strtoul(s, NULL, 10);
> > - len = s - name;
> > - strlcpy(earlycon->name, name, min(len + 1, sizeof(earlycon->name)));
> > +
> > earlycon->data = &early_console_dev;
> >
> > if (port->iotype == UPIO_MEM || port->iotype == UPIO_MEM16 ||
> > port->iotype == UPIO_MEM32 || port->iotype == UPIO_MEM32BE)
> > - pr_info("%s%d at MMIO%s %pa (options '%s')\n",
> > - earlycon->name, earlycon->index,
> > + pr_info("%s%d (%s) at MMIO%s %pa (options '%s')\n",
> > + earlycon->name, earlycon->index, name,
> > (port->iotype == UPIO_MEM) ? "" :
> > (port->iotype == UPIO_MEM16) ? "16" :
> > (port->iotype == UPIO_MEM32) ? "32" : "32be",
> >
>
