From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
To: Yoann Congal <yoann.congal@smile.fr>, Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de, Masahiro Yamada <masahiroy@kernel.org>
Subject: Re: [PATCH] kconfig: default to zero if int/hex symbol lacks default property
Date: Mon, 13 May 2024 11:45:00 +0200 [thread overview]
Message-ID: <0060272e-7086-4dd9-8935-e915f6831387@prevas.dk> (raw)
In-Reply-To: <7192141a-248b-44cd-8030-3e1a3b30b6ca@smile.fr>
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.congal@smile.fr/
> [1]: https://lore.kernel.org/all/20231104222715.3967791-1-yoann.congal@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=8n9r7kQrLTPpKwqXG1d1sd0WjJ8PQhOXHXxnSyNQ@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
next prev parent reply other threads:[~2024-05-13 9:45 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-05 13:53 [PATCH] kconfig: default to zero if int/hex symbol lacks default property Yoann Congal
2024-05-06 17:43 ` Tom Rini
2024-05-07 7:17 ` Yoann Congal
2024-05-07 17:23 ` Tom Rini
2024-05-13 8:02 ` Yoann Congal
2024-05-13 8:28 ` Rasmus Villemoes
2024-05-13 9:03 ` Yoann Congal
2024-05-13 9:45 ` Rasmus Villemoes [this message]
2024-05-13 9:07 ` Rasmus Villemoes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0060272e-7086-4dd9-8935-e915f6831387@prevas.dk \
--to=rasmus.villemoes@prevas.dk \
--cc=masahiroy@kernel.org \
--cc=trini@konsulko.com \
--cc=u-boot@lists.denx.de \
--cc=yoann.congal@smile.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).