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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi George,

On Sat, Jul 19, 2025 at 01:17, george chan <gchan9527@gmail.com> wrote:

> Hi Mattijs,
>
> Congrats to be a new dad.

Thank you! Sorry for all the delays. I'm just back from paternity leave.
I hope we can work together to get your patches merged.

>
> 在 2025年7月18日週五 20:03，Mattijs Korpershoek <mkorpershoek@kernel.org> 寫道：
>
>> Hi George,
>>
>> On Wed, Jul 16, 2025 at 19:55, george chan <gchan9527@gmail.com> wrote:
>>
>> > Hi Mattijs,
>> >
>> > Thx for reply.
>> >
>> > 在 2025年7月16日週三 17:05，Mattijs Korpershoek <mkorpershoek@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.

If this is indeed a bug, maybe fixing it would be the easiest. Now that
Simon is back, pinged him about this.

>> >
>> > (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
>> >
>> >>
>>