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 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]>
---
v3. Refactor based on Petr suggestion
v2. Use a different logic to avoid calling match/setup multiple
times as discussed with Petr.
---
kernel/printk/console_cmdline.h | 1 +
kernel/printk/printk.c | 31 +++++++++++++++++++++----------
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
index 11f19c466af5..3ca74ad391d6 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 17602d7b7ffc..5cf47a7b880c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2116,7 +2116,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;
@@ -2131,6 +2131,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;
}
}
@@ -2140,6 +2142,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;
@@ -2194,7 +2197,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;
}
@@ -2215,7 +2218,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;
@@ -2636,7 +2639,7 @@ early_param("keep_bootcon", keep_bootcon_setup);
* Care need to be taken with consoles that are statically
* enabled such as netconsole
*/
-static int try_enable_new_console(struct console *newcon)
+static int try_enable_new_console(struct console *newcon, bool user_specified)
{
struct console_cmdline *c;
int i;
@@ -2644,6 +2647,8 @@ static int try_enable_new_console(struct console *newcon)
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 */
@@ -2674,8 +2679,13 @@ static int try_enable_new_console(struct console *newcon)
/*
* Some consoles, such as pstore and netconsole, can be enabled even
* without matching.
+ *
+ * Note: We only do this test on the !user_specified pass so that such
+ * a statically enabled console that isn't user specified gets a chance
+ * to have its match() or setup() function called on our second pass
+ * through this function.
*/
- if (newcon->flags & CON_ENABLED)
+ if (newcon->flags & CON_ENABLED && !user_specified)
return 0;
return -ENOENT;
@@ -2752,11 +2762,12 @@ void register_console(struct console *newcon)
}
}
- /*
- * See if this console matches one we selected on
- * the command line or if it was statically enabled
- */
- err = try_enable_new_console(newcon);
+ /* See if this console matches one we selected on the command line */
+ err = try_enable_new_console(newcon, true);
+
+ /* If not, try to match against the platform default(s) */
+ if (err == -ENOENT)
+ err = try_enable_new_console(newcon, false);
/* printk() messages are not printed to the Braille console. */
if (err || newcon->flags & CON_BRL)
On Thu 2020-02-06 15:02:25, 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.
>
> This tentative fix 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.
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 17602d7b7ffc..5cf47a7b880c 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2674,8 +2679,13 @@ static int try_enable_new_console(struct console *newcon)
> /*
> * Some consoles, such as pstore and netconsole, can be enabled even
> * without matching.
> + *
> + * Note: We only do this test on the !user_specified pass so that such
> + * a statically enabled console that isn't user specified gets a chance
> + * to have its match() or setup() function called on our second pass
> + * through this function.
I had some troubles to part the comment. I wonder if the following is
more clear:
* Accept pre-enabled consoles only when match() and setup()
* was called.
And I would do the same check as in the for cycle:
if (newcon->flags & CON_ENABLED && c->user_specified == user_specified)
return 0;
With the above change:
Reviewed-by: Petr Mladek <[email protected]>
I could do the change when pushing if you agree and v4 is not needed
for other reasons.
Best Regards,
Petr
PS: JFYI, I am going to look at the 3rd patch tomorrow. I have to go now.
On Tue, 2020-02-11 at 15:41 +0100, Petr Mladek wrote:
>
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 17602d7b7ffc..5cf47a7b880c 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2674,8 +2679,13 @@ static int try_enable_new_console(struct console *newcon)
> > /*
> > * Some consoles, such as pstore and netconsole, can be enabled even
> > * without matching.
> > + *
> > + * Note: We only do this test on the !user_specified pass so that such
> > + * a statically enabled console that isn't user specified gets a chance
> > + * to have its match() or setup() function called on our second pass
> > + * through this function.
>
> I had some troubles to part the comment. I wonder if the following is
> more clear:
>
> * Accept pre-enabled consoles only when match() and setup()
> * was called.
Yeah the sentence is a bit convoluted, I agree. As for the fix, see
below
> And I would do the same check as in the for cycle:
>
> if (newcon->flags & CON_ENABLED && c->user_specified == user_specified)
> return 0;
Fair enough, this is simpler.
> With the above change:
>
> Reviewed-by: Petr Mladek <[email protected]>
>
> I could do the change when pushing if you agree and v4 is not needed
> for other reasons.
Of course. I'm travelling this week (which is why I somewhat rushed
sending you the patches last week, hence the glitches you noted), so I
won't have a chance to repost until sometime next week.
>
> PS: JFYI, I am going to look at the 3rd patch tomorrow. I have to go now.
No worries. It's not super important, it's a minor thing I noticed
while testing (when I artifically make my consoles not match to test
the "default" fallback). It's not directly related to the fix in patch
2, but is completely standlaone.
Cheers,
Ben.
On (20/02/06 15:02), Benjamin Herrenschmidt wrote:
[..]
> 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;
> @@ -2131,6 +2131,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;
> }
> }
> @@ -2140,6 +2142,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;
> @@ -2194,7 +2197,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;
> }
> @@ -2215,7 +2218,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);
> }
A silly question:
Can the same console first be added by
console_setup()->__add_preferred_console(true)
and then by
add_preferred_console()->__add_preferred_console(false)
-ss
On Thu 2020-02-13 14:52:36, Sergey Senozhatsky wrote:
> On (20/02/06 15:02), Benjamin Herrenschmidt wrote:
> [..]
> > 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;
> > @@ -2131,6 +2131,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;
> > }
> > }
> > @@ -2140,6 +2142,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;
> > @@ -2194,7 +2197,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;
> > }
> > @@ -2215,7 +2218,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);
> > }
>
> A silly question:
>
> Can the same console first be added by
> console_setup()->__add_preferred_console(true)
> and then by
> add_preferred_console()->__add_preferred_console(false)
I guess that this might happen. It should be safe because
user_specified flag is set only to true when found again,
see:
if (user_specified)
c->user_specified = true;
Best Regards,
Petr
On (20/02/13 09:39), Petr Mladek wrote:
> > > struct console_cmdline *c;
> > > int i;
> > > @@ -2131,6 +2131,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;
> > > }
> > > }
[..]
> > A silly question:
> >
> > Can the same console first be added by
> > console_setup()->__add_preferred_console(true)
> > and then by
> > add_preferred_console()->__add_preferred_console(false)
>
> I guess that this might happen. It should be safe because
> user_specified flag is set only to true when found again,
> see:
>
> if (user_specified)
> c->user_specified = true;
Yikes, I didn't see the if-condition. Yes, you are right.
-ss