[PATCH] bootm: Fix flags used for bootargs string substitution

[Simon Glass]

Hi Piotr,

On Tue, 17 Oct 2023 at 10:35, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Oct 17, 2023 at 12:53:05PM +0200, Piotr Kubik wrote:
>
> > Commit <51bb33846ad2> introduced a feature of bootargs
>
> Checkpatch will complain that this should be:
> In commit 51bb33846ad2 ("bootm: Support string substitution in
> bootargs")
> And this is the kind of thing I would fixup on applying if there was no
> other feedback.  However:
>
> > string substitution and changed a flag used in
> > bootm_process_cmdline_env() call to be either true or false.
> > With this flag value, condition in bootm_process_cmdline()
> > `if (flags & BOOTM_CL_SUBST)` is never true
> > and process_subst() is never called.
> >
> > Signed-off-by: Piotr Kubik <piotr.kubik at iopsys.eu>
> > ---
> >  boot/bootm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/boot/bootm.c b/boot/bootm.c
> > index 8f96a80d42..e96489e549 100644
> > --- a/boot/bootm.c
> > +++ b/boot/bootm.c
> > @@ -778,7 +778,8 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
> >       if (!ret && (states & BOOTM_STATE_OS_BD_T))
> >               ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images);
> >       if (!ret && (states & BOOTM_STATE_OS_PREP)) {
> > -             ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX);
> > +             ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ?
> > +                                                                             BOOTM_CL_ALL : 0);

Oh wow that is a terrible bug. We have lots of test coverage of
bootm_process_cmdline_env() and below, but bootm itself is still quite
spotty.

I wonder if we could add something to run_fit_test to check this. We
have lots of tests for bootm_process_cmdline_env() but not much of
bootm itself. It might be possible to add just a few lines there, e.g.
to set the bootargs to something and check what ends up in bootargs?

>
> This gets hard to read. I would prefer a comment and something like:
> int flags = 0;
> if (images->os.os == IH_OS_LINUX)
>   flags = BOOTM_CL_ALL;
> ret = bootm_process_cmdline_env(flags);
>
> As the compiler should be just as smart, and that'll be clear about
> what's going on.  Thanks.

Regards,
Simon