From mboxrd@z Thu Jan  1 00:00:00 1970
From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Date: Tue, 2 Jun 2020 14:05:39 +0200
Subject: [PATCH] env: Add option to only ever append environment
In-Reply-To: <f6df1bd9-a276-8b80-c6d1-db0912811c93@denx.de>
References: <20200529175404.627741-1-marex@denx.de>
 <7f9cd3ee-6661-4780-90cf-d2e81591a7ce@prevas.dk>
 <f6df1bd9-a276-8b80-c6d1-db0912811c93@denx.de>
Message-ID: <02e0326f-c93c-da9e-e0ec-dd3bc2dc9e4a@prevas.dk>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.

Rasmus