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

[george chan]

Hi Mattijs,

Congrats to be a new dad.

在 2025年7月18日週五 20:03，Mattijs Korpershoek <mkorpershoek at kernel.org> 寫道：

> Hi George,
>
> On Wed, Jul 16, 2025 at 19:55, george chan <gchan9527 at gmail.com> wrote:
>
> > 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
>
> Simon, is it *intentional* that override the bootargs variable in
> commit daffb0be2c83 ("bootstd: cros: Add ARM support") ?
>
> Shouldn't we get the bootargs from the environment, and append cmdline
> to the existing bootargs instead ?
>
>
> > 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.
>
> What do you mean by the vendor_boot cmdline is not handled?
>

Yes I mean it. If origin/master logic (for fastboot, my test case) had gone
through android_image_get_kernel then this patch is not necessary.


> When parsing the vendor_boot image, we go through
> android_vendor_boot_image_v3_v4_parse_hdr()
>
> That function copies the vendor_boot cmdline with:
>
>         data->kcmdline_extra = hdr->cmdline;
>

Yes and that it is. Since many routes now through bootm_boot_start() with
single cmdline so obviously vndboot_cmd is yet to finalized unless in
previous stage logic combine boot/vendor cmdline by purpose. So here a
better choice is to extend with 3rd param for vndrboot_cmdline. And let the
new bootm_boot_start_ex() for example to combine/modify.

for example:
Int bootm_boot_start_ex(ulong addr, const char *cmdline, const char
*vendor_cmdline) {
...
ret = android_image_modify_bootargs_env(cmdline, vendor_cmdline);
...
}

I did not do this way because it is a bit  clumsy. But in code maintain
point it might be good.


> (to the struct andr_image_data).
>
> Later on, in android_image_get_kernel(), this gets copied to the
> bootargs:
>
>         if (img_data.kcmdline_extra && *img_data.kcmdline_extra) {
>                 if (*newbootargs) /* If there is something in newbootargs,
> a space is needed */
>                         strcat(newbootargs, " ");
>                 strcat(newbootargs, img_data.kcmdline_extra);
>         }
>
>         env_set("bootargs", newbootargs);
>
> >
> > (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.
>
> The difference is that in the patch I've linked is that we only change
> Android specific boot behaviour. So less risk to impact ChromeOS.
>

Yes. As above suggested _ex() function to get around the limitation. Just
leave those old caller to stick with the old bootm_boot_start().


> >
> > 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.
>
> I'm not sure about why this is better, as from what I understand we
> handle vendor_boot cmdline properly already.
>

This is my previous patch aimed to do....I moved all bootargs handling from
android_image_get_kernel into a new function and reuse it in
bootm_boot_start. I am glad you have same assumption on how bootargs should
be handled.


> Can you provide me with a test case or some example commands that show
> that vendor_boot cmdline is not handled properly?
>

May be above explain is enough?


> >
> > 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.
>
> I'm sorry there is so much back and forth on this series. I hope we can
> come to a common agreement and get something merged.
>

Yes I am struggle with this as well. If there are still some uncertain,
please consider merge patches with review-by and leave theose questioning
patch alone?

Thx again for reviewing.

Regards,
George


> Thanks
> Mattijs
>
> >
> > Regards,
> > George
> >
> >>
>