Message-ID: <Z9GPVVTnngGbmbuv@pathway.suse.cz>
Date: Wed, 12 Mar 2025 14:42:45 +0100
From: Petr Mladek <pmladek@...e.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Adam Simonelli <adamsimonelli@...il.com>, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org, Jiri Slaby <jirislaby@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH v6 3/3] tty: Change order of ttynull to be linked sooner
 if enabled as a console.

On Thu 2025-03-06 09:10:29, Andy Shevchenko wrote:
> On Thu, Mar 6, 2025 at 6:22 AM Adam Simonelli <adamsimonelli@...il.com> wrote:
> >
> > On Wednesday, March 5, 2025 1:52:00 PM EST Andy Shevchenko wrote:
> > > On Wed, Mar 5, 2025 at 4:06 AM Adam Simonelli <adamsimonelli@...il.com> wrote:
> > > > On Tuesday, March 4, 2025 1:51:52 AM EST Andy Shevchenko wrote:
> > > > > On Tue, Mar 4, 2025 at 5:55 AM <adamsimonelli@...il.com> wrote:
> 
> ...
> 
> > > > > >  obj-y                          += vt/
> > > > >
> > > > > + blank line.
> > > > >
> > > > > > +# If ttynull is configured to be a console by default, ensure that it is linked
> > > > > > +# earlier before a real one is selected.
> > > > > > +obj-$(CONFIG_NULL_TTY_DEFAULT_CONSOLE) \
> > > > > > +                               += ttynull.o
> > > > >
> > > > > Here is the question: are you sure that all console drivers that exist
> > > > > in the kernel happen to be here? Have you grepped the source tree for
> > > > > checking this?
> > > > >
> > > > Grepping for console_initcall, the only other places I see outside of
> > > > drivers/tty/ is
> > > >
> > > > arch/mips/fw/arc/arc_con.c
> > > > arch/mips/sibyte/common/cfe_console.c
> > > > arch/powerpc/kernel/legacy_serial.c
> > > > arch/powerpc/kernel/udbg.c
> > > > arch/powerpc/platforms/powermac/setup.c
> > > > arch/um/drivers/stderr_console.c
> > > > arch/xtensa/platforms/iss/console.c
> > > > drivers/s390/char/con3215.c
> > > > drivers/s390/char/con3270.c
> > > > drivers/s390/char/sclp_con.c
> > > > drivers/s390/char/sclp_vt220.c
> > >
> > > Which means you need to test your stuff on those cases, to see how the
> > > linker order is done there. It might be that your change wouldn't work
> > > there as expected (quick workaround is to mark the new option as
> > > depends on !S390 && !PPC and so on.
> 
> > It will be difficult to test other arches, I mean I guess it is possible with
> > QEMU, and cross-building, though I did do an experimental test on x86:
> >
> > Making it temporarily adding an architecture specific console like
> > powerpc/some mips/s390/arches with Xen enabled.
> 
> Thanks. Make sure the summary of this gets into the commit message.
> Also consider updating the relevant documentation under
> Documentation/, if any.

Honestly, I am not sure what is the preferred behavior here. I am not
familiar with the arch-specific consoles.

But it made me think and investigate the various console drivers.
It is a kind of a mess and I am not sure that I understand it correctly.

Anyway, I suggested to use "add_preferred_console()" in console_initcall()
in the v5 review, see https://lore.kernel.org/r/Z73teICMWNx7BiHT@pathway.suse.cz

And I expected that it would be enough and the hack with linking order
won't be needed anymore.

Now, I see that I was wrong. The problem is that many console drivers
call register_console() in console_initcall(). Such consoles would be
registered by default when their register_console() is called before
the ttynull.c calls add_preferred_console().

By other words, the hack with the linking order is still needed to
call add_preferred_console() in time.

Resume: From my POV, the solution with add_preferred_console() in
	console_initcall() still relies on the linking order.
	So, v6 is not better than v5.

	IMHO, v6 is actually be worse, see below.

> > -------------------------------------------------------------------------------
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index 05c5aa951da7..bcd248c44fc8 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -1159,6 +1159,8 @@ void __init setup_arch(char **cmdline_p)
> >
> >         e820__setup_pci_gap();
> >
> > +       add_preferred_console("ttyS", 0, NULL);
> > +
> >  #ifdef CONFIG_VT
> >  #if defined(CONFIG_VGA_CONSOLE)
> >         if (!efi_enabled(EFI_BOOT) || (efi_mem_type(0xa0000) != EFI_CONVENTIONAL_MEMORY))
> > -------------------------------------------------------------------------------
> >
> > to see what /proc/consoles will look like, to pretend that x86 is an arch that
> > sets a console somewhere, and I get:
> >
> > ttynull0             --- (EC    )  242:0
> > ttyS0                -W- (E  p a)    4:64
> >
> > and I got console messages to ttyS0 with no issue.
> >
> > which in my mind is acceptable I would think. ttynull is first in the list,
> > which is desired effect of CONFIG_NULL_TTY_DEFAULT_CONSOLE, it doesn't have to
> > be _exclusive_ AFAIK, especially if there are long-time default consoles that.
> > users or the hardware expects.
> >
> >
> > The only arch that seems to _unconditionally_ add a console without some other
> > circumstance, like boot loader env var, and command line option, or firmware
> > flag, or suboption (like CONFIG_SERIAL_PMACZILOG_CONSOLE) is Jazz.
> >
> > Like platforms/powernv adds it if CONFIG_HVC_OPAL is disabled, or the firmware
> > is missing "FW_FEATURE_OPAL". I would assume that a user of this situation
> > turning on CONFIG_NULL_TTY_DEFAULT_CONSOLE in addition, will just get ttynull
> > and hvc in /proc/consoles instead of just hvc. Could that cause something to
> > break?
> 
> > Correct me if I am wrong, I could very very very well be wrong.
> 
> I leave this to Petr to comment as I'm not that expert in the area.

It depends on the expectations. I see two possibilities:

1. view:

   CONFIG_NULL_TTY_DEFAULT_CONSOLE is disabled by default.
   We could assume that people would enable it only when they really
   want to use ttynull for /dev/console. And they do not mind when
   some other platform-specific console is enabled as well.

   Note that we do not need the hack with the linking order for this.
   A solution is to call add_preferred_console() directly in
   console_init(). It has already been used in an earlier version, see
   https://lore.kernel.org/all/10194425.EvYhyI6sBW@nerdopolis2/

   To make it clear. I consider this as a cleaner and more reliable
   solution than using the linking order hack.


2. view:

   The new config option wants to prefer "ttynull" when CONFIG_CONSOLE_VT
   is disabled. So, it is an alternative for "ttyX" from "vt".

   A conservative approach would be to enable it by default in the same
   situations where "ttyX" from "vt" is enabled by default.

   To make "ttynull" behave the same as "vt" would require using:

     + register_console() in console_initcall().
     + the same console_initcall() ordering => move ttynull.o
	    linking order

   => going back to v5 approach.


I would normally prefer the conservative approach. But I hate the
dependency on the linking order. It is so non-intuitive and
fragile.

So, I personally prefer to call add_preffed_console() directly in
console_init() in the end. I guess that
CONFIG_NULL_TTY_DEFAULT_CONSOLE won't be used on architectures
with a better native console. So, it will be good enough
in practice.

Best Regards,
Petr

PS: I am sorry that you have already sent two more versions
    in the meantime. But I was not able to answer this quickly.
    I needed to find time and understand the various aspects.

Powered by blists - more mailing lists