Hi Simon, Now that you are back, could you please have a look at ...
On Fri, Jul 18, 2025 at 14:03, Mattijs Korpershoek <[email protected]> wrote: > Hi George, > > On Wed, Jul 16, 2025 at 19:55, george chan <[email protected]> wrote: > >> Hi Mattijs, >> >> Thx for reply. >> >> 在 2025年7月16日週三 17:05，Mattijs Korpershoek <[email protected]> 寫道： >> >>> 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 >> >>>
