Hi all,

With the recent serial core changes, we can now add DEVNAME:0.0 style
addressing for the serial ports. When using DEVNAME:0.0 naming, we don't
need to care which ttyS instance number is allocated depending on HSUART
settings or if the devicetree has added aliases for all the ports.

This also allows us to also drop the old console_setup() parsing for
character device names.

Regards,

Tony

Changes since v2:

- Console name got constified and already applied as suggested by Ilpo
  and Andy

- Add printk/conopt.c to save console command line options

- Add a patch to drop old console_setup() character device name parsing

- Use cleanup.h to simplify freeing as suggested by Andy

- Use types.h instead of kernel.h as suggested by Andy

- Use strcspn() as suggested by Andy

- Various coding improvments suggested by Andy

Changes since v1:

- Constify printk add_preferred_console() as suggested by Jiri

- Use proper kernel command line helpers for parsing console as
  suggested by Jiri

- Update description for HSUART based on Andy's comments

- Standardize on DEVNAME:0.0 style naming as suggested by Andy

- Added missing put_device() calls paired with device_find_child()

Tony Lindgren (3):
  printk: Save console options for add_preferred_console_match()
  serial: core: Add support for DEVNAME:0.0 style naming for kernel
    console
  serial: core: Move console character device handling from printk

 drivers/tty/serial/serial_base.h     |  14 ++++
 drivers/tty/serial/serial_base_bus.c | 104 ++++++++++++++++++++++++
 drivers/tty/serial/serial_core.c     |   4 +
 include/linux/printk.h               |   3 +
 kernel/printk/Makefile               |   2 +-
 kernel/printk/conopt.c               | 115 +++++++++++++++++++++++++++
 kernel/printk/console_cmdline.h      |   4 +
 kernel/printk/printk.c               |  41 +++-------
 8 files changed, 254 insertions(+), 33 deletions(-)
 create mode 100644 kernel/printk/conopt.c

-- 
2.42.1

On Tue 2023-11-21 13:31:54, Tony Lindgren wrote:
> Hi all,
> 
> With the recent serial core changes, we can now add DEVNAME:0.0 style
> addressing for the serial ports. When using DEVNAME:0.0 naming, we don't
> need to care which ttyS instance number is allocated depending on HSUART
> settings or if the devicetree has added aliases for all the ports.
> 
> This also allows us to also drop the old console_setup() parsing for
> character device names.
> 
> Tony Lindgren (3):
>   printk: Save console options for add_preferred_console_match()
>   serial: core: Add support for DEVNAME:0.0 style naming for kernel
>     console
>   serial: core: Move console character device handling from printk

First, I appreciate the effort to match aliases to the same console.

Well, my understanding is that it solves the problem only for the newly
added console=DEVICENAME:0.0 format. But it does not handle the
existing problems with matching console names passed via earlycon=
and console= parameters. Am I right?

Now, the bad news. This patchset causes regressions which are
not acceptable. I have found two so far but there might be more.

I used the following kernel command line:

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


1. 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

   This is most likely caused by the different ordering of
   __add_preferred_console() calls.

   The ordering is important because it defines which console
   will get associated with /dev/console. It is a so called
   preferred console defined by the last console= parameter.

   Unfortunately also the ordering of the other parameters
   is important when a console defined by the last console=
   parameter is not registered at all. In this case,
   /dev/console gets associated with the first console
   with tty binding according to the order on the command line.

   If you think that it is weird behavior then I agree.
   But it is a historical mess. It is how people used it
   when the various features were added. Many changes
   in this code caused regressions and had to be reverted.

   See the following to get the picture:

       + commit c6c7d83b9c9e6a8 ("Revert "console: don't
	 prefer first registered if DT specifies stdout-path")

       + commit dac8bbbae1d0ccb ("Revert "printk: fix double
	 printing with earlycon"").


2. The serial console gets registered much later with this
   patchset:

	original # dmesg | grep printk:
	[    0.000000] printk: legacy bootconsole [uart8250] enabled
	[    0.000000] printk: debug: ignoring loglevel setting.
	[    0.016859] printk: log_buf_len: 1048576 bytes
	[    0.017324] printk: early log buf free: 259624(99%)
	[    0.141859] printk: legacy console [tty0] enabled
	[    0.142399] printk: legacy bootconsole [uart8250] disabled
	[    0.143032] printk: legacy console [ttyS0] enabled

   vs.

	patched # dmesg | grep printk:
	[    0.000000] printk: legacy bootconsole [uart8250] enabled
	[    0.000000] printk: debug: ignoring loglevel setting.
	[    0.018142] printk: log_buf_len: 1048576 bytes
	[    0.018757] printk: early log buf free: 259624(99%)
	[    0.160706] printk: legacy console [tty0] enabled
	[    0.161213] printk: legacy bootconsole [uart8250] disabled
	[    1.592929] printk: legacy console [ttyS0] enabled

   This is pretty bad because it would complicate or even prevent
   debugging of the boot stage via serial console.

   The graphical console is not usable when the system dies. Also
   finding the right arguments for the earlycon= parameter is
   tricky so that people enable it only when they have to debug
   very early messages.


I am going to look at the patches more closely to see if I could
provide some hints.

Best Regards,
Petr

* Petr Mladek <pmladek@suse.com> [231201 14:36]:
> Well, my understanding is that it solves the problem only for the newly
> added console=DEVICENAME:0.0 format. But it does not handle the
> existing problems with matching console names passed via earlycon=
> and console= parameters. Am I right?

Yes that's where the remaining problems are.

> Now, the bad news. This patchset causes regressions which are
> not acceptable. I have found two so far but there might be more.
> 
> I used the following kernel command line:
> 
>    earlycon=uart8250,io,0x3f8,115200 console=ttyS0,115200 console=tty0 ignore_loglevel log_buf_len=1M
> 
> 
> 1. 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
> 
>    This is most likely caused by the different ordering of
>    __add_preferred_console() calls.

Yes I noticed that too. We can't drop the console parsing from
console_setup() until we have some solution for flagging
register_console() that we do have a console specified on the
kernel command line and try_enable_default_console() should not
be called. It seems some changes to the console_set_on_cmdline
handling might do the trick here.

>    The ordering is important because it defines which console
>    will get associated with /dev/console. It is a so called
>    preferred console defined by the last console= parameter.
> 
>    Unfortunately also the ordering of the other parameters
>    is important when a console defined by the last console=
>    parameter is not registered at all. In this case,
>    /dev/console gets associated with the first console
>    with tty binding according to the order on the command line.
> 
>    If you think that it is weird behavior then I agree.
>    But it is a historical mess. It is how people used it
>    when the various features were added. Many changes
>    in this code caused regressions and had to be reverted.

Yeah agreed it's a mess :)

>    See the following to get the picture:
> 
>        + commit c6c7d83b9c9e6a8 ("Revert "console: don't
> 	 prefer first registered if DT specifies stdout-path")
> 
>        + commit dac8bbbae1d0ccb ("Revert "printk: fix double
> 	 printing with earlycon"").

OK thanks.

> 2. The serial console gets registered much later with this
>    patchset:
> 
> 	original # dmesg | grep printk:
> 	[    0.000000] printk: legacy bootconsole [uart8250] enabled
> 	[    0.000000] printk: debug: ignoring loglevel setting.
> 	[    0.016859] printk: log_buf_len: 1048576 bytes
> 	[    0.017324] printk: early log buf free: 259624(99%)
> 	[    0.141859] printk: legacy console [tty0] enabled
> 	[    0.142399] printk: legacy bootconsole [uart8250] disabled
> 	[    0.143032] printk: legacy console [ttyS0] enabled
> 
>    vs.
> 
> 	patched # dmesg | grep printk:
> 	[    0.000000] printk: legacy bootconsole [uart8250] enabled
> 	[    0.000000] printk: debug: ignoring loglevel setting.
> 	[    0.018142] printk: log_buf_len: 1048576 bytes
> 	[    0.018757] printk: early log buf free: 259624(99%)
> 	[    0.160706] printk: legacy console [tty0] enabled
> 	[    0.161213] printk: legacy bootconsole [uart8250] disabled
> 	[    1.592929] printk: legacy console [ttyS0] enabled
> 
>    This is pretty bad because it would complicate or even prevent
>    debugging of the boot stage via serial console.

I think I have a patch coming for 8250 isa ports for that issue.
This issue should go away if we call add_preferred_console_match()
from serial8250_isa_init_ports() with options for the port like
"ttyS0", "ttyS", 0.

>    The graphical console is not usable when the system dies. Also
>    finding the right arguments for the earlycon= parameter is
>    tricky so that people enable it only when they have to debug
>    very early messages.
> 
> 
> I am going to look at the patches more closely to see if I could
> provide some hints.

Great, help with the early console handling is much appreciated.

I'll post an updated patchset this week that does not touch
console_setup() beyond saving the console options. And then we
hopefully have something that avoids the regressions and can be
used for further changes later on.

Regards,

Tony