Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

Petr Mladek
2022-09-22T15:17:28Z

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