diff mbox series

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

Message ID 20170303154946.15399-1-aleksey.makarov@linaro.org
State New
Headers show
Series None | expand

Commit Message

Aleksey Makarov March 3, 2017, 3:49 p.m. UTC 
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(-)

-- 
2.11.1

Comments

Aleksey Makarov March 7, 2017, 2:54 p.m. UTC | #1 
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

>
Sergey Senozhatsky March 8, 2017, 5:33 a.m. UTC | #2 
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
diff mbox series

Patch

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;