Hi Simon, Tom Thanks for your feedback > On Tue, 17 Oct 2023 at 10:35, Tom Rini <tr...@konsulko.com> wrote: >> >> Checkpatch will complain that this should be: >> In commit 51bb33846ad2 ("bootm: Support string substitution in >> bootargs")
Actually it did not complain, but I'll fix. > On 18.10.2023 05:32, Simon Glass wrote: > 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? That's a good idea, I'll check >> + ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ? >> + >> BOOTM_CL_ALL : 0); >> 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. Well, I followed previous way of coding this part and it was: ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ? BOOTM_CL_SILENT : 0); But sure, I can update to make it more readable. Regards, Piotr