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

[Marek Vasut]

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.