Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Sep 2022 17:17:20 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Chris Down <chris@chrisdown.name>
Cc:     linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        John Ogness <john.ogness@linutronix.de>,
        Geert Uytterhoeven <geert@linux-m68k.org>, kernel-team@fb.com
Subject: Re: [PATCH v3 1/2] printk: console: Create console= parser that
 supports named options
Message-ID: <Yyx8gGNTe6VWPC6Y@alley>
References: <cover.1658339046.git.chris@chrisdown.name>
 <732ee897b2bd49ada3f7dee396475c5a2195071b.1658339046.git.chris@chrisdown.name>
 <Yw80MmyBl4js09If@alley>
 <YxYLKU8TWZ4T5ONF@chrisdown.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YxYLKU8TWZ4T5ONF@chrisdown.name>
Precedence: bulk

On Mon 2022-09-05 15:43:53, Chris Down wrote:
> Hi Petr,
> 
> Thanks a lot for looking this over!
> 
> Petr Mladek writes:
> > I think that the function does not work as expected.
> > 
> > > +	bool seen_serial_opts = false;
> > > +	char *key;
> > > +
> > > +	while ((key = strsep(&options, ",")) != NULL) {
> > > +		char *value;
> > 
> > strsep() replaces ',' with '\0'.
> > 
> > > +
> > > +		value = strchr(key, ':');
> > > +		if (value)
> > > +			*(value++) = '\0';
> > 
> > This replaces ':' with '\0'.
> > 
> > > +
> > > +		if (!seen_serial_opts && isdigit(key[0]) && !value) {
> > 
> > This catches the classic options in the format "bbbbpnf".
> > 
> > > +			seen_serial_opts = true;
> > > +			c->options = key;
> > > +			continue;
> > > +		}
> > > +
> > > +		pr_err("ignoring invalid console option: '%s%s%s'\n", key,
> > > +		       value ? ":" : "", value ?: "");
> > 
> > IMHO, this would warn even about "io", "mmio", ...  that are used by:
> 
> Oh dear, I should have known it won't be that simple :-D
> 
> > 
> > 		console=uart[8250],io,<addr>[,options]
> > 		console=uart[8250],mmio,<addr>[,options]
> > 
> > Warning: I am not completely sure about this. It seems that
> > 	 this format is handled by univ8250_console_match().
> > 
> > 	 IMHO, the "8250" is taken as "idx" in console_setup().
> > 	 And "idx" parameter is ignored by univ8250_console_match().
> > 	 This probably explains why "8250" is optional in console=
> > 	 parameter.
> > 
> > > +	}
> > > +}
> > 
> > Sigh, the parsing of console= parameter is really hacky. Very old code.
> > The name and idx is handled in console_setup(). The rest
> > is handled by particular drivers via "options".
> > 
> > This patch makes it even more tricky. It tries to handle some
> > *options in add_preferred_console(). But it replaces all ','
> > and ':' by '\0' so that drivers do not see the original *options
> > any longer.
> 
> Other than the mmio/io stuff, I think it works properly, right? Maybe I'm
> missing something? :-)

One important thing is to do not warn about mmio/io when they are
valid options.

Another important thing is to restore *options string. I mean to
revert all the added '\0' when they are not longer needed.

The *options buffer is passed as paramter to both con->match()
and con->setup() callbacks, see try_enable_preferred_console().
They must be able to see all the values.

Well, I would remove the newly added "logleve:X" when it is handled
in __add_preferred_console(). Otherwise, we would need to double
check all con->match() and con->setup() callbacks that they are
able to handle (ignore) this new option.


> Here's a userspace test for the parser that seems to work.
> parse_console_cmdline_options() should also ignore empty options instead of
> warning, but it still functions correctly in that case, it's just noisy
> right now.

> % make CFLAGS='-Wall -Wextra -Werror' loglevel
> cc -Wall -Wextra -Werror    loglevel.c   -o loglevel
> % ./loglevel 9600n8
> options: 9600n8
> level: 0
> level set: 0
> % ./loglevel 9600n8,loglevel:3
> options: 9600n8
> level: 3
> level set: 1
> % ./loglevel 9600n8,loglevel:123
> ignoring invalid console option: 'loglevel:123'
> options: 9600n8
> level: 0
> level set: 0
> % ./loglevel 9600n8,loglevel:3,foo:bar
> ignoring invalid console option: 'foo:bar'
> options: 9600n8
> level: 3
> level set: 1
> % ./loglevel 9600n8,loglevel
> ignoring invalid console option: 'loglevel'
> options: 9600n8
> level: 0
> level set: 0
> % ./loglevel loglevel
> ignoring invalid console option: 'loglevel'
> options: (null)
> level: 0
> level set: 0
> % ./loglevel loglevel:7
> options: (null)
> level: 7
> level set: 1

> Seems to work ok as far as I can tell, maybe I've misunderstood your
> concern?  Or maybe your concern is just about the mmio/io case where the
> driver wants that as part of the options?

Yes, the mmio/io stuff is the known problem.

My concern is that the function is destructive. It modifies the given
*options buffer that it later passed to another functions. It means
that it modifies the existing behavior.

I am afraid that you might just add an extra check to keep the
particular mmio/io parameters. But I am not sure if these are there only
special parameters used by console drivers.

We found about mmio/io parameters because they were mentioned in
Documentation/admin-guide/kernel-parameters.txt. But there might
be another parameters used by some exotic console that are documented
somewhere else.

I would prefer to do it the other (conservative) way around. I mean
that the new parser in __add_preferred_console() will only search
for the newly added ",loglevel:X" option and remove it from *options
buffer. Everything else should stay in the buffer so that it can
be parsed by the particular console drivers.

It actually already works this way. console_setup() searches for
"tty[S]X" prefix and it "eats" this prefix. The rest of *str buffer
is passed via *options parameter down to __add_preferred_console().

Best Regards,
Petr