Received-SPF: pass (google.com: domain of linux-kernel+bounces-190464-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 27 May 2024 14:13:19 +0300
From: Tony Lindgren <tony.lindgren@linux.intel.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Tony Lindgren <tony@atomide.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
	Steven Rostedt <rostedt@goodmis.org>,
	John Ogness <john.ogness@linutronix.de>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	"David S . Miller" <davem@davemloft.net>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Dhruva Gole <d-gole@ti.com>,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	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, Sebastian Reichel <sre@kernel.org>,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH v7 1/7] printk: Save console options for
 add_preferred_console_match()
Message-ID: <ZlRqz2b0ZrtkxScL@tlindgre-MOBL1>
References: <20240327110021.59793-1-tony@atomide.com>
 <20240327110021.59793-2-tony@atomide.com>
 <ZlC6_Um4P4b-_WQE@pathway.suse.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZlC6_Um4P4b-_WQE@pathway.suse.cz>

Hi,

On Fri, May 24, 2024 at 06:06:21PM +0200, Petr Mladek wrote:
> I have finally found time to looks at this again.

Great good to hear.

> 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

Thanks and sorry for missing this issue. I thought I had this issue
already handled, but looking at what I tested with earlier, looks like
I had the console options the wrong way around.
 
> 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

OK

> 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)

OK thanks for tracking down where things go wrong.

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

To me it seems we can fix this by keeping track of the console position
in the kernel command line. I'll send a fix for this to discuss.

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

OK certainly no objection from me if we can make this happen without
making things more complex :)
 
> 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?

Thanks, I'll take a look if .match(), or some parts related to it, can
be used.

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

I don't think we can make much assumptions about the devices early on,
and we also have the console index -1 issue.

Regards,

Tony