On 6/2/20 2:05 PM, 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 newdata, but the existing hash table can never be dropped and reloadedwith newly imported data. This may be used in combination with staticflags 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).
Anything which does not explicitly spell _SPL or _TPL is U-Boot only, except for some remaining options which need fixing.
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?
Yes, because you might need to differentiate between the env behavior in TPL/SPL/U-Boot.
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.
I don't have a way to test it in SPL, so I'm not adding untested config options.