Re: [PATCH] serial: earlycon: stop abusing console::index
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.00] earlycon: earlycon0 (pl011) at MMIO 0x7ff8
> > (options '')
> > [0.00] 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-ser...@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 = >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 = _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",
> >
>
Re: [PATCH] serial: earlycon: stop abusing console::index
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.00] earlycon: earlycon0 (pl011) at MMIO 0x7ff8
> > (options '')
> > [0.00] 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-ser...@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 = >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 = _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",
> >
>
Re: [PATCH] serial: earlycon: stop abusing console::index
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.
The current scheme could be fixed easily enough (by just using a single digit).
Or using a separator, ie. uart/0, pl011/0, etc.
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.00] earlycon: earlycon0 (pl011) at MMIO 0x7ff8
> (options '')
> [0.00] 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-ser...@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 = >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 = _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",
>
Re: [PATCH] serial: earlycon: stop abusing console::index
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.
The current scheme could be fixed easily enough (by just using a single digit).
Or using a separator, ie. uart/0, pl011/0, etc.
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.00] earlycon: earlycon0 (pl011) at MMIO 0x7ff8
> (options '')
> [0.00] 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-ser...@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 = >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 = _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",
>
[PATCH] serial: earlycon: stop abusing console::index
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.
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. Additionally, for
earlycon drivers without a numeric suffix, "0" is added regardless. Thus
the code is broken in some cases, and generally inconsistent.
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.00] earlycon: earlycon0 (pl011) at MMIO 0x7ff8 (options
'')
[0.00] 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-ser...@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 = >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 = _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",
--
1.9.1
[PATCH] serial: earlycon: stop abusing console::index
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.
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. Additionally, for
earlycon drivers without a numeric suffix, "0" is added regardless. Thus
the code is broken in some cases, and generally inconsistent.
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.00] earlycon: earlycon0 (pl011) at MMIO 0x7ff8 (options
'')
[0.00] 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-ser...@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 = >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 = _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",
--
1.9.1
