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

[Tom Rini <trini@konsulko.com>]

On Tue, Jun 02, 2020 at 06:06:17PM +0200, Marek Vasut wrote:
> On 6/2/20 6:00 PM, Tom Rini wrote:
> > On Tue, Jun 02, 2020 at 05:55:25PM +0200, Marek Vasut wrote:
> >> 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.
> > 
> > Yes, today you can end up with cases where you build something that doesn't
> > work as intended (likely something around falcon boot and/or boot count
> > limit in env).  Which is what I'm getting at here.  Is there some
> > cases where it would make any sense to enable this option in full U-Boot
> > but disable it in SPL?
> 
> Yes, like my current use case, where I want to configure the SPL
> differently than U-Boot itself. SPL doesn't even have environment
> support enabled, but it might be needed later.

Sorry I wasn't clear enough.  Does it make sense (when? how?) to have
environment in SPL but mismatch this feature?

> And also, I don't want to end up in the same problem we currently have
> e.g. with USB gadget, where I have to manually #ifdef CONFIG_SPL_BUILD
> #undef CONFIG_ options in the board config file.

Yes, don't do that, I've had to fix a few of those of late in catching
converted but still in config header options.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200602/a2300727/attachment.sig>