[PATCH] env: env_sf: don't set .init op if not needed

[PATCH] env: env_sf: don't set .init op if not needed

[Michael Walle]

Commit 92765f45bb95 ("env: Access Environment in SPI flashes before
relocation") at least breaks the Kontron sl28 board. I guess it also
breaks others which use a (late) SPI environment.

Unfortunately, we cannot set the .init op and fall back to the same
behavior as it would be unset. Thus guard the .init op by #if's.

Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before relocation")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 env/sf.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/env/sf.c b/env/sf.c
index 2eb2de1a4e..18d44a7ddc 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -385,7 +385,7 @@ out:
 }
 #endif
 
-static int env_sf_init(void)
+static int __maybe_unused env_sf_init(void)
 {
 #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
 	return env_sf_init_addr();
@@ -393,8 +393,13 @@ static int env_sf_init(void)
 	return env_sf_init_early();
 #endif
 	/*
-	 * we must return with 0 if there is nothing done,
-	 * else env_set_inited() get not called in env_init()
+	 * We shouldn't end up here. Unfortunately, there is no
+	 * way to return a value which yields the same behavior
+	 * as if the .init op wouldn't be set at all. See
+	 * env_init(); env_set_inited() is only called if we
+	 * return 0, but the default environment is only loaded
+	 * if -ENOENT is returned. Therefore, we need the ugly
+	 * ifdeferry for the .init op.
 	 */
 	return 0;
 }
@@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = {
 	ENV_NAME("SPIFlash")
 	.load		= env_sf_load,
 	.save		= CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL,
+#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY)
 	.init		= env_sf_init,
+#endif
 };
-- 
2.20.1

[PATCH] env: env_sf: don't set .init op if not needed

[Heiko Schocher]

Hello Michael,

Am 01.11.2020 um 14:38 schrieb Michael Walle:
> Commit 92765f45bb95 ("env: Access Environment in SPI flashes before
> relocation") at least breaks the Kontron sl28 board. I guess it also
> breaks others which use a (late) SPI environment.
> 
> Unfortunately, we cannot set the .init op and fall back to the same
> behavior as it would be unset. Thus guard the .init op by #if's.
> 
> Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before relocation")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>   env/sf.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/env/sf.c b/env/sf.c
> index 2eb2de1a4e..18d44a7ddc 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -385,7 +385,7 @@ out:
>   }
>   #endif
>   
> -static int env_sf_init(void)
> +static int __maybe_unused env_sf_init(void)
>   {
>   #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>   	return env_sf_init_addr();
> @@ -393,8 +393,13 @@ static int env_sf_init(void)
>   	return env_sf_init_early();
>   #endif
>   	/*
> -	 * we must return with 0 if there is nothing done,
> -	 * else env_set_inited() get not called in env_init()
> +	 * We shouldn't end up here. Unfortunately, there is no
> +	 * way to return a value which yields the same behavior
> +	 * as if the .init op wouldn't be set at all. See
> +	 * env_init(); env_set_inited() is only called if we
> +	 * return 0, but the default environment is only loaded
> +	 * if -ENOENT is returned. Therefore, we need the ugly
> +	 * ifdeferry for the .init op.
>   	 */
>   	return 0;
>   }
> @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = {
>   	ENV_NAME("SPIFlash")
>   	.load		= env_sf_load,
>   	.save		= CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL,
> +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY)
>   	.init		= env_sf_init,
> +#endif
>   };
> 

Ok, tested this patch on an imx6 based board with SPI NOR and it works.

But.... there is a problem with environment in spi nor and ENV_APPEND
enabled, with current implementation (also before my patches applied):

I enabled now ENV_APPEND on this board and

CONFIG_ENV_IS_NOWHERE
CONFIG_ENV_IS_IN_SPI_FLASH

and the Environment from SPI NOR never loaded as gd->env_valid is
always ENV_INVALID and env_load() never called from env_relocate().

What do you think about following patch:

diff --git a/env/sf.c b/env/sf.c
index 2eb2de1a4e..7f3491b458 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -393,9 +393,13 @@ static int env_sf_init(void)
         return env_sf_init_early();
  #endif
         /*
-        * we must return with 0 if there is nothing done,
-        * else env_set_inited() get not called in env_init()
+        * We must return with 0 if there is nothing done,
+        * to get inited bit set in env_init().
+        * We need to set env_valid to ENV_VALID, so later
+        * env_load() loads the Environment from SPI NOR.
          */
+       gd->env_addr = (ulong)&default_environment[0];
+       gd->env_valid = ENV_VALID;
         return 0;
  }

Can you try it?

Another option would be to reutrn -ENOENT and set init bit also
when a init function returns -ENOENT:

diff --git a/env/env.c b/env/env.c
index 42c7d8155e..37b4b54cb7 100644
--- a/env/env.c
+++ b/env/env.c
@@ -329,6 +329,8 @@ int env_init(void)
         for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
                 if (!drv->init || !(ret = drv->init()))
                         env_set_inited(drv->location);
+               if (ret == -ENOENT)
+                       env_set_inited(drv->location);

                 debug("%s: Environment %s init done (ret=%d)\n", __func__,
                       drv->name, ret);
diff --git a/env/sf.c b/env/sf.c
index 2eb2de1a4e..66279fb4f4 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -396,7 +396,7 @@ static int env_sf_init(void)
          * we must return with 0 if there is nothing done,
          * else env_set_inited() get not called in env_init()
          */
-       return 0;
+       return -ENOENT;
  }

But this may has impact on other environment drivers ... but may is
the cleaner approach as env_init() later sets the default environment
if ret is -ENOENT ...

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[PATCH] env: env_sf: don't set .init op if not needed

[Wolfgang Denk]

Dear Heiko,

In message <a80c8548-a8a5-fb98-f9e3-fc659c2bdfec@denx.de> you wrote:
>
> I enabled now ENV_APPEND on this board and
>
> CONFIG_ENV_IS_NOWHERE
> CONFIG_ENV_IS_IN_SPI_FLASH

This gives me the creeps.  I know this is not cause by anything in
your patch, but anyway...

Apparently the meaning of CONFIG_ENV_IS_NOWHERE is nowhere
documented :-(

But common sense says that "IS NOWHERE" means that there is no
storage defined for the environment.  I would expect, that Kconfig
does not even allow to enable any CONFIG_ENV_IS_IN_* when
CONFIG_ENV_IS_NOWHERE is selected - these are logically exclusive.

May I suggest that:

1) our Kconfig files are changed such that CONFIG_ENV_IS_NOWHERE and
   CONFIG_ENV_IS_IN_* are indeed exclusive, so that we adhere to the
   POLA [1] ?

2) for cases like this one, where there actually _is_ some storage
   defined, but it shall be used in a non-standard way, a new
   CONFIG_ option gets created that expresses in it's name what it
   does?

[1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment

Thanks!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You can fool some of the people all of the time, and You can fool all
of the people some of the time, but You can't fool mom.

[PATCH] env: env_sf: don't set .init op if not needed

[Heiko Schocher]

Hello Wolfgang,

Am 02.11.2020 um 13:51 schrieb Wolfgang Denk:
> Dear Heiko,
> 
> In message <a80c8548-a8a5-fb98-f9e3-fc659c2bdfec@denx.de> you wrote:
>>
>> I enabled now ENV_APPEND on this board and
>>
>> CONFIG_ENV_IS_NOWHERE
>> CONFIG_ENV_IS_IN_SPI_FLASH
> 
> This gives me the creeps.  I know this is not cause by anything in
> your patch, but anyway...
> 
> Apparently the meaning of CONFIG_ENV_IS_NOWHERE is nowhere
> documented :-(

env/Kconfig says:

config ENV_IS_NOWHERE
         bool "Environment is not stored"
         default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \
                      !ENV_IS_IN_FAT && !ENV_IS_IN_FLASH && \
                      !ENV_IS_IN_MMC && !ENV_IS_IN_NAND && \
                      !ENV_IS_IN_NVRAM && !ENV_IS_IN_ONENAND && \
                      !ENV_IS_IN_REMOTE && !ENV_IS_IN_SPI_FLASH && \
                      !ENV_IS_IN_UBI
         help
           Define this if you don't want to or can't have an environment stored
           on a storage medium. In this case the environment will still exist
           while U-Boot is running, but once U-Boot exits it will not be
           stored. U-Boot will therefore always start up with a default
           environment.


> But common sense says that "IS NOWHERE" means that there is no
> storage defined for the environment.  I would expect, that Kconfig

Yes and use default one ...

> does not even allow to enable any CONFIG_ENV_IS_IN_* when
> CONFIG_ENV_IS_NOWHERE is selected - these are logically exclusive.

Hmm...

> May I suggest that:
> 
> 1) our Kconfig files are changed such that CONFIG_ENV_IS_NOWHERE and
>     CONFIG_ENV_IS_IN_* are indeed exclusive, so that we adhere to the
>     POLA [1] ?

Yes if we really want such a hard setting without having an environment!

Currently CONFIG_ENV_IS_NOWHERE is not that hard and means U-Boot has
no Environment ... it means use the built in default environment...

> 2) for cases like this one, where there actually _is_ some storage
>     defined, but it shall be used in a non-standard way, a new
>     CONFIG_ option gets created that expresses in it's name what it
>     does?

Not in a none standard way! Instead you can define more than one
environment storage devices and load them in a board specific order
(defined thorugh board specfif function env_get_location())

For example:
- load first default environment (and you are correct ENV_IS_NOWHERE
     is here really a misleading name).

- load environment from SPI NOR ...

May we only should do a simple rename ?

ENV_IS_NOWHERE -> ENV_IS_IN_UBOOT (or ENV_IS_IN_DEFAULT) ?

If we really want a hard "there is no storage" switch (which really
means there is *no* environment ... I do not even know, if U-boot
works without!) we should introduce a new ENV_IS_IN_DEFAULT which
loads the default environment...

(I do not like this, as all? boards have a default environment, so
  it is enabled for all? boards ... which makes it obsolete...)

better suggestions?

bye,
Heiko
> 
> [1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment
> 
> Thanks!
> 
> Best regards,
> 
> Wolfgang Denk
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[PATCH] env: env_sf: don't set .init op if not needed

[Wolfgang Denk]

Dear Heiko,

In message <c32e8504-1f86-8579-3240-e4cf41e847c9@denx.de> you wrote:
>
> > Apparently the meaning of CONFIG_ENV_IS_NOWHERE is nowhere
> > documented :-(
>
> env/Kconfig says:

Ah, I missed that.  I was only checkng the README and the doc/
files...

> config ENV_IS_NOWHERE
>          bool "Environment is not stored"

OK, but this is a pretty clear statement.

>          help
>            Define this if you don't want to or can't have an environment stored
>            on a storage medium. In this case the environment will still exist
>            while U-Boot is running, but once U-Boot exits it will not be
>            stored. U-Boot will therefore always start up with a default
>            environment.
>
>
> > But common sense says that "IS NOWHERE" means that there is no
> > storage defined for the environment.  I would expect, that Kconfig
>
> Yes and use default one ...

Correct.  So this matches my understanding of the name of the config
option: use this if there is no storage defined for the environment.

> > does not even allow to enable any CONFIG_ENV_IS_IN_* when
> > CONFIG_ENV_IS_NOWHERE is selected - these are logically exclusive.
>
> Hmm...
>
> > May I suggest that:
> > 
> > 1) our Kconfig files are changed such that CONFIG_ENV_IS_NOWHERE and
> >     CONFIG_ENV_IS_IN_* are indeed exclusive, so that we adhere to the
> >     POLA [1] ?
>
> Yes if we really want such a hard setting without having an environment!

Well, this is exactly what this config option is supposed to do -
both according to the name and according to the documentation: "the
environment ... will not be stored."

> Currently CONFIG_ENV_IS_NOWHERE is not that hard and means U-Boot has
> no Environment ... it means use the built in default environment...

I would rephrase this: Currently the implemantation does not enforce
the correct behaviour, so it has been misused for other (also
useful) things.  But this is not clean and should be fixed.

> > 2) for cases like this one, where there actually _is_ some storage
> >     defined, but it shall be used in a non-standard way, a new
> >     CONFIG_ option gets created that expresses in it's name what it
> >     does?
>
> Not in a none standard way! Instead you can define more than one
> environment storage devices and load them in a board specific order
> (defined thorugh board specfif function env_get_location())

Yes, agreed.  But this logically impossible if there is no storage
at all, which is what CONFIG_ENV_IS_NOWHERE says.

For such cases we must not define CONFIG_ENV_IS_NOWHERE, but
something else / something new instead.

> For example:
> - load first default environment (and you are correct ENV_IS_NOWHERE
>      is here really a misleading name).
>
> - load environment from SPI NOR ...
>
> May we only should do a simple rename ?
>
> ENV_IS_NOWHERE -> ENV_IS_IN_UBOOT (or ENV_IS_IN_DEFAULT) ?

In my understanding, ENV_IS_IN_* defines the storage device used to
save the persistent copy (the external representation(s)) of the
environment, which get written when you use "env save".  So both
your suggestions are not acceptable, as the the envrionment never
gets saved to the default environment - this is just the data that
is used to initialize it in the same was as in an emergency when the
persistent the environment (more precisely: when all copies of it)
cannot be read (I/O or checksum error).

> If we really want a hard "there is no storage" switch (which really

You miss the fact that we already have this: CONFIG_ENV_IS_NOWHERE

It is just not implemented correctly, and I ask for a cleanup.

> means there is *no* environment ... I do not even know, if U-boot
> works without!) we should introduce a new ENV_IS_IN_DEFAULT which
> loads the default environment...

You misunderstand.  "No storage" means no storage for writing the
_persistent_ environment, see above.  Int his case there is still
the same emergency handling as in cases of corrupted peristent
environment - the fallback to the builtin (socalled "default")
environment.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Uncertain fortune is thoroughly mastered by the equity of the  calcu-
lation.                                               - Blaise Pascal

[PATCH] env: env_sf: don't set .init op if not needed

[Rasmus Villemoes]

On 03/11/2020 08.52, Wolfgang Denk wrote:
> Dear Heiko,
> 
> In message <c32e8504-1f86-8579-3240-e4cf41e847c9@denx.de> you wrote:
>>
>>> Apparently the meaning of CONFIG_ENV_IS_NOWHERE is nowhere
>>> documented :-(
>>
>> env/Kconfig says:
> 
> Ah, I missed that.  I was only checkng the README and the doc/
> files...
> 
>> config ENV_IS_NOWHERE
>>          bool "Environment is not stored"
> 
> OK, but this is a pretty clear statement.
> 
>>          help
>>            Define this if you don't want to or can't have an environment stored
>>            on a storage medium. In this case the environment will still exist
>>            while U-Boot is running, but once U-Boot exits it will not be
>>            stored. U-Boot will therefore always start up with a default
>>            environment.
>>
>>
>>> But common sense says that "IS NOWHERE" means that there is no
>>> storage defined for the environment.  I would expect, that Kconfig
>>
>> Yes and use default one ...
> 
> Correct.  So this matches my understanding of the name of the config
> option: use this if there is no storage defined for the environment.
> 
>>> does not even allow to enable any CONFIG_ENV_IS_IN_* when
>>> CONFIG_ENV_IS_NOWHERE is selected - these are logically exclusive.
>>
>> Hmm...
>>
>>> May I suggest that:
>>>
>>> 1) our Kconfig files are changed such that CONFIG_ENV_IS_NOWHERE and
>>>     CONFIG_ENV_IS_IN_* are indeed exclusive, so that we adhere to the
>>>     POLA [1] ?
>>
>> Yes if we really want such a hard setting without having an environment!
> 
> Well, this is exactly what this config option is supposed to do -
> both according to the name and according to the documentation: "the
> environment ... will not be stored."
> 
>> Currently CONFIG_ENV_IS_NOWHERE is not that hard and means U-Boot has
>> no Environment ... it means use the built in default environment...
> 
> I would rephrase this: Currently the implemantation does not enforce
> the correct behaviour, so it has been misused for other (also
> useful) things.  But this is not clean and should be fixed.
> 
>>> 2) for cases like this one, where there actually _is_ some storage
>>>     defined, but it shall be used in a non-standard way, a new
>>>     CONFIG_ option gets created that expresses in it's name what it
>>>     does?
>>
>> Not in a none standard way! Instead you can define more than one
>> environment storage devices and load them in a board specific order
>> (defined thorugh board specfif function env_get_location())
> 
> Yes, agreed.  But this logically impossible if there is no storage
> at all, which is what CONFIG_ENV_IS_NOWHERE says.

Then should all the current config options CONFIG_ENV_IS_ be renamed to
CONFIG_ENV_MAY_BE_? Because that's really what they mean.

I certainly agree the current naming isn't very accurate, but the
ability to choose CONFIG_ENV_IS_NOWHERE along with some "real" storage
option is quite useful. We use it so that we can use the exact same
U-Boot binary for both development and production, just different .dtbs;
there's a board-specific env_get_location() which reads a DT property to
decide whether to return ENVL_SPI_FLASH or ENVL_NOWHERE. Whether there
actually needs to be a stub nowhere.c driver for that or one could just
return ENVL_UNKNOWN I don't know.

Rasmus

[PATCH] env: env_sf: don't set .init op if not needed

[Wolfgang Denk]

Dear Rasmus,

In message <8ff3b8ad-8c4e-fe99-69c8-7c174e997a49@prevas.dk> you wrote:
>
> >> Not in a none standard way! Instead you can define more than one
> >> environment storage devices and load them in a board specific order
> >> (defined thorugh board specfif function env_get_location())
> > 
> > Yes, agreed.  But this logically impossible if there is no storage
> > at all, which is what CONFIG_ENV_IS_NOWHERE says.
>
> Then should all the current config options CONFIG_ENV_IS_ be renamed to
> CONFIG_ENV_MAY_BE_? Because that's really what they mean.

This is not correct.

The CONFIG_ENV_IS_FOO means: if you use "env save", then U-Boot will
write the config to external storage using the FOO storage device.

> I certainly agree the current naming isn't very accurate, but the
> ability to choose CONFIG_ENV_IS_NOWHERE along with some "real" storage
> option is quite useful. We use it so that we can use the exact same
> U-Boot binary for both development and production, just different .dtbs;
> there's a board-specific env_get_location() which reads a DT property to
> decide whether to return ENVL_SPI_FLASH or ENVL_NOWHERE. Whether there
> actually needs to be a stub nowhere.c driver for that or one could just
> return ENVL_UNKNOWN I don't know.

If it makes sense do have a nulldev, then it should be added as a
CONFIG_ENV_IS_ option.  This is something fundamental different from
NOWHERE.

If you define CONFIG_ENV_IS_NOWHERE, I would for example expect that
all "env save" related code is omitted as we will never need it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A metaphor is like a simile.

[PATCH] env: env_sf: don't set .init op if not needed

[Rasmus Villemoes]

On 05/11/2020 17.40, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <8ff3b8ad-8c4e-fe99-69c8-7c174e997a49@prevas.dk> you wrote:
>>
>>>> Not in a none standard way! Instead you can define more than one
>>>> environment storage devices and load them in a board specific order
>>>> (defined thorugh board specfif function env_get_location())
>>>
>>> Yes, agreed.  But this logically impossible if there is no storage
>>> at all, which is what CONFIG_ENV_IS_NOWHERE says.
>>
>> Then should all the current config options CONFIG_ENV_IS_ be renamed to
>> CONFIG_ENV_MAY_BE_? Because that's really what they mean.
> 
> This is not correct.
> 
> The CONFIG_ENV_IS_FOO means: if you use "env save", then U-Boot will
> write the config to external storage using the FOO storage device.

Wolfgang, you're wrong. What you're saying was once true, when the
location was a "choice" in Kconfig, but it hasn't been that since
fb69464eae. Nowadays one can select multiple possible backends, and only
one of them will be used when doing "env save".

Later (208bd2b8), _NOWHERE was made non-mutually-exclusive with the real
storage targets.

> If you define CONFIG_ENV_IS_NOWHERE, I would for example expect that
> all "env save" related code is omitted as we will never need it.

I haven't checked, but that functionality does seem to exist - not
depending on whether CONFIG_ENV_IS_NOWHERE is not selected, but whether
any of the CONFIG_ENV_IS_<somehere> is. See the ENV_IS_IN_DEVICE logic
in cmd/nvedit.c. So if you select CONFIG_ENV_IS_NOWHERE and not any of
the others, I think the build works as you'd expect.

Rasmus

[PATCH] env: env_sf: don't set .init op if not needed

[Tom Rini]

On Fri, Nov 06, 2020 at 08:46:27AM +0100, Rasmus Villemoes wrote:
> On 05/11/2020 17.40, Wolfgang Denk wrote:
> > Dear Rasmus,
> > 
> > In message <8ff3b8ad-8c4e-fe99-69c8-7c174e997a49@prevas.dk> you wrote:
> >>
> >>>> Not in a none standard way! Instead you can define more than one
> >>>> environment storage devices and load them in a board specific order
> >>>> (defined thorugh board specfif function env_get_location())
> >>>
> >>> Yes, agreed.  But this logically impossible if there is no storage
> >>> at all, which is what CONFIG_ENV_IS_NOWHERE says.
> >>
> >> Then should all the current config options CONFIG_ENV_IS_ be renamed to
> >> CONFIG_ENV_MAY_BE_? Because that's really what they mean.
> > 
> > This is not correct.
> > 
> > The CONFIG_ENV_IS_FOO means: if you use "env save", then U-Boot will
> > write the config to external storage using the FOO storage device.
> 
> Wolfgang, you're wrong. What you're saying was once true, when the
> location was a "choice" in Kconfig, but it hasn't been that since
> fb69464eae. Nowadays one can select multiple possible backends, and only
> one of them will be used when doing "env save".
> 
> Later (208bd2b8), _NOWHERE was made non-mutually-exclusive with the real
> storage targets.

Yes.  It's intentional that "NOWHERE" means that if this is our run-time
location for the environment, then you cannot save the environment.  But
since we finally implemented the ability to have more than one option
enabled and pick the correct one at run time, we build both "mmc" and
"nowhere".

> > If you define CONFIG_ENV_IS_NOWHERE, I would for example expect that
> > all "env save" related code is omitted as we will never need it.
> 
> I haven't checked, but that functionality does seem to exist - not
> depending on whether CONFIG_ENV_IS_NOWHERE is not selected, but whether
> any of the CONFIG_ENV_IS_<somehere> is. See the ENV_IS_IN_DEVICE logic
> in cmd/nvedit.c. So if you select CONFIG_ENV_IS_NOWHERE and not any of
> the others, I think the build works as you'd expect.

So, CMD_SAVEENV exists.  If you don't enable that, I would expect
save-related code that can be garbage collected to be discarded.  It's
unclear without further digging if there's code that could be discarded
that is not, but checking the map file for a "nowhere" only build would
probably be somewhat instructive.

-- 
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/20201106/b4a1c4be/attachment.sig>

[PATCH] env: env_sf: don't set .init op if not needed

[Wolfgang Denk]

Dear Tom,

In message <20201106204557.GG5340@bill-the-cat> you wrote:
> 
> > Later (208bd2b8), _NOWHERE was made non-mutually-exclusive with the real
> > storage targets.
>
> Yes.  It's intentional that "NOWHERE" means that if this is our run-time
> location for the environment, then you cannot save the environment.  But
> since we finally implemented the ability to have more than one option
> enabled and pick the correct one at run time, we build both "mmc" and
> "nowhere".

But this breaks logic.  If you wanna have a nulldev, than implement
a /dev/null.

CONFIG_ENV_IS_NOWHERE should be the means to be used when no
environment storage drivers are needed / selected, not even
/dev/null.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
My challenge to the goto-less programmer  is  to  recode  tcp_input()
without any gotos ... without any loss of efficiency (there has to be
a catch).                                             - W. R. Stevens

[PATCH] env: env_sf: don't set .init op if not needed

[Wolfgang Denk]

Dear Rasmus,

In message <ac9500de-d236-be83-04a8-8f68be1c7672@prevas.dk> you wrote:
>
> > The CONFIG_ENV_IS_FOO means: if you use "env save", then U-Boot will
> > write the config to external storage using the FOO storage device.
>
> Wolfgang, you're wrong. What you're saying was once true, when the
> location was a "choice" in Kconfig, but it hasn't been that since
> fb69464eae. Nowadays one can select multiple possible backends, and only
> one of them will be used when doing "env save".

I know that.  But that slection is made from the selected drivers
enabled by the CONFIG_ENV_IS_* settings.

> Later (208bd2b8), _NOWHERE was made non-mutually-exclusive with the real
> storage targets.

And this is a buf that should be fixed, as it makes no sense at all.

> > If you define CONFIG_ENV_IS_NOWHERE, I would for example expect that
> > all "env save" related code is omitted as we will never need it.
>
> I haven't checked, but that functionality does seem to exist - not
> depending on whether CONFIG_ENV_IS_NOWHERE is not selected, but whether
> any of the CONFIG_ENV_IS_<somehere> is. See the ENV_IS_IN_DEVICE logic
> in cmd/nvedit.c. So if you select CONFIG_ENV_IS_NOWHERE and not any of
> the others, I think the build works as you'd expect.

This is intransparent and error prone.  This check must not be
implemented somewhere in the code, but at Kconfig level.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
      Bugs are by far the largest and  most successful class of
      entity, with nearly a million known species. In this res-
      pect they outnumber all the other  known  creatures about
      four to one.  -- Professor Snope's Encyclopedia of Animal

[PATCH] env: env_sf: don't set .init op if not needed

[Michael Walle]

Am 2020-11-02 08:00, schrieb Heiko Schocher:
> Hello Michael,
> 
> Am 01.11.2020 um 14:38 schrieb Michael Walle:
>> Commit 92765f45bb95 ("env: Access Environment in SPI flashes before
>> relocation") at least breaks the Kontron sl28 board. I guess it also
>> breaks others which use a (late) SPI environment.
>> 
>> Unfortunately, we cannot set the .init op and fall back to the same
>> behavior as it would be unset. Thus guard the .init op by #if's.
>> 
>> Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before 
>> relocation")
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>   env/sf.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>> 
>> diff --git a/env/sf.c b/env/sf.c
>> index 2eb2de1a4e..18d44a7ddc 100644
>> --- a/env/sf.c
>> +++ b/env/sf.c
>> @@ -385,7 +385,7 @@ out:
>>   }
>>   #endif
>>   -static int env_sf_init(void)
>> +static int __maybe_unused env_sf_init(void)
>>   {
>>   #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>>   	return env_sf_init_addr();
>> @@ -393,8 +393,13 @@ static int env_sf_init(void)
>>   	return env_sf_init_early();
>>   #endif
>>   	/*
>> -	 * we must return with 0 if there is nothing done,
>> -	 * else env_set_inited() get not called in env_init()
>> +	 * We shouldn't end up here. Unfortunately, there is no
>> +	 * way to return a value which yields the same behavior
>> +	 * as if the .init op wouldn't be set at all. See
>> +	 * env_init(); env_set_inited() is only called if we
>> +	 * return 0, but the default environment is only loaded
>> +	 * if -ENOENT is returned. Therefore, we need the ugly
>> +	 * ifdeferry for the .init op.
>>   	 */
>>   	return 0;
>>   }
>> @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = {
>>   	ENV_NAME("SPIFlash")
>>   	.load		= env_sf_load,
>>   	.save		= CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : 
>> NULL,
>> +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || 
>> defined(CONFIG_ENV_SPI_EARLY)
>>   	.init		= env_sf_init,
>> +#endif
>>   };
>> 
> 
> Ok, tested this patch on an imx6 based board with SPI NOR and it works.
> 
> But.... there is a problem with environment in spi nor and ENV_APPEND
> enabled, with current implementation (also before my patches applied):
> 
> I enabled now ENV_APPEND on this board and
> 
> CONFIG_ENV_IS_NOWHERE
> CONFIG_ENV_IS_IN_SPI_FLASH
> 
> and the Environment from SPI NOR never loaded as gd->env_valid is
> always ENV_INVALID and env_load() never called from env_relocate().
> 
> What do you think about following patch:
> 
> diff --git a/env/sf.c b/env/sf.c
> index 2eb2de1a4e..7f3491b458 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -393,9 +393,13 @@ static int env_sf_init(void)
>         return env_sf_init_early();
>  #endif
>         /*
> -        * we must return with 0 if there is nothing done,
> -        * else env_set_inited() get not called in env_init()
> +        * We must return with 0 if there is nothing done,
> +        * to get inited bit set in env_init().
> +        * We need to set env_valid to ENV_VALID, so later
> +        * env_load() loads the Environment from SPI NOR.
>          */
> +       gd->env_addr = (ulong)&default_environment[0];
> +       gd->env_valid = ENV_VALID;
>         return 0;
>  }
> 
> Can you try it?

This works for me...

> Another option would be to reutrn -ENOENT and set init bit also
> when a init function returns -ENOENT:
> 
> diff --git a/env/env.c b/env/env.c
> index 42c7d8155e..37b4b54cb7 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -329,6 +329,8 @@ int env_init(void)
>         for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); 
> prio++) {
>                 if (!drv->init || !(ret = drv->init()))
>                         env_set_inited(drv->location);
> +               if (ret == -ENOENT)
> +                       env_set_inited(drv->location);
> 
>                 debug("%s: Environment %s init done (ret=%d)\n", 
> __func__,
>                       drv->name, ret);
> diff --git a/env/sf.c b/env/sf.c
> index 2eb2de1a4e..66279fb4f4 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -396,7 +396,7 @@ static int env_sf_init(void)
>          * we must return with 0 if there is nothing done,
>          * else env_set_inited() get not called in env_init()
>          */
> -       return 0;
> +       return -ENOENT;
>  }
> 
> But this may has impact on other environment drivers ... but may is
> the cleaner approach as env_init() later sets the default environment
> if ret is -ENOENT ...

.. and also this.

So we have four solutions
(1) revert the series
(2) apply my patch
(3) use the first solution from Heiko
(4) use the second solution from Heiko

I'm fine with all four. If it will be (3) or (4) will you prepare a
patch, Heiko?

-michael

[PATCH] env: env_sf: don't set .init op if not needed

[Heiko Schocher]

Hello Michael,

Am 02.11.2020 um 21:15 schrieb Michael Walle:
> Am 2020-11-02 08:00, schrieb Heiko Schocher:
>> Hello Michael,
>>
>> Am 01.11.2020 um 14:38 schrieb Michael Walle:
>>> Commit 92765f45bb95 ("env: Access Environment in SPI flashes before
>>> relocation") at least breaks the Kontron sl28 board. I guess it also
>>> breaks others which use a (late) SPI environment.
>>>
>>> Unfortunately, we cannot set the .init op and fall back to the same
>>> behavior as it would be unset. Thus guard the .init op by #if's.
>>>
>>> Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before relocation")
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>> ? env/sf.c | 13 ++++++++++---
>>> ? 1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/env/sf.c b/env/sf.c
>>> index 2eb2de1a4e..18d44a7ddc 100644
>>> --- a/env/sf.c
>>> +++ b/env/sf.c
>>> @@ -385,7 +385,7 @@ out:
>>> ? }
>>> ? #endif
>>> ? -static int env_sf_init(void)
>>> +static int __maybe_unused env_sf_init(void)
>>> ? {
>>> ? #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>>> ????? return env_sf_init_addr();
>>> @@ -393,8 +393,13 @@ static int env_sf_init(void)
>>> ????? return env_sf_init_early();
>>> ? #endif
>>> ????? /*
>>> -???? * we must return with 0 if there is nothing done,
>>> -???? * else env_set_inited() get not called in env_init()
>>> +???? * We shouldn't end up here. Unfortunately, there is no
>>> +???? * way to return a value which yields the same behavior
>>> +???? * as if the .init op wouldn't be set at all. See
>>> +???? * env_init(); env_set_inited() is only called if we
>>> +???? * return 0, but the default environment is only loaded
>>> +???? * if -ENOENT is returned. Therefore, we need the ugly
>>> +???? * ifdeferry for the .init op.
>>> ?????? */
>>> ????? return 0;
>>> ? }
>>> @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = {
>>> ????? ENV_NAME("SPIFlash")
>>> ????? .load??????? = env_sf_load,
>>> ????? .save??????? = CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL,
>>> +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY)
>>> ????? .init??????? = env_sf_init,
>>> +#endif
>>> ? };
>>>
>>
>> Ok, tested this patch on an imx6 based board with SPI NOR and it works.
>>
>> But.... there is a problem with environment in spi nor and ENV_APPEND
>> enabled, with current implementation (also before my patches applied):
>>
>> I enabled now ENV_APPEND on this board and
>>
>> CONFIG_ENV_IS_NOWHERE
>> CONFIG_ENV_IS_IN_SPI_FLASH
>>
>> and the Environment from SPI NOR never loaded as gd->env_valid is
>> always ENV_INVALID and env_load() never called from env_relocate().
>>
>> What do you think about following patch:
>>
>> diff --git a/env/sf.c b/env/sf.c
>> index 2eb2de1a4e..7f3491b458 100644
>> --- a/env/sf.c
>> +++ b/env/sf.c
>> @@ -393,9 +393,13 @@ static int env_sf_init(void)
>> ??????? return env_sf_init_early();
>> ?#endif
>> ??????? /*
>> -??????? * we must return with 0 if there is nothing done,
>> -??????? * else env_set_inited() get not called in env_init()
>> +??????? * We must return with 0 if there is nothing done,
>> +??????? * to get inited bit set in env_init().
>> +??????? * We need to set env_valid to ENV_VALID, so later
>> +??????? * env_load() loads the Environment from SPI NOR.
>> ???????? */
>> +?????? gd->env_addr = (ulong)&default_environment[0];
>> +?????? gd->env_valid = ENV_VALID;
>> ??????? return 0;
>> ?}
>>
>> Can you try it?
> 
> This works for me...
> 
>> Another option would be to reutrn -ENOENT and set init bit also
>> when a init function returns -ENOENT:
>>
>> diff --git a/env/env.c b/env/env.c
>> index 42c7d8155e..37b4b54cb7 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -329,6 +329,8 @@ int env_init(void)
>> ??????? for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
>> ??????????????? if (!drv->init || !(ret = drv->init()))
>> ??????????????????????? env_set_inited(drv->location);
>> +?????????????? if (ret == -ENOENT)
>> +?????????????????????? env_set_inited(drv->location);
>>
>> ??????????????? debug("%s: Environment %s init done (ret=%d)\n", __func__,
>> ????????????????????? drv->name, ret);
>> diff --git a/env/sf.c b/env/sf.c
>> index 2eb2de1a4e..66279fb4f4 100644
>> --- a/env/sf.c
>> +++ b/env/sf.c
>> @@ -396,7 +396,7 @@ static int env_sf_init(void)
>> ???????? * we must return with 0 if there is nothing done,
>> ???????? * else env_set_inited() get not called in env_init()
>> ???????? */
>> -?????? return 0;
>> +?????? return -ENOENT;
>> ?}
>>
>> But this may has impact on other environment drivers ... but may is
>> the cleaner approach as env_init() later sets the default environment
>> if ret is -ENOENT ...
> 
> .. and also this.
> 
> So we have four solutions
> (1) revert the series
> (2) apply my patch
> (3) use the first solution from Heiko
> (4) use the second solution from Heiko
> 
> I'm fine with all four. If it will be (3) or (4) will you prepare a
> patch, Heiko?

I tend to implement solution [4] ... I can send a patch...

Simon? Tom? Any suggestions?

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[PATCH] env: env_sf: don't set .init op if not needed

[Tom Rini]

On Tue, Nov 03, 2020 at 05:40:16AM +0100, Heiko Schocher wrote:
> Hello Michael,
> 
> Am 02.11.2020 um 21:15 schrieb Michael Walle:
> > Am 2020-11-02 08:00, schrieb Heiko Schocher:
> > > Hello Michael,
> > > 
> > > Am 01.11.2020 um 14:38 schrieb Michael Walle:
> > > > Commit 92765f45bb95 ("env: Access Environment in SPI flashes before
> > > > relocation") at least breaks the Kontron sl28 board. I guess it also
> > > > breaks others which use a (late) SPI environment.
> > > > 
> > > > Unfortunately, we cannot set the .init op and fall back to the same
> > > > behavior as it would be unset. Thus guard the .init op by #if's.
> > > > 
> > > > Fixes: 92765f45bb95 ("env: Access Environment in SPI flashes before relocation")
> > > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > > ---
> > > > ? env/sf.c | 13 ++++++++++---
> > > > ? 1 file changed, 10 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/env/sf.c b/env/sf.c
> > > > index 2eb2de1a4e..18d44a7ddc 100644
> > > > --- a/env/sf.c
> > > > +++ b/env/sf.c
> > > > @@ -385,7 +385,7 @@ out:
> > > > ? }
> > > > ? #endif
> > > > ? -static int env_sf_init(void)
> > > > +static int __maybe_unused env_sf_init(void)
> > > > ? {
> > > > ? #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
> > > > ????? return env_sf_init_addr();
> > > > @@ -393,8 +393,13 @@ static int env_sf_init(void)
> > > > ????? return env_sf_init_early();
> > > > ? #endif
> > > > ????? /*
> > > > -???? * we must return with 0 if there is nothing done,
> > > > -???? * else env_set_inited() get not called in env_init()
> > > > +???? * We shouldn't end up here. Unfortunately, there is no
> > > > +???? * way to return a value which yields the same behavior
> > > > +???? * as if the .init op wouldn't be set at all. See
> > > > +???? * env_init(); env_set_inited() is only called if we
> > > > +???? * return 0, but the default environment is only loaded
> > > > +???? * if -ENOENT is returned. Therefore, we need the ugly
> > > > +???? * ifdeferry for the .init op.
> > > > ?????? */
> > > > ????? return 0;
> > > > ? }
> > > > @@ -404,5 +409,7 @@ U_BOOT_ENV_LOCATION(sf) = {
> > > > ????? ENV_NAME("SPIFlash")
> > > > ????? .load??????? = env_sf_load,
> > > > ????? .save??????? = CONFIG_IS_ENABLED(SAVEENV) ? ENV_SAVE_PTR(env_sf_save) : NULL,
> > > > +#if (defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)) || defined(CONFIG_ENV_SPI_EARLY)
> > > > ????? .init??????? = env_sf_init,
> > > > +#endif
> > > > ? };
> > > > 
> > > 
> > > Ok, tested this patch on an imx6 based board with SPI NOR and it works.
> > > 
> > > But.... there is a problem with environment in spi nor and ENV_APPEND
> > > enabled, with current implementation (also before my patches applied):
> > > 
> > > I enabled now ENV_APPEND on this board and
> > > 
> > > CONFIG_ENV_IS_NOWHERE
> > > CONFIG_ENV_IS_IN_SPI_FLASH
> > > 
> > > and the Environment from SPI NOR never loaded as gd->env_valid is
> > > always ENV_INVALID and env_load() never called from env_relocate().
> > > 
> > > What do you think about following patch:
> > > 
> > > diff --git a/env/sf.c b/env/sf.c
> > > index 2eb2de1a4e..7f3491b458 100644
> > > --- a/env/sf.c
> > > +++ b/env/sf.c
> > > @@ -393,9 +393,13 @@ static int env_sf_init(void)
> > > ??????? return env_sf_init_early();
> > > ?#endif
> > > ??????? /*
> > > -??????? * we must return with 0 if there is nothing done,
> > > -??????? * else env_set_inited() get not called in env_init()
> > > +??????? * We must return with 0 if there is nothing done,
> > > +??????? * to get inited bit set in env_init().
> > > +??????? * We need to set env_valid to ENV_VALID, so later
> > > +??????? * env_load() loads the Environment from SPI NOR.
> > > ???????? */
> > > +?????? gd->env_addr = (ulong)&default_environment[0];
> > > +?????? gd->env_valid = ENV_VALID;
> > > ??????? return 0;
> > > ?}
> > > 
> > > Can you try it?
> > 
> > This works for me...
> > 
> > > Another option would be to reutrn -ENOENT and set init bit also
> > > when a init function returns -ENOENT:
> > > 
> > > diff --git a/env/env.c b/env/env.c
> > > index 42c7d8155e..37b4b54cb7 100644
> > > --- a/env/env.c
> > > +++ b/env/env.c
> > > @@ -329,6 +329,8 @@ int env_init(void)
> > > ??????? for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
> > > ??????????????? if (!drv->init || !(ret = drv->init()))
> > > ??????????????????????? env_set_inited(drv->location);
> > > +?????????????? if (ret == -ENOENT)
> > > +?????????????????????? env_set_inited(drv->location);
> > > 
> > > ??????????????? debug("%s: Environment %s init done (ret=%d)\n", __func__,
> > > ????????????????????? drv->name, ret);
> > > diff --git a/env/sf.c b/env/sf.c
> > > index 2eb2de1a4e..66279fb4f4 100644
> > > --- a/env/sf.c
> > > +++ b/env/sf.c
> > > @@ -396,7 +396,7 @@ static int env_sf_init(void)
> > > ???????? * we must return with 0 if there is nothing done,
> > > ???????? * else env_set_inited() get not called in env_init()
> > > ???????? */
> > > -?????? return 0;
> > > +?????? return -ENOENT;
> > > ?}
> > > 
> > > But this may has impact on other environment drivers ... but may is
> > > the cleaner approach as env_init() later sets the default environment
> > > if ret is -ENOENT ...
> > 
> > .. and also this.
> > 
> > So we have four solutions
> > (1) revert the series
> > (2) apply my patch
> > (3) use the first solution from Heiko
> > (4) use the second solution from Heiko
> > 
> > I'm fine with all four. If it will be (3) or (4) will you prepare a
> > patch, Heiko?
> 
> I tend to implement solution [4] ... I can send a patch...
> 
> Simon? Tom? Any suggestions?

Lets go with #4 above then please, thanks!

-- 
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/20201103/4235ae21/attachment.sig>