[PATCH v4 3/6] bootm: Append bootargs value when bootmeth_android provide cmdline

[george chan]

Hi Mattijs,

Thx for reply.

在 2025年7月16日週三 17:05，Mattijs Korpershoek <mkorpershoek at kernel.org> 寫道：

> Hi George,
>
> Thank you for the patch.
>
...

>
> > -     ret = env_set("bootargs", cmdline);
> > +     ret = android_image_modify_bootargs_env(cmdline, NULL);
>
> I don't think it can be done this way. bootm_boot_start() is used in the
> ChromeOS bootmethod as well (boot/bootmeth_cros.c)
>

I got your point. I have 3 ideas come out.

(1) The logic purposely empty the env bootargs, either developer don't use
env bootargs or those use cases touched bootargs in some early step. And
then wanna disregard previous u-boot bootmeth operation, and empty bootargs
for that ongoing bootmeth stage...well that's not congruent behavior; I
have a gut feeling this is a bug or band-aid anyway, it closed the door for
people (potentially abootimg) inject needed boot param between bootmeth
stage. Perhaps, either clean up bootargs before bootmeth load stage, or add
kconfig to check and enable this logic, a better representation.

(2) One more thing, the vendor_boot cmdline is not handle neither in
original logic nor in url from you provided. So I can say the url one is
not capable for extended with Android boot img version > 2 since
vendor_boot cmdline not handled.

(3) I don't think it is very much different between your mentioned url with
my patch. There is nothing advanced, but just existing code logic reuse and
that logic handled vendor_cmdline in other path. I prefer code logic reuse.

And I believe above are not the important thing....

>
> Changing this would potentially break ChromeOS boot behaviour so I'd
> prefer to find another solution.
>
> I know that TI has a downstream patch that changes bootmeth_android.c
> instead:
>
>
> https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2024.04-next&id=9d802d798ac143e06ffe545606aa657a6c32cc63


...


> Would that work for you?
>
>
Well, the one from url also fine with me.

The important thing is here. So as to ease the change with "minimal impact"
gets in. I can add one extra check to maintain original behavior in case
people have concern of breakage. Code example as below:

+ If (IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_PREPEND_ENV_BOOTARGS))
+   ret = android_image_modify_bootargs_env(cmdline, NULL);
+ else
      ret = env_set("bootargs", cmdline);

This logic eliminated the concern, but it also limited those potential use
cases for Android boot header version > 2, unless the newly introduced
CONFIG_ANDROID_BOOT_IMAGE_PREPEND_ENV_BOOTARGS is defined.

Or this one with good extendible.
+ /* Clean up bootargs purposely */
+ if (IS_ENABLED(SOME_FLAG))
+    env_set("bootargs", "");
+ ret = android_image_modify_bootargs_env(cmdline, NULL);

Either way. I prefer first one and can blindly apply without afford. I
leave the idea above to people more concern with it. Please let me know
your decision, I can provide one more revision later.

Regards,
George

>