In the following circumstances, the rule of selecting the console
corresponding to the last "console=" entry on the command line as
the preferred console (CON_CONSDEV, ie, /dev/console) fails. This
is a specific example, but it could happen with different consoles
that have a similar name aliasing mechanism.
- The kernel command line has both console=tty0 and console=ttyS0
in that order (the latter with speed etc... arguments). This is common
with some cloud setups such as Amazon Linux.
- add_preferred_console is called early to register "uart0". In
our case that happens from acpi_parse_spcr() on arm64 since the
"enable_console" argument is true on that architecture. This causes
"uart0" to become entry 0 of the console_cmdline array.
Now, because of the above, what happens is:
- add_preferred_console is called by the cmdline parsing for tty0
and ttyS0 respectively, thus occupying entries 1 and 2 of the
console_cmdline array (since this happens after ACPI SPCR parsing).
At that point preferred_console is set to 2 as expected.
- When the tty layer kicks in, it will call register_console for tty0.
This will match entry 1 in console_cmdline array. It isn't our preferred
console but because it's our only console at this point, it will end up
"first" in the consoles list.
- When 8250 probes the actual serial port later on, it calls
register_console for ttyS0. At that point the loop in register_console
tries to match it with the entries in the console_cmdline array. Ideally
this should match ttyS0 in entry 2, which is preferred, causing it to
be inserted first and to replace tty0 as CONSDEV. However, 8250 provides
a "match" hook in its struct console, and that hook will match "uart"
as an alias to "ttyS". So we match uart0 at entry 0 in the array which
is not the preferred console and will not match entry 2 which is since
we break out of the loop on the first match. As a result, we don't set
CONSDEV and don't insert it first, but second in the console list.
As a result, we end up with tty0 remaining first in the array, and thus
/dev/console going there instead of the last user specified one which
is ttyS0.
This tentative fix changes the loop in register_console to scan first
for consoles specified on the command line, and only if none is found,
to then scan for consoles specified by the architecture.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
v2. Use a different logic to avoid calling match/setup multiple
times as discussed with Petr.
NOTE: This may look convoluted because I'm trying to keep the existing
behaviour identical when it comes to things like Braille selection,
setup failures, on Braille consoles, or setup failures on normal consoles
which all have subtly different results in the current code.
Some of those behaviour are a bit dubious and we might be able to simply
rely on CON_ENABLED and CON_BRL flags in newcon after the search but I
don't want to change those corner cases in this patch.
kernel/printk/console_cmdline.h | 1 +
kernel/printk/printk.c | 105 ++++++++++++++++++++------------
2 files changed, 67 insertions(+), 39 deletions(-)
diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
index 11f19c466af5..621c41802c2f 100644
--- a/kernel/printk/console_cmdline.h
+++ b/kernel/printk/console_cmdline.h
@@ -6,6 +6,7 @@ struct console_cmdline
{
char name[16]; /* Name of the driver */
int index; /* Minor dev. to use */
+ bool user_specified; /* Specified by command line vs. platform */
char *options; /* Options for the driver */
#ifdef CONFIG_A11Y_BRAILLE_CONSOLE
char *brl_options; /* Options for braille driver */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5aa96098c64d..6fc821be0e7f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2047,7 +2047,7 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
#endif
static int __add_preferred_console(char *name, int idx, char *options,
- char *brl_options)
+ char *brl_options, bool user_specified)
{
struct console_cmdline *c;
int i;
@@ -2062,6 +2062,8 @@ static int __add_preferred_console(char *name, int idx, char *options,
if (strcmp(c->name, name) == 0 && c->index == idx) {
if (!brl_options)
preferred_console = i;
+ if (user_specified)
+ c->user_specified = true;
return 0;
}
}
@@ -2071,6 +2073,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
preferred_console = i;
strlcpy(c->name, name, sizeof(c->name));
c->options = options;
+ c->user_specified = user_specified;
braille_set_options(c, brl_options);
c->index = idx;
@@ -2114,7 +2117,7 @@ static int __init console_setup(char *str)
idx = simple_strtoul(s, NULL, 10);
*s = 0;
- __add_preferred_console(buf, idx, options, brl_options);
+ __add_preferred_console(buf, idx, options, brl_options, true);
console_set_on_cmdline = 1;
return 1;
}
@@ -2135,7 +2138,7 @@ __setup("console=", console_setup);
*/
int add_preferred_console(char *name, int idx, char *options)
{
- return __add_preferred_console(name, idx, options, NULL);
+ return __add_preferred_console(name, idx, options, NULL, false);
}
bool console_suspend_enabled = true;
@@ -2542,6 +2545,53 @@ static int __init keep_bootcon_setup(char *str)
early_param("keep_bootcon", keep_bootcon_setup);
+enum con_match {
+ con_matched,
+ con_matched_preferred,
+ con_braille,
+ con_failed,
+ con_no_match,
+};
+
+static enum con_match try_match_new_console(struct console *newcon, bool user_specified)
+{
+ struct console_cmdline *c;
+ int i;
+
+ for (i = 0, c = console_cmdline;
+ i < MAX_CMDLINECONSOLES && c->name[0];
+ i++, c++) {
+ if (c->user_specified != user_specified)
+ continue;
+ if (!newcon->match ||
+ newcon->match(newcon, c->name, c->index, c->options) != 0) {
+ /* default matching */
+ BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
+ if (strcmp(c->name, newcon->name) != 0)
+ continue;
+ if (newcon->index >= 0 &&
+ newcon->index != c->index)
+ continue;
+ if (newcon->index < 0)
+ newcon->index = c->index;
+
+ if (_braille_register_console(newcon, c))
+ return con_braille;
+
+ if (newcon->setup &&
+ newcon->setup(newcon, c->options) != 0)
+ return con_failed;
+ }
+ newcon->flags |= CON_ENABLED;
+ if (i == preferred_console) {
+ newcon->flags |= CON_CONSDEV;
+ return con_matched_preferred;
+ }
+ return con_matched;
+ }
+ return con_no_match;
+}
+
/*
* The console driver calls this routine during kernel initialization
* to register the console printing procedure with printk() and to
@@ -2563,11 +2613,10 @@ early_param("keep_bootcon", keep_bootcon_setup);
*/
void register_console(struct console *newcon)
{
- int i;
unsigned long flags;
struct console *bcon = NULL;
- struct console_cmdline *c;
static bool has_preferred;
+ enum con_match match;
if (console_drivers)
for_each_console(bcon)
@@ -2615,41 +2664,19 @@ void register_console(struct console *newcon)
}
}
- /*
- * See if this console matches one we selected on
- * the command line.
- */
- for (i = 0, c = console_cmdline;
- i < MAX_CMDLINECONSOLES && c->name[0];
- i++, c++) {
- if (!newcon->match ||
- newcon->match(newcon, c->name, c->index, c->options) != 0) {
- /* default matching */
- BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
- if (strcmp(c->name, newcon->name) != 0)
- continue;
- if (newcon->index >= 0 &&
- newcon->index != c->index)
- continue;
- if (newcon->index < 0)
- newcon->index = c->index;
-
- if (_braille_register_console(newcon, c))
- return;
-
- if (newcon->setup &&
- newcon->setup(newcon, c->options) != 0)
- break;
- }
-
- newcon->flags |= CON_ENABLED;
- if (i == preferred_console) {
- newcon->flags |= CON_CONSDEV;
- has_preferred = true;
- }
- break;
- }
+ /* See if this console matches one we selected on the command line */
+ match = try_match_new_console(newcon, true);
+ /* If it didn't, try matching the platform ones */
+ if (match == con_no_match)
+ match = try_match_new_console(newcon, false);
+ /* If we matched a Braille console, bail out */
+ if (match == con_braille)
+ return;
+ /* Check if we found a preferred one */
+ if (match == con_matched_preferred)
+ has_preferred = true;
+ /* If we don't have an enabled console, bail out */
if (!(newcon->flags & CON_ENABLED))
return;
On Mon 2019-12-16 12:08:14, Benjamin Herrenschmidt wrote:
> In the following circumstances, the rule of selecting the console
> corresponding to the last "console=" entry on the command line as
> the preferred console (CON_CONSDEV, ie, /dev/console) fails. This
> is a specific example, but it could happen with different consoles
> that have a similar name aliasing mechanism.
>
> v2. Use a different logic to avoid calling match/setup multiple
> times as discussed with Petr.
>
> NOTE: This may look convoluted because I'm trying to keep the existing
> behaviour identical when it comes to things like Braille selection,
> setup failures, on Braille consoles, or setup failures on normal consoles
> which all have subtly different results in the current code.
>
> Some of those behaviour are a bit dubious and we might be able to simply
> rely on CON_ENABLED and CON_BRL flags in newcon after the search but I
> don't want to change those corner cases in this patch.
Yes, it is dubious. IMHO, the 5 error codes make it even harder to
see what happens in which case.
The code really need simplification. I would prefer to take the risk
and reduce the amount of added conditions as much as possible.
I have an idea, see below.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2542,6 +2545,53 @@ static int __init keep_bootcon_setup(char *str)
>
> early_param("keep_bootcon", keep_bootcon_setup);
>
> +enum con_match {
> + con_matched,
> + con_matched_preferred,
> + con_braille,
> + con_failed,
> + con_no_match,
> +};
Please, replace this with int, where:
+ con_matched -> 0
+ con_matched_preferred -> 0 and make "has_preferred" global variable
+ con_braile -> 0 later check for CON_BRL flag
+ con_failed -> -EFAULT
+ con_no_match -> -ENOENT
> @@ -2615,41 +2664,19 @@ void register_console(struct console *newcon)
> + /* See if this console matches one we selected on the command line */
> + match = try_match_new_console(newcon, true);
> + /* If it didn't, try matching the platform ones */
> + if (match == con_no_match)
> + match = try_match_new_console(newcon, false);
> + /* If we matched a Braille console, bail out */
> + if (match == con_braille)
> + return;
> + /* Check if we found a preferred one */
> + if (match == con_matched_preferred)
> + has_preferred = true;
>
> + /* If we don't have an enabled console, bail out */
> if (!(newcon->flags & CON_ENABLED))
> return;
Some of the comments describe what is obvious. I would simplify
it the following way:
/* Prefer command line over platform specific defaults. */
err = try_match_new_console(newcon, true);
if (err = -ENOENT)
err = try_match_new_console(newcon, false);
/* printk() messages are not printed to Braille consoles. */
if (err || console->flags | CON_BRL)
return;
Finally, please split the change into two patches:
1st patch will "just" introduce try_match_new_console(console) and
use it the following way:
err = try_match_new_console(newcon);
/* printk() messages are not printed to the Braille console. */
if (err || console->flags | CON_BRL)
return;
2nd patch will add the user_specified logic.
This way bisection will distinguish regressions caused
by the refactoring and by the changed search order.
Best Regards,
Petr
PS: I have vacation between December 23 and January 1. I believe
that v3 will be ready for merging. Anyway, I will not push it
into linux-next before I am back from vacation. I would like
to stay off the computer and do not want to eventually break
linux-next for too long.
On Thu, 2019-12-19 at 14:50 +0100, Petr Mladek wrote:
>
> > NOTE: This may look convoluted because I'm trying to keep the existing
> > behaviour identical when it comes to things like Braille selection,
> > setup failures, on Braille consoles, or setup failures on normal consoles
> > which all have subtly different results in the current code.
> >
> > Some of those behaviour are a bit dubious and we might be able to simply
> > rely on CON_ENABLED and CON_BRL flags in newcon after the search but I
> > don't want to change those corner cases in this patch.
>
> Yes, it is dubious. IMHO, the 5 error codes make it even harder to
> see what happens in which case.
Agreed.
> The code really need simplification. I would prefer to take the risk
> and reduce the amount of added conditions as much as possible.
> I have an idea, see below.
I wanted you to say that :-) I'll rework along those lines. Just a nit
or two:
>
>
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2542,6 +2545,53 @@ static int __init keep_bootcon_setup(char *str)
> >
> > early_param("keep_bootcon", keep_bootcon_setup);
> >
> > +enum con_match {
> > + con_matched,
> > + con_matched_preferred,
> > + con_braille,
> > + con_failed,
> > + con_no_match,
> > +};
>
> Please, replace this with int, where:
>
> + con_matched -> 0
> + con_matched_preferred -> 0 and make "has_preferred" global variable
> + con_braile -> 0 later check for CON_BRL flag
> + con_failed -> -EFAULT
> + con_no_match -> -ENOENT
Not fan of using -EFAULT here, it's a detail since it's rather kernel
internal, but I'd rather use -ENXIO for no match and -EIO for failed
(or pass the original error code up if any). That said it's really bike
shed painting at this point :-)
>
> > @@ -2615,41 +2664,19 @@ void register_console(struct console *newcon)
> > + /* See if this console matches one we selected on the command line */
> > + match = try_match_new_console(newcon, true);
> > + /* If it didn't, try matching the platform ones */
> > + if (match == con_no_match)
> > + match = try_match_new_console(newcon, false);
> > + /* If we matched a Braille console, bail out */
> > + if (match == con_braille)
> > + return;
> > + /* Check if we found a preferred one */
> > + if (match == con_matched_preferred)
> > + has_preferred = true;
> >
> > + /* If we don't have an enabled console, bail out */
> > if (!(newcon->flags & CON_ENABLED))
> > return;
>
> Some of the comments describe what is obvious. I would simplify
> it the following way:
>
> /* Prefer command line over platform specific defaults. */
> err = try_match_new_console(newcon, true);
> if (err = -ENOENT)
> err = try_match_new_console(newcon, false);
>
> /* printk() messages are not printed to Braille consoles. */
> if (err || console->flags | CON_BRL)
> return;
So this changes the existing behaviour in one way that may or may not
matter, I don't know:
If setup() fails, the existing code will not exit. That means that if
the console has CON_ENABLED already set (some do set it statically or
set it from outside this function, I haven't looked into details the
various circumstances this can happen), the existing code will still
insert it. Your patch will make us not insert it.
> Finally, please split the change into two patches:
>
> 1st patch will "just" introduce try_match_new_console(console) and
> use it the following way:
>
> err = try_match_new_console(newcon);
>
> /* printk() messages are not printed to the Braille console. */
> if (err || console->flags | CON_BRL)
> return;
>
> 2nd patch will add the user_specified logic.
>
> This way bisection will distinguish regressions caused
> by the refactoring and by the changed search order.
>
> Best Regards,
> Petr
>
> PS: I have vacation between December 23 and January 1. I believe
> that v3 will be ready for merging. Anyway, I will not push it
> into linux-next before I am back from vacation. I would like
> to stay off the computer and do not want to eventually break
> linux-next for too long.
No worries. This isn't super urgent.
Cheers,
Ben.
On Fri 2019-12-20 08:48:24, Benjamin Herrenschmidt wrote:
> On Thu, 2019-12-19 at 14:50 +0100, Petr Mladek wrote:
> > The code really need simplification. I would prefer to take the risk
> > and reduce the amount of added conditions as much as possible.
> > I have an idea, see below.
>
> I wanted you to say that :-) I'll rework along those lines. Just a nit
> or two:
> >
> >
> > > --- a/kernel/printk/printk.c
> > > +++ b/kernel/printk/printk.c
> > > @@ -2542,6 +2545,53 @@ static int __init keep_bootcon_setup(char *str)
> > >
> > > early_param("keep_bootcon", keep_bootcon_setup);
> > >
> > > +enum con_match {
> > > + con_matched,
> > > + con_matched_preferred,
> > > + con_braille,
> > > + con_failed,
> > > + con_no_match,
> > > +};
> >
> > Please, replace this with int, where:
> >
> > + con_matched -> 0
> > + con_matched_preferred -> 0 and make "has_preferred" global variable
> > + con_braile -> 0 later check for CON_BRL flag
> > + con_failed -> -EFAULT
> > + con_no_match -> -ENOENT
>
> Not fan of using -EFAULT here, it's a detail since it's rather kernel
> internal, but I'd rather use -ENXIO for no match and -EIO for failed
> (or pass the original error code up if any). That said it's really bike
> shed painting at this point :-)
Sigh, either variant is somehow confusing.
I think that -ENOENT is a bit better than -EIO. It is abbreviation of
"No entry or No entity" which quite fits here. Also the device might
exist but it is not used when not requested.
I do not mind about -EFAULT vs -EIO. Well, -EIO might actually
better describe the reality.
That said, I do not want to spend much time on bikesheding. Feel free
to use whatever looks better to you.
> > > @@ -2615,41 +2664,19 @@ void register_console(struct console *newcon)
> > /* Prefer command line over platform specific defaults. */
> > err = try_match_new_console(newcon, true);
> > if (err = -ENOENT)
> > err = try_match_new_console(newcon, false);
> >
> > /* printk() messages are not printed to Braille consoles. */
> > if (err || console->flags | CON_BRL)
> > return;
>
> So this changes the existing behaviour in one way that may or may not
> matter, I don't know:
>
> If setup() fails, the existing code will not exit. That means that if
> the console has CON_ENABLED already set (some do set it statically or
> set it from outside this function, I haven't looked into details the
> various circumstances this can happen), the existing code will still
> insert it. Your patch will make us not insert it.
Great catch! There is still a lot to learn about this code.
It seems that, for example, pstore and netconsole are not added
into console_cmdline and do not match. They relly on the explicitely
set CON_ENABLED.
I would prefer to hide this into the new "shorter" function. I would
rename it to try_enable_new_console() and add the following at the end:
/*
* For example, pstore, netconsole, are enabled even
* without matching.
*/
if (console->flags & CON_ENABLED)
return 0;
Best Regards,
Petr
On (19/12/20 10:11), Petr Mladek wrote:
[..]
> > > > +enum con_match {
> > > > + con_matched,
> > > > + con_matched_preferred,
> > > > + con_braille,
> > > > + con_failed,
> > > > + con_no_match,
> > > > +};
> > >
> > > Please, replace this with int, where:
> > >
> > > + con_matched -> 0
> > > + con_matched_preferred -> 0 and make "has_preferred" global variable
> > > + con_braile -> 0 later check for CON_BRL flag
> > > + con_failed -> -EFAULT
> > > + con_no_match -> -ENOENT
> >
> > Not fan of using -EFAULT here, it's a detail since it's rather kernel
> > internal, but I'd rather use -ENXIO for no match and -EIO for failed
> > (or pass the original error code up if any). That said it's really bike
> > shed painting at this point :-)
>
> Sigh, either variant is somehow confusing.
>
> I think that -ENOENT is a bit better than -EIO. It is abbreviation of
> "No entry or No entity" which quite fits here. Also the device might
> exist but it is not used when not requested.
Can we please keep the enum? Enum is super self-descriptive, can't
get any better. Any other alternative - be it -EFAULT or -EIO or
-ENOENT - would force one to always look at what is actually going
on in try_match_new_console() and what particular errno means. None
of those errnos fit, they make things cryptic. IMHO.
-ss
On Mon 2020-01-06 14:15:08, Sergey Senozhatsky wrote:
> On (19/12/20 10:11), Petr Mladek wrote:
> [..]
> > > > > +enum con_match {
> > > > > + con_matched,
> > > > > + con_matched_preferred,
> > > > > + con_braille,
> > > > > + con_failed,
> > > > > + con_no_match,
> > > > > +};
> > > >
> > > > Please, replace this with int, where:
> > > >
> > > > + con_matched -> 0
> > > > + con_matched_preferred -> 0 and make "has_preferred" global variable
> > > > + con_braile -> 0 later check for CON_BRL flag
> > > > + con_failed -> -EFAULT
> > > > + con_no_match -> -ENOENT
> > >
> > > Not fan of using -EFAULT here, it's a detail since it's rather kernel
> > > internal, but I'd rather use -ENXIO for no match and -EIO for failed
> > > (or pass the original error code up if any). That said it's really bike
> > > shed painting at this point :-)
> >
> > Sigh, either variant is somehow confusing.
> >
> > I think that -ENOENT is a bit better than -EIO. It is abbreviation of
> > "No entry or No entity" which quite fits here. Also the device might
> > exist but it is not used when not requested.
>
> Can we please keep the enum? Enum is super self-descriptive, can't
> get any better. Any other alternative - be it -EFAULT or -EIO or
> -ENOENT - would force one to always look at what is actually going
> on in try_match_new_console() and what particular errno means. None
> of those errnos fit, they make things cryptic. IMHO.
I agree that the enums are more self-descriptive. My problem with it is
that there are 5 values. I wanted to check how they were handled
and neither 'con_matched' nor 'con_failed' were later used.
I though how to improve it. And I ended with feeling that the enum
did more harm then good. -E??? codes are a bit less descriptive
but there are only two. The meaning can be explained easily by
a comment above the function.
If you want to keep the enum then please handle the return values
by switch(). Or make it clear that all possible return values
are handled properly.
Best Regards,
Petr
On Mon, 2020-01-06 at 11:25 +0100, Petr Mladek wrote:
>
> I agree that the enums are more self-descriptive. My problem with it is
> that there are 5 values. I wanted to check how they were handled
> and neither 'con_matched' nor 'con_failed' were later used.
>
> I though how to improve it. And I ended with feeling that the enum
> did more harm then good. -E??? codes are a bit less descriptive
> but there are only two. The meaning can be explained easily by
> a comment above the function.
>
> If you want to keep the enum then please handle the return values
> by switch(). Or make it clear that all possible return values
> are handled properly.
Agreed. The enum was originally due to me trying to figure out all the
different cases happening separately from how we react to them. In fact
I'm not even convinced we don't have bugs in the latter (the failure
path of Braille consoles is dubious).
In any case, I'm back now, I'll try to respin this week.
Cheers,
Ben.
On (20/01/06 11:25), Petr Mladek wrote:
> On Mon 2020-01-06 14:15:08, Sergey Senozhatsky wrote:
> > On (19/12/20 10:11), Petr Mladek wrote:
> > [..]
> > > > > > +enum con_match {
> > > > > > + con_matched,
> > > > > > + con_matched_preferred,
> > > > > > + con_braille,
> > > > > > + con_failed,
> > > > > > + con_no_match,
> > > > > > +};
> > > > >
> > > > > Please, replace this with int, where:
> > > > >
> > > > > + con_matched -> 0
> > > > > + con_matched_preferred -> 0 and make "has_preferred" global variable
> > > > > + con_braile -> 0 later check for CON_BRL flag
> > > > > + con_failed -> -EFAULT
> > > > > + con_no_match -> -ENOENT
> > > >
> > > > Not fan of using -EFAULT here, it's a detail since it's rather kernel
> > > > internal, but I'd rather use -ENXIO for no match and -EIO for failed
> > > > (or pass the original error code up if any). That said it's really bike
> > > > shed painting at this point :-)
> > >
> > > Sigh, either variant is somehow confusing.
> > >
> > > I think that -ENOENT is a bit better than -EIO. It is abbreviation of
> > > "No entry or No entity" which quite fits here. Also the device might
> > > exist but it is not used when not requested.
> >
> > Can we please keep the enum? Enum is super self-descriptive, can't
> > get any better. Any other alternative - be it -EFAULT or -EIO or
> > -ENOENT - would force one to always look at what is actually going
> > on in try_match_new_console() and what particular errno means. None
> > of those errnos fit, they make things cryptic. IMHO.
>
> I agree that the enums are more self-descriptive. My problem with it is
> that there are 5 values. I wanted to check how they were handled
> and neither 'con_matched' nor 'con_failed' were later used.
Right, I also saw that not all con_match were used, but I didn't consider
it to be an issue, con_match describes all possible cases (completeness)
but not all of those cases exist in the code.
try_match_new_console() is going to return multiple error codes anyway,
all of which should be handled. Switching to `int' (4 billion possible
values) probably doesn't really help us.
> I though how to improve it. And I ended with feeling that the enum
> did more harm then good. -E??? codes are a bit less descriptive
> but there are only two. The meaning can be explained easily by
> a comment above the function.
I understand it. It's just we don't have appropriate errnos. So instead
of only documenting the logic (because enum is self-documenting), with
errnos we also need to document the return values we check. IMHO.
-ss