Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <32fde8cd451ea0eaff38108d9f2f2d4a97a43097.camel@kernel.crashing.org>
Subject: Re: [PATCH v2] printk: Fix preferred console selection with
 multiple matches
From:   Benjamin Herrenschmidt <benh@kernel.crashing.org>
To:     Petr Mladek <pmladek@suse.com>
Cc:     linux-kernel@vger.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri, 20 Dec 2019 08:48:24 +1100
In-Reply-To: <20191219135053.xr67lybhycepcxkp@pathway.suse.cz>
References: <2712d7e2fb68bca06a33e2e062fc8e65a2652410.camel@kernel.crashing.org>
         <20191219135053.xr67lybhycepcxkp@pathway.suse.cz>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 2019-12-19 at 14:50 +0100, Petr Mladek wrote:
> 
> > NOTE: This may look convoluted because I'm trying to keep the existing
> > behaviour identical when it comes to things like Braille selection,
> > setup failures, on Braille consoles, or setup failures on normal consoles
> > which all have subtly different results in the current code.
> > 
> > Some of those behaviour are a bit dubious and we might be able to simply
> > rely on CON_ENABLED and CON_BRL flags in newcon after the search but I
> > don't want to change those corner cases in this patch.
> 
> Yes, it is dubious. IMHO, the 5 error codes make it even harder to
> see what happens in which case.

Agreed.

> The code really need simplification. I would prefer to take the risk
> and reduce the amount of added conditions as much as possible.
> I have an idea, see below.

I wanted you to say that :-) I'll rework along those lines. Just a nit
or two:
> 
> 
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2542,6 +2545,53 @@ static int __init keep_bootcon_setup(char *str)
> >  
> >  early_param("keep_bootcon", keep_bootcon_setup);
> >  
> > +enum con_match {
> > +	con_matched,
> > +	con_matched_preferred,
> > +	con_braille,
> > +	con_failed,
> > +	con_no_match,
> > +};
> 
> Please, replace this with int, where:
> 
>    + con_matched -> 0
>    + con_matched_preferred -> 0 and make "has_preferred" global variable
>    + con_braile -> 0		later check for CON_BRL flag
>    + con_failed -> -EFAULT
>    + con_no_match -> -ENOENT

Not fan of using -EFAULT here, it's a detail since it's rather kernel
internal, but I'd rather use -ENXIO for no match and -EIO for failed
(or pass the original error code up if any). That said it's really bike
shed painting at this point :-)
> 
> > @@ -2615,41 +2664,19 @@ void register_console(struct console *newcon)
> > +	/* See if this console matches one we selected on the command line */
> > +	match = try_match_new_console(newcon, true);
> > +	/* If it didn't, try matching the platform ones */
> > +	if (match == con_no_match)
> > +		match = try_match_new_console(newcon, false);
> > +	/* If we matched a Braille console, bail out */
> > +	if (match == con_braille)
> > +		return;
> > +	/* Check if we found a preferred one */
> > +	if (match == con_matched_preferred)
> > +		has_preferred = true;
> >  
> > +	/* If we don't have an enabled console, bail out */
> >  	if (!(newcon->flags & CON_ENABLED))
> >  		return;
> 
> Some of the comments describe what is obvious. I would simplify
> it the following way:
> 
> 	/* Prefer command line over platform specific defaults. */
> 	err = try_match_new_console(newcon, true);
> 	if (err = -ENOENT)
> 		err = try_match_new_console(newcon, false);
> 
> 	/* printk() messages are not printed to Braille consoles. */
> 	if (err || console->flags | CON_BRL)
> 		return;

So this changes the existing behaviour in one way that may or may not
matter, I don't know:

If setup() fails, the existing code will not exit. That means that if
the console has CON_ENABLED already set (some do set it statically or
set it from outside this function, I haven't looked into details the
various circumstances this can happen), the existing code will still
insert it. Your patch will make us not insert it.

> Finally, please split the change into two patches:
> 
> 1st patch will "just" introduce try_match_new_console(console) and
>  use it the following way:
> 
> 	err = try_match_new_console(newcon);
> 
> 	/* printk() messages are not printed to the Braille console. */
> 	if (err || console->flags | CON_BRL)
> 		return;
> 
> 2nd patch will add the user_specified logic.
> 
> This way bisection will distinguish regressions caused
> by the refactoring and by the changed search order.
> 
> Best Regards,
> Petr
> 
> PS: I have vacation between December 23 and January 1. I believe
> that v3 will be ready for merging. Anyway, I will not push it
> into linux-next before I am back from vacation. I would like
> to stay off the computer and do not want to eventually break
> linux-next for too long.

No worries. This isn't super urgent.

Cheers,
Ben.