Message-ID: <Zm_j2eq3QcaA-g-e@tlindgre-MOBL1>
Date: Mon, 17 Jun 2024 10:20:57 +0300
From: Tony Lindgren <tony.lindgren@...ux.intel.com>
To: Petr Mladek <pmladek@...e.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Tony Lindgren <tony@...mide.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] printk: Add update_preferred_console()

On Fri, Jun 14, 2024 at 07:17:20PM +0200, Petr Mladek wrote:
> On Thu 2024-06-13 15:51:08, Tony Lindgren wrote:
> > The earlier attempt on doing this caused a regression with the kernel
> > command line console order as it added calling __add_preferred_console()
> > again later on during init. A better approach was suggested by Petr where
> > we add the deferred console to the console_cmdline[] and update it later
> > on when the console is ready.
> 
> The patch seems to work well. And I am surprised that it is so small ;-)

Your intuition at work :)

> > diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
> > index 5ebacb982f9e..a34f55ef6f37 100644
> > --- a/drivers/tty/serial/serial_base_bus.c
> > +++ b/drivers/tty/serial/serial_base_bus.c
> > @@ -210,7 +210,13 @@ void serial_base_port_device_remove(struct serial_port_device *port_dev)
> >  static int serial_base_add_one_prefcon(const char *match, const char *dev_name,
> >  				       int port_id)
> 
> I would suggest to rename also functions on the serial_base side.
> The function is not adding prefcon. It is doing some match_and_update
> job.

OK good idea, I'll do a separate patch for that.

> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2486,8 +2495,8 @@ __setup("console_msg_format=", console_msg_format_setup);
> >   */
> >  static int __init console_setup(char *str)
> >  {
> > -	char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for "ttyS" */
> > -	char *s, *options, *brl_options = NULL;
> 
> I would add
> 
> 	static_assert(sizeof(console_cmdline[0].devname) >= sizeof(console_cmdline[0].name));

That check should still be >= sizeof(console_cmdline[0].name) + 4, right?
For the "number only" consoles we add the "ttyS" prefix.

> The name "chardev" sounds as generic as "devname". I would use one of
> 
>    + "name" like the parameter in __add_preferred_console
>    + "ttyname" as it is mostly used for "tty*" console names
>    + "conname" like a name in struct console.
> 
> Also please split the variables per-line so that future diff's are
> easier to follow. Something like:
> 
> 	static_assert(sizeof(console_cmdline[0].devname) >= sizeof(console_cmdline[0].name));
> 	char buf[sizeof(console_cmdline[0].devname)];
> 	char *brl_options = NULL;
> 	char *ttyname = NULL;
> 	char *devname = NULL;
> 	char *options;
> 	char *s;
> 	int idx;

OK I'll use ttyname like you're suggesting.

> > @@ -2523,12 +2538,12 @@ static int __init console_setup(char *str)
> >  #endif
> >  
> >  	for (s = buf; *s; s++)
> > -		if (isdigit(*s) || *s == ',')
> > +		if ((chardev && isdigit(*s)) || *s == ',')
> >  			break;
> >  	idx = simple_strtoul(s, NULL, 10);
> 
> The @idx value is not really important when @devname is used.
> But it still would be more clear to set it to -1.

OK

> My proposal might be kind of naive. Some people might say that
> it describes obvious things. But the API is for device driver
> users which do not know much about how printk handles
> the console command line and the registration.
> 
> <proposal>
> /**
>  * match_devname_and_update_preferred_console - Update a preferred console
>  *	when matching devname is found.
>  * @devname: DEVNAME:0.0 style device name
>  * @name: Name of the corresponding console driver, e.g. "ttyS"
>  * @idx: Console index, e.g. port number.
>  *
>  * The function checks whether a device with the given @devname is
>  * preferred via the console=DEVNAME:0.0 command line option.
>  * It fills the missing console driver name and console index
>  * so that a later register_console() call could find (match)
>  * and enable this device.
>  *
>  * It might be used when a driver subsystem initializes particular
>  * devices with already known DEVNAME:0.0 style names. And it
>  * could predict which console driver name and index this device
>  * would later get associated with.
>  *
>  * Return: 0 on success, negative error code on failure.
>  */
> </proposal>

That looks good to me.

> At least, this is my understanding of how this works.
> 
> I do not know the whole history. And maybe I get something wrong.
> IMHO, the main problem is that the printk console code
> historically uses TTY device names. But we want to register/enable
> the consoles ASAP when the HW devices are ready for writing().
> It happens before the TTY subsystem gets initialized so
> that we could not use the sysfs kobjects for matching
> the tty device driver names with HW device driver names.
> And we need this kind of hacks.

Yes that's correct.

> > +int update_preferred_console(const char *devname, const char *name, const short idx)
> > +{
> > +	struct console_cmdline *c = console_cmdline;
> > +	int i;
> > +
> > +	if (!devname || !strlen(devname) || !name || !strlen(name) || idx < 0)
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < MAX_CMDLINECONSOLES && (c->name[0] || c->devname[0]);
> > +	     i++, c++) {
> > +		if (!strcmp(devname, c->devname)) {
> 
> I would add here:
> 
> 			pr_info("associate the preferred console \"%s\" with \"%s%d\"\n",
> 				devname, name, idx);
> 

OK

Regards,

Tony

Powered by blists - more mailing lists