From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Tue, 2 Jun 2020 13:36:27 -0400 Subject: [PATCH] env: Add option to only ever append environment In-Reply-To: References: <20200529175404.627741-1-marex@denx.de> <7f9cd3ee-6661-4780-90cf-d2e81591a7ce@prevas.dk> <02e0326f-c93c-da9e-e0ec-dd3bc2dc9e4a@prevas.dk> <20200602124450.GA21630@bill-the-cat> <20200602143848.GH21630@bill-the-cat> <098da29c-9457-9a8e-7ca2-67b71d3df8f9@denx.de> <20200602160000.GM21630@bill-the-cat> Message-ID: <20200602173627.GN21630@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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: