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

[Raul Rangel]

On Fri, Jul 28, 2023 at 11:57 AM Raul Rangel <rrangel@xxxxxxxxxxxx> 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?

> >
> > Here is the proposal:
> >
> > From d2fb7c54bed4c67df229c56fcc1b0af83ada5227 Mon Sep 17 00:00:00 2001
> > From: Raul E Rangel <rrangel@xxxxxxxxxxxx>
> > Date: Fri, 7 Jul 2023 19:17:25 -0600
> > Subject: [PATCH] init: Don't proxy the empty console= options to earlycon
> >
> > Right now we are proxying the `console=XXX` command line args to the
> > param_setup_earlycon. This is done because the early consoles used to
> > be enabled via the `console` parameter.
> >
> > The `earlycon` parameter was added later by the commit 18a8bd949d6adb311
> > ("serial: convert early_uart to earlycon for 8250"). It allowed to
> > setup the early consoles using another callback. And it was indeed
> > more self-explanatory and cleaner approach.
> >
> > As a result, for example, the following parameters have the same effect:
> >
> >     console=uart[8250],mmio,<addr>[,options]
> >     earlycon=uart[8250],mmio,<addr>[,options]
> >
> > Nevertheless, `console` and `earlycon` parameters behave different when
> > the options do not match an early console. setup_earlycon() accepts only
> > console names in __earlycon_table. Also the empty options are handled
> > differently:
> >
> >   + When `earlycon=` or just `earlycon` is specified on the command line,
> >     param_setup_earlycon() tries to enable an early console via the SPCR
> >     table or the device tree.
> >
> >   + When `console=` is specified on the command line, it's intention is to
> >     disable the console.
> >
> > Here comes the problem. The current code calls param_setup_earlycon()
> > even when `console=` is used with no options. As a result, it might
> > enable the early console when it is defined in the SPCR table or
> > a device tree. This is obviously not what users want.
> >
> > The early console should be enabled via SPCR or DT only when `earlycon`
> > is used explicitly with no options.
> >
> > Signed-off-by: Raul E Rangel <rrangel@xxxxxxxxxxxx>
> > [pmladek@xxxxxxxx: Add comments and more details into the commit message]
> > ---
> >  init/main.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/init/main.c b/init/main.c
> > index ad920fac325c..3b059e316e62 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -733,13 +733,25 @@ 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)
> > -               ) {
> > +               if (p->early && parameq(param, p->str)) {
> >                         if (p->setup_func(val) != 0)
> >                                 pr_warn("Malformed early option '%s'\n", param);
> >                 }
> > +
> > +               /*
> > +                * Early consoles can be historically enabled also when earlycon
> > +                * specific options are passed via console=[earlycon options]
> > +                * parameter.
> > +                *
> > +                * Do not try it with an empty value. "console=" is used to
> > +                * disable real normal consoles. While "earlycon" is used to
> > +                * enable an early console via SPCR or DT.
> > +                */
> > +               if (strcmp(param, "console") == 0 && val && val[0] &&
> > +                   strcmp(p->str, "earlycon") == 0) {
> > +                       if (p->setup_func(val) != 0)
> > +                               pr_warn("Failed to setup earlycon via console=%s\n", val);
> > +               }
> >         }
> >         /* We accept everything at this stage. */
> >         return 0;
> > --
> > 2.35.3
> >
>
> Signed-off-by: Raul E Rangel <rrangel@xxxxxxxxxxxx>

Thanks,
Raul