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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Simon,

Now that you are back, could you please have a look at ...

On Fri, Jul 18, 2025 at 14:03, Mattijs Korpershoek <mkorpershoek@kernel.org> wrote:

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

... this question ?

We are wondering why bootm_boot_start() overrides the bootargs variable
instead of appending 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?
>
> 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;
>
> (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.
>
>>
>> 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.
>
> Can you provide me with a test case or some example commands that show
> that vendor_boot cmdline is not handled properly?
>
>>
>> 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.
>
> Thanks
> Mattijs
>
>>
>> Regards,
>> George
>>
>>>