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
