Re: [PATCH v7 1/7] printk: Save console options for add_preferred_console_match()

Tony Lindgren
2024-05-27T11:13:34Z

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