Date:   Mon, 5 Sep 2022 15:43:53 +0100
From:   Chris Down <chris@...isdown.name>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        John Ogness <john.ogness@...utronix.de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>, kernel-team@...com
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

Powered by blists - more mailing lists