Hi Piotr,
On Tue, 17 Oct 2023 at 10:35, Tom Rini trini@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@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