Re: [PATCH] serial: earlycon: stop abusing console::index

[Mark Rutland]

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 <mark.rutland@xxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Jiri Slaby <jslaby@xxxxxxxx>
> > Cc: Leif Lindholm <leif.lindholm@xxxxxxx>
> > Cc: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
> > Cc: Rob Herring <robh@xxxxxxxxxx>
> > Cc: Will Deacon <will.deacon@xxxxxxx>
> > Cc: linux-serial@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > ---
> >  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",
> > 
>