Re: [PATCH v8 03/21] printk: Prioritise user-specified configuration over SPCR/DT

[Petr Mladek <pmladek@suse.com>]

On Fri 2025-11-28 03:43:21, Chris Down wrote:
> ACPI firmware routinely calls add_preferred_console() via
> acpi_parse_spcr() before we ever look at the kernel command line. After
> that first registration we short-circuit on every duplicate name/index
> match, so the subsequent console=ttyS0,... parameter never refreshes the
> UART options that the firmware supplied.
> 
> Historically that just meant you couldn't tweak baud/flow control for a
> firmware-provided serial console unless you picked a different device
> name, but the per-console loglevel plumbing in this series relies on
> those later console= entries being able to update the stored option
> string. Without that, console=ttyS0,loglevel:5 simply never takes effect
> on machines that get their console from SPCR/DT.
> 
> Teach __add_preferred_console() to update the existing slot when the
> same console is mentioned again: we keep the original slot, but replace
> its option string (and re-run braille option parsing) so that later
> callers can override what firmware seeded. This keeps today's behaviour
> unchanged for drivers, while allowing the cmdline UART parameters (and
> soon the loglevel hints) to override the ACPI defaults.

Another great catch!

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2560,7 +2560,12 @@ static int __add_preferred_console(const char *name, const short idx,
>  		    (devname && strcmp(c->devname, devname) == 0)) {


__add_preferred_console() can be called also after processing the
command line, for example via the device tree:

  + serial8250_init()
    + serial8250_register_ports()
      + uart_add_one_port()
        + serial_ctrl_register_port()
	  + serial_core_register_port()
	    + serial_core_add_one_port()
	      + of_console_check()
	        + add_preferred_console

Or a platform default:

  + hvc_rtas_console_init()
    + add_preferred_console()

I would rather make sure that we update the values only when the
console is defined on the command line:

			/*
			 * Make sure that only command line is allowed
			 * to modify the default preference and options.
			 */
			if (!user_specified)
				return 0;

It probably won't have any effect. For example, of_console_check()
calls add_preferred_console() only when (!console_set_on_cmdline).
And hvc_rtas_console_init() does not pass any @options.

But I would rather be on the safe side and make the logic explicit here.

>  			if (!brl_options)
>  				preferred_console = i;
> +
> +			if (options)
> +				c->options = options;
> +
>  			set_user_specified(c, user_specified);
> +			braille_set_options(c, brl_options);
>  			return 0;
>  		}
>  	}

Otherwise, it looks good to me.

Best Regards,
Petr