mbox series

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

Message ID 20170315102854.1763-1-aleksey.makarov@linaro.org
Headers show
Series printk: fix double printing with earlycon | expand

Message

Aleksey Makarov March 15, 2017, 10:28 a.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.

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

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

Comments

Aleksey Makarov March 16, 2017, 10:36 a.m. UTC | #1
On 03/15/2017 07:58 PM, Petr Mladek wrote:
> On Wed 2017-03-15 13:28:52, 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 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.

>>

>> Reported-by: Sudeep Holla <sudeep.holla@arm.com>

>> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>

>> ---

>>  kernel/printk/printk.c | 45 +++++++++++++++++++++++++++++----------------

>>  1 file changed, 29 insertions(+), 16 deletions(-)

>>

>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c

>> index fd752f0c8ef1..7dc53b2831fb 100644

>> --- a/kernel/printk/printk.c

>> +++ b/kernel/printk/printk.c

>> @@ -1902,20 +1902,25 @@ static int __add_preferred_console(char *name, int idx, char *options,

>>  	int i;

>>  

>>  	/*

>> -	 *	See if this tty is not yet registered, and

>> -	 *	if we have a slot free.

>> +	 * Don't check if the console has already been registered, because it is

>> +	 * pointless.  After all, we can not check if two entries refer to

>> +	 * the same console if one is matched with console->match(), and another

>> +	 * by name/index:

>> +	 *

>> +	 *	pl011,mmio,0x87e024000000,115200 -- added from SPCR

>> +	 *	ttyAMA0 -- added from command line

>> +	 *

>> +	 * Also this allows to maintain an invariant that will help to find if

>> +	 * the matching console is preferred, see register_console():

> 

> It is an interesting idea.

> 

> I just wonder if the check for duplicates was there for a serious

> reason. It is hard to say because it was already in the initial git

> commit. In each case, MAX_CMDLINECONSOLES is 8. There is not much

> space for duplicates.

> 

> Note that add_preferred_console() is called also from _probe() calls,

> see

> 

>    uart_add_one_port() -> of_console_check()

>    sunserial_console_match() -> add_preferred_console()

> 

> I wonder they might be called repeatedly, for example because

> of suspend/restore, hotplug, module load/unload.

> 

> I would feel more comfortable if we keep the check for

> duplicates here.


Now I see the problem, thank you.

> It is a pity that the console->match() calls have side effects.

> I still wonder if the 4th version might be more safe. 


I pushed v4 to the linaro git server:

https://git.linaro.org/people/aleksey.makarov/linux.git/commit/?h=amakarov/console2.19.v4&id=47a8227e37ca54d9cc7051abe9b3c2d072f4f75f

The problem with that approach is again a side effect.
Function match_console_name() can change newcon->index.
But it can be called in the very first pass that looks for braille console
and if the call to _braille_register_console() fails,
this newcon with changed index is passed to newcon->match().

This can be fixed by introducing a predicate that checks if the
console_cmdline entry has braille options and calling match_console_name()
only for such consoles, but I think that the code is too convoluted
and the v5 approach is better.

I am going to fix v5 preserving both the check for duplicates
and the invariant, but tell me please if you prefer the v4 approach.

> The

> newcon->setup() call is called only when the console matches.

> AFAIK, there is only one braille console. We should be on

> the safe side if this one does not implement the match()

> callback. Or is it even more complicated?


As you can see from the original code, the check for braille console
was performed in that branch of code where we missed newcon->match(),
so yes, it looks like braille console(s) does not have the match() method.
I used that in v4 to factor out matching for braille from the loop.

Thank you
Aleksey Makarov

> To be honest, I am not much familiar with the console registration

> code. I am still trying to get a better picture. It is pity that

> many function have unexpected side effects.

> 

> Best Regards,

> Petr

> --

> 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

> 



--
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
Petr Mladek March 16, 2017, 1:54 p.m. UTC | #2
On Thu 2017-03-16 13:36:35, Aleksey Makarov wrote:
> 

> 

> On 03/15/2017 07:58 PM, Petr Mladek wrote:

> > On Wed 2017-03-15 13:28:52, 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.

> >>

> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c

> >> index fd752f0c8ef1..7dc53b2831fb 100644

> >> --- a/kernel/printk/printk.c

> >> +++ b/kernel/printk/printk.c

> >> @@ -1902,20 +1902,25 @@ static int __add_preferred_console(char *name, int idx, char *options,

> >>  	int i;

> >>  

> >>  	/*

> >> -	 *	See if this tty is not yet registered, and

> >> -	 *	if we have a slot free.

> >> +	 * Don't check if the console has already been registered, because it is

> >> +	 * pointless.  After all, we can not check if two entries refer to

> >> +	 * the same console if one is matched with console->match(), and another

> >> +	 * by name/index:

> >> +	 *

> >> +	 *	pl011,mmio,0x87e024000000,115200 -- added from SPCR

> >> +	 *	ttyAMA0 -- added from command line

> >> +	 *

> >> +	 * Also this allows to maintain an invariant that will help to find if

> >> +	 * the matching console is preferred, see register_console():

> > 

> > It is an interesting idea.

> > 

> > I just wonder if the check for duplicates was there for a serious

> > reason. It is hard to say because it was already in the initial git

> > commit. In each case, MAX_CMDLINECONSOLES is 8. There is not much

> > space for duplicates.

> > 

> > Note that add_preferred_console() is called also from _probe() calls,

> > see

> > 

> >    uart_add_one_port() -> of_console_check()

> >    sunserial_console_match() -> add_preferred_console()

> > 

> > I wonder they might be called repeatedly, for example because

> > of suspend/restore, hotplug, module load/unload.

> > 

> > I would feel more comfortable if we keep the check for

> > duplicates here.

> 

> Now I see the problem, thank you.

> 

> > It is a pity that the console->match() calls have side effects.

> > I still wonder if the 4th version might be more safe. 

> 

> I pushed v4 to the linaro git server:

> 

> https://git.linaro.org/people/aleksey.makarov/linux.git/commit/?h=amakarov/console2.19.v4&id=47a8227e37ca54d9cc7051abe9b3c2d072f4f75f

> 

> The problem with that approach is again a side effect.

> Function match_console_name() can change newcon->index.

> But it can be called in the very first pass that looks for braille console

> and if the call to _braille_register_console() fails,

> this newcon with changed index is passed to newcon->match().


I see. We need to make sure that newcon->match() will not get called
when checking for the braille console.


> This can be fixed by introducing a predicate that checks if the

> console_cmdline entry has braille options and calling match_console_name()

> only for such consoles, but I think that the code is too convoluted

> and the v5 approach is better.


I personally prefer v4. The braille console adds an unexpected
twist into the code because it skips the rest of the function.
IMHO, it is better when it is is tested separately with clear
conditions and comments.

I would personally add the following into
kernel/printk/console_cmdline.h

#define for_each_console_cmdline(c, i) \
	for (i = 0, c = console_cmdline;		\
	     i < MAX_CMDLINECONSOLES && c->name[0];	\
	     i++, c++)

It can be used in both __add_preferred_console()
and register_console().

Also I would add the following into kernel/printk/braille.h

static inline bool
is_braille_console(struct console_cmdline *c)
{
	return !!c->brl_options;
}

Then the braille-specific code in register_console() might
look like:

	/*
	 * Braille console is handled a very special way.
	 * Is is not listed in the console_drivers list.
	 * Instead it registers its own keyboard and vt notifiers.
	 *
	 * Be careful and avoid calling c->match() here
	 * because it might have side effects!
	 */
	if (is_braille_console(c) &&
	    match_console_name(newcon, c) &&
	    _braille_register_console(newcon, c))
		return;


Finally, I would prefer to move

	newcon->flags |= CON_ENABLED;

outside the match_console() function. The function will still
have side effects because of the c->match() calls. But we should
not make it worse. In fact, it would be great to remove side effects
from the c->match() functions in the long term (not in this patch set).

> I am going to fix v5 preserving both the check for duplicates

> and the invariant, but tell me please if you prefer the v4 approach.


I guess that you planed to shuffle console_cmdline entries to keep
the preferred console first/last. I am afraid that it won't be
a nice code either. But I might be wrong.


> > The

> > newcon->setup() call is called only when the console matches.

> > AFAIK, there is only one braille console. We should be on

> > the safe side if this one does not implement the match()

> > callback. Or is it even more complicated?

> 

> As you can see from the original code, the check for braille console

> was performed in that branch of code where we missed newcon->match(),

> so yes, it looks like braille console(s) does not have the match() method.

> I used that in v4 to factor out matching for braille from the loop.


The check for blr_options is sufficient and better.

I suggest to wait at least two days until you spend to much time
on reshuffling the code. It is possible that others would prefer
v5 or suggest even other solution.

Best Regards,
Petr
--
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
Aleksey Makarov March 17, 2017, 10:32 a.m. UTC | #3
On 03/16/2017 04:54 PM, Petr Mladek wrote:
> On Thu 2017-03-16 13:36:35, Aleksey Makarov wrote:

>>

>>

>> On 03/15/2017 07:58 PM, Petr Mladek wrote:

>>> On Wed 2017-03-15 13:28:52, Aleksey Makarov wrote:


[..]

> I personally prefer v4. The braille console adds an unexpected

> twist into the code because it skips the rest of the function.

> IMHO, it is better when it is is tested separately with clear

> conditions and comments.

> 

> I would personally add the following into

> kernel/printk/console_cmdline.h

> 

> #define for_each_console_cmdline(c, i) \

> 	for (i = 0, c = console_cmdline;		\

> 	     i < MAX_CMDLINECONSOLES && c->name[0];	\

> 	     i++, c++)

> 

> It can be used in both __add_preferred_console()

> and register_console().


Ok

> Also I would add the following into kernel/printk/braille.h

> 

> static inline bool

> is_braille_console(struct console_cmdline *c)

> {

> 	return !!c->brl_options;

> }

> 

> Then the braille-specific code in register_console() might

> look like:

> 

> 	/*

> 	 * Braille console is handled a very special way.

> 	 * Is is not listed in the console_drivers list.

> 	 * Instead it registers its own keyboard and vt notifiers.

> 	 *

> 	 * Be careful and avoid calling c->match() here

> 	 * because it might have side effects!

> 	 */

> 	if (is_braille_console(c) &&

> 	    match_console_name(newcon, c) &&

> 	    _braille_register_console(newcon, c))

> 		return;


Yes, this is exactly what I was going to do.

> Finally, I would prefer to move

> 

> 	newcon->flags |= CON_ENABLED;

> 

> outside the match_console() function. The function will still

> have side effects because of the c->match() calls. But we should

> not make it worse. In fact, it would be great to remove side effects

> from the c->match() functions in the long term (not in this patch set).


I see the motivation but I am afraid this is not possible until
side effects are removed from newcon->match() and match_console().
The point is that match_console() also calls newcon->setup() (side effect)
and this CON_ENABLED flag indicates if this call was successful.

>> I am going to fix v5 preserving both the check for duplicates

>> and the invariant, but tell me please if you prefer the v4 approach.

> 

> I guess that you planed to shuffle console_cmdline entries to keep

> the preferred console first/last. I am afraid that it won't be

> a nice code either. But I might be wrong.


You are right, I tried that, the code was awful.

Thank you
Aleksey Makarov

>>> The

>>> newcon->setup() call is called only when the console matches.

>>> AFAIK, there is only one braille console. We should be on

>>> the safe side if this one does not implement the match()

>>> callback. Or is it even more complicated?

>>

>> As you can see from the original code, the check for braille console

>> was performed in that branch of code where we missed newcon->match(),

>> so yes, it looks like braille console(s) does not have the match() method.

>> I used that in v4 to factor out matching for braille from the loop.

> 

> The check for blr_options is sufficient and better.

> 

> I suggest to wait at least two days until you spend to much time

> on reshuffling the code. It is possible that others would prefer

> v5 or suggest even other solution.

> 

> Best Regards,

> Petr

> --

> 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

> 

--
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
Aleksey Makarov March 17, 2017, 1:34 p.m. UTC | #4
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;

>  

> 


--
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
Petr Mladek March 27, 2017, 2:14 p.m. UTC | #5
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
--
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
Aleksey Makarov March 27, 2017, 4:28 p.m. UTC | #6
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
--
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 28, 2017, 2:04 a.m. UTC | #7
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
--
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