Messages in this thread | | | Date | Mon, 5 Sep 2022 15:43:53 +0100 | From | Chris Down <> | Subject | Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options |
| |
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? :-)
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.
---
/* Only change is pr_err to fprintf */
#define _DEFAULT_SOURCE
#include <stdio.h> #include <string.h> #include <stdbool.h> #include <ctype.h> #include <stdlib.h>
#define clamp(a, b, c) (a) #define CON_LOGLEVEL 128
struct console_cmdline { char *options; int level; short flags; };
static void parse_console_cmdline_options(struct console_cmdline *c, char *options) { bool seen_serial_opts = false; char *key;
while ((key = strsep(&options, ",")) != NULL) { char *value;
value = strchr(key, ':'); if (value) *(value++) = '\0';
if (strcmp(key, "loglevel") == 0 && value && isdigit(value[0]) && strlen(value) == 1) { c->level = clamp(value[0] - '0', LOGLEVEL_EMERG, LOGLEVEL_DEBUG + 1); c->flags |= CON_LOGLEVEL; continue; }
if (!seen_serial_opts && isdigit(key[0]) && !value) { seen_serial_opts = true; c->options = key; continue; }
fprintf(stderr, "ignoring invalid console option: '%s%s%s'\n", key, value ? ":" : "", value ?: ""); } }
int main(int argc, char *argv[]) { struct console_cmdline con = { 0 };
if (argc != 2) return EXIT_FAILURE;
parse_console_cmdline_options(&con, argv[1]); printf("options: %s\n", con.options); printf("level: %d\n", con.level); }
---
% 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?
>I thought a lot how to do it a clean way. IMHO, it would be great to >parse everything at a single place but it might require updating >all drivers. I am not sure if it is worth it. > >So, I suggest to do it another way. We could implement a generic >function to find in the new key[:value] format. It would check >if the given option (key) exists and read the optional value. > >The optional value would allow to define another new options >that would not need any value, e.g. "kthread" or "atomic" that >might be used in the upcoming code that allows to offload >console handling to kthreads.
This could also work, and avoids the current null pointer shoving. It also can make the ordering less strict, which seems like a good thing.
I will think about it some more. I'm curious about your comments on the above test still though.
Thanks a lot for the detailed review and ideas!
Chris
| |