| Message ID | 20200529175404.627741-1-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: Add option to only ever append environment | expand |
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. Rasmus
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.
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
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. > 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. > 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.
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) ?
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).
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?
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.
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.
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.
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?
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. 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.
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.
On 6/2/20 7:36 PM, Tom Rini wrote: [...] >>>>>> 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? If you have only one env source in SPL and multiple in U-Boot for example. But this is besides the point, I want to be able to configure my env handling whichever I need it to without working around problems like the ones below. >> 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. This is the result of not having a dedicated SPL/TPL config options though.
On Tue, Jun 02, 2020 at 09:06:42PM +0200, Marek Vasut wrote: > On 6/2/20 7:36 PM, Tom Rini wrote: > [...] > >>>>>> 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? > > If you have only one env source in SPL and multiple in U-Boot for > example. But this is besides the point, Yes, so lets set that aside. > I want to be able to configure > my env handling whichever I need it to without working around problems > like the ones below. You're instead adding two others kinds of problems. You're adding code that would make use of a symbol that doesn't exist. You're also adding what seems like a non-functional runtime (we set the variable in full U-Boot and can't read it in SPL). So can you confirm that having this enabled in full U-Boot but disabled in SPL does not result in the case of a mismatch in the environment, in the case of having access to more than just the default compiled environment? > >> 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. > > This is the result of not having a dedicated SPL/TPL config options though. Then we should fix that. But not every option is/should be listed in triplicate.
On 6/3/20 1:32 AM, Tom Rini wrote: > On Tue, Jun 02, 2020 at 09:06:42PM +0200, Marek Vasut wrote: >> On 6/2/20 7:36 PM, Tom Rini wrote: >> [...] >>>>>>>> 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? >> >> If you have only one env source in SPL and multiple in U-Boot for >> example. But this is besides the point, > > Yes, so lets set that aside. > >> I want to be able to configure >> my env handling whichever I need it to without working around problems >> like the ones below. > > You're instead adding two others kinds of problems. You're adding code > that would make use of a symbol that doesn't exist. You're also adding > what seems like a non-functional runtime (we set the variable in full > U-Boot and can't read it in SPL). So can you confirm that having this > enabled in full U-Boot but disabled in SPL does not result in the case > of a mismatch in the environment, in the case of having access to more > than just the default compiled environment? I have the env completely disabled in SPL, so it does not. >>>> 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. >> >> This is the result of not having a dedicated SPL/TPL config options though. > > Then we should fix that. But not every option is/should be listed in > triplicate. OK, then I can re-do this patch without the CONFIG_IS_ENABLED() and then add another #undef into the board config.
diff --git a/env/Kconfig b/env/Kconfig index ca7fef682b..8166e5df91 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -604,6 +604,15 @@ config DELAY_ENVIRONMENT later by U-Boot code. With CONFIG_OF_CONTROL this is instead controlled by the value of /config/load-environment. +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 } else if (ret == -ENOMSG) { /* Handle "bad CRC" case */ if (best_prio == -1) diff --git a/lib/hashtable.c b/lib/hashtable.c index b96dbe19be..c2bf75fb76 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -822,6 +822,10 @@ int himport_r(struct hsearch_data *htab, if (nvars) memcpy(localvars, vars, sizeof(vars[0]) * nvars); +#if CONFIG_IS_ENABLED(ENV_APPEND) + flag |= H_NOCLEAR; +#endif + if ((flag & H_NOCLEAR) == 0 && !nvars) { /* Destroy old hash table if one exists */ debug("Destroy Hash Table: %p table = %p\n", htab,
Add configuration option which prevents the environment hash table to be ever cleared and reloaded with different content. This is useful in case the first environment loaded into the hash table contains e.g. sensitive content which must not be dropped or reloaded. Signed-off-by: Marek Vasut <marex@denx.de> --- env/Kconfig | 9 +++++++++ env/env.c | 2 ++ lib/hashtable.c | 4 ++++ 3 files changed, 15 insertions(+)