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

[PATCH 0/2] 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
appears twice.

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

First patch "printk: fix name and type of some variables" was sent some time
ago [3].  It fixes 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 traverse
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:

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

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.

[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 (2):
  printk: fix name and type of some variables
  printk: fix double printing with earlycon

 kernel/printk/printk.c | 66 +++++++++++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 28 deletions(-)

-- 
2.11.1

[PATCH 1/2] printk: fix name and type of some variables

[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.

The variable selected_console holds an index into the console_cmdline
array, pointing to the console that was made preferred.

Rename the variables:
selected_console -> preferred_console
preferred_console -> has_preferred
and make the new has_preferred local static bool.

Renaming was suggested by Peter Hurley

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
Tested-by: Christopher Covington <cov@codeaurora.org>
---
 kernel/printk/printk.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 34da86e73d00..ed2a9b31f214 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -267,7 +267,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);
@@ -1908,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);
@@ -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 = preferred_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;
 			}
 		}
 	}
@@ -2481,9 +2481,9 @@ void register_console(struct console *newcon)
 		}
 
 		newcon->flags |= CON_ENABLED;
-		if (i == selected_console) {
+		if (i == preferred_console) {
 			newcon->flags |= CON_CONSDEV;
-			preferred_console = selected_console;
+			has_preferred = true;
 		}
 		break;
 	}
-- 
2.11.1

Re: [PATCH 1/2] printk: fix name and type of some variables

[Steven Rostedt]

On Wed,  1 Mar 2017 19:13:45 +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.
> 
> The variable selected_console holds an index into the console_cmdline
> array, pointing to the console that was made preferred.
> 
> Rename the variables:
> selected_console -> preferred_console
> preferred_console -> has_preferred
> and make the new has_preferred local static bool.

I like the conversion to the boolean, but I'm thinking I prefer
selected_console. The console command line is always confusing about
which one it uses (I can't remember if it's the first or the last one),
thus it may not really be preferred, and just selected ;-)

That said, I'm fine with the patch. What do others think? Keep
"selected_console" over "preferred_console". Not to mention, that
rename makes reviewing this patch a bit more complex.

If anything, perhaps break this patch up into two. The
non-controversial boolean change, and then the rename of
selected_console to preferred_console. That way we can ack one and nak
the other ;-)

-- Steve

> 
> Renaming was suggested by Peter Hurley
> 
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> Tested-by: Christopher Covington <cov@codeaurora.org>
> ---
>  kernel/printk/printk.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 34da86e73d00..ed2a9b31f214 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -267,7 +267,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);
> @@ -1908,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);
> @@ -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 = preferred_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;
>  			}
>  		}
>  	}
> @@ -2481,9 +2481,9 @@ void register_console(struct console *newcon)
>  		}
>  
>  		newcon->flags |= CON_ENABLED;
> -		if (i == selected_console) {
> +		if (i == preferred_console) {
>  			newcon->flags |= CON_CONSDEV;
> -			preferred_console = selected_console;
> +			has_preferred = true;
>  		}
>  		break;
>  	}

[PATCH 2/2] 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 appears twice.

The root cause is that the code traverse 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:

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

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 | 52 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ed2a9b31f214..92008ae9db3f 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,37 +2478,29 @@ 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;
 
-			if (_braille_register_console(newcon, c))
-				return;
+		if (_braille_register_console(newcon, c))
+			return;
 
-			if (newcon->setup &&
-			    newcon->setup(newcon, c->options) != 0)
-				break;
-		}
+		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;
 	}
 
 	if (!(newcon->flags & CON_ENABLED))
 		return;
 
+	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
-- 
2.11.1

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

[Steven Rostedt]

On Wed,  1 Mar 2017 19:13:46 +0300
Aleksey Makarov <aleksey.makarov@linaro.org> wrote:

> If a console was specified by ACPI SPCR table _and_
> command line parameters like "console=ttyAMA0" _and_ "earlycon"
> were specified, then log messages appears twice.
> 
> The root cause is that the code traverse 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:
> 
> 	ttyAMA0 -- from command line
> 	pl011,mmio,0x87e024000000,115200 -- from SPCR
> 
> 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 | 52 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ed2a9b31f214..92008ae9db3f 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,37 +2478,29 @@ 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;
>  
> -			if (_braille_register_console(newcon, c))
> -				return;
> +		if (_braille_register_console(newcon, c))
> +			return;
>  
> -			if (newcon->setup &&
> -			    newcon->setup(newcon, c->options) != 0)
> -				break;
> -		}
> +		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;
>  	}
>  
>  	if (!(newcon->flags & CON_ENABLED))
>  		return;
>  

We could use a comment right about here, explaining why the below is
needed.

-- Steve

> +	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