On Tue, Jun 02, 2020 at 02:09:57PM +0200, Marek Vasut wrote: > 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 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). > > Anything which does not explicitly spell _SPL or _TPL is U-Boot only, > except for some remaining options which need fixing.
No, it's not true that every option in Kconfig needs to be listed in triplicate. > > 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. I'm not sure it's valid to say that env can behave different (outside specific cases like readonly before full 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. Then you should default to making SPL behave the same way as full U-Boot. -- Tom
signature.asc
Description: PGP signature