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

Groups > linux.kernel > #1591069 > unrolled thread

[PATCH v2 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 v2 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-02 14:20 +0100
    Re: [PATCH v2 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-02 15:40 +0100
    [PATCH v3 3/3] printk: fix double printing with earlycon Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-03 17:00 +0100
      Re: [PATCH v3 3/3] printk: fix double printing with earlycon Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2017-03-06 16:10 +0100
        Re: [PATCH v3 3/3] printk: fix double printing with earlycon Aleksey Makarov <amakarov.linux@gmail.com> - 2017-03-07 16:10 +0100
          Re: [PATCH v3 3/3] printk: fix double printing with earlycon Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-08 06:40 +0100
            Re: [PATCH v3 3/3] printk: fix double printing with earlycon Aleksey Makarov <amakarov.linux@gmail.com> - 2017-03-08 16:00 +0100

#1591069 — [PATCH v2 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, match the console against the `console_cmdline` entry
pointed by `preferred_console` instead of the first match.

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

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

[toc] | [next] | [standalone]

#1591139

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

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

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

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

All the best
Aleksey Makarov

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

#1592083 — [PATCH v3 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, match the console against the `console_cmdline` entry
pointed by `preferred_console` and if that fails, match against other
entries.  The problem is that newcon->match() is not just a match
function, it calls setup() so it has side effects.

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

v2 -> v3:

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

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

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

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

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

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

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

> +	if (!newcon->match ||
> +	    newcon->match(newcon, c->name, c->index, c->options) != 0) {
> +		/* default matching */
> +		BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
> +		if (strcmp(c->name, newcon->name) != 0)
> +			return CONSOLE_MATCH_NEXT;
> +		if (newcon->index >= 0 && newcon->index != c->index)
> +			return CONSOLE_MATCH_NEXT;

who is checking CONSOLE_MATCH_NEXT?

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

			newcon->flags & CON_ENABLED  ?

> +				newcon->flags |= CON_CONSDEV;
> +				has_preferred = true;
> +			}
> +			goto match;
> +		case CONSOLE_MATCH_RETURN:
> +			return;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	/*
>  	 *	See if this console matches one we selected on
>  	 *	the command line.
>  	 */
>  	for (i = 0, c = console_cmdline;
>  	     i < MAX_CMDLINECONSOLES && c->name[0];
>  	     i++, c++) {
> -		if (!newcon->match ||
> -		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
> -			/* default matching */
> -			BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
> -			if (strcmp(c->name, newcon->name) != 0)
> -				continue;
> -			if (newcon->index >= 0 &&
> -			    newcon->index != c->index)
> -				continue;
> -			if (newcon->index < 0)
> -				newcon->index = c->index;
> -
> -			if (_braille_register_console(newcon, c))
> -				return;
>  
> -			if (newcon->setup &&
> -			    newcon->setup(newcon, c->options) != 0)
> -				break;
> -		}
> +		if (preferred_console == i)
> +			continue;
>  
> -		newcon->flags |= CON_ENABLED;
> -		if (i == preferred_console) {
> -			newcon->flags |= CON_CONSDEV;
> -			has_preferred = true;
> +		switch (match_console(newcon, c)) {
> +		case CONSOLE_MATCH:
> +			goto match;
> +		case CONSOLE_MATCH_RETURN:
> +			return;
> +		default:
> +			break;

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

	-ss

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

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

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

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

>> +	if (!newcon->match ||
>> +	    newcon->match(newcon, c->name, c->index, c->options) != 0) {
>> +		/* default matching */
>> +		BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
>> +		if (strcmp(c->name, newcon->name) != 0)
>> +			return CONSOLE_MATCH_NEXT;
>> +		if (newcon->index >= 0 && newcon->index != c->index)
>> +			return CONSOLE_MATCH_NEXT;
>
> who is checking CONSOLE_MATCH_NEXT?

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

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

Sure.  Thank you.

>> +				newcon->flags |= CON_CONSDEV;
>> +				has_preferred = true;
>> +			}
>> +			goto match;
>> +		case CONSOLE_MATCH_RETURN:
>> +			return;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
>> +	/*
>>  	 *	See if this console matches one we selected on
>>  	 *	the command line.
>>  	 */
>>  	for (i = 0, c = console_cmdline;
>>  	     i < MAX_CMDLINECONSOLES && c->name[0];
>>  	     i++, c++) {
>> -		if (!newcon->match ||
>> -		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
>> -			/* default matching */
>> -			BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
>> -			if (strcmp(c->name, newcon->name) != 0)
>> -				continue;
>> -			if (newcon->index >= 0 &&
>> -			    newcon->index != c->index)
>> -				continue;
>> -			if (newcon->index < 0)
>> -				newcon->index = c->index;
>> -
>> -			if (_braille_register_console(newcon, c))
>> -				return;
>>
>> -			if (newcon->setup &&
>> -			    newcon->setup(newcon, c->options) != 0)
>> -				break;
>> -		}
>> +		if (preferred_console == i)
>> +			continue;
>>
>> -		newcon->flags |= CON_ENABLED;
>> -		if (i == preferred_console) {
>> -			newcon->flags |= CON_CONSDEV;
>> -			has_preferred = true;
>> +		switch (match_console(newcon, c)) {
>> +		case CONSOLE_MATCH:
>> +			goto match;
>> +		case CONSOLE_MATCH_RETURN:
>> +			return;
>> +		default:
>> +			break;
>
> sorry, it was a rather long for me today. need to look more at this.
> for what is now CONSOLE_MATCH_NEXT we used to have continue,

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

> and now we break out of the loop?

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

Thank you
Aleksey Makarov

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

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

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

Hello,

sorry for the delay.

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

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

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

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

or something like this

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

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

[..]
> > > +	/*
> > >  	 *	See if this console matches one we selected on
> > >  	 *	the command line.
> > >  	 */
> > >  	for (i = 0, c = console_cmdline;
> > >  	     i < MAX_CMDLINECONSOLES && c->name[0];
> > >  	     i++, c++) {
> > > -		if (!newcon->match ||
> > > -		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
> > > -			/* default matching */
> > > -			BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
> > > -			if (strcmp(c->name, newcon->name) != 0)
> > > -				continue;
> > > -			if (newcon->index >= 0 &&
> > > -			    newcon->index != c->index)
> > > -				continue;
> > > -			if (newcon->index < 0)
> > > -				newcon->index = c->index;
> > > -
> > > -			if (_braille_register_console(newcon, c))
> > > -				return;
> > > 
> > > -			if (newcon->setup &&
> > > -			    newcon->setup(newcon, c->options) != 0)
> > > -				break;
> > > -		}
> > > +		if (preferred_console == i)
> > > +			continue;
> > > 
> > > -		newcon->flags |= CON_ENABLED;
> > > -		if (i == preferred_console) {
> > > -			newcon->flags |= CON_CONSDEV;
> > > -			has_preferred = true;
> > > +		switch (match_console(newcon, c)) {
> > > +		case CONSOLE_MATCH:
> > > +			goto match;
> > > +		case CONSOLE_MATCH_RETURN:
> > > +			return;
> > > +		default:
> > > +			break;
> > 
> > sorry, it was a rather long for me today. need to look more at this.
> > for what is now CONSOLE_MATCH_NEXT we used to have continue,
> 
> CONSOLE_MATCH is for the case when the console matches against the description,
> CONSOLE_MATCH_NEXT - it does not, we should try next,

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

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

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

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



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

	match_console() returned CONSOLE_MATCH_STOP

is a bit better than

	match_console() returned CONSOLE_MATCH_RETURN.

isn't it? :)


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

	-ss

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

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

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

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

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

Thank you
Aleksey Makarov

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web