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) ? -- Tom
signature.asc
Description: PGP signature
