On 13/05/2024 11.03, Yoann Congal wrote: > > > Le 13/05/2024 à 10:28, Rasmus Villemoes a écrit : >> On 07/05/2024 19.23, Tom Rini wrote: >>> On Tue, May 07, 2024 at 09:17:46AM +0200, Yoann Congal wrote:
>>>> But, my more general use case is the Yocto dev trying to change the U-Boot >>>> (or any kconfig based software) config and accidentally trigger this. >>>> In Yocto, what they will see is a do_configure task taking a very long >>>> time but they won't see the looping logs the detect the problem (This is >>>> caught later by filing the RAM and the build process is killed by OOM, or >>>> the disk run out of space filed multi-GB log files). >>>> >>>> I'm sadly aware that defining a default value for this kind of config >>>> (fixed addresses) is not really sensible but the build time infinite loop >>>> is not good either. >>> >>> Exactly, but to me it's worse to cover up the build issue and introduce >>> a runtime issue instead. Something builds but then crashes at run time >>> is worse to me than builds fail / get stuck. Using your example, >>> CMD_PSTORE_MEM_ADDR depends on CMD_PSTORE and so if someone enables >>> CMD_PSTORE in a config fragment but doesn't define the address, the >>> build should not complete. Using 0x0 means that oops, now it fails to >>> work and might not be obvious why either. >> >> I agree with Tom. Yes, we've also hit those yocto failures with a >> multi-GB do_configure log file, and that's pretty annoying when one just >> starts a build cooking over night and then the next day one's laptop has >> a filled disk. So clearly there is/was a problem to solve. >> >> However, blindly adding a "default default" of 0 is not the right thing >> to do. It seems much better that, when there's no configured default and >> kconfig gets EOF when asking the user for input (as in the 'make >> oldconfig < /dev/null' case), kconfig should exit with an error >> explicitly saying "no value given for CONFIG_XYZ which also doesn't have >> a default value". That stops the build, and leaves precisely an >> indication what the problem is. Then the developer must either provide a >> value for CONFIG_XYZ in the fragments, or send a patch to add a >> reasonable default when that is at all possible (not so for e.g. PSTORE). > > > Agreed, that was the spirit of my first attempt at a fix[0] which was then > made broader[1] and finally rejected from "-next" because it triggered a > build failure[2]. > > I still think my first approach (exit with an error instead of starting the > infinite loop) was the right one (at least for U-Boot). The problem is that > this approach was rejected by the Linux kconfig maintainer (Masahiro Yamada > CC'd). I'm willing to clean it up for U-boot but this would diverge from > linux kconfig... Is that acceptable? > > [0]: > https://lore.kernel.org/all/20231031111647.111093-1-yoann.con...@smile.fr/ > [1]: > https://lore.kernel.org/all/20231104222715.3967791-1-yoann.con...@smile.fr/ > [2]: > https://lore.kernel.org/all/20231107210004.GA2065849@dev-arch.thelio-3990X/ Thanks for the pointers to the previous discussions. I see that we arrived at more or less the same patch in at least one of your versions. Masahiro, can you reconsider that "default default". Even if "most int/hex items" in linux usually have a default, one could still hit cases where they don't (usually some arch/soc/board specific physical address, e.g. for an early console, but naturally a bootloader is more prone to have those kinds of kconfig options). And if it really is not a problem for linux itself, the "default default" is a no-op for linux, but does cause unwanted behaviour for downstream users of kconfig. In https://lore.kernel.org/lkml/CAK7LNAS8a=8n9r7kqrltppkwqxg1d1sd0wjj8pqhoxhxxns...@mail.gmail.com/, which was a reply to a patch almost identical to the one I arrived at, you said It is strange (and consistent) to bail out only for particular types. [assume you meant inconsistent], but AFAICT, bool/tristate values always have (and always have had) a default, so this isn't really inconsistent as the bail out is only applicable to int/hex types (and maybe string? dunno if they default default to ""). Rasmus
