[printk,v3,40/40] tty: serial: sh-sci: use setup() callback for early console

  When setting up the early console, the setup() callback of the
regular console is used. It is called manually before registering
the early console instead of providing a setup() callback for the
early console. This is probably because the early setup needs a
different @options during the early stage.

The issue here is that the setup() callback is called without the
console_list_lock held and functions such as uart_set_options()
expect that.

Rather than manually calling the setup() function before registering,
provide an early console setup() callback that will use the different
early options. This ensures that the error checking, ordering, and
locking context when setting up the early console are correct.

Note that technically the current implementation works because it is
only used in early boot. And since the early console setup is
performed before registering, it cannot race with anything and thus
does not need any locking. However, longterm maintenance is easier
when drivers rely on the subsystem API rather than manually
implementing steps that could cause breakage in the future.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/sh-sci.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
  

Hi John,

CC linux-sh (SH-specific code)
CC linux-renesas-soc (JFYI)

On Mon, Nov 7, 2022 at 3:20 PM John Ogness <john.ogness@linutronix.de> wrote:
> When setting up the early console, the setup() callback of the
> regular console is used. It is called manually before registering
> the early console instead of providing a setup() callback for the
> early console. This is probably because the early setup needs a
> different @options during the early stage.
>
> The issue here is that the setup() callback is called without the
> console_list_lock held and functions such as uart_set_options()
> expect that.
>
> Rather than manually calling the setup() function before registering,
> provide an early console setup() callback that will use the different
> early options. This ensures that the error checking, ordering, and
> locking context when setting up the early console are correct.
>
> Note that technically the current implementation works because it is
> only used in early boot. And since the early console setup is
> performed before registering, it cannot race with anything and thus
> does not need any locking. However, longterm maintenance is easier
> when drivers rely on the subsystem API rather than manually
> implementing steps that could cause breakage in the future.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3054,15 +3054,26 @@ static struct console serial_console = {
>  };
>
>  #ifdef CONFIG_SUPERH
> +static char early_serial_buf[32];
> +
> +static int early_serial_console_setup(struct console *co, char *options)
> +{
> +       WARN_ON(options);
> +       /*
> +        * Use @early_serial_buf because @options will always be
> +        * NULL at this early stage.
> +        */
> +       return serial_console_setup(co, early_serial_buf);
> +}
> +
>  static struct console early_serial_console = {
>         .name           = "early_ttySC",
>         .write          = serial_console_write,
> +       .setup          = early_serial_console_setup,
>         .flags          = CON_PRINTBUFFER,
>         .index          = -1,
>  };
>
> -static char early_serial_buf[32];
> -
>  static int sci_probe_earlyprintk(struct platform_device *pdev)
>  {
>         const struct plat_sci_port *cfg = dev_get_platdata(&pdev->dev);
> @@ -3074,8 +3085,6 @@ static int sci_probe_earlyprintk(struct platform_device *pdev)
>
>         sci_init_single(pdev, &sci_ports[pdev->id], pdev->id, cfg, true);
>
> -       serial_console_setup(&early_serial_console, early_serial_buf);
> -
>         if (!strstr(early_serial_buf, "keep"))
>                 early_serial_console.flags |= CON_BOOT;
>
> --
> 2.30.2

LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  

Ccing Bartosz who should be familiar with the early platform code.

On Mon 2022-11-07 15:22:38, John Ogness wrote:
> When setting up the early console, the setup() callback of the
> regular console is used. It is called manually before registering
> the early console instead of providing a setup() callback for the
> early console. This is probably because the early setup needs a
> different @options during the early stage.

This last sentece makes a bit nervous ;-)

I think that I understood it in the end, see below.

> The issue here is that the setup() callback is called without the
> console_list_lock held and functions such as uart_set_options()
> expect that.
> 
> Rather than manually calling the setup() function before registering,
> provide an early console setup() callback that will use the different
> early options. This ensures that the error checking, ordering, and
> locking context when setting up the early console are correct.
> 
> Note that technically the current implementation works because it is
> only used in early boot. And since the early console setup is
> performed before registering, it cannot race with anything and thus
> does not need any locking. However, longterm maintenance is easier
> when drivers rely on the subsystem API rather than manually
> implementing steps that could cause breakage in the future.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/sh-sci.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 62f773286d44..f3a1cfec757a 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3054,15 +3054,26 @@ static struct console serial_console = {
>  };
>  
>  #ifdef CONFIG_SUPERH
> +static char early_serial_buf[32];
> +
> +static int early_serial_console_setup(struct console *co, char *options)
> +{
> +	WARN_ON(options);
> +	/*
> +	 * Use @early_serial_buf because @options will always be
> +	 * NULL at this early stage.
> +	 */

The commit message says that we use @early_serial_buf because
the early console probably needs another parameters.

It suggests that @options might be for the later stage and
we need to replace them there. Are we sure that this will always
be NULL?

Background:

The console->setup() is called in two situations:

   1. when the console is registered as the default console, see
     try_enable_default_console(). In this case, @options
     is really NULL.

   2. when the console is preferred either via the commnadline,
      or device tree, or SPCR, see try_enable_preferred_console().
      In this case, some real @options would be passed.

     From the code POV, the preferred consoles are added by calling
     add_preferred_console().


Now, it means that the WARN_ON() is correct only when this console
is always registered before the preferred consoles are defined.

I think that this is really the case. This console
is actually registered via the "earlyprintk" parameter that
is proceed by the arch-specific code before the preferred
consoles are added the standard way via the kernel commandline.

Note that "earlyprintk" and "earlycon" are two different parameters.

"earlyprintk" normally initializes "early_console" that is
called directly by early_printk(). It is used for super early
debugging. These messages even do not end in the ring buffer.

"earlycon" defines a "normal" console that is used by the standard
printk(). They are later replaced by properly initialized console
drivers that are in sysfs, ...

Note that "earlycon" calls add_preferred_console() so that
the @options are stored and passed from try_enable_preferred_console().

But "earlyprintk" does not call add_preferred_console() so
we need this hack to store and pass the console options
another way.

> +	return serial_console_setup(co, early_serial_buf);
> +}
> +

So I would do something like:

static int early_serial_console_setup(struct console *co, char *options)
{
	/*
	 * This early console is registered using earlyprintk= parameter
	 * that does not call add_preferred_console(). The @options
	 * are passed using a custom buffer.
	 */
	WARN_ON(options);

	return serial_console_setup(co, early_serial_buf);
}

Also we should explain this in the commit message.

Best Regards,
Petr