Hi Mattijs, Congrats to be a new dad.
在 2025年7月18日週五 20:03，Mattijs Korpershoek <[email protected]> 寫道： > 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 ? > > > > 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 > > > >> >
