Message-ID: <10194425.EvYhyI6sBW@nerdopolis2>
Date: Wed, 26 Feb 2025 08:39:23 -0500
From: Adam Simonelli <adamsimonelli@...il.com>
To: Petr Mladek <pmladek@...e.com>
Cc: linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
 Jiri Slaby <jirislaby@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Andy Shevchenko <andy.shevchenko@...il.com>, Petr Mladek <pmladek@...e.com>,
 Steven Rostedt <rostedt@...dmis.org>,
 John Ogness <john.ogness@...utronix.de>,
 Sergey Senozhatsky <senozhatsky@...omium.org>
Subject:
 Re: [PATCH v5 1/2] ttynull: Add an option to allow ttynull to be used as a
 console device

On Tuesday, February 25, 2025 11:19:04 AM EST Petr Mladek wrote:
> Hi Adam,
> 
> please add printk maintainers into Cc as already suggested by Andy
> at https://lore.kernel.org/r/CAHp75VeBaetiQBykfLk_weBHdzZF1nWp=k8BJu+OKNp6iYRRTg@mail.gmail.com
> 
OK, I have added the other printk maintainers in this thread, and will update
my git send email script.
> The motivation is that the console registration code is in
> kernel/printk/printk.c. It is historically pretty tricky.
> Some ordering is defined rather by chance than by design.
> And we should be careful when adding new rules and hacks.
> 
> On Mon 2025-02-24 07:39:14, adamsimonelli@...il.com wrote:
> > From: Adam Simonelli <adamsimonelli@...il.com>
> > 
> > The new config option, CONFIG_NULL_TTY_CONSOLE will allow ttynull to be
> > initialized by console_initcall() and selected as a possible console
> > device.
> > 
> > Signed-off-by: Adam Simonelli <adamsimonelli@...il.com>
> > ---
> >  drivers/tty/Kconfig   | 15 ++++++++++++++-
> >  drivers/tty/ttynull.c | 20 +++++++++++++++++++-
> >  2 files changed, 33 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> > index 63a494d36a1f..b4afae8b0e74 100644
> > --- a/drivers/tty/Kconfig
> > +++ b/drivers/tty/Kconfig
> > @@ -383,7 +383,20 @@ config NULL_TTY
> >  	  available or desired.
> >  
> >  	  In order to use this driver, you should redirect the console to this
> > -	  TTY, or boot the kernel with console=ttynull.
> > +	  TTY, boot the kernel with console=ttynull, or enable
> > +	  CONFIG_NULL_TTY_CONSOLE.
> > +
> > +	  If unsure, say N.
> > +
> > +config NULL_TTY_CONSOLE
> 
> It makes sense to enable this behavior by a CONFIG_ setting
> but the name is misleading.
> 
> > +
> > +        bool "Support for console on ttynull"
> > +        depends on NULL_TTY=y && !VT_CONSOLE
> > +	help
> > +	  Say Y here if you want the NULL TTY to be used as a /dev/console
> > +	  device.
> > +
> > +	  This is similar to CONFIG_VT_CONSOLE, but without the dependency on
> > +	  CONFIG_VT. It uses the ttynull driver as the system console.
> 
> It is true that CONFIG_VT_CONSOLE causes that the virtual terminal
> will get associated with /dev/console. But it works only "by chance".
> 
> It works because "register_console(&vt_console_driver)" in
> con_init() is the first register_console() call. And it also
> works only by chance because of the linking order.
> 
> Anyway, there are more similar CONFIG_ options, for example,
> CONFIG_LP_CONSOLE, or CONFIG_VIRTIO_CONSOLE. And they are not
> default when CONFIG_VT_CONSOLE is enabled. They are registered only
> when the related console= option is defined on the command line.
> 
> I want to say that CONFIG_<BLA>_CONSOLE does not mean
> that the BLA console will be registered by default.
> And we should us a better descriptive name, for example,
> 
>   NULL_TTY_DEFAULT_CONSOLE
>   NULL_TTY_DEV_CONSOLE
> 
OK. I can change the option name.
> >  	  If unsure, say N.
> >  
> > diff --git a/drivers/tty/ttynull.c b/drivers/tty/ttynull.c
> > index 6b2f7208b564..ec3dd3fd41c0 100644
> > --- a/drivers/tty/ttynull.c
> > +++ b/drivers/tty/ttynull.c
> > @@ -57,6 +57,13 @@ static struct tty_driver *ttynull_device(struct console *c, int *index)
> >  static struct console ttynull_console = {
> >  	.name = "ttynull",
> >  	.device = ttynull_device,
> > +
> > +	/*
> > +	 * Match the index and flags from other boot consoles when CONFIG_NULL_TTY_CONSOLE is
> > +	 * enabled, otherwise, use the default values for the index and flags.
> > +	*/
> > +	.index = IS_ENABLED(CONFIG_NULL_TTY_CONSOLE) ? -1 : 0,
> 
> This should not be needed. "con->index" is always initialized to "0"
> for the default console, see:
> 
OK, I had this in an #ifdef before, it was the cleanest way to set it to -1
that I could think of, other than the ifdef... If I still need this, I will try
to think of something else to set it to -1 when the option is enabled
> static void try_enable_default_console(struct console *newcon)
> {
> 	if (newcon->index < 0)
> 		newcon->index = 0;
> [...]
> }
> 
> > +	.flags = IS_ENABLED(CONFIG_NULL_TTY_CONSOLE) ? CON_PRINTBUFFER : 0,
> 
> This does not make much sense to me.
> 
> CON_PRINTBUFFER prevents duplicated output when the same device has
> already been registered as a boot console. But ttynull does not have
> a boot console variant. Also it is a "null" device. It never prints
> anything. The output could never be duplicated by definition.
> 
OK, I was duplicating what I saw in other consoles. I can try to remove it
> >  };
> >  
> >  static int __init ttynull_init(void)
> > @@ -90,11 +97,22 @@ static int __init ttynull_init(void)
> >  	}
> >  
> >  	ttynull_driver = driver;
> > -	register_console(&ttynull_console);
> > +	if (!console_is_registered(&ttynull_console))
> > +		register_console(&ttynull_console);
> >  
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_NULL_TTY_CONSOLE
> > +static int __init ttynull_register(void)
> > +{
> > +	if (!console_is_registered(&ttynull_console))
> > +		register_console(&ttynull_console);
> > +	return 0;
> > +}
> > +console_initcall(ttynull_register);
> > +#endif
> 
> This looks strange. I guess that you needed to move this into
> console_initcall() because it is called earlier together
> with the other console_initcall() calls for serial ports.
> Otherwise, the hack with the linking order (2nd patch) did
> not work.
> 
> But you needed to keep it in ttynull_init() so that ttynull
> did not get registered prematurely when CONFIG_NULL_TTY_CONSOLE
> was not enabled.
> 
> Sigh, it looks like a dirty hack which works rather by chance
> than by design.
> 
> 
> Thinking loudly:
> 
> The register_console() code is a historic mess. I dream about
> having time to clean it up. Anyway, there are basically two
> modes:
> 
> 1. try_enable_default_console(newcon) is called only when
>    there is no @preferred_console and there is no registered
>    console with tty binding (valid con->device).
> 
>    The first register_console() caller wins. The order is defined
>    by the __con_initcall section. Which is defined by the linking
>    order.
> 
>    IMHO, it is quite fragile and non-intuitive.
> 
> 
> 2. try_enable_preferred_console() is called when some
>    console is preferred via console_cmdline[]. The entries
>    are added by __add_preferred_console() calls.
> 
>    This approach was created to handle console= command line
>    parameters. But it was later used to define default consoles
>    also via SPCR and device tree, see add_preferred_console()
>    callers.
> 
>    It is also a bit tricky because the last added entry
>    is preferred. Plus the .user_specified entries are
>    preferred over the entries added via SPCR or device tree.
> 
>    Anyway, I think that the preference and ordering defined
>    by console_cmdline[] array is a more intuitive approach.
> 
> 
> My proposal is to call:
> 
> #ifdef CONFIG_NULL_TTY_DEFAULT_CONSOLE
> 	add_preferred_console("ttynull", 0, NULL);
> #endif
> 
> somewhere in the kernel code. The question is where.
> I wonder if the following would work:
> 

> #ifdef CONFIG_NULL_TTY_DEFAULT_CONSOLE
> static int __init ttynull_default_console(void)
> {
> 	add_preferred_console("ttynull", 0, NULL);
> 	return 0;
> }
> console_initcall(ttynull_register);
> #endif
> 
> Best Regards,
> Petr
> 
OK, actually in earlier revisions locally, I did actually have



diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index dddb15f48d59..c1554a789de8 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3712,6 +3712,11 @@ void __init console_init(void)
 	initcall_t call;
 	initcall_entry_t *ce;
 
+#ifdef CONFIG_NULL_TTY_CONSOLE
+       if (!strstr(boot_command_line, "console="))
+               add_preferred_console("ttynull", 0, NULL);
+#endif
+
 	/* Setup the default TTY line discipline. */
 	n_tty_init();
 


Which worked as far as I could tell, at least on x86. Not sure if that was the
right place, and yeah, I was trying to better copy how CONFIG_VT_CONSOLE worked
because I thought that was more correct.

Powered by blists - more mailing lists