[PATCH v2 0/3] printk: fix double printing with earlycon

[PATCH v2 0/3] printk: fix double printing with earlycon

[Aleksey Makarov]

If a console was specified by ACPI SPCR table _and_ command line parameters like
"console=ttyAMA0" _and_ "earlycon" were specified, then log messages
appear twice.

This issue was addressed in the patch [1] but the approach was wrong and
a revert [2] was suggested.

First two patches "printk: fix name/type/scope of preferred_console var" and
"printk: rename selected_console -> preferred_console" were sent sent some
time ago as one patch "printk: fix name and type of some variables" [3].
They fix name/type/scope of some variables without changing the logic.

The real fix is in the second patch.  The root cause is that the code traverses
the list of specified consoles (the `console_cmdline` array) and stops at the
first match.  But it may happen that the same console is referred by
the elements of this array twice:

	pl011,mmio,0x87e024000000,115200 -- from SPCR
	ttyAMA0 -- from command line

but in this case `preferred_console` points to the second entry and
the flag CON_CONSDEV is not set, so bootconsole is not deregistered.

To fix that, match the console against the `console_cmdline` entry
pointed by `preferred_console` instead of the first match.

v2:
- split the patch that renames `selected_console` and `preferred_console`
  into two patches (Steven Rostedt)
- add a comment explaining why we need a separate match to check for
  preferred_console (Steven Rostedt)
- v1 of this patchset changed the logic of console initialization a bit.
  That could lead to bugs/incompatibilities.  Use the exactly the same
  logic as in the original code.

v1:
https://lkml.kernel.org/r/20170301161347.4202-1-aleksey.makarov@linaro.org

[1] https://lkml.kernel.org/r/1485963998-921-1-git-send-email-sudeep.holla@arm.com
    commit aea9a80ba98a ("tty: serial: pl011: add ttyAMA for matching pl011 console")
[2] https://lkml.kernel.org/r/20170301152304.29635-1-aleksey.makarov@linaro.org
[3] https://lkml.kernel.org/r/1455299022-11641-2-git-send-email-aleksey.makarov@linaro.org

Aleksey Makarov (3):
  printk: fix name/type/scope of preferred_console var
  printk: rename selected_console -> preferred_console
  printk: fix double printing with earlycon

 kernel/printk/printk.c | 63 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 22 deletions(-)

-- 
2.11.1

[PATCH v2 1/3] printk: fix name/type/scope of preferred_console var

[Aleksey Makarov]

The variable preferred_console is used only inside register_console()
and its semantics is boolean.  It is negative when no console has been
made preferred.

Make it static bool and rename to has_preferred.

Renaming was suggested by Peter Hurley

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 kernel/printk/printk.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 34da86e73d00..3c2234f21291 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -268,7 +268,6 @@ static struct console *exclusive_console;
 static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
 
 static int selected_console = -1;
-static int preferred_console = -1;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
@@ -2406,6 +2405,7 @@ void register_console(struct console *newcon)
 	unsigned long flags;
 	struct console *bcon = NULL;
 	struct console_cmdline *c;
+	static bool has_preferred;
 
 	if (console_drivers)
 		for_each_console(bcon)
@@ -2432,15 +2432,15 @@ void register_console(struct console *newcon)
 	if (console_drivers && console_drivers->flags & CON_BOOT)
 		bcon = console_drivers;
 
-	if (preferred_console < 0 || bcon || !console_drivers)
-		preferred_console = selected_console;
+	if (!has_preferred || bcon || !console_drivers)
+		has_preferred = selected_console >= 0;
 
 	/*
 	 *	See if we want to use this console driver. If we
 	 *	didn't select a console we take the first one
 	 *	that registers here.
 	 */
-	if (preferred_console < 0) {
+	if (!has_preferred) {
 		if (newcon->index < 0)
 			newcon->index = 0;
 		if (newcon->setup == NULL ||
@@ -2448,7 +2448,7 @@ void register_console(struct console *newcon)
 			newcon->flags |= CON_ENABLED;
 			if (newcon->device) {
 				newcon->flags |= CON_CONSDEV;
-				preferred_console = 0;
+				has_preferred = true;
 			}
 		}
 	}
@@ -2483,7 +2483,7 @@ void register_console(struct console *newcon)
 		newcon->flags |= CON_ENABLED;
 		if (i == selected_console) {
 			newcon->flags |= CON_CONSDEV;
-			preferred_console = selected_console;
+			has_preferred = true;
 		}
 		break;
 	}
-- 
2.11.1

Re: [PATCH v2 1/3] printk: fix name/type/scope of preferred_console var

[Steven Rostedt]

On Thu,  2 Mar 2017 16:11:32 +0300
Aleksey Makarov <aleksey.makarov@linaro.org> wrote:

> The variable preferred_console is used only inside register_console()
> and its semantics is boolean.  It is negative when no console has been
> made preferred.
> 
> Make it static bool and rename to has_preferred.
> 
> Renaming was suggested by Peter Hurley
> 

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> ---

Re: [PATCH v2 1/3] printk: fix name/type/scope of preferred_console var

[Sergey Senozhatsky]

On (03/02/17 09:49), Steven Rostedt wrote:
> > The variable preferred_console is used only inside register_console()
> > and its semantics is boolean.  It is negative when no console has been
> > made preferred.
> > 
> > Make it static bool and rename to has_preferred.
> > 
> > Renaming was suggested by Peter Hurley
> > 
> 
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v2 1/3] printk: fix name/type/scope of preferred_console var

[Petr Mladek]

On Thu 2017-03-02 16:11:32, Aleksey Makarov wrote:
> The variable preferred_console is used only inside register_console()
> and its semantics is boolean.  It is negative when no console has been
> made preferred.
> 
> Make it static bool and rename to has_preferred.
> 
> Renaming was suggested by Peter Hurley
> 
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>

I am sorry for the late reply. I was sick.

The change looks fine to me.

Acked-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH v2 2/3] printk: rename selected_console -> preferred_console

[Aleksey Makarov]

The variable selected_console is set in __add_preferred_console()
to point to the last console parameter that was added to the
console_cmdline array.

Rename it to preferred_console so that the name reflects the usage.

Suggested-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 kernel/printk/printk.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 3c2234f21291..ed2a9b31f214 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -267,7 +267,7 @@ static struct console *exclusive_console;
 
 static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
 
-static int selected_console = -1;
+static int preferred_console = -1;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
@@ -1907,14 +1907,14 @@ static int __add_preferred_console(char *name, int idx, char *options,
 	     i++, c++) {
 		if (strcmp(c->name, name) == 0 && c->index == idx) {
 			if (!brl_options)
-				selected_console = i;
+				preferred_console = i;
 			return 0;
 		}
 	}
 	if (i == MAX_CMDLINECONSOLES)
 		return -E2BIG;
 	if (!brl_options)
-		selected_console = i;
+		preferred_console = i;
 	strlcpy(c->name, name, sizeof(c->name));
 	c->options = options;
 	braille_set_options(c, brl_options);
@@ -2433,7 +2433,7 @@ void register_console(struct console *newcon)
 		bcon = console_drivers;
 
 	if (!has_preferred || bcon || !console_drivers)
-		has_preferred = selected_console >= 0;
+		has_preferred = preferred_console >= 0;
 
 	/*
 	 *	See if we want to use this console driver. If we
@@ -2481,7 +2481,7 @@ void register_console(struct console *newcon)
 		}
 
 		newcon->flags |= CON_ENABLED;
-		if (i == selected_console) {
+		if (i == preferred_console) {
 			newcon->flags |= CON_CONSDEV;
 			has_preferred = true;
 		}
-- 
2.11.1

Re: [PATCH v2 2/3] printk: rename selected_console -> preferred_console

[Steven Rostedt]

On Thu,  2 Mar 2017 16:11:33 +0300
Aleksey Makarov <aleksey.makarov@linaro.org> wrote:

> The variable selected_console is set in __add_preferred_console()
> to point to the last console parameter that was added to the
> console_cmdline array.
> 
> Rename it to preferred_console so that the name reflects the usage.

As I said previously, I prefer "selected_console" but since
"__add_preferred_console()" sets it, I'm fine with this change.
Although, I would probably have changed that function to
"__add_selected_console()" :)

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> 
> Suggested-by: Peter Hurley <peter@hurleysoftware.com>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>

Re: [PATCH v2 2/3] printk: rename selected_console -> preferred_console

[Sergey Senozhatsky]

On (03/02/17 10:01), Steven Rostedt wrote:
> > The variable selected_console is set in __add_preferred_console()
> > to point to the last console parameter that was added to the
> > console_cmdline array.
> > 
> > Rename it to preferred_console so that the name reflects the usage.
> 
> As I said previously, I prefer "selected_console" but since
> "__add_preferred_console()" sets it, I'm fine with this change.
> Although, I would probably have changed that function to
> "__add_selected_console()" :)
> 
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

agree, I'd *probably* do a function rename. just 3 lines of code to
touch (afaics).

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v2 2/3] printk: rename selected_console -> preferred_console

[Petr Mladek]

On Thu 2017-03-02 10:01:53, Steven Rostedt wrote:
> On Thu,  2 Mar 2017 16:11:33 +0300
> Aleksey Makarov <aleksey.makarov@linaro.org> wrote:
> 
> > The variable selected_console is set in __add_preferred_console()
> > to point to the last console parameter that was added to the
> > console_cmdline array.
> > 
> > Rename it to preferred_console so that the name reflects the usage.
> 
> As I said previously, I prefer "selected_console" but since
> "__add_preferred_console()" sets it, I'm fine with this change.
> Although, I would probably have changed that function to
> "__add_selected_console()" :)

If I get it correctly, the selected_console/preferred_console
value is used to keep the console first in the console_drivers list.
IMHO, the main effect is that each line will first appear on this
console, see call_console_drivers(). But the message will still
appear also on all other enabled consoles. From this point,
the name "preferred" sounds better to me. More consoles
are selected (enabled) and only one is preferred (first).

Well, I am not a native speaker and might be wrong. Also
it is possible that I missed something.

Anyway, the change looks fine to me.

Acked-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH v2 3/3] printk: fix double printing with earlycon

[Aleksey Makarov]

If a console was specified by ACPI SPCR table _and_ command line
parameters like "console=ttyAMA0" _and_ "earlycon" were specified,
then log messages appear twice.

The root cause is that the code traverses the list of specified
consoles (the `console_cmdline` array) and stops at the first match.
But it may happen that the same console is referred by the elements
of this array twice:

	pl011,mmio,0x87e024000000,115200 -- from SPCR
	ttyAMA0 -- from command line

but in this case `preferred_console` points to the second entry and
the flag CON_CONSDEV is not set, so bootconsole is not deregistered.

To fix that, match the console against the `console_cmdline` entry
pointed by `preferred_console` instead of the first match.

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 kernel/printk/printk.c | 49 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ed2a9b31f214..ad35011f8374 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2380,6 +2380,24 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+static int match_console(struct console *newcon, struct console_cmdline *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)
+			return -ENODEV;
+		if (newcon->index >= 0 &&
+		    newcon->index != c->index)
+			return -ENODEV;
+		if (newcon->index < 0)
+			newcon->index = c->index;
+	}
+
+	return 0;
+}
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -2460,17 +2478,9 @@ void register_console(struct console *newcon)
 	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 (match_console(newcon, c)) {
+			continue;
+		} else {
 
 			if (_braille_register_console(newcon, c))
 				return;
@@ -2481,10 +2491,6 @@ void register_console(struct console *newcon)
 		}
 
 		newcon->flags |= CON_ENABLED;
-		if (i == preferred_console) {
-			newcon->flags |= CON_CONSDEV;
-			has_preferred = true;
-		}
 		break;
 	}
 
@@ -2492,6 +2498,19 @@ void register_console(struct console *newcon)
 		return;
 
 	/*
+	 * Check if this console was set as preferred by command line parameters
+	 * or by call to add_preferred_console().
+	 * There may be several entries in the console_cmdline array referring
+	 * to the same console so we can not just use the first match.  Instead
+	 * check the entry pointed by preferred_console explicitly.
+	 */
+	if (preferred_console >= 0 &&
+	    match_console(newcon, console_cmdline + preferred_console) == 0) {
+		newcon->flags |= CON_CONSDEV;
+		has_preferred = true;
+	}
+
+	/*
 	 * If we have a bootconsole, and are switching to a real console,
 	 * don't print everything out again, since when the boot console, and
 	 * the real console are the same physical device, it's annoying to
-- 
2.11.1

Re: [PATCH v2 3/3] printk: fix double printing with earlycon

[Aleksey Makarov]

On 03/02/2017 04:11 PM, Aleksey Makarov wrote:
> If a console was specified by ACPI SPCR table _and_ command line
> parameters like "console=ttyAMA0" _and_ "earlycon" were specified,
> then log messages appear twice.
> 
> The root cause is that the code traverses the list of specified
> consoles (the `console_cmdline` array) and stops at the first match.
> But it may happen that the same console is referred by the elements
> of this array twice:
> 
> 	pl011,mmio,0x87e024000000,115200 -- from SPCR
> 	ttyAMA0 -- from command line
> 
> but in this case `preferred_console` points to the second entry and
> the flag CON_CONSDEV is not set, so bootconsole is not deregistered.
> 
> To fix that, match the console against the `console_cmdline` entry
> pointed by `preferred_console` instead of the first match.
> 
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> ---
>  kernel/printk/printk.c | 49 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ed2a9b31f214..ad35011f8374 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2380,6 +2380,24 @@ static int __init keep_bootcon_setup(char *str)
>  
>  early_param("keep_bootcon", keep_bootcon_setup);
>  
> +static int match_console(struct console *newcon, struct console_cmdline *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)
> +			return -ENODEV;
> +		if (newcon->index >= 0 &&
> +		    newcon->index != c->index)
> +			return -ENODEV;
> +		if (newcon->index < 0)
> +			newcon->index = c->index;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * The console driver calls this routine during kernel initialization
>   * to register the console printing procedure with printk() and to
> @@ -2460,17 +2478,9 @@ void register_console(struct console *newcon)
>  	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 (match_console(newcon, c)) {
> +			continue;
> +		} else {

My bad.  The first version of this patch was closer to the original logic,
despite this patch looks simpler.

The problem is that newcon->setup() is called twice, firstly from
newcon->match(), then explicitly.  And it could be called third time later 
in another call to newcon->match().

The correct sequence is 1) try to match against preferred_console,
2) if that fails check other entries of console_cmdline.  That would require
some work so I will send a correction tomorrow.

All the best
Aleksey Makarov

[PATCH v3 3/3] printk: fix double printing with earlycon

[Aleksey Makarov]

If a console was specified by ACPI SPCR table _and_ command line
parameters like "console=ttyAMA0" _and_ "earlycon" were specified,
then log messages appear twice.

The root cause is that the code traverses the list of specified
consoles (the `console_cmdline` array) and stops at the first match.
But it may happen that the same console is referred by the elements
of this array twice:

	pl011,mmio,0x87e024000000,115200 -- from SPCR
	ttyAMA0 -- from command line

but in this case `preferred_console` points to the second entry and
the flag CON_CONSDEV is not set, so bootconsole is not deregistered.

To fix that, match the console against the `console_cmdline` entry
pointed by `preferred_console` and if that fails, match against other
entries.  The problem is that newcon->match() is not just a match
function, it calls setup() so it has side effects.

Reported-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---

v2 -> v3:

v2 still changes the logic of the code and calls newcon->match()
several times.  V3 fixes that.  It initially matches  the console
against the preferred_console entry, and then if that fails, against
other entries.

 kernel/printk/printk.c | 82 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 59 insertions(+), 23 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ed2a9b31f214..99bea4ae8f4a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2380,6 +2380,32 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+static enum { CONSOLE_MATCH, CONSOLE_MATCH_RETURN, CONSOLE_MATCH_NEXT }
+match_console(struct console *newcon, struct console_cmdline *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)
+			return CONSOLE_MATCH_NEXT;
+		if (newcon->index >= 0 && newcon->index != c->index)
+			return CONSOLE_MATCH_NEXT;
+		if (newcon->index < 0)
+			newcon->index = c->index;
+
+		if (_braille_register_console(newcon, c))
+			return CONSOLE_MATCH_RETURN;
+
+		if (newcon->setup &&
+		    newcon->setup(newcon, c->options) != 0)
+			return CONSOLE_MATCH;
+	}
+
+	newcon->flags |= CON_ENABLED;
+	return CONSOLE_MATCH;
+}
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -2454,40 +2480,50 @@ void register_console(struct console *newcon)
 	}
 
 	/*
+	 * Check if this console was set as preferred by command line parameters
+	 * or by call to add_preferred_console().  There may be several entries
+	 * in the console_cmdline array matching with the same console so we
+	 * can not just use the first match.  Instead check the entry pointed
+	 * by preferred_console and then all other entries.
+	 */
+	if (preferred_console >= 0) {
+		switch (match_console(newcon,
+				      console_cmdline + preferred_console)) {
+		case CONSOLE_MATCH:
+			if (newcon->flags | CON_ENABLED) {
+				newcon->flags |= CON_CONSDEV;
+				has_preferred = true;
+			}
+			goto match;
+		case CONSOLE_MATCH_RETURN:
+			return;
+		default:
+			break;
+		}
+	}
+
+	/*
 	 *	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;
-		}
+		if (preferred_console == i)
+			continue;
 
-		newcon->flags |= CON_ENABLED;
-		if (i == preferred_console) {
-			newcon->flags |= CON_CONSDEV;
-			has_preferred = true;
+		switch (match_console(newcon, c)) {
+		case CONSOLE_MATCH:
+			goto match;
+		case CONSOLE_MATCH_RETURN:
+			return;
+		default:
+			break;
 		}
-		break;
 	}
 
+match:
 	if (!(newcon->flags & CON_ENABLED))
 		return;
 
-- 
2.11.1

Re: [PATCH v3 3/3] printk: fix double printing with earlycon

[Sergey Senozhatsky]

On (03/03/17 18:49), Aleksey Makarov wrote:
[..]
> +static enum { CONSOLE_MATCH, CONSOLE_MATCH_RETURN, CONSOLE_MATCH_NEXT }
> +match_console(struct console *newcon, struct console_cmdline *c)

that enum in function return is interesting :)
can we make it less hackish?

> +	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)
> +			return CONSOLE_MATCH_NEXT;
> +		if (newcon->index >= 0 && newcon->index != c->index)
> +			return CONSOLE_MATCH_NEXT;

who is checking CONSOLE_MATCH_NEXT?

> +		if (newcon->index < 0)
> +			newcon->index = c->index;
> +
> +		if (_braille_register_console(newcon, c))
> +			return CONSOLE_MATCH_RETURN;
> +
> +		if (newcon->setup &&
> +		    newcon->setup(newcon, c->options) != 0)
> +			return CONSOLE_MATCH;
> +	}
> +
> +	newcon->flags |= CON_ENABLED;
> +	return CONSOLE_MATCH;
> +}
> +
>  /*
>   * The console driver calls this routine during kernel initialization
>   * to register the console printing procedure with printk() and to
> @@ -2454,40 +2480,50 @@ void register_console(struct console *newcon)
>  	}
>  
>  	/*
> +	 * Check if this console was set as preferred by command line parameters
> +	 * or by call to add_preferred_console().  There may be several entries
> +	 * in the console_cmdline array matching with the same console so we
> +	 * can not just use the first match.  Instead check the entry pointed
> +	 * by preferred_console and then all other entries.
> +	 */
> +	if (preferred_console >= 0) {
> +		switch (match_console(newcon,
> +				      console_cmdline + preferred_console)) {
> +		case CONSOLE_MATCH:
> +			if (newcon->flags | CON_ENABLED) {

			newcon->flags & CON_ENABLED  ?

> +				newcon->flags |= CON_CONSDEV;
> +				has_preferred = true;
> +			}
> +			goto match;
> +		case CONSOLE_MATCH_RETURN:
> +			return;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	/*
>  	 *	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;
> -		}
> +		if (preferred_console == i)
> +			continue;
>  
> -		newcon->flags |= CON_ENABLED;
> -		if (i == preferred_console) {
> -			newcon->flags |= CON_CONSDEV;
> -			has_preferred = true;
> +		switch (match_console(newcon, c)) {
> +		case CONSOLE_MATCH:
> +			goto match;
> +		case CONSOLE_MATCH_RETURN:
> +			return;
> +		default:
> +			break;

sorry, it was a rather long for me today. need to look more at this.
for what is now CONSOLE_MATCH_NEXT we used to have continue, and now
we break out of the loop?

	-ss

Re: [PATCH v3 3/3] printk: fix double printing with earlycon

[Aleksey Makarov]

On 03/06/2017 03:59 PM, Sergey Senozhatsky wrote:
> On (03/03/17 18:49), Aleksey Makarov wrote:
> [..]
>> +static enum { CONSOLE_MATCH, CONSOLE_MATCH_RETURN, CONSOLE_MATCH_NEXT }
>> +match_console(struct console *newcon, struct console_cmdline *c)
>
> that enum in function return is interesting :)
> can we make it less hackish?

We probably can, but I can not figure out how to do that.
Suggestions will be appreciated.
We should signal 3 different outcomes.
I thought that using standard errnos is not quite desciptive.

>> +	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)
>> +			return CONSOLE_MATCH_NEXT;
>> +		if (newcon->index >= 0 && newcon->index != c->index)
>> +			return CONSOLE_MATCH_NEXT;
>
> who is checking CONSOLE_MATCH_NEXT?

The caller checks two other values,  this one goes under the default case.
Should I check it explicitly?

>> +		if (newcon->index < 0)
>> +			newcon->index = c->index;
>> +
>> +		if (_braille_register_console(newcon, c))
>> +			return CONSOLE_MATCH_RETURN;
>> +
>> +		if (newcon->setup &&
>> +		    newcon->setup(newcon, c->options) != 0)
>> +			return CONSOLE_MATCH;
>> +	}
>> +
>> +	newcon->flags |= CON_ENABLED;
>> +	return CONSOLE_MATCH;
>> +}
>> +
>>  /*
>>   * The console driver calls this routine during kernel initialization
>>   * to register the console printing procedure with printk() and to
>> @@ -2454,40 +2480,50 @@ void register_console(struct console *newcon)
>>  	}
>>
>>  	/*
>> +	 * Check if this console was set as preferred by command line parameters
>> +	 * or by call to add_preferred_console().  There may be several entries
>> +	 * in the console_cmdline array matching with the same console so we
>> +	 * can not just use the first match.  Instead check the entry pointed
>> +	 * by preferred_console and then all other entries.
>> +	 */
>> +	if (preferred_console >= 0) {
>> +		switch (match_console(newcon,
>> +				      console_cmdline + preferred_console)) {
>> +		case CONSOLE_MATCH:
>> +			if (newcon->flags | CON_ENABLED) {
>
> 			newcon->flags & CON_ENABLED  ?

Sure.  Thank you.

>> +				newcon->flags |= CON_CONSDEV;
>> +				has_preferred = true;
>> +			}
>> +			goto match;
>> +		case CONSOLE_MATCH_RETURN:
>> +			return;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
>> +	/*
>>  	 *	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;
>> -		}
>> +		if (preferred_console == i)
>> +			continue;
>>
>> -		newcon->flags |= CON_ENABLED;
>> -		if (i == preferred_console) {
>> -			newcon->flags |= CON_CONSDEV;
>> -			has_preferred = true;
>> +		switch (match_console(newcon, c)) {
>> +		case CONSOLE_MATCH:
>> +			goto match;
>> +		case CONSOLE_MATCH_RETURN:
>> +			return;
>> +		default:
>> +			break;
>
> sorry, it was a rather long for me today. need to look more at this.
> for what is now CONSOLE_MATCH_NEXT we used to have continue,

CONSOLE_MATCH is for the case when the console matches against the description,
CONSOLE_MATCH_NEXT - it does not, we should try next,
CONSOLE_MATCH_RETURN - we should return as the braille console is registered now.

> and now we break out of the loop?

`goto match` or just when we have seen all the entries

Thank you
Aleksey Makarov

>
> 	-ss
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 3/3] printk: fix double printing with earlycon

[Sergey Senozhatsky]

Hello,

sorry for the delay.

On (03/07/17 15:54), Aleksey Makarov wrote:
> On 03/06/2017 03:59 PM, Sergey Senozhatsky wrote:
> > On (03/03/17 18:49), Aleksey Makarov wrote:
> > [..]
> > > +static enum { CONSOLE_MATCH, CONSOLE_MATCH_RETURN, CONSOLE_MATCH_NEXT }
> > > +match_console(struct console *newcon, struct console_cmdline *c)
> > 
> > that enum in function return is interesting :)
> > can we make it less hackish?
> We probably can, but I can not figure out how to do that.
> Suggestions will be appreciated.
> We should signal 3 different outcomes.
> I thought that using standard errnos is not quite desciptive.

no problems with the enum on its own. errnos probably can also do
the trick.

the way it's defined, however, is a bit unusual and may be
inconvenient - we can add, say, 5 more CONSOLE_MATCH_FOO someday
in the future and match_console() function definition thus will be:

static enum { CONSOLE_MATCH, CONSOLE_MATCH_RETURN, CONSOLE_MATCH_NEXT,
		CONSOLE_MATCH_FOO1, CONSOLE_MATCH_FOO2,
		CONSOLE_MATCH_FOO3, CONSOLE_MATCH_FOO4,
		CONSOLE_MATCH_FOO5}
match_console(struct console *newcon, struct console_cmdline *c)
{
	...
}

or something like this

static enum { CONSOLE_MATCH,
	CONSOLE_MATCH_RETURN,
	CONSOLE_MATCH_NEXT,
	CONSOLE_MATCH_FOO1,
	CONSOLE_MATCH_FOO2,
	CONSOLE_MATCH_FOO3,
	CONSOLE_MATCH_FOO4,
	CONSOLE_MATCH_FOO5 }
match_console(struct console *newcon, struct console_cmdline *c)
{
	..
}

or anything else. which is, to my admittedly imperfect taste, slightly
"unpretty".

[..]
> > > +	/*
> > >  	 *	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;
> > > -		}
> > > +		if (preferred_console == i)
> > > +			continue;
> > > 
> > > -		newcon->flags |= CON_ENABLED;
> > > -		if (i == preferred_console) {
> > > -			newcon->flags |= CON_CONSDEV;
> > > -			has_preferred = true;
> > > +		switch (match_console(newcon, c)) {
> > > +		case CONSOLE_MATCH:
> > > +			goto match;
> > > +		case CONSOLE_MATCH_RETURN:
> > > +			return;
> > > +		default:
> > > +			break;
> > 
> > sorry, it was a rather long for me today. need to look more at this.
> > for what is now CONSOLE_MATCH_NEXT we used to have continue,
> 
> CONSOLE_MATCH is for the case when the console matches against the description,
> CONSOLE_MATCH_NEXT - it does not, we should try next,

my bad, sorry. I misread the patch: there was another `break' right after
that switch, that you have removed; and I just wrongly concluded that
CONSOLE_MATCH_NEXT would now 'break' from 'default' label *and* `break'
from the console_cmdline loop right after it.

bikeshedding:
may be explicit CONSOLE_MATCH_NEXT test will save us from problems (in
case if match_console() will return more codes someday), may be it won't.
hard to say. 'default: continue' is probably OK. or may be can do without
that 'match' label at all. something like this (_may be_)

	for (i = 0, c = console_cmdline; ... ) {
		if (preferred_console == i)
			continue;

		match = match_console(newcon, c);
		if (match == CONSOLE_MATCH_NEXT)
			continue;
		if (match == CONSOLE_MATCH_FOUND)
			break;
		if (match == CONSOLE_MATCH_STOP)
			return;
	}
	...



CONSOLE_MATCH_RETURN  -  basically means that we should stop matching.
can we thus rename it to CONSOLE_MATCH_STOP, or similar?

	match_console() returned CONSOLE_MATCH_STOP

is a bit better than

	match_console() returned CONSOLE_MATCH_RETURN.

isn't it? :)


// I also used CONSOLE_MATCH_FOUND in the example above instead of
// CONSOLE_MATCH. not insisting that CONSOLE_MATCH_FOUND is much
// better than CONSOLE_MATCH though.

	-ss

Re: [PATCH v3 3/3] printk: fix double printing with earlycon

[Aleksey Makarov]

On 03/08/2017 06:33 AM, Sergey Senozhatsky wrote:
> Hello,
>
> sorry for the delay.
>
> On (03/07/17 15:54), Aleksey Makarov wrote:
>> On 03/06/2017 03:59 PM, Sergey Senozhatsky wrote:
>>> On (03/03/17 18:49), Aleksey Makarov wrote:
>>> [..]
>>>> +static enum { CONSOLE_MATCH, CONSOLE_MATCH_RETURN, CONSOLE_MATCH_NEXT }
>>>> +match_console(struct console *newcon, struct console_cmdline *c)
>>>
>>> that enum in function return is interesting :)
>>> can we make it less hackish?
>> We probably can, but I can not figure out how to do that.
>> Suggestions will be appreciated.
>> We should signal 3 different outcomes.
>> I thought that using standard errnos is not quite desciptive.
>
> no problems with the enum on its own. errnos probably can also do
> the trick.
>
> the way it's defined, however, is a bit unusual and may be
> inconvenient - we can add, say, 5 more CONSOLE_MATCH_FOO someday
> in the future and match_console() function definition thus will be:
>
> static enum { CONSOLE_MATCH, CONSOLE_MATCH_RETURN, CONSOLE_MATCH_NEXT,
> 		CONSOLE_MATCH_FOO1, CONSOLE_MATCH_FOO2,
> 		CONSOLE_MATCH_FOO3, CONSOLE_MATCH_FOO4,
> 		CONSOLE_MATCH_FOO5}
> match_console(struct console *newcon, struct console_cmdline *c)
> {
> 	...
> }
>
> or something like this
>
> static enum { CONSOLE_MATCH,
> 	CONSOLE_MATCH_RETURN,
> 	CONSOLE_MATCH_NEXT,
> 	CONSOLE_MATCH_FOO1,
> 	CONSOLE_MATCH_FOO2,
> 	CONSOLE_MATCH_FOO3,
> 	CONSOLE_MATCH_FOO4,
> 	CONSOLE_MATCH_FOO5 }
> match_console(struct console *newcon, struct console_cmdline *c)
> {
> 	..
> }
>
> or anything else. which is, to my admittedly imperfect taste, slightly
> "unpretty".

I agree that this enum thing does not look good and I have an idea how to
get rid of it completely.  The idea is to factor out the braille
code to a separate pass.  That way the match function can return a boolean
value.    

I am traveling now so I will need some time to
send a new version of this patch.

Thank you
Aleksey Makarov

>
> [..]
>>>> +	/*
>>>>  	 *	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;
>>>> -		}
>>>> +		if (preferred_console == i)
>>>> +			continue;
>>>>
>>>> -		newcon->flags |= CON_ENABLED;
>>>> -		if (i == preferred_console) {
>>>> -			newcon->flags |= CON_CONSDEV;
>>>> -			has_preferred = true;
>>>> +		switch (match_console(newcon, c)) {
>>>> +		case CONSOLE_MATCH:
>>>> +			goto match;
>>>> +		case CONSOLE_MATCH_RETURN:
>>>> +			return;
>>>> +		default:
>>>> +			break;
>>>
>>> sorry, it was a rather long for me today. need to look more at this.
>>> for what is now CONSOLE_MATCH_NEXT we used to have continue,
>>
>> CONSOLE_MATCH is for the case when the console matches against the description,
>> CONSOLE_MATCH_NEXT - it does not, we should try next,
>
> my bad, sorry. I misread the patch: there was another `break' right after
> that switch, that you have removed; and I just wrongly concluded that
> CONSOLE_MATCH_NEXT would now 'break' from 'default' label *and* `break'
> from the console_cmdline loop right after it.
>
> bikeshedding:
> may be explicit CONSOLE_MATCH_NEXT test will save us from problems (in
> case if match_console() will return more codes someday), may be it won't.
> hard to say. 'default: continue' is probably OK. or may be can do without
> that 'match' label at all. something like this (_may be_)
>
> 	for (i = 0, c = console_cmdline; ... ) {
> 		if (preferred_console == i)
> 			continue;
>
> 		match = match_console(newcon, c);
> 		if (match == CONSOLE_MATCH_NEXT)
> 			continue;
> 		if (match == CONSOLE_MATCH_FOUND)
> 			break;
> 		if (match == CONSOLE_MATCH_STOP)
> 			return;
> 	}
> 	...
>
>
>
> CONSOLE_MATCH_RETURN  -  basically means that we should stop matching.
> can we thus rename it to CONSOLE_MATCH_STOP, or similar?
>
> 	match_console() returned CONSOLE_MATCH_STOP
>
> is a bit better than
>
> 	match_console() returned CONSOLE_MATCH_RETURN.
>
> isn't it? :)
>
>
> // I also used CONSOLE_MATCH_FOUND in the example above instead of
> // CONSOLE_MATCH. not insisting that CONSOLE_MATCH_FOUND is much
> // better than CONSOLE_MATCH though.
>
> 	-ss
>

Re: [PATCH v2 0/3] printk: fix double printing with earlycon

[Sudeep Holla]

Hi Aleksey,

(Sorry for the delayed response, was on vacation)

On 02/03/17 13:11, Aleksey Makarov wrote:
> If a console was specified by ACPI SPCR table _and_ command line parameters like
> "console=ttyAMA0" _and_ "earlycon" were specified, then log messages
> appear twice.
> 
> This issue was addressed in the patch [1] but the approach was wrong and
> a revert [2] was suggested.
> 
> First two patches "printk: fix name/type/scope of preferred_console var" and
> "printk: rename selected_console -> preferred_console" were sent sent some
> time ago as one patch "printk: fix name and type of some variables" [3].
> They fix name/type/scope of some variables without changing the logic.
> 
> The real fix is in the second patch.  The root cause is that the code traverses
> the list of specified consoles (the `console_cmdline` array) and stops at the
> first match.  But it may happen that the same console is referred by
> the elements of this array twice:
> 
> 	pl011,mmio,0x87e024000000,115200 -- from SPCR
> 	ttyAMA0 -- from command line
> 
> but in this case `preferred_console` points to the second entry and
> the flag CON_CONSDEV is not set, so bootconsole is not deregistered.
> 
> To fix that, match the console against the `console_cmdline` entry
> pointed by `preferred_console` instead of the first match.
> 
> v2:
> - split the patch that renames `selected_console` and `preferred_console`
>   into two patches (Steven Rostedt)
> - add a comment explaining why we need a separate match to check for
>   preferred_console (Steven Rostedt)
> - v1 of this patchset changed the logic of console initialization a bit.
>   That could lead to bugs/incompatibilities.  Use the exactly the same
>   logic as in the original code.
> 

Tested the series(v3 of patch 3) and works as expected. Thanks for the
proper fix.

Tested-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep