Re: [PATCH 3/8] printk: Separate code for adding/updating preferred console metadata

[Petr Mladek <pmladek@suse.com>]

On Thu 2026-02-19 22:48:49, Chris Down wrote:
> Petr Mladek writes:
> > @@ -2531,28 +2587,14 @@ static int __add_preferred_console(const char *name, const short idx,
> > 	for (i = 0, pc = preferred_consoles;
> > 	     i < MAX_PREFERRED_CONSOLES && (pc->name[0] || pc->devname[0]);
> > 	     i++, pc++) {
> > -		if ((name && strcmp(pc->name, name) == 0 && pc->index == idx) ||
> > -		    (devname && strcmp(pc->devname, devname) == 0)) {
> > -			if (!brl_options)
> > -				preferred_dev_console = i;
> > -			set_user_specified(pc, user_specified);
> > -			return 0;
> > -		}
> 
> So, in this old code when __add_preferred_console() finds an existing
> matching entry, it returns immediately without touching the options at all.
> 
> But in the new patch the loop breaks out and falls through to
> update_preferred_console(), which only conditionally updates options.
> 
> This has different behaviour. For example if you imagine SPCR/DT setting the
> options, before they win out against the command line, but now they do not.
> 
> This may well be intended, since the user cmdline probably should win over
> the platform defaults anyway. I'd argue that sounds like the right behaviour
> anyway. But I don't see this change being mentioned as intentional in the
> changelog. Is it?  If it is, let's mention it there.

Good point!

I considered this as a bug fix and I described this in the last
paragraph of the commit message:

<paste>
While this is primarily a refactoring, it fixes two minor behavioral
consistencies: console options can now be overridden via the command
line, and Braille options are preserved even if a non-Braille console
with the same name was previously defined.
</paste>


I hope that this change should be on the safe size because:

  1. People should be able to override any default value on the
     command line. It seems to be the only sane approach.

  2. 20 of 33 add_preferred_console() callers do not pass options[*]

  3. I somehow hooped that add_preferred_console() is mostly called
     before the command line is proceed. Otherwise, the command line
     would not be able to overwrite the options. Also /dev/console
     is supposed to be associated with the last console= defined
     on the command line.

     I have to admit that I checked this only on x86_64 where the spcr
     is handled before the command line, see:

       + setup_arch()
	 + acpi_boot_init()
	   + acpi_parse_spcr()

      Hmm, I see that the device tree seems to be handled later, for example,

	+ serial_core_add_one_port()
	  + of_console_check()


Honestly, I am a bit afraid of regressions caused by this change.
But I hope that it will be a corner case and users might
be persuaded that their command line was bogus.

As I said, people should be able to override the defaults via the
command line. And I do not see any other way how to achieve it.

By other words, the sooner we enforce a sane rule the better.
I hope that it is not too late. And the only way to check it
in the real life is to try it. /o\.

But you are right, I should be more clear about this in
the commit message. I am going to improve it in v2.

[*] 

$> git grep add_preferred_console | grep -v -e "__add_pre" -e "xen_add_pref" -e "printk" -e "console.h" | wc -l
33
$> git grep add_preferred_console | grep -v -e "__add_pre" -e "xen_add_pref" -e "printk" -e "console.h" | grep ", NULL);" | wc -l
20


Best Regards,
Petr