Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1601256 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-15 11:40 +0100
    [PATCH v6 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-17 12:50 +0100
      Re: [PATCH v6 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-17 14:50 +0100
    [PATCH v7 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-17 14:50 +0100
      Re: [PATCH v7 3/3] printk: fix double printing with earlycon Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-20 07:20 +0100
    [PATCH v8 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-20 11:10 +0100
      Re: [PATCH v8 3/3] printk: fix double printing with earlycon Petr Mladek <pmladek@suse.com> - 2017-03-27 16:20 +0200
        Re: [PATCH v8 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-27 18:30 +0200
          Re: [PATCH v8 3/3] printk: fix double printing with earlycon Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-28 04:10 +0200
            Re: [PATCH v8 3/3] printk: fix double printing with earlycon Petr Mladek <pmladek@suse.com> - 2017-03-28 15:00 +0200
              Re: [PATCH v8 3/3] printk: fix double printing with earlycon Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-30 08:00 +0200
                Re: [PATCH v8 3/3] printk: fix double printing with earlycon Petr Mladek <pmladek@suse.com> - 2017-04-04 13:20 +0200
                  Re: [PATCH v8 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-04-05 20:30 +0200
    [PATCH v9 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-04-05 22:30 +0200
      Re: [PATCH v9 3/3] printk: fix double printing with earlycon Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-06 00:00 +0200
        Re: [PATCH v9 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-04-06 06:50 +0200
      Re: [PATCH v9 3/3] printk: fix double printing with earlycon Petr Mladek <pmladek@suse.com> - 2017-04-10 16:30 +0200

#1601256 — [PATCH v5 0/3] printk: fix double printing with earlycon

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 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, introduce an invariant "The last non-braille console is always the
preferred one" on the entries of the console_cmdline array and don't try to
check for double entries.  Then traverse it in reverse order to be sure that if
the console is preferred then it will be the first matching entry.

v5:
- rewrite 3/3.  Insetead of rearranging the loop, introduce an invariant
  "The last non-braille console is always the preferred one" on the
  entries of the console_cmdline array and don't try to check for double
  entries.  Then traverse it in reverse order to be sure that if the console
  is preferred then it will be the first matching entry.
- add a better explanation from Petr Mladek for 2/3.

v4:
It was not sent upstream due to some problems in implementation of 3/3.
- add some Acked-by: and Reviewed-by: to 1/3 and 2/3
- v2 was closer to the original logic, even though v3 looked 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().  Implement correct sequence:
    1) try to match against preferred_console,
    2) if that fails check other entries of console_cmdline.
  Also move braille console matching/registration to a separate pass to simplify
  the code.

v3 (only for 3/3):
https://lkml.kernel.org/r/20170303154946.15399-1-aleksey.makarov@linaro.org
- 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.

v2:
https://lkml.kernel.org/r/20170302131153.22733-1-aleksey.makarov@linaro.org
- 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 | 61 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 24 deletions(-)

-- 
2.12.0

[toc] | [next] | [standalone]

#1603232 — [PATCH v6 3/3] printk: fix double printing with earlycon

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, split the loop where we search for matching entry of
console_cmdline into three parts that do not intersect:
1) search for braille console
2) check for preferred_console
3) match other entries so that these three parts do not

To to that introduce predicate _braille_is_braille_console() that checks if
its argument is an entry describing a braille console.

Also introduce a macro for_each_console_cmdline() to traverse
the console_cmdline array.

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

v5 -> v6:
- drop v5 and continue to work on v4:
- introduce _braille_is_braille_console(). It helps to split original loop
  into three parts: 1) search for braille console, 2) check for
  preferred_console, 3) match other entries so that these three parts do not
  intersect.
- introduce for_each_console_cmdline() macros to traverse console_cmdline
  (Petr Mladek)

 kernel/printk/braille.h |  12 ++++++
 kernel/printk/printk.c  | 104 ++++++++++++++++++++++++++++++++++--------------
 2 files changed, 86 insertions(+), 30 deletions(-)

diff --git a/kernel/printk/braille.h b/kernel/printk/braille.h
index 769d771145c8..183aebf6e1dc 100644
--- a/kernel/printk/braille.h
+++ b/kernel/printk/braille.h
@@ -18,6 +18,12 @@ _braille_register_console(struct console *console, struct console_cmdline *c);
 int
 _braille_unregister_console(struct console *console);
 
+static inline int
+_braille_is_braille_console(struct console_cmdline *c)
+{
+	return !!c->brl_options;
+}
+
 #else
 
 static inline void
@@ -43,6 +49,12 @@ _braille_unregister_console(struct console *console)
 	return 0;
 }
 
+static inline int
+_braille_is_braille_console(struct console_cmdline *c)
+{
+	return 0;
+}
+
 #endif
 
 #endif
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fd752f0c8ef1..ab2433681ca5 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -270,6 +270,11 @@ static struct console *exclusive_console;
 
 static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
 
+#define for_each_console_cmdline(i, c)			\
+	for (i = 0, c = console_cmdline;		\
+	     i < MAX_CMDLINECONSOLES && c->name[0];	\
+	     i++, c++)
+
 static int preferred_console = -1;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
@@ -1905,9 +1910,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
 	 *	See if this tty is not yet registered, and
 	 *	if we have a slot free.
 	 */
-	for (i = 0, c = console_cmdline;
-	     i < MAX_CMDLINECONSOLES && c->name[0];
-	     i++, c++) {
+	for_each_console_cmdline(i, c) {
 		if (strcmp(c->name, name) == 0 && c->index == idx) {
 			if (!brl_options)
 				preferred_console = i;
@@ -2383,6 +2386,37 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+static bool match_console_name(struct console *newcon,
+			       struct console_cmdline *c)
+{
+	BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
+	if (strcmp(c->name, newcon->name) != 0)
+		return false;
+	if (newcon->index >= 0 && newcon->index != c->index)
+		return false;
+	if (newcon->index < 0)
+		newcon->index = c->index;
+	return true;
+}
+
+static bool match_console(struct console *newcon, struct console_cmdline *c)
+{
+	if (newcon->match &&
+	    newcon->match(newcon, c->name, c->index, c->options) == 0) {
+		newcon->flags |= CON_ENABLED;
+		return true;
+	}
+
+	if (match_console_name(newcon, c)) {
+		if (!newcon->setup || newcon->setup(newcon, c->options) == 0)
+			newcon->flags |= CON_ENABLED;
+
+		return true;
+	}
+
+	return false;
+}
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -2457,40 +2491,50 @@ void register_console(struct console *newcon)
 	}
 
 	/*
-	 *	See if this console matches one we selected on
-	 *	the command line.
+	 * See if this console matches one we selected on the command line.
+	 * Do it in three steps:
+	 *
+	 * 1) check if it is a braille console..
 	 */
-	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;
-		}
+	for_each_console_cmdline(i, c)
+		if (_braille_is_braille_console(c) &&
+		    match_console_name(newcon, c) &&
+		    _braille_register_console(newcon, c))
+			return;
 
-		newcon->flags |= CON_ENABLED;
-		if (i == preferred_console) {
+	/*
+	 * 2) 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, one with newcon->match(), another by name/index:
+	 *
+	 *	pl011,mmio,0x87e024000000,115200 -- added from SPCR
+	 *	ttyAMA0 -- added from command line
+	 *
+	 * so we can not use the first match.  Instead check the
+	 * entry pointed by preferred_console and then all other entries.
+	 */
+	if (preferred_console >= 0 &&
+	    match_console(newcon, console_cmdline + preferred_console)) {
+		if (newcon->flags & CON_ENABLED) {
 			newcon->flags |= CON_CONSDEV;
 			has_preferred = true;
 		}
-		break;
+		goto match;
+	}
+
+	/*
+	 * 3) check other entries
+	 */
+	for_each_console_cmdline(i, c) {
+		if (preferred_console == i || _braille_is_braille_console(c))
+			continue;
+
+		if (match_console(newcon, c))
+			goto match;
 	}
 
+match:
 	if (!(newcon->flags & CON_ENABLED))
 		return;
 
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1603321 — Re: [PATCH v6 3/3] printk: fix double printing with earlycon

On 03/17/2017 02:43 PM, Aleksey Makarov wrote:

[..]

> @@ -2457,40 +2491,50 @@ void register_console(struct console *newcon)
>  	}
>  
>  	/*
> -	 *	See if this console matches one we selected on
> -	 *	the command line.
> +	 * See if this console matches one we selected on the command line.
> +	 * Do it in three steps:
> +	 *
> +	 * 1) check if it is a braille console..
>  	 */
> -	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;
> -		}
> +	for_each_console_cmdline(i, c)
> +		if (_braille_is_braille_console(c) &&
> +		    match_console_name(newcon, c) &&
> +		    _braille_register_console(newcon, c))
> +			return;

I am sorry to say that, but it looks like this does not work either.
newcon->index still can be changed here in match_console_name(), 
but (correctly implemented) _braille_register_console() may refuse
to register it, and the changed newcon is passed to newcon->match() below.

I tried the shuffle (i. e. v5) approach again and it seems I managed to
write quite nice code.  I am going to send it in a minute.

Thank you
Aleksey Makarov

> -		newcon->flags |= CON_ENABLED;
> -		if (i == preferred_console) {
> +	/*
> +	 * 2) 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, one with newcon->match(), another by name/index:
> +	 *
> +	 *	pl011,mmio,0x87e024000000,115200 -- added from SPCR
> +	 *	ttyAMA0 -- added from command line
> +	 *
> +	 * so we can not use the first match.  Instead check the
> +	 * entry pointed by preferred_console and then all other entries.
> +	 */
> +	if (preferred_console >= 0 &&
> +	    match_console(newcon, console_cmdline + preferred_console)) {
> +		if (newcon->flags & CON_ENABLED) {
>  			newcon->flags |= CON_CONSDEV;
>  			has_preferred = true;
>  		}
> -		break;
> +		goto match;
> +	}
> +
> +	/*
> +	 * 3) check other entries
> +	 */
> +	for_each_console_cmdline(i, c) {
> +		if (preferred_console == i || _braille_is_braille_console(c))
> +			continue;
> +
> +		if (match_console(newcon, c))
> +			goto match;
>  	}
>  
> +match:
>  	if (!(newcon->flags & CON_ENABLED))
>  		return;
>  
> 

[toc] | [prev] | [next] | [standalone]

#1603322 — [PATCH v7 3/3] printk: fix double printing with earlycon

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, introduce an invariant "The last non-braille console
is always the preferred one" on the entries of the console_cmdline
array.  Then traverse it in reverse order to be sure that if
the console is preferred then it will be the first matching entry.

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

v6 -> v7:
- return back to v5 :)
- leave the check for already registered entries and add a function that
  maintains the invariant.

v5 -> v6:
- drop v5 and continue to work on v4:
- introduce _braille_is_braille_console(). It helps to split original loop
  into three parts: 1) search for braille console, 2) check for
  preferred_console, 3) match other entries so that these three parts do not
  intersect.
- introduce for_each_console_cmdline() macros to traverse console_cmdline
  (Petr Mladek)

 kernel/printk/printk.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fd752f0c8ef1..88f86fb23bc4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1895,6 +1895,34 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
 }
 #endif
 
+/*
+ * This function maintains an invariant that will help to find if
+ * the matching console is preferred, see register_console():
+ *
+ *      The last non-braille console is always the preferred one.
+ */
+static void ensure_preferred_is_last(int i)
+{
+	int last;
+
+	for (last = MAX_CMDLINECONSOLES - 1;
+	     last >= 0 && !console_cmdline[last].name[0];
+	     last--)
+		;
+
+	BUG_ON(last < 0);
+
+	if (i != last) {
+		struct console_cmdline t;
+
+		t = console_cmdline[i];
+		console_cmdline[i] = console_cmdline[last];
+		console_cmdline[last] = t;
+	}
+
+	preferred_console = last;
+}
+
 static int __add_preferred_console(char *name, int idx, char *options,
 				   char *brl_options)
 {
@@ -1910,7 +1938,7 @@ 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)
-				preferred_console = i;
+				ensure_preferred_is_last(i);
 			return 0;
 		}
 	}
@@ -2457,12 +2485,20 @@ void register_console(struct console *newcon)
 	}
 
 	/*
-	 *	See if this console matches one we selected on
-	 *	the command line.
+	 * See if this console matches one we selected on the command line.
+	 *
+	 * The console_cmdline array is traversed in the reverse order because
+	 * we want to be sure that if this console is preferred then it will be
+	 * the first matching entry.  We use the invariant that is maintained in
+	 * __add_preferred_console().
 	 */
-	for (i = 0, c = console_cmdline;
-	     i < MAX_CMDLINECONSOLES && c->name[0];
-	     i++, c++) {
+	for (i = MAX_CMDLINECONSOLES - 1; i >= 0; i--) {
+
+		if (!console_cmdline[i].name[0])
+			continue;
+
+		c = console_cmdline + i;
+
 		if (!newcon->match ||
 		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
 			/* default matching */
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1604192 — Re: [PATCH v7 3/3] printk: fix double printing with earlycon

Hello,

On (03/17/17 16:43), Aleksey Makarov wrote:
[..]
> +static void ensure_preferred_is_last(int i)
> +{
> +	int last;
> +
> +	for (last = MAX_CMDLINECONSOLES - 1;
> +	     last >= 0 && !console_cmdline[last].name[0];
> +	     last--)
> +		;
> +
> +	BUG_ON(last < 0);

let's not panic().


a nitpick, console swap can be done as

	if (i != last)
		swap(console_cmdline[i], console_cmdline[last]);
	preferred_console = last;


		-ss

[toc] | [prev] | [next] | [standalone]

#1604324 — [PATCH v8 3/3] printk: fix double printing with earlycon

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, introduce an invariant "The last non-braille console
is always the preferred one" on the entries of the console_cmdline
array.  Then traverse it in reverse order to be sure that if
the console is preferred then it will be the first matching entry.

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

v7 -> v8:
- add an explanation to the comment how console_cmdline can contain entries
  referring to the same console
- move the body of the function introduced in the previous version to cycle
- don't panic() (Sergey Senozhatsky).  Don't check this condition because
  the loop condition guarantees that it holds.
- use swap() (Sergey Senozhatsky)

v6 -> v7:
- return back to v5 
- leave the check for already registered entries and add a function that
  maintains the invariant.

v5 -> v6:
- drop v5 and continue to work on v4:
- introduce _braille_is_braille_console(). It helps to split original loop
  into three parts: 1) search for braille console, 2) check for
  preferred_console, 3) match other entries so that these three parts do not
  intersect.
- introduce for_each_console_cmdline() macros to traverse console_cmdline
  (Petr Mladek)

 kernel/printk/printk.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fd752f0c8ef1..462036e7a767 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1909,8 +1909,28 @@ static int __add_preferred_console(char *name, int idx, char *options,
 	     i < MAX_CMDLINECONSOLES && c->name[0];
 	     i++, c++) {
 		if (strcmp(c->name, name) == 0 && c->index == idx) {
-			if (!brl_options)
-				preferred_console = i;
+			int last;
+
+			if (brl_options)
+				return 0;
+
+			/*
+			 * Maintain an invariant that will help to find if
+			 * the matching console is preferred, see
+			 * register_console():
+			 *
+			 * The last non-braille console is always
+			 * the preferred one.
+			 */
+			for (last = MAX_CMDLINECONSOLES - 1;
+			     last >= 0 && !console_cmdline[last].name[0];
+			     last--)
+				;
+
+			if (i != last)
+				swap(console_cmdline[i], console_cmdline[last]);
+
+			preferred_console = last;
 			return 0;
 		}
 	}
@@ -2457,12 +2477,27 @@ void register_console(struct console *newcon)
 	}
 
 	/*
-	 *	See if this console matches one we selected on
-	 *	the command line.
+	 * See if this console matches one we selected on the command line.
+	 *
+	 * There may be several entries in the console_cmdline array matching
+	 * with the same console, one with newcon->match(), another by
+	 * name/index:
+	 *
+	 *	pl011,mmio,0x87e024000000,115200 -- added from SPCR
+	 *	ttyAMA0 -- added from command line
+	 *
+	 * Traverse the console_cmdline array in reverse order to be
+	 * sure that if this console is preferred then it will be the first
+	 * matching entry.  We use the invariant that is maintained in
+	 * __add_preferred_console().
 	 */
-	for (i = 0, c = console_cmdline;
-	     i < MAX_CMDLINECONSOLES && c->name[0];
-	     i++, c++) {
+	for (i = MAX_CMDLINECONSOLES - 1; i >= 0; i--) {
+
+		if (!console_cmdline[i].name[0])
+			continue;
+
+		c = console_cmdline + i;
+
 		if (!newcon->match ||
 		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
 			/* default matching */
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1609877 — Re: [PATCH v8 3/3] printk: fix double printing with earlycon

On Mon 2017-03-20 13:03:00, 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, introduce an invariant "The last non-braille console
> is always the preferred one" on the entries of the console_cmdline
> array.  Then traverse it in reverse order to be sure that if
> the console is preferred then it will be the first matching entry.

Sigh, I am afraid that we need to go this way. I hate the side
effects of the match() functions. It would be great to get
rid of them. But it is non-trivial and out of scope for this fix.


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

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index fd752f0c8ef1..462036e7a767 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1909,8 +1909,28 @@ static int __add_preferred_console(char *name, int idx, char *options,
>  	     i < MAX_CMDLINECONSOLES && c->name[0];
>  	     i++, c++) {
>  		if (strcmp(c->name, name) == 0 && c->index == idx) {
> -			if (!brl_options)
> -				preferred_console = i;
> +			int last;
> +
> +			if (brl_options)
> +				return 0;
> +
> +			/*
> +			 * Maintain an invariant that will help to find if
> +			 * the matching console is preferred, see
> +			 * register_console():
> +			 *
> +			 * The last non-braille console is always
> +			 * the preferred one.
> +			 */
> +			for (last = MAX_CMDLINECONSOLES - 1;
> +			     last >= 0 && !console_cmdline[last].name[0];
> +			     last--)
> +				;

This is a rather non-trivial code to find the last element.
I might make sense to count it in a global variable.
Then we might remove the check for console_cmdline[i].name[0]
also in the other for cycles and make them better readable.

> +
> +			if (i != last)
> +				swap(console_cmdline[i], console_cmdline[last]);

I was not aware of the swap() function. It is great to know ;-)

Otherwise, I am find with this approach.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1609974 — Re: [PATCH v8 3/3] printk: fix double printing with earlycon

On 03/27/2017 05:14 PM, Petr Mladek wrote:
> On Mon 2017-03-20 13:03:00, Aleksey Makarov wrote:

[..]

>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index fd752f0c8ef1..462036e7a767 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1909,8 +1909,28 @@ static int __add_preferred_console(char *name, int idx, char *options,
>>  	     i < MAX_CMDLINECONSOLES && c->name[0];
>>  	     i++, c++) {
>>  		if (strcmp(c->name, name) == 0 && c->index == idx) {
>> -			if (!brl_options)
>> -				preferred_console = i;
>> +			int last;
>> +
>> +			if (brl_options)
>> +				return 0;
>> +
>> +			/*
>> +			 * Maintain an invariant that will help to find if
>> +			 * the matching console is preferred, see
>> +			 * register_console():
>> +			 *
>> +			 * The last non-braille console is always
>> +			 * the preferred one.
>> +			 */
>> +			for (last = MAX_CMDLINECONSOLES - 1;
>> +			     last >= 0 && !console_cmdline[last].name[0];
>> +			     last--)
>> +				;
>
> This is a rather non-trivial code to find the last element.
> I might make sense to count it in a global variable.
> Then we might remove the check for console_cmdline[i].name[0]
> also in the other for cycles and make them better readable.

Having an additional variable console_cmdline_last pointing to the last element
would require maintaining consistency between this variable and
contents of console_cmdline.  For the code we have it is not hard, but when code
is changed we need to check this.  Also there exists preferred_console that
has almost the same meaning but it points not to the last element, but to the
last non-braille element.  Also we need to have a special value (-1) for this
variable for empty array. So I personally would instead try to rewrite this:

	for (last = MAX_CMDLINECONSOLES - 1; last >= 0; last--)
		if (console_cmdline[last].name[0])
			break;

Is it better?  If not, I will send a version with console_cmdline_last.

>> +
>> +			if (i != last)
>> +				swap(console_cmdline[i], console_cmdline[last]);
>
> I was not aware of the swap() function. It is great to know ;-)

Yes, same for me.  Thanks to Sergey Senozhatsky.

Thank you for review
Aleksey Makarov

[toc] | [prev] | [next] | [standalone]

#1610259 — Re: [PATCH v8 3/3] printk: fix double printing with earlycon

On (03/27/17 19:28), Aleksey Makarov wrote:
[..]
> > > +			/*
> > > +			 * Maintain an invariant that will help to find if
> > > +			 * the matching console is preferred, see
> > > +			 * register_console():
> > > +			 *
> > > +			 * The last non-braille console is always
> > > +			 * the preferred one.
> > > +			 */
> > > +			for (last = MAX_CMDLINECONSOLES - 1;
> > > +			     last >= 0 && !console_cmdline[last].name[0];
> > > +			     last--)
> > > +				;
> > 
> > This is a rather non-trivial code to find the last element.
> > I might make sense to count it in a global variable.
> > Then we might remove the check for console_cmdline[i].name[0]
> > also in the other for cycles and make them better readable.
> 
> Having an additional variable console_cmdline_last pointing to the last element
> would require maintaining consistency between this variable and
> contents of console_cmdline.  For the code we have it is not hard, but when code
> is changed we need to check this.  Also there exists preferred_console that
> has almost the same meaning but it points not to the last element, but to the
> last non-braille element.  Also we need to have a special value (-1) for this
> variable for empty array. So I personally would instead try to rewrite this:
> 
> 	for (last = MAX_CMDLINECONSOLES - 1; last >= 0; last--)
> 		if (console_cmdline[last].name[0])
> 			break;
> 
> Is it better?  If not, I will send a version with console_cmdline_last.

personally I'm fine with the nested loop. the latest version
	"for (last = MAX_CMDLINECONSOLES - 1; last >= 0;..."

is even easier to read.


so we do not just iterate console_cmdline anymore, but also modify it.
this, probably, has impact on the following scenario

CPU0						CPU1

add_preferred_console()			add_preferred_console()
  __add_preferred_console()		  __add_preferred_console()
      swap(i1, last)			    swap(i2, last)

	temp1 = i1
	i1 = last				temp2 = i2
	last = temp1				i2 = last
						last = temp2

so both i1 and i2 will point to 'last' now, IOW, we will have two
identical entries in console_cmdline, while i1 or i2 will be lost.


neither add_preferred_console() nor __add_preferred_console() have any
serialization. and I assume that we can call add_preferred_console()
concurrently, can't we?

	-ss

[toc] | [prev] | [next] | [standalone]

#1610758 — Re: [PATCH v8 3/3] printk: fix double printing with earlycon

On Tue 2017-03-28 11:04:04, Sergey Senozhatsky wrote:
> On (03/27/17 19:28), Aleksey Makarov wrote:
> [..]
> > > > +			/*
> > > > +			 * Maintain an invariant that will help to find if
> > > > +			 * the matching console is preferred, see
> > > > +			 * register_console():
> > > > +			 *
> > > > +			 * The last non-braille console is always
> > > > +			 * the preferred one.
> > > > +			 */
> > > > +			for (last = MAX_CMDLINECONSOLES - 1;
> > > > +			     last >= 0 && !console_cmdline[last].name[0];
> > > > +			     last--)
> > > > +				;
> > > 
> > > This is a rather non-trivial code to find the last element.
> > > I might make sense to count it in a global variable.
> > > Then we might remove the check for console_cmdline[i].name[0]
> > > also in the other for cycles and make them better readable.
> > 
> > Having an additional variable console_cmdline_last pointing to the last element
> > would require maintaining consistency between this variable and
> > contents of console_cmdline.  For the code we have it is not hard, but when code
> > is changed we need to check this.  Also there exists preferred_console that
> > has almost the same meaning but it points not to the last element, but to the
> > last non-braille element.  Also we need to have a special value (-1) for this
> > variable for empty array. So I personally would instead try to rewrite this:
> > 
> > 	for (last = MAX_CMDLINECONSOLES - 1; last >= 0; last--)
> > 		if (console_cmdline[last].name[0])
> > 			break;
> > 
> > Is it better?  If not, I will send a version with console_cmdline_last.
> 
> personally I'm fine with the nested loop. the latest version
> 	"for (last = MAX_CMDLINECONSOLES - 1; last >= 0;..."
> 
> is even easier to read.

The number of elements is bumped on a single location, so there
is not much to synchronize. The old approach was fine because
the for cycles were needed anyway, they started on the 0th element,
and NULL ended arrays are rather common practice.

But we are searching the array from the end now. Also we use the
for cycle just to get the number here. This is not a common
practice and it makes the code more complicated and strange from
my point of view.

If you do not like -1, you could use console_cmdline_cnt and
start with 0. I would actually do so because it is a common
approach and easy to understand.

> 
> so we do not just iterate console_cmdline anymore, but also modify it.
> this, probably, has impact on the following scenario
> 
> CPU0						CPU1
> 
> add_preferred_console()			add_preferred_console()
>   __add_preferred_console()		  __add_preferred_console()
>       swap(i1, last)			    swap(i2, last)
> 
> 	temp1 = i1
> 	i1 = last				temp2 = i2
> 	last = temp1				i2 = last
> 						last = temp2
> 
> so both i1 and i2 will point to 'last' now, IOW, we will have two
> identical entries in console_cmdline, while i1 or i2 will be lost.
> 
> 
> neither add_preferred_console() nor __add_preferred_console() have any
> serialization. and I assume that we can call add_preferred_console()
> concurrently, can't we?

Very good point!

Well, if this race exists, it was racy even before. Of course,
the old race was only when new entry was added. It would
be more visible now because also shuffling would be racy.

OK, most add_preferred_console() calls are in functions
that are defined as console_initcall(). They seem to
be defined only when the respective drivers are built in.
It seems that these initcalls are serialized, see console_init().

add_preferred_console() is used also in uart_add_one_port():

   -> uart_add_one_port()
    -> of_console_check()
     -> add_preferred_console()

But there the calls are synchronized as well via
port_mutex.

Finally, __add_preferred_console() is called also from
console_setup(). It is called via do_early_param()
even before the console_initcall() functions. It is
serialized as well.

If I did not miss anything, it would seem that
__add_preferred_console() are called synchronously
and only during boot by design.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1612578 — Re: [PATCH v8 3/3] printk: fix double printing with earlycon

On (03/28/17 14:56), Petr Mladek wrote:
[..]
> > > Is it better?  If not, I will send a version with console_cmdline_last.
> > 
> > personally I'm fine with the nested loop. the latest version
> > 	"for (last = MAX_CMDLINECONSOLES - 1; last >= 0;..."
> > 
> > is even easier to read.
> 
> The number of elements is bumped on a single location, so there
> is not much to synchronize. The old approach was fine because
> the for cycles were needed anyway, they started on the 0th element,
> and NULL ended arrays are rather common practice.
> 
> But we are searching the array from the end now. Also we use the
> for cycle just to get the number here. This is not a common
> practice and it makes the code more complicated and strange from
> my point of view.

I'm fine with either way :)

[..]
> > neither add_preferred_console() nor __add_preferred_console() have any
> > serialization. and I assume that we can call add_preferred_console()
> > concurrently, can't we?
[..]
> If I did not miss anything, it would seem that
> __add_preferred_console() are called synchronously
> and only during boot by design.

thanks. I think you are right. it's console_initcall or __init.

	-ss

[toc] | [prev] | [next] | [standalone]

#1615916 — Re: [PATCH v8 3/3] printk: fix double printing with earlycon

On Thu 2017-03-30 14:55:46, Sergey Senozhatsky wrote:
> On (03/28/17 14:56), Petr Mladek wrote:
> [..]
> > > > Is it better?  If not, I will send a version with console_cmdline_last.
> > > 
> > > personally I'm fine with the nested loop. the latest version
> > > 	"for (last = MAX_CMDLINECONSOLES - 1; last >= 0;..."
> > > 
> > > is even easier to read.
> > 
> > The number of elements is bumped on a single location, so there
> > is not much to synchronize. The old approach was fine because
> > the for cycles were needed anyway, they started on the 0th element,
> > and NULL ended arrays are rather common practice.
> > 
> > But we are searching the array from the end now. Also we use the
> > for cycle just to get the number here. This is not a common
> > practice and it makes the code more complicated and strange from
> > my point of view.
> 
> I'm fine with either way :)

Alekesey, any chance to use the global variable to count used or point
to the last element?

I know that you have already spent a lot of time with it. It was great
work. But the current solution of the cycle looks weird to me.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1617271 — Re: [PATCH v8 3/3] printk: fix double printing with earlycon

On 04/04/2017 02:12 PM, Petr Mladek wrote:
> On Thu 2017-03-30 14:55:46, Sergey Senozhatsky wrote:
>> On (03/28/17 14:56), Petr Mladek wrote:

[..]

> Alekesey, any chance to use the global variable to count used or point
> to the last element?
>
> I know that you have already spent a lot of time with it. It was great
> work. But the current solution of the cycle looks weird to me.

Sorry for the delay.  I will send next version soon.

Thank you
Aleksey Makarov

[toc] | [prev] | [next] | [standalone]

#1617352 — [PATCH v9 3/3] printk: fix double printing with earlycon

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, introduce an invariant "The last non-braille console
is always the preferred one" on the entries of the console_cmdline
array.  Then traverse it in reverse order to be sure that if
the console is preferred then it will be the first matching entry.
Introduce variable console_cmdline_cnt that keeps the number
of elements of the console_cmdline array (Petr Mladek).  It helps
to get rid of the loop that searches for the end of this array.

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

v8 -> v9:
- Introduce variable console_cmdline_cnt that keeps the number
  of elements of the console_cmdline array (Petr Mladek).  It helps
  to get rid of the loop that searches for the end of this array.
	For the record: I think that this console_cmdline_cnt implementation
	is worse than just a loop that finds the end of the array because 
	1) we have to support consistency between console_cmdline_cnt and
	  console_cmdline_cnt
	2) it makes patch a bit more intrusive

v7 -> v8:
- add an explanation to the comment how console_cmdline can contain entries
  referring to the same console
- move the body of the function introduced in the previous version to cycle
- don't panic() (Sergey Senozhatsky).  Don't check this condition because
  the loop condition guarantees that it holds.
- use swap() (Sergey Senozhatsky)

v6 -> v7:
- return back to v5 
- leave the check for already registered entries and add a function that
  maintains the invariant.

v5 -> v6:
- drop v5 and continue to work on v4:
- introduce _braille_is_braille_console(). It helps to split original loop
  into three parts: 1) search for braille console, 2) check for
  preferred_console, 3) match other entries so that these three parts do not
  intersect.
- introduce for_each_console_cmdline() macros to traverse console_cmdline
  (Petr Mladek)

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fd752f0c8ef1..be657af45758 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -269,6 +269,7 @@ static struct console *exclusive_console;
 #define MAX_CMDLINECONSOLES 8
 
 static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
+static int console_cmdline_cnt;
 
 static int preferred_console = -1;
 int console_set_on_cmdline;
@@ -1905,12 +1906,26 @@ static int __add_preferred_console(char *name, int idx, char *options,
 	 *	See if this tty is not yet registered, and
 	 *	if we have a slot free.
 	 */
-	for (i = 0, c = console_cmdline;
-	     i < MAX_CMDLINECONSOLES && c->name[0];
-	     i++, c++) {
+	for (i = 0, c = console_cmdline; i < console_cmdline_cnt; i++, c++) {
 		if (strcmp(c->name, name) == 0 && c->index == idx) {
-			if (!brl_options)
-				preferred_console = i;
+
+			if (brl_options)
+				return 0;
+
+			/*
+			 * Maintain an invariant that will help to find if
+			 * the matching console is preferred, see
+			 * register_console():
+			 *
+			 * The last non-braille console is always
+			 * the preferred one.
+			 */
+			if (i != console_cmdline_cnt - 1)
+				swap(console_cmdline[i],
+				     console_cmdline[console_cmdline_cnt - 1]);
+
+			preferred_console = console_cmdline_cnt - 1;
+
 			return 0;
 		}
 	}
@@ -1923,6 +1938,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
 	braille_set_options(c, brl_options);
 
 	c->index = idx;
+	console_cmdline_cnt++;
 	return 0;
 }
 /*
@@ -2457,12 +2473,24 @@ void register_console(struct console *newcon)
 	}
 
 	/*
-	 *	See if this console matches one we selected on
-	 *	the command line.
+	 * See if this console matches one we selected on the command line.
+	 *
+	 * There may be several entries in the console_cmdline array matching
+	 * with the same console, one with newcon->match(), another by
+	 * name/index:
+	 *
+	 *	pl011,mmio,0x87e024000000,115200 -- added from SPCR
+	 *	ttyAMA0 -- added from command line
+	 *
+	 * Traverse the console_cmdline array in reverse order to be
+	 * sure that if this console is preferred then it will be the first
+	 * matching entry.  We use the invariant that is maintained in
+	 * __add_preferred_console().
 	 */
-	for (i = 0, c = console_cmdline;
-	     i < MAX_CMDLINECONSOLES && c->name[0];
-	     i++, c++) {
+	for (i = console_cmdline_cnt - 1; i >= 0; i--) {
+
+		c = console_cmdline + i;
+
 		if (!newcon->match ||
 		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
 			/* default matching */
-- 
2.12.1

[toc] | [prev] | [next] | [standalone]

#1617392 — Re: [PATCH v9 3/3] printk: fix double printing with earlycon

On Wed, Apr 5, 2017 at 11:20 PM, 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 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, introduce an invariant "The last non-braille console
> is always the preferred one" on the entries of the console_cmdline
> array.  Then traverse it in reverse order to be sure that if
> the console is preferred then it will be the first matching entry.
> Introduce variable console_cmdline_cnt that keeps the number
> of elements of the console_cmdline array (Petr Mladek).  It helps
> to get rid of the loop that searches for the end of this array.

>  #define MAX_CMDLINECONSOLES 8
>
>  static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
> +static int console_cmdline_cnt;

This should be equal to -1 at the beginning, am I right?

>  static int preferred_console = -1;
>  int console_set_on_cmdline;
> @@ -1905,12 +1906,26 @@ static int __add_preferred_console(char *name, int idx, char *options,
>          *      See if this tty is not yet registered, and
>          *      if we have a slot free.
>          */
> -       for (i = 0, c = console_cmdline;
> -            i < MAX_CMDLINECONSOLES && c->name[0];
> -            i++, c++) {
> +       for (i = 0, c = console_cmdline; i < console_cmdline_cnt; i++, c++) {
>                 if (strcmp(c->name, name) == 0 && c->index == idx) {
> -                       if (!brl_options)
> -                               preferred_console = i;
> +

> +                       if (brl_options)
> +                               return 0;

Is it invariant or brl_options may appear while looping?

> +
> +                       /*
> +                        * Maintain an invariant that will help to find if
> +                        * the matching console is preferred, see
> +                        * register_console():
> +                        *
> +                        * The last non-braille console is always
> +                        * the preferred one.
> +                        */
> +                       if (i != console_cmdline_cnt - 1)
> +                               swap(console_cmdline[i],
> +                                    console_cmdline[console_cmdline_cnt - 1]);

i'm wondering if you can iterate from the end to beginning as you do below.
It would simplify things.

> +
> +                       preferred_console = console_cmdline_cnt - 1;
> +
>                         return 0;
>                 }
>         }
> @@ -1923,6 +1938,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
>         braille_set_options(c, brl_options);
>
>         c->index = idx;
> +       console_cmdline_cnt++;
>         return 0;
>  }
>  /*
> @@ -2457,12 +2473,24 @@ void register_console(struct console *newcon)
>         }
>
>         /*
> -        *      See if this console matches one we selected on
> -        *      the command line.
> +        * See if this console matches one we selected on the command line.
> +        *
> +        * There may be several entries in the console_cmdline array matching
> +        * with the same console, one with newcon->match(), another by
> +        * name/index:
> +        *
> +        *      pl011,mmio,0x87e024000000,115200 -- added from SPCR
> +        *      ttyAMA0 -- added from command line
> +        *
> +        * Traverse the console_cmdline array in reverse order to be
> +        * sure that if this console is preferred then it will be the first
> +        * matching entry.  We use the invariant that is maintained in
> +        * __add_preferred_console().
>          */
> -       for (i = 0, c = console_cmdline;
> -            i < MAX_CMDLINECONSOLES && c->name[0];
> -            i++, c++) {

> +       for (i = console_cmdline_cnt - 1; i >= 0; i--) {



> +
> +               c = console_cmdline + i;
> +
>                 if (!newcon->match ||
>                     newcon->match(newcon, c->name, c->index, c->options) != 0) {
>                         /* default matching */
> --
> 2.12.1
>



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1617500 — Re: [PATCH v9 3/3] printk: fix double printing with earlycon

On 04/06/2017 12:57 AM, Andy Shevchenko wrote:
> On Wed, Apr 5, 2017 at 11:20 PM, 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 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, introduce an invariant "The last non-braille console
>> is always the preferred one" on the entries of the console_cmdline
>> array.  Then traverse it in reverse order to be sure that if
>> the console is preferred then it will be the first matching entry.
>> Introduce variable console_cmdline_cnt that keeps the number
>> of elements of the console_cmdline array (Petr Mladek).  It helps
>> to get rid of the loop that searches for the end of this array.
>
>>  #define MAX_CMDLINECONSOLES 8
>>
>>  static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
>> +static int console_cmdline_cnt;
>
> This should be equal to -1 at the beginning, am I right?

No, this is not an index of the last element, this is count of
elements of cmdline_console array.  So it is 0 initially.

>>  static int preferred_console = -1;
>>  int console_set_on_cmdline;
>> @@ -1905,12 +1906,26 @@ static int __add_preferred_console(char *name, int idx, char *options,
>>          *      See if this tty is not yet registered, and
>>          *      if we have a slot free.
>>          */
>> -       for (i = 0, c = console_cmdline;
>> -            i < MAX_CMDLINECONSOLES && c->name[0];
>> -            i++, c++) {
>> +       for (i = 0, c = console_cmdline; i < console_cmdline_cnt; i++, c++) {
>>                 if (strcmp(c->name, name) == 0 && c->index == idx) {
>> -                       if (!brl_options)
>> -                               preferred_console = i;
>> +
>
>> +                       if (brl_options)
>> +                               return 0;
>
> Is it invariant or brl_options may appear while looping?

I am not sure I understand your question.
If we find that we are registering a braille console that
has already been registered, we just return without updating
preferred console (it is only about regular consoles) and
without swapping it with the last element of the array (because it
is explicitly mentioned in the invariant:  The last
*non-braille* console is always the preferred one)

>> +
>> +                       /*
>> +                        * Maintain an invariant that will help to find if
>> +                        * the matching console is preferred, see
>> +                        * register_console():
>> +                        *
>> +                        * The last non-braille console is always
>> +                        * the preferred one.
>> +                        */
>> +                       if (i != console_cmdline_cnt - 1)
>> +                               swap(console_cmdline[i],
>> +                                    console_cmdline[console_cmdline_cnt - 1]);
>
> i'm wondering if you can iterate from the end to beginning as you do below.
> It would simplify things.

You mean iterate to find the last element?
Yes I can and it is how this was implemented in v8,
Petr Mladek asked to introduce console_cmdline_cnt.

Thank you for review
Aleksey Makarov

>> +
>> +                       preferred_console = console_cmdline_cnt - 1;
>> +
>>                         return 0;
>>                 }
>>         }
>> @@ -1923,6 +1938,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
>>         braille_set_options(c, brl_options);
>>
>>         c->index = idx;
>> +       console_cmdline_cnt++;
>>         return 0;
>>  }
>>  /*
>> @@ -2457,12 +2473,24 @@ void register_console(struct console *newcon)
>>         }
>>
>>         /*
>> -        *      See if this console matches one we selected on
>> -        *      the command line.
>> +        * See if this console matches one we selected on the command line.
>> +        *
>> +        * There may be several entries in the console_cmdline array matching
>> +        * with the same console, one with newcon->match(), another by
>> +        * name/index:
>> +        *
>> +        *      pl011,mmio,0x87e024000000,115200 -- added from SPCR
>> +        *      ttyAMA0 -- added from command line
>> +        *
>> +        * Traverse the console_cmdline array in reverse order to be
>> +        * sure that if this console is preferred then it will be the first
>> +        * matching entry.  We use the invariant that is maintained in
>> +        * __add_preferred_console().
>>          */
>> -       for (i = 0, c = console_cmdline;
>> -            i < MAX_CMDLINECONSOLES && c->name[0];
>> -            i++, c++) {
>
>> +       for (i = console_cmdline_cnt - 1; i >= 0; i--) {
>
>
>
>> +
>> +               c = console_cmdline + i;
>> +
>>                 if (!newcon->match ||
>>                     newcon->match(newcon, c->name, c->index, c->options) != 0) {
>>                         /* default matching */
>> --
>> 2.12.1
>>
>
>
>

[toc] | [prev] | [next] | [standalone]

#1619979 — Re: [PATCH v9 3/3] printk: fix double printing with earlycon

On Wed 2017-04-05 23:20:00, 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, introduce an invariant "The last non-braille console
> is always the preferred one" on the entries of the console_cmdline
> array.  Then traverse it in reverse order to be sure that if
> the console is preferred then it will be the first matching entry.
> Introduce variable console_cmdline_cnt that keeps the number
> of elements of the console_cmdline array (Petr Mladek).  It helps
> to get rid of the loop that searches for the end of this array.
> 
> Reported-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>

This version looks fine to me. Just a small nitpick below.
Anyway:

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

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index fd752f0c8ef1..be657af45758 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -269,6 +269,7 @@ static struct console *exclusive_console;
>  #define MAX_CMDLINECONSOLES 8
>  
>  static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
> +static int console_cmdline_cnt;
>  
>  static int preferred_console = -1;
>  int console_set_on_cmdline;
> @@ -1905,12 +1906,26 @@ static int __add_preferred_console(char *name, int idx, char *options,
>  	 *	See if this tty is not yet registered, and
>  	 *	if we have a slot free.
>  	 */
> -	for (i = 0, c = console_cmdline;
> -	     i < MAX_CMDLINECONSOLES && c->name[0];
> -	     i++, c++) {
> +	for (i = 0, c = console_cmdline; i < console_cmdline_cnt; i++, c++) {
>  		if (strcmp(c->name, name) == 0 && c->index == idx) {
> -			if (!brl_options)
> -				preferred_console = i;
> +

This extra new line is non-standard and looks slightly weird to me.
I just point it out. I personally do not mind ;-)


> +			if (brl_options)
> +				return 0;
> +
> +			/*
> +			 * Maintain an invariant that will help to find if
> +			 * the matching console is preferred, see
> +			 * register_console():
> +			 *
> +			 * The last non-braille console is always
> +			 * the preferred one.
> +			 */
> +			if (i != console_cmdline_cnt - 1)
> +				swap(console_cmdline[i],
> +				     console_cmdline[console_cmdline_cnt - 1]);
> +
> +			preferred_console = console_cmdline_cnt - 1;
> +
>  			return 0;
>  		}
>  	}
> @@ -2457,12 +2473,24 @@ void register_console(struct console *newcon)
>  	}
>  
>  	/*
> -	 *	See if this console matches one we selected on
> -	 *	the command line.
> +	 * See if this console matches one we selected on the command line.
> +	 *
> +	 * There may be several entries in the console_cmdline array matching
> +	 * with the same console, one with newcon->match(), another by
> +	 * name/index:
> +	 *
> +	 *	pl011,mmio,0x87e024000000,115200 -- added from SPCR
> +	 *	ttyAMA0 -- added from command line
> +	 *
> +	 * Traverse the console_cmdline array in reverse order to be
> +	 * sure that if this console is preferred then it will be the first
> +	 * matching entry.  We use the invariant that is maintained in
> +	 * __add_preferred_console().
>  	 */
> -	for (i = 0, c = console_cmdline;
> -	     i < MAX_CMDLINECONSOLES && c->name[0];
> -	     i++, c++) {
> +	for (i = console_cmdline_cnt - 1; i >= 0; i--) {
> +

Same here.

> +		c = console_cmdline + i;
> +
>  		if (!newcon->match ||
>  		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
>  			/* default matching */

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web