[PATCH] env: Add option to only ever append environment

[Marek Vasut]

On 6/2/20 4:43 PM, Tom Rini wrote:
> 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.

That makes no sense e.g. if you only have default env in SPL while
multiple envs in U-Boot.