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

Groups > linux.kernel > #1617352 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [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

#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] | [next] | [standalone]

#1617392

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

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

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