Messages in this thread | | | Date | Thu, 22 Sep 2022 17:17:20 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options |
| |
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
| |