Re: [PATCH] init: Don't proxy console= to earlycon

[Petr Mladek <pmladek@suse.com>]

On Wed 2024-08-21 09:30:17, Raul Rangel wrote:
> On Wed, Aug 21, 2024 at 8:38 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Thu 2024-08-08 11:30:53, Raul Rangel wrote:
> > > On Fri, Jul 28, 2023 at 11:57 AM Raul Rangel <rrangel@chromium.org> wrote:
> > > > > Your patch is good then. Well, would you mind to add a comment into
> > > > > the code and make the commit message more clear even for dummies like
> > > > > me?
> > > > >
> > > > > Something like the patch below. It would be better to split it into
> > > > > two:
> > > > >
> > > > >    + 1st shuffling the check and adding the first part of the comment
> > > > >    + 2nd fixing the case with empty console= options.
> > > > >
> > > > > I could prepare the patchset. I would keep your SOB for the 2nd patch
> > > > > if you agreed.
> > > >
> > > > Yes, feel free. Thanks!
> > > >
> > >
> > > Hey Petr,
> > > I was just following up on this. Were you going to prepare the two patches?
> >
> > I have been quite overloaded last two years. It would help me a lot if
> > you could prepare the two patches and send them for review.
> >
> > Thanks for re-opening this. It has already fallen through cracks on my
> > side /o\.
> >
> > Best Regards,
> > Petr
> 
> No worries, we were just going through our tech debt backlog and this
> patch came up.
> 
> Thinking about this a little bit more, I'm wondering if we can clean
> up the hack. Would something like the following patch work? My thought
> is we declare an early handler for `console=`, so we can handle the
> `console=uartXXXX` case, and leave the non-early handler in place to
> handle the `console=ttyXX` cases.

I am not completely sure but I guess that it should work.

> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index a5f380584cdae7..7a48fe6fc0dffc 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -241,6 +241,21 @@ static int __init param_setup_earlycon(char *buf)
>  }
>  early_param("earlycon", param_setup_earlycon);
> 
> +static int __init param_setup_earlycon_console_alias(char *buf)
> +{
> +       /* We only want to handle `console=XXXX` in this handler. We leave
> +          `console` and `console=` to be handled later by the non-early printk
> +          handler.
> +       */

I would prefer to be more clear about why we do not want to handle
this case here. Something like:

<proposal>
	/* `console` and `console=` are used to disable console	completely. */
</proposal>

> +       if (!buf || !buf[0]) {
> +               return 0;
> +       }
> +
> +       /* `console=uartXXXX` is actually an alias for `earlycon=uartXXXX`. */
> +       /* `console=uartXXXX` is actually an alias for `earlycon=uartXXXX`. */
> +       return param_setup_earlycon(buf);
> +}
> +early_param("console=", param_setup_earlycon_console_alias);

I am not sure if '=' can be part of the param. All other early_param()
definitions are without '='. Also it seems that next_arg() in lib/cmdline.c
replaces '=' with '\0'.


> +
>  #ifdef CONFIG_OF_EARLY_FLATTREE
> 
>  int __init of_setup_earlycon(const struct earlycon_id *match,
> diff --git a/init/main.c b/init/main.c
> index 8e1f6127e172c3..b6f0cb424176da 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -740,9 +740,7 @@ static int __init do_early_param(char *param, char *val,
>         const struct obs_kernel_param *p;
> 
>         for (p = __setup_start; p < __setup_end; p++) {
> -               if ((p->early && parameq(param, p->str)) ||
> -                   (strcmp(param, "console") == 0 &&
> -                    strcmp(p->str, "earlycon") == 0 && val && val[0])) {
> +               if ((p->early && parameq(param, p->str))) {
>                         if (p->setup_func(val) != 0)
>                                 pr_warn("Malformed early option '%s'\n", param);
>                 }

I really like this change if it works ;-)

Best Regards,
Petr