[v3,0/3] Fixes for console command line ordering

Hi,

Recent changes to add support for DEVNAME:0.0 style consoles caused a
regression with the preferred console order where the last console on
the kernel command line is no longer the preferred console.

The following three changes fix the issue using Petr's suggestion that
does not involve calling __add_preferred_console() later on again, and
adds the deferred consoles to the console_cmdline[] directly to be
updated when the console is ready.

We revert the earlier printk related changes, and then add back the
DEVNAME:0.0 functionality based on Petr's code snippet. And we end up
reducing the code quite a bit too this way.

The reason we want DEVNAME:0.0 style consoles is it helps addressing the
console based on the connected serial port controller device rather than
using the hardcoded ttyS addressing. And that helps with issues related
to the console moving around after togging the HSUART option in the BIOS,
or when new ports are enabled in devicetree and aliases are not updated.

Regards,

Tony

Changes since v2:

- Use match_devname_and_update_preferred_console() naming and update
  the comments

- Add a patch to rename the serial functions to use match and update
  naming

- Use ttyname instad of chardev in console_setup()

- Split variables per-line in console_setup()

- Initialize idx to -1 for devname in console_setup()

- Add pr_info() statement when a preferred console is associated with
  a devname

Changes since v1:

- Revert the problem causing printk changes and switch to using the
  solution based on Petr's suggestion and code snippet

Tony Lindgren (3):
  printk: Revert add_preferred_console_match() related commits
  printk: Add match_devname_and_update_preferred_console()
  serial: core: Rename preferred console handling for match and update

 drivers/tty/serial/8250/8250_core.c  |   4 +-
 drivers/tty/serial/serial_base.h     |   4 +-
 drivers/tty/serial/serial_base_bus.c |  23 +++--
 include/linux/printk.h               |   5 +-
 kernel/printk/Makefile               |   2 +-
 kernel/printk/conopt.c               | 146 ---------------------------
 kernel/printk/console_cmdline.h      |   7 +-
 kernel/printk/printk.c               | 122 ++++++++++++++++------
 8 files changed, 111 insertions(+), 202 deletions(-)
 delete mode 100644 kernel/printk/conopt.c


base-commit: 6ba59ff4227927d3a8530fc2973b80e94b54d58f

On Tue, Jun 18, 2024 at 03:51:44PM +0200, Petr Mladek wrote:
> On Tue 2024-06-18 07:54:50, Tony Lindgren wrote:
> > We are now matching and updating the preferred console, not adding it.
> > Let's update the naming accordingly to avoid confusion.
> > 
> > --- a/drivers/tty/serial/serial_base_bus.c
> > +++ b/drivers/tty/serial/serial_base_bus.c
> > @@ -304,7 +305,7 @@ int serial_base_add_preferred_console(struct uart_driver *drv,
> 
> I was curious whether this patch renamed everything. And it seems
> that it did not rename serial_base_add_preferred_console().

Oops, will update the naming for that too.

> >  	const char *port_match __free(kfree) = NULL;
> >  	int ret;
> >  
> > -	ret = serial_base_add_prefcon(drv->dev_name, port->line);
> > +	ret = serial_base_match_and_update_prefcon(drv->dev_name, port->line);
> >  	if (ret)
> >  		return ret;
> >  
> 
> Honestly, I do not understand what are all these layers for.
> Especially, serial_base_match_and_update_prefcon() looks suspicious:
> 
> static int serial_base_match_and_update_prefcon(const char *name, int idx)
> {
> 	const char *char_match __free(kfree) = NULL;
> 	const char *nmbr_match __free(kfree) = NULL;
> 	int ret;
> 
> 	/* Handle ttyS specific options */
> 	if (strstarts(name, "ttyS")) {
> 		/* No name, just a number */
> 		nmbr_match = kasprintf(GFP_KERNEL, "%i", idx);
> 		if (!nmbr_match)
> 			return -ENODEV;
> 
> 		ret = serial_base_match_and_update_one_prefcon(nmbr_match, name, idx);
> 		if (ret)
> 			return ret;
> 
> 		/* Sparc ttya and ttyb */
> 		ret = serial_base_add_sparc_console(name, idx);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	/* Handle the traditional character device name style console=ttyS0 */
> 	char_match = kasprintf(GFP_KERNEL, "%s%i", name, idx);
> 	if (!char_match)
> 		return -ENOMEM;
> 
> 	return serial_base_match_and_update_one_prefcon(char_match, name, idx);
> }
> 
> It seems to try whether c->devname matches a number "X", or "ttySX".
> It even tries the sparc-specific transformations in
> serial_base_add_sparc_console()
> 
> But this is the original format which does _not_ include ":".
> It never will be stored in c->devname and will never match.

Good catch, this won't do anything now with console_setup()
checking for ":" for deferred consoles. So we should revert commit
a0f32e2dd998 ("serial: core: Handle serial console options").

> I think that it has been the case even before this patchset.

For the earlier case, I tested things with serial handling removed
from console_setup() to let the serial layer handle the quirks.

With the new handling, we could just eventually defer the serial
consoles in console_setup(), and let the serial core do the quirk
handling. No immediate need for that though, that would be just
longer term clean-up.

> I think that we should remove these layers and check just
> the "DEVNAME:X.Y" format, aka "%s:%d.%d" [*].

Yes let's revert the quirk handling.
 
> [*] It would be nice to use the same printf format "%s:%d.%d"
>     in both serial_base_device_init() and also in the functions
>     matching the devname to make it clear that these are
>     the same names. Heh, I just guess that these are the same
>     names.

I'll do a separate clean-up patch for that, and that can then
be used for the match() too eventually.

Regards,

Tony

On Wed, Jun 19, 2024 at 07:39:04AM +0300, Tony Lindgren wrote:
> On Tue, Jun 18, 2024 at 03:51:44PM +0200, Petr Mladek wrote:
> > It seems to try whether c->devname matches a number "X", or "ttySX".
> > It even tries the sparc-specific transformations in
> > serial_base_add_sparc_console()
> > 
> > But this is the original format which does _not_ include ":".
> > It never will be stored in c->devname and will never match.
> 
> Good catch, this won't do anything now with console_setup()
> checking for ":" for deferred consoles. So we should revert commit
> a0f32e2dd998 ("serial: core: Handle serial console options").

Heh actually we can revert a lot more, basically leaving only
the renamed serial_base_match_and_update_preferred_console().

Regards,

Tony

On Wed 2024-06-19 10:17:46, Tony Lindgren wrote:
> On Wed, Jun 19, 2024 at 07:39:04AM +0300, Tony Lindgren wrote:
> > On Tue, Jun 18, 2024 at 03:51:44PM +0200, Petr Mladek wrote:
> > > It seems to try whether c->devname matches a number "X", or "ttySX".
> > > It even tries the sparc-specific transformations in
> > > serial_base_add_sparc_console()
> > > 
> > > But this is the original format which does _not_ include ":".
> > > It never will be stored in c->devname and will never match.
> > 
> > Good catch, this won't do anything now with console_setup()
> > checking for ":" for deferred consoles. So we should revert commit
> > a0f32e2dd998 ("serial: core: Handle serial console options").
> 
> Heh actually we can revert a lot more, basically leaving only
> the renamed serial_base_match_and_update_preferred_console().

I wonder if it would be cleaner to revert all patches adding
the feature and then add back just the minimalist solution.

Best Regards,
Petr

On Wed, Jun 19, 2024 at 02:08:40PM +0200, Petr Mladek wrote:
> On Wed 2024-06-19 10:17:46, Tony Lindgren wrote:
> > On Wed, Jun 19, 2024 at 07:39:04AM +0300, Tony Lindgren wrote:
> > > On Tue, Jun 18, 2024 at 03:51:44PM +0200, Petr Mladek wrote:
> > > > It seems to try whether c->devname matches a number "X", or "ttySX".
> > > > It even tries the sparc-specific transformations in
> > > > serial_base_add_sparc_console()
> > > > 
> > > > But this is the original format which does _not_ include ":".
> > > > It never will be stored in c->devname and will never match.
> > > 
> > > Good catch, this won't do anything now with console_setup()
> > > checking for ":" for deferred consoles. So we should revert commit
> > > a0f32e2dd998 ("serial: core: Handle serial console options").
> > 
> > Heh actually we can revert a lot more, basically leaving only
> > the renamed serial_base_match_and_update_preferred_console().
> 
> I wonder if it would be cleaner to revert all patches adding
> the feature and then add back just the minimalist solution.

Yeah let's do that. Otherwise it's hard to see what's going on :)

Regards,

Tony