Hi George, On Sat, Jul 19, 2025 at 01:17, george chan <[email protected]> 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 <[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. 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 >> > >> >> >>
