by Petr Mladek

[permalink] [raw]

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 <[email protected]>
> ---
> 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