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@xxxxxxxxxxxxx>
> ---
> 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
