On Tue, Jun 02, 2020 at 05:55:25PM +0200, Marek Vasut wrote:
On 6/2/20 4:38 PM, Tom Rini wrote:
On Tue, Jun 02, 2020 at 02:47:12PM +0200, Marek Vasut wrote:
On 6/2/20 2:44 PM, Tom Rini wrote:
On Tue, Jun 02, 2020 at 02:05:39PM +0200, Rasmus Villemoes wrote:
On 02/06/2020 13.04, Marek Vasut wrote:
On 6/2/20 8:42 AM, Rasmus Villemoes wrote: > On 29/05/2020 19.54, Marek Vasut wrote: >> +config ENV_APPEND >> + bool "Always append the environment with new data" >> + default n >> + help >> + If defined, the environment hash table is only ever appended with new >> + data, but the existing hash table can never be dropped and reloaded >> + with newly imported data. This may be used in combination with static >> + flags to e.g. to protect variables which must not be modified. >> + >> config ENV_ACCESS_IGNORE_FORCE >> bool "Block forced environment operations" >> default n >> diff --git a/env/env.c b/env/env.c >> index 024d36fdbe..967a9d36d7 100644 >> --- a/env/env.c >> +++ b/env/env.c >> @@ -204,7 +204,9 @@ int env_load(void) >> ret = drv->load(); >> if (!ret) { >> printf("OK\n"); >> +#if !CONFIG_IS_ENABLED(ENV_APPEND) >> return 0; >> +#endif > > Don't use CONFIG_IS_ENABLED() unless you actually introduce both > CONFIG_FOO and CONFIG_SPL_FOO. Otherwise the above > CONFIG_IS_ENABLED(ENV_APPEND) is guaranteed to evaluate to false in SPL. > Of course that only matters if environment support is enabled in SPL, > but some actually use that.
We actually want to use CONFIG_IS_ENABLED() as much as possible to make these options future-proof, so that others won't have to chase down all kinds of #ifdef CONFIG stuff and fix it later on for SPL/TPL/etc.
That makes no sense. You're introducing something whose help text doesn't spell out that the option only applies to U-Boot proper, and is completely ignored in SPL (since CONFIG_SPL_ENV_APPEND never exists). The reason it's ignored in SPL is that you use the SPL-or-not-SPL-aware CONFIG_IS_ENABLED() helper, and you say that's so that somebody in the future can implement CONFIG_SPL_ENV_APPEND?
If you intend for ENV_APPEND to be something that's either set or not set for a given board, then the code needs to use the SPL-agnostic IS_ENABLED(CONFIG_ENV_APPEND). If you intend it to be something that can be set independently for the env support in SPL vs U-Boot proper, you need to add both config options and, as you do, use CONFIG_IS_ENABLED.
How will this code behave if there is a mismatch between SPL and full U-Boot (disabled SPL, enabled full, as the patch stands today) ?
One will append the environment, the other will override it (if you have multiple envs enabled).
So it sounds like it wouldn't be valid to have this option differ between SPL and main U-Boot?
Consider the case where you have default env in SPL, and multiple envs in U-Boot proper.
Yes, today you can end up with cases where you build something that doesn't work as intended (likely something around falcon boot and/or boot count limit in env). Which is what I'm getting at here. Is there some cases where it would make any sense to enable this option in full U-Boot but disable it in SPL?