From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Subject: Re: [PATCH] serial: earlycon: stop abusing console::index Date: Wed, 8 Jun 2016 18:20:52 +0100 Message-ID: <20160608172051.GF13355@leverpostej> References: <1464967173-27744-1-git-send-email-mark.rutland@arm.com> <5758508E.4000206@hurleysoftware.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <5758508E.4000206@hurleysoftware.com> Sender: linux-kernel-owner@vger.kernel.org To: Peter Hurley Cc: linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Jiri Slaby , Leif Lindholm , Rob Herring , Will Deacon , linux-serial@vger.kernel.org List-Id: linux-serial@vger.kernel.org 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 > > Cc: Greg Kroah-Hartman > > Cc: Jiri Slaby > > Cc: Leif Lindholm > > Cc: Peter Hurley > > Cc: Rob Herring > > Cc: Will Deacon > > Cc: linux-serial@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > --- > > 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 > > > > 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", > > >