Date: Fri, 24 May 2024 18:06:21 +0200
From: Petr Mladek <pmladek@...e.com>
To: Tony Lindgren <tony@...mide.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, Jonathan Corbet <corbet@....net>,
	Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	"David S . Miller" <davem@...emloft.net>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Dhruva Gole <d-gole@...com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Johan Hovold <johan@...nel.org>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Vignesh Raghavendra <vigneshr@...com>, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org, Sebastian Reichel <sre@...nel.org>,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v7 1/7] printk: Save console options for
 add_preferred_console_match()

Hi,

I have finally found time to looks at this again.

On Wed 2024-03-27 12:59:35, Tony Lindgren wrote:
> Driver subsystems may need to translate the preferred console name to the
> character device name used. We already do some of this in console_setup()
> with a few hardcoded names, but that does not scale well.
> 
> The console options are parsed early in console_setup(), and the consoles
> are added with __add_preferred_console(). At this point we don't know much
> about the character device names and device drivers getting probed.
> 
> To allow driver subsystems to set up a preferred console, let's save the
> kernel command line console options. To add a preferred console from a
> driver subsystem with optional character device name translation, let's
> add a new function add_preferred_console_match().
> 
> This allows the serial core layer to support console=DEVNAME:0.0 style
> hardware based addressing in addition to the current console=ttyS0 style
> naming. And we can start moving console_setup() character device parsing
> to the driver subsystem specific code.
> 
> We use a separate array from the console_cmdline array as the character
> device name and index may be unknown at the console_setup() time. And
> eventually there's no need to call __add_preferred_console() until the
> subsystem is ready to handle the console.
>
> Adding the console name in addition to the character device name, and a
> flag for an added console, could be added to the struct console_cmdline.
> And the console_cmdline array handling could be modified accordingly. But
> that complicates things compared saving the console options, and then
> adding the consoles when the subsystems handling the consoles are ready.

Honestly, I think that the separate array was a bad decision.
It breaks the preferred console handling.
Also I wonder if this duplicates another matching.

Let me explain this in in more details.

First, about breaking the preferred console:

The patchset still causes the regression with /dev/console association
as already reported for v3, see
https://lore.kernel.org/r/ZWnvc6-LnXdjOQLY@alley

I used the following kernel command line:

   earlycon=uart8250,io,0x3f8,115200 console=ttyS0,115200 console=tty0 ignore_loglevel log_buf_len=1M

The patchset caused that /dev/console became associated with
ttyS0 instead of tty0, see the "C" flag:

	original # cat /proc/consoles
	tty0                 -WU (EC    )    4:1
	ttyS0                -W- (E  p a)    4:64

   vs.

	patched # cat /proc/consoles
	ttyS0                -W- (EC p a)    4:64
	tty0                 -WU (E     )    4:1


I have added some debugging messages which nicely show the reason.
In the original code, __add_preferred_console() is called twice
when processing the command line:

[    0.099312] __add_preferred_console[0]: ttyS, 0 (preferrred_console == 0)
[    0.099982] __add_preferred_console[1]: tty, 0 (preferrred_console == 1)

The patchset causes that it is called once again here:

[    0.216268] __add_preferred_console: Updating preferred console: ttyS, 0 [0]
[    0.216271] task:swapper/0       state:R  running task     stack:0     pid:0     tgid:0     ppid:0      flags:0x00000000
[    0.216318] Call Trace:
[    0.216327]  <TASK>
[    0.216337]  sched_show_task.part.0+0x1dd/0x1e7
[    0.216355]  __add_preferred_console.part.0.cold+0x29/0xa4
[    0.216374]  add_preferred_console_match+0x8e/0xb0
[    0.216391]  serial_base_add_prefcon+0x9c/0x140
[    0.216408]  serial8250_isa_init_ports+0x144/0x160
[    0.216423]  ? __pfx_univ8250_console_init+0x10/0x10
[    0.216439]  univ8250_console_init+0x1c/0x30
[    0.216452]  console_init+0x122/0x1c0
[    0.216466]  start_kernel+0x44a/0x590
[    0.216480]  x86_64_start_reservations+0x24/0x30
[    0.216493]  x86_64_start_kernel+0x90/0x90
[    0.216506]  common_startup_64+0x13e/0x141
[    0.216528]  </TASK>

This extra call tries to add "ttyS, 0" once again and it hits this
code:

static int __add_preferred_console(const char *name, const short idx, char *options,
				   char *brl_options, bool user_specified)
{
[...]
	/*
	 *	See if this tty is not yet registered, and
	 *	if we have a slot free.
	 */
	for (i = 0, c = console_cmdline;
	     i < MAX_CMDLINECONSOLES && c->name[0];
	     i++, c++) {
		if (strcmp(c->name, name) == 0 && c->index == idx) {
			if (!brl_options)
---->				preferred_console = i;
			set_user_specified(c, user_specified);
			return 0;
		}
	}


The code thinks that "ttyS0" has been mentioned on the command line
once again. And preferred_console is _wrongly_ set back to '0'.

My view:

The delayed __add_preferred_console() is a way to hell.

The preferences are defined by the ordering on the command line.
All entries have to be added when the command line options are
being proceed to keep the order.

A solution might be to store "devname" separately in
struct console_cmdline and allow empty "name". We could
implement then a function similar to
add_preferred_console_match() which would try to match
"devname" and set/update "name", "index" value when matched.

Note that we might also need to add some synchronization
if it might be possible to modify struct console_cmdline
in parallel.


Second, about the possible duplication:

I might get it wrong. IMHO, in principle, this patchset tries
to achieve similar thing as the "match()" callback, see
the commit c7cef0a84912cab3c9 ("console: Add extensible
console matching").

The .match() callback in struct console is to match, for example,
console=uart8250,io,0x3f8,115200 when the uart8250 driver
calls register_console() when it is being properly initialized
as "ttyS".

BTW: The .match() needs saved options because it internally calls
     .setup() callback. IMHO, this is a very ugly detail
     which complicates design of the register_console() code.


Both approaches try to match a "driver/device-specific name" with
the generic "ttySX".

    console=uart8250,io,0x3f8,115200	=> ttyS0
vs.
    console=00:00:0.0,115200		=> ttyS0


Where console=uart8250,io,0x3f8,115200 is handled by:

    - "uart" is added to console_cmdline[]
    - matched directly via newcon->match() callback

vs. console=00:00:0.0,115200

    - 00:00:0.0 is added to conopt[]
    - "ttyS0" added to console_cmdline[] when "00:00:0.0" initialized
    - "ttyS0" is then matched directly


Question: Would it it be able to use the existing .match() callback
	  also to match the DEVNAME?

	  Or somehow reuse the approach?

	  Could register_console() know how to generate possible
	  DEVNAME for the given struct console?


Best Regards,
Petr

Powered by blists - more mailing lists