Hi George,
On Wed, Jul 16, 2025 at 19:55, george chan <gchan9...@gmail.com> wrote:
> Hi Mattijs,
>
> Thx for reply.
>
> 在 2025年7月16日週三 17:05，Mattijs Korpershoek <mkorpersh...@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?
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
>
>>
