Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Fri, 1 Dec 2023 15:36:35 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     Tony Lindgren <tony@atomide.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        "David S . Miller" <davem@davemloft.net>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Andy Shevchenko <andriy.shevchenko@intel.com>,
        Dhruva Gole <d-gole@ti.com>,
        Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        John Ogness <john.ogness@linutronix.de>,
        Johan Hovold <johan@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Subject: Re: [PATCH v3 0/3] Add support for DEVNAME:0.0 style hardware based
 addressing
Message-ID: <ZWnvc6-LnXdjOQLY@alley>
References: <20231121113203.61341-1-tony@atomide.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231121113203.61341-1-tony@atomide.com>
Precedence: bulk

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