[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[Rasmus Villemoes]

The various env storage drivers almost all have their own logic [1]
for deciding whether to compile and provide the .save method, many of
which fail to honour CONFIG_SPL_SAVEENV. For example, fat.c and sf.c
define a CMD_SAVEENV macro only for !CONFIG_SPL_BUILD, while ext4.c
"only" depends on CONFIG_CMD_SAVEENV - but CONFIG_SPL_SAVEENV=y,
CONFIG_CMD_SAVEENV=n is a valid combination.

A lot of that ifdeffery can be removed while at the same time
providing the .save method if either CONFIG_SPL_SAVEENV (for an SPL
build) or CONFIG_CMD_SAVEENV (for U-Boot proper) is set. The first two
patches introduce infrastructure for that, while the last three are
example conversions for the above-mentioned three storage drivers. The
sf.c is the one I need to use in the SPL and have actually tested,
ext4.c and fat.c are included mostly as low-hanging fruit.

[1] Here's the current conditions for which these three drivers
provide .save:

          SPL                     U-Boot
ext4.c    CONFIG_CMD_SAVEENV=y    CONFIG_CMD_SAVEENV=y
fat.c     never                   CONFIG_CMD_SAVEENV=y
sf.c      never                   CONFIG_CMD_SAVEENV=y [2]

[2] It always compiles env_sf_save for U-Boot proper, but then the use
of env_save_ptr() ends up with a build warning in case
CONFIG_CMD_SAVEENV=n - fat.c doesn't have that proplem.

Rasmus Villemoes (5):
  env: add SAVEENV as an alias of the CMD_SAVEENV symbol
  env_internal.h: add alternative ENV_SAVE_PTR macro
  env/fat.c: remove private CMD_SAVEENV logic
  env/ext4.c: remove CONFIG_CMD_SAVEENV ifdef
  env/sf.c: drop private CMD_SAVEENV logic

 env/Kconfig            |  3 +++
 env/ext4.c             |  4 +---
 env/fat.c              |  9 +--------
 env/sf.c               | 12 +-----------
 include/env_internal.h |  2 ++
 5 files changed, 8 insertions(+), 22 deletions(-)

-- 
2.23.0

[PATCH 1/5] env: add SAVEENV as an alias of the CMD_SAVEENV symbol

[Rasmus Villemoes]

Currently, testing whether to compile in support for saving the
environment is a bit awkward when one needs to take SPL_SAVEENV into
account, and quite a few storage drivers currently do not honour
SPL_SAVEENV.

To make it a bit easier to decide whether environment saving should be
enabled, introduce SAVEENV as an alias for the CMD_SAVEENV
symbol. Then one can simply use

  CONFIG_IS_ENABLED(SAVEENV)

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 env/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/env/Kconfig b/env/Kconfig
index 0d6f559b39..969308fe6c 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -3,6 +3,9 @@ menu "Environment"
 config ENV_SUPPORT
 	def_bool y
 
+config SAVEENV
+	def_bool y if CMD_SAVEENV
+
 config ENV_IS_NOWHERE
 	bool "Environment is not stored"
 	default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \
-- 
2.23.0

[PATCH 2/5] env_internal.h: add alternative ENV_SAVE_PTR macro

[Rasmus Villemoes]

The current definition of the env_save_ptr does not take SPL_SAVEENV
into account. Moreover, the way it is implemented means that drivers
need to guard the definitions of their _save methods with ifdefs to
avoid "defined but unused" warnings in case CMD_SAVEENV=n.

The ifdeffery can be avoided by using a "something ? x : NULL"
construction instead and still have the compiler elide the _save
method when it is not referenced. Unfortunately we can't just switch
the existing env_save_ptr macro, since that would give a lot of build
errors unless all the ifdeffery is removed at the same time.
Conversely, removing that ifdeffery first would merely lead to the
"defined but unused" warnings temporarily, but for some storage
drivers it requires a bit more work than just removing their private
CMD_SAVEENV logic.

So introduce an alternative to env_save_ptr, which for lack of a
better name is simply uppercased, allowing one to update storage
drivers piecemeal to both reduce their ifdeffery and honour
CONFIG_SPL_SAVEENV.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 include/env_internal.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/env_internal.h b/include/env_internal.h
index 90a4df8a72..e89fbdb1b7 100644
--- a/include/env_internal.h
+++ b/include/env_internal.h
@@ -207,6 +207,8 @@ struct env_driver {
 #define env_save_ptr(x) NULL
 #endif
 
+#define ENV_SAVE_PTR(x) (CONFIG_IS_ENABLED(SAVEENV) ? (x) : NULL)
+
 extern struct hsearch_data env_htab;
 
 #endif /* DO_DEPS_ONLY */
-- 
2.23.0

[PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic

[Rasmus Villemoes]

Always compile the env_fat_save() function, and let
CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether
it actually ends up being compiled in.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 env/fat.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/env/fat.c b/env/fat.c
index 1836556f36..cf2e5e2b26 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -26,12 +26,8 @@
 # endif
 #else
 # define LOADENV
-# if defined(CONFIG_CMD_SAVEENV)
-#  define CMD_SAVEENV
-# endif
 #endif
 
-#ifdef CMD_SAVEENV
 static int env_fat_save(void)
 {
 	env_t __aligned(ARCH_DMA_MINALIGN) env_new;
@@ -76,7 +72,6 @@ static int env_fat_save(void)
 
 	return 0;
 }
-#endif /* CMD_SAVEENV */
 
 #ifdef LOADENV
 static int env_fat_load(void)
@@ -135,7 +130,5 @@ U_BOOT_ENV_LOCATION(fat) = {
 #ifdef LOADENV
 	.load		= env_fat_load,
 #endif
-#ifdef CMD_SAVEENV
-	.save		= env_save_ptr(env_fat_save),
-#endif
+	.save		= ENV_SAVE_PTR(env_fat_save),
 };
-- 
2.23.0

[PATCH 4/5] env/ext4.c: remove CONFIG_CMD_SAVEENV ifdef

[Rasmus Villemoes]

Removing this ifdef/endif pair yields a "defined but unused warning"
for CONFIG_CMD_SAVEENV=n, but that vanishes if we use the ENV_SAVE_PTR
macro instead. This gives slightly better compile testing, and
moreover, it's possible to have

  CONFIG_CMD_SAVEENV=n
  CONFIG_SPL_SAVEENV=y
  SPL_ENV_IS_IN_EXT4=y

in which case env_ext4_save would erroneously not be compiled in.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 env/ext4.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/env/ext4.c b/env/ext4.c
index 1f6b1b5bd8..911e19c6d3 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -41,7 +41,6 @@ __weak const char *env_ext4_get_dev_part(void)
 	return (const char *)CONFIG_ENV_EXT4_DEVICE_AND_PART;
 }
 
-#ifdef CONFIG_CMD_SAVEENV
 static int env_ext4_save(void)
 {
 	env_t	env_new;
@@ -83,7 +82,6 @@ static int env_ext4_save(void)
 	puts("done\n");
 	return 0;
 }
-#endif /* CONFIG_CMD_SAVEENV */
 
 static int env_ext4_load(void)
 {
@@ -137,5 +135,5 @@ U_BOOT_ENV_LOCATION(ext4) = {
 	.location	= ENVL_EXT4,
 	ENV_NAME("EXT4")
 	.load		= env_ext4_load,
-	.save		= env_save_ptr(env_ext4_save),
+	.save		= ENV_SAVE_PTR(env_ext4_save),
 };
-- 
2.23.0

[PATCH 5/5] env/sf.c: drop private CMD_SAVEENV logic

[Rasmus Villemoes]

Deciding whether to compile the env_sf_save() function based solely on
CONFIG_SPL_BUILD is wrong: For U-Boot proper, it leads to a build
warning in case CONFIG_CMD_SAVEENV=n (because the env_save_ptr() macro
causes the function to indeed not be referenced anywhere). And for
SPL, when one selects CONFIG_SPL_SAVEENV, one obviously expects to
actually be able to save the environment.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 env/sf.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/env/sf.c b/env/sf.c
index 5ef4055219..22b70ad319 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -21,16 +21,12 @@
 #include <u-boot/crc.h>
 
 #ifndef CONFIG_SPL_BUILD
-#define CMD_SAVEENV
 #define INITENV
 #endif
 
 #ifdef CONFIG_ENV_OFFSET_REDUND
-#ifdef CMD_SAVEENV
 static ulong env_offset		= CONFIG_ENV_OFFSET;
 static ulong env_new_offset	= CONFIG_ENV_OFFSET_REDUND;
-#endif
-
 #endif /* CONFIG_ENV_OFFSET_REDUND */
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -69,7 +65,6 @@ static int setup_flash_device(void)
 }
 
 #if defined(CONFIG_ENV_OFFSET_REDUND)
-#ifdef CMD_SAVEENV
 static int env_sf_save(void)
 {
 	env_t	env_new;
@@ -148,7 +143,6 @@ static int env_sf_save(void)
 
 	return ret;
 }
-#endif /* CMD_SAVEENV */
 
 static int env_sf_load(void)
 {
@@ -187,7 +181,6 @@ out:
 	return ret;
 }
 #else
-#ifdef CMD_SAVEENV
 static int env_sf_save(void)
 {
 	u32	saved_size, saved_offset, sector;
@@ -247,7 +240,6 @@ static int env_sf_save(void)
 
 	return ret;
 }
-#endif /* CMD_SAVEENV */
 
 static int env_sf_load(void)
 {
@@ -313,9 +305,7 @@ U_BOOT_ENV_LOCATION(sf) = {
 	.location	= ENVL_SPI_FLASH,
 	ENV_NAME("SPI Flash")
 	.load		= env_sf_load,
-#ifdef CMD_SAVEENV
-	.save		= env_save_ptr(env_sf_save),
-#endif
+	.save		= ENV_SAVE_PTR(env_sf_save),
 #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
 	.init		= env_sf_init,
 #endif
-- 
2.23.0

[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[Wolfgang Denk]

Dear Rasmus,

In message <20200219094726.26798-1-rasmus.villemoes@prevas.dk> you wrote:
>
> [1] Here's the current conditions for which these three drivers
> provide .save:
>
>           SPL                     U-Boot
> ext4.c    CONFIG_CMD_SAVEENV=y    CONFIG_CMD_SAVEENV=y
> fat.c     never                   CONFIG_CMD_SAVEENV=y
> sf.c      never                   CONFIG_CMD_SAVEENV=y [2]

Some questions:


1) I'm not sure if your changes cover the situation that you want to
   have "saveenv" available in U-Boot proper, but do NOT want to
   have it in SPL.  This may makie sense in situations where you
   need to be able to read the saved environment in SPL (say, to set
   up the configures console baud rate), but cannot affort the fule
   size resulting from adding "saveenv" etc.

   It is mandatory that this possibility is kept.

2) It seems wrong to me to make such cleanup in any way dependent on
   file system type or a mix of arbitrary storage driver types.
   this should be handled in two independent, orthogonal steps:
   a) clean up any drivers or file system accessors that do not fit
      into the general model
   b) adapt the general model to such changes

   Maybe it makes sense to change the order of these steps, if this
   results in less intrusive patches - I have no ides.

   In any case testing must _also_ include all the other many ways
   of storing the environment, including parallel or SPI NOR flash,
   NAND flash, UBI, UNIFS, etc.

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
In a business, marketroids, salespukes, and  lawyers  have  different
goals from those who actually do work and produce something. Usually,
is  is the former who triumph over the latter, due to the simple rule
that those who print the money make the rules.
         -- Tom Christiansen in <5jdcls$b04$2@csnews.cs.colorado.edu>

[PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic

[Wolfgang Denk]

Dear Rasmus,

In message <20200219094726.26798-4-rasmus.villemoes@prevas.dk> you wrote:
> Always compile the env_fat_save() function, and let
> CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether
> it actually ends up being compiled in.

Have you tested that this works?  How do the sizes of the
images differe before and after applying your changes?

[Same question for ext4]

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
Shakespeare's Law of Prototyping: (Hamlet III, iv, 156-160)
        O, throw away the worser part of it,
        And live the purer with the other half.

[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[Rasmus Villemoes]

On 19/02/2020 14.25, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <20200219094726.26798-1-rasmus.villemoes@prevas.dk> you wrote:
>>
>> [1] Here's the current conditions for which these three drivers
>> provide .save:
>>
>>           SPL                     U-Boot
>> ext4.c    CONFIG_CMD_SAVEENV=y    CONFIG_CMD_SAVEENV=y
>> fat.c     never                   CONFIG_CMD_SAVEENV=y
>> sf.c      never                   CONFIG_CMD_SAVEENV=y [2]
> 
> Some questions:
> 
> 
> 1) I'm not sure if your changes cover the situation that you want to
>    have "saveenv" available in U-Boot proper, but do NOT want to
>    have it in SPL. 

They do, that's the whole point of introducing the simple
CONFIG_IS_ENABLED(SAVEENV) - for answering the question "do we want to
enable saving the environment in this context". Then the .save method
gets built and linked in precisely if that's the case, so one gets a
consistent matrix that says

           SPL                     U-Boot
 ext4.c    CONFIG_SPL_SAVEENV=y    CONFIG_CMD_SAVEENV=y
 fat.c     CONFIG_SPL_SAVEENV=y    CONFIG_CMD_SAVEENV=y
 sf.c      CONFIG_SPL_SAVEENV=y    CONFIG_CMD_SAVEENV=y

But I can't fix the whole world in one go, so only the above three get
fixed to that state for now.

>    It is mandatory that this possibility is kept.

Of course, and _nothing_ changes in that regard. [It is of course
possible that I messed up with the implementation, but it is certainly
the intention to keep it that way.]

> 
> 2) It seems wrong to me to make such cleanup in any way dependent on
>    file system type or a mix of arbitrary storage driver types.
>    this should be handled in two independent, orthogonal steps:
>    a) clean up any drivers or file system accessors that do not fit
>       into the general model
>    b) adapt the general model to such changes
> 
>    Maybe it makes sense to change the order of these steps, if this
>    results in less intrusive patches - I have no ides.
> 
>    In any case testing must _also_ include all the other many ways
>    of storing the environment, including parallel or SPI NOR flash,
>    NAND flash, UBI, UNIFS, etc.

See above, I can't fix, much less test, all those backend drivers.
Nothing changes for those, they provide a .save method under exactly the
same (probably mutually inconsistent...) conditions as they used to. So
I expect that when someone else runs into wanting CONFIG_SPL_SAVEENV
honoured with, say, mmc backend, they probably quickly discover that
doesn't work at all, and then they can fix mmc.c to make it work.

But it's not in all cases as simply as just removing the
custom/arbitrary ifdef logic, sometimes those ifdefs cover code that
depends on certain macros or whatnot that are not available for an
SPL_BUILD.

Rasmus

[PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic

[Rasmus Villemoes]

On 19/02/2020 14.27, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <20200219094726.26798-4-rasmus.villemoes@prevas.dk> you wrote:
>> Always compile the env_fat_save() function, and let
>> CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether
>> it actually ends up being compiled in.
> 
> Have you tested that this works?  How do the sizes of the
> images differe before and after applying your changes?

With or without these patches, I get

$ size u-boot spl/u-boot-spl
   text    data     bss     dec     hex filename
 407173   45308   98352  550833   867b1 u-boot
  52403    3360     276   56039    dae7 spl/u-boot-spl
$ nm spl/u-boot-spl | grep env_fat
0090c5e8 t env_fat_load
$ nm u-boot | grep env_fat
17826cb4 t env_fat_load
17826c10 t env_fat_save

for a wandboard_defconfig modified by

-CONFIG_SPL_FS_EXT4=y
+CONFIG_SPL_FS_FAT=y
+CONFIG_SPL_ENV_SUPPORT=y
+CONFIG_ENV_IS_IN_FAT=y

So in the "read-only environment access in SPL" case, everything is the
same before and after.

Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my
patches we get

$ size u-boot spl/u-boot-spl
   text    data     bss     dec     hex filename
 407173   45308   98352  550833   867b1 u-boot
  58298    3360   65860  127518   1f21e spl/u-boot-spl
$ nm spl/u-boot-spl | grep env_fat
0090c6e0 t env_fat_load
0090c63c t env_fat_save

but without,

$ size u-boot spl/u-boot-spl
   text    data     bss     dec     hex filename
 407173   45308   98352  550833   867b1 u-boot
  52659    3360     280   56299    dbeb spl/u-boot-spl
$ nm spl/u-boot-spl | grep env_fat
0090c5e8 t env_fat_load

So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored.

> [Same question for ext4]

Actually, the situation for ext4 is even worse than indicated.

Just from reading code in ext4.c, env_ext4_save gets built into the SPL
if CONFIG_CMD_SAVEENV, whether or not CONFIG_SPL_SAVEENV is set. So I
expected my patch to simply reduce the spl image size in the
CONFIG_SPL_SAVEENV=n case. But one cannot compare - currently building with

CONFIG_CMD_SAVEENV=y
CONFIG_SPL_ENV_IS_IN_EXT4=y
CONFIG_SPL_SAVEENV=n

simply fails the SPL build because env_ext4_save refers to hexport_r,
which is only compiled if

!(defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_SAVEENV))

- which took me a while to read, it's a little easier if spelled

!defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_SAVEENV)

Anyway, a side-effect of my ext4 patch is that the above combination
actually builds, because env_ext4_save is not linked in, so hexport_r
isn't needed. And turning on SPL_SAVEENV also works as expected.

Rasmus

[PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic

[Wolfgang Denk]

Dear Rasmus,

In message <5265fdd5-3992-4e5f-3235-5586b3b77dd7@prevas.dk> you wrote:
>
> So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored.

OK, but what about bords that don't store the envionment in a file
system, but instead for example in (parallel or SPI) NOR flash or in
a UBI volume?


> Actually, the situation for ext4 is even worse than indicated.
...
> Anyway, a side-effect of my ext4 patch is that the above combination
> actually builds, because env_ext4_save is not linked in, so hexport_r
> isn't needed. And turning on SPL_SAVEENV also works as expected.

OK. 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
When you die, the first thing you lose is your life. The  next  thing
is the illusions.                       - Terry Pratchett, _Pyramids_

[PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic

[Tom Rini]

On Fri, Feb 21, 2020 at 05:14:14PM +0100, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <5265fdd5-3992-4e5f-3235-5586b3b77dd7@prevas.dk> you wrote:
> >
> > So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored.
> 
> OK, but what about bords that don't store the envionment in a file
> system, but instead for example in (parallel or SPI) NOR flash or in
> a UBI volume?

I think the intent is that there is no change today but the door is now
open for someone that can test / confirm changes there to do so.

-- 
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/20200221/90560075/attachment.sig>

[PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic

[Rasmus Villemoes]

On 21/02/2020 17.19, Tom Rini wrote:
> On Fri, Feb 21, 2020 at 05:14:14PM +0100, Wolfgang Denk wrote:
>> Dear Rasmus,
>>
>> In message <5265fdd5-3992-4e5f-3235-5586b3b77dd7@prevas.dk> you wrote:
>>>
>>> So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored.
>>
>> OK, but what about bords that don't store the envionment in a file
>> system, but instead for example in (parallel or SPI) NOR flash or in
>> a UBI volume?
> 
> I think the intent is that there is no change today but the door is now
> open for someone that can test / confirm changes there to do so.

Yes, exactly. I could have just fixed sf.c which is the one I need for
my current project, but it turns out that without the ability to say
CONFIG_IS_ENABLED(SAVEENV) the changes to sf.c would be significantly
uglier, so it seemed better to provide the infrastructure that will also
be useful for converting other storage drivers to honour CONFIG_SPL_SAVEENV.

Rasmus

[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[Rasmus Villemoes]

On 19/02/2020 10.47, Rasmus Villemoes wrote:
> The various env storage drivers almost all have their own logic [1]
> for deciding whether to compile and provide the .save method, many of
> which fail to honour CONFIG_SPL_SAVEENV. For example, fat.c and sf.c
> define a CMD_SAVEENV macro only for !CONFIG_SPL_BUILD, while ext4.c
> "only" depends on CONFIG_CMD_SAVEENV - but CONFIG_SPL_SAVEENV=y,
> CONFIG_CMD_SAVEENV=n is a valid combination.
> 
> A lot of that ifdeffery can be removed while at the same time
> providing the .save method if either CONFIG_SPL_SAVEENV (for an SPL
> build) or CONFIG_CMD_SAVEENV (for U-Boot proper) is set. The first two
> patches introduce infrastructure for that, while the last three are
> example conversions for the above-mentioned three storage drivers. The
> sf.c is the one I need to use in the SPL and have actually tested,
> ext4.c and fat.c are included mostly as low-hanging fruit.

Dear Wolfgang

Can I ask whether you request changes to this patch series or if my
answers to your various comments have been satisfactory?

Thanks,
Rasmus

[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[Wolfgang Denk]

Dear Rasmus Villemoes,

In message <9c03710e-5eec-da6e-6c15-2f8a14cfcc36@prevas.dk> you wrote:
>
> Can I ask whether you request changes to this patch series or if my
> answers to your various comments have been satisfactory?

I think you did no really answer to some of my concerns.

In Message <20200219132715.1F81A240036@gemini.denx.de> I asked:

| Have you tested that this works?  How do the sizes of the
| images differe before and after applying your changes?

You replied:

...
Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my
patches we get

| $ size u-boot spl/u-boot-spl
|    text    data     bss     dec     hex filename
|  407173   45308   98352  550833   867b1 u-boot
|   58298    3360   65860  127518   1f21e spl/u-boot-spl
| ....
| but without,
| 
| $ size u-boot spl/u-boot-spl
|    text    data     bss     dec     hex filename
|  407173   45308   98352  550833   867b1 u-boot
|   52659    3360     280   56299    dbeb spl/u-boot-spl

We can observe that

- the text size of the SPL grows from 52659 to 58298, i. e. by about
  5.5 kB or more than 10%
- the BSS size explodes from 280 to 65860 bytes, i. e. it grows from
  a few hndet bytes to more than 64 kB

I can see where the increase in text size is coming from - your
removal of #ifdef's now unconditionally includes some code that was
omitted before, for example functions env_fat_save(),
env_ext4_save(), env_sf_save(), plus a few variables.

It is not obvious to me but scary to see such an explosion of BSS
size.

It's difficult to comment here as it is not clear to me which exact
configuration you reported about, and it's also not clear if this is
a typical result, of if it's the only configuration you ever
tested.


Your patch description sounds as if it was just a #ifdef cleanup
without actual impact on the generated code, but the SPL size
differences above make it clear that it is not - or that your
testing has issues.

You also failed to comment on impact on other environment storage
configurations (NOR flash, NAND flash, UBI volume, ...).  If it's
only #ifdef changes without impact on function, then we should get
exactly the same images.  You did not comment if you have verified
that.

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
All easy problems have already been solved.

[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[Rasmus Villemoes]

On 25/03/2020 08.50, Wolfgang Denk wrote:
> Dear Rasmus Villemoes,
> 
> In message <9c03710e-5eec-da6e-6c15-2f8a14cfcc36@prevas.dk> you wrote:
>>
>> Can I ask whether you request changes to this patch series or if my
>> answers to your various comments have been satisfactory?
> 
> I think you did no really answer to some of my concerns.
> 
> In Message <20200219132715.1F81A240036@gemini.denx.de> I asked:
> 
> | Have you tested that this works?  How do the sizes of the
> | images differe before and after applying your changes?
> 
> You replied:
> 
> ...
> Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my
> patches we get
> 
> | $ size u-boot spl/u-boot-spl
> |    text    data     bss     dec     hex filename
> |  407173   45308   98352  550833   867b1 u-boot
> |   58298    3360   65860  127518   1f21e spl/u-boot-spl
> | ....
> | but without,
> | 
> | $ size u-boot spl/u-boot-spl
> |    text    data     bss     dec     hex filename
> |  407173   45308   98352  550833   867b1 u-boot
> |   52659    3360     280   56299    dbeb spl/u-boot-spl
> 
> We can observe that
> 
> - the text size of the SPL grows from 52659 to 58298, i. e. by about
>   5.5 kB or more than 10%
> - the BSS size explodes from 280 to 65860 bytes, i. e. it grows from
>   a few hndet bytes to more than 64 kB
> 
> I can see where the increase in text size is coming from - your
> removal of #ifdef's now unconditionally includes some code that was
> omitted before, for example functions env_fat_save(),
> env_ext4_save(), env_sf_save(), plus a few variables.

As intended for CONFIG_SPL_SAVEENV=y, no?

With my patches and CONFIG_SPL_SAVEENV=n, those env_fat_save,
env_ext4_save etc. are compiled, but then discarded (being static, they
are discarded already at compile-time, but otherwise they would be at
link-time), instead of being ifdeffed out unconditionally just because
of CONFIG_SPL_BUILD. I know that you share the opinion that one should
use IS_ENABLED() in preference to preprocessor conditionals, so I really
don't understand what you can possibly have against this approach.

> It is not obvious to me but scary to see such an explosion of BSS
> size.

> It's difficult to comment here as it is not clear to me which exact
> configuration you reported about,

Huh? I wrote exactly what I used to obtain those numbers for the FAT
case. Let me quote a bit more

====
With or without these patches, I get

$ size u-boot spl/u-boot-spl
   text    data     bss     dec     hex filename
 407173   45308   98352  550833   867b1 u-boot
  52403    3360     276   56039    dae7 spl/u-boot-spl
$ nm spl/u-boot-spl | grep env_fat
0090c5e8 t env_fat_load
$ nm u-boot | grep env_fat
17826cb4 t env_fat_load
17826c10 t env_fat_save

for a wandboard_defconfig modified by

-CONFIG_SPL_FS_EXT4=y
+CONFIG_SPL_FS_FAT=y
+CONFIG_SPL_ENV_SUPPORT=y
+CONFIG_ENV_IS_IN_FAT=y

So in the "read-only environment access in SPL" case, everything is the
same before and after.
====

That was the answer to "does it affect the generated code when one
doesn't enable CONFIG_SPL_SAVEENV". It doesn't, not at all, the code is
exactly the same. The next part then demonstrated how CONFIG_SPL_SAVEENV
is currently being ignored because of the ifdeffery in fat.c:

====
Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my
patches we get

$ size u-boot spl/u-boot-spl
   text    data     bss     dec     hex filename
 407173   45308   98352  550833   867b1 u-boot
  58298    3360   65860  127518   1f21e spl/u-boot-spl
$ nm spl/u-boot-spl | grep env_fat
0090c6e0 t env_fat_load
0090c63c t env_fat_save

but without,

$ size u-boot spl/u-boot-spl
   text    data     bss     dec     hex filename
 407173   45308   98352  550833   867b1 u-boot
  52659    3360     280   56299    dbeb spl/u-boot-spl
$ nm spl/u-boot-spl | grep env_fat
0090c5e8 t env_fat_load

So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored.
====

The .bss increase is simply due to the extra code that no longer gets
discarded by the linker, more precisely the .map file says there's a

 .bss.tmpbuf_cluster
                0x0000000000000000    0x10000 fs/built-in.o

that gets discarded without my patches (but with the config options
chosen so one would _expect_ to have save support in SPL). So yes, of
course there's a price to pay for enabling environment save support in
the SPL, with some backends being more expensive (in terms of footprint)
than others.

> and it's also not clear if this is
> a typical result, of if it's the only configuration you ever
> tested.
> 
> 
> Your patch description sounds as if it was just a #ifdef cleanup
> without actual impact on the generated code, but the SPL size
> differences above make it clear that it is not - or that your
> testing has issues.

There is _no_ change in code size, u-boot or spl, when
CONFIG_SPL_SAVEENV=n. My patches _only_ affect the case where
CONFIG_SPL_SAVEENV=y, and only in a way that the developer most likely
intended, namely actually allowing one to save the environment.

It is a cleanup, it removes pointless and actively harmful ifdeffery
that each storage driver has grown for no good reason (perhaps the "if
it's an SPL build, we don't need the .save method" logic all predates
the introduction of CONFIG_SPL_SAVEENV).

> You also failed to comment on impact on other environment storage
> configurations (NOR flash, NAND flash, UBI volume, ...).

I don't touch those files at all, so they are not affected. Some still
fail to honour CONFIG_SPL_SAVEENV (i.e., even if one sets
CONFIG_SPL_SAVEENV, saving the environment in the SPL is not actually
supported).

 If it's
> only #ifdef changes without impact on function, then we should get
> exactly the same images.  You did not comment if you have verified
> that.

To repeat myself, for CONFIG_SPL_SAVEENV=n, these patches don't change
anything, except get rid of a lot of pointless ifdefs. For
CONFIG_SPL_SAVEENV=y, the patches serve to honour the developer's
intention of actually being able to save the environment from SPL, at
least for fat, ext4 and sf.

Rasmus

[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[Wolfgang Denk]

Dear Rasmus,

In message <51077b65-56c1-464a-8721-77b6a7bf385d@prevas.dk> you wrote:
> > 
> > I think you did no really answer to some of my concerns.
> > 
> > In Message <20200219132715.1F81A240036@gemini.denx.de> I asked:
> > 
> > | Have you tested that this works?  How do the sizes of the
> > | images differe before and after applying your changes?
> > 
> > You replied:
> > 
> > ...
> > Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my
> > patches we get
> > 
> > | $ size u-boot spl/u-boot-spl
> > |    text    data     bss     dec     hex filename
> > |  407173   45308   98352  550833   867b1 u-boot
> > |   58298    3360   65860  127518   1f21e spl/u-boot-spl
> > | ....
> > | but without,
> > | 
> > | $ size u-boot spl/u-boot-spl
> > |    text    data     bss     dec     hex filename
> > |  407173   45308   98352  550833   867b1 u-boot
> > |   52659    3360     280   56299    dbeb spl/u-boot-spl
> > 
> > We can observe that
> > 
> > - the text size of the SPL grows from 52659 to 58298, i. e. by about
> >   5.5 kB or more than 10%
> > - the BSS size explodes from 280 to 65860 bytes, i. e. it grows from
> >   a few hndet bytes to more than 64 kB
> > 
> > I can see where the increase in text size is coming from - your
> > removal of #ifdef's now unconditionally includes some code that was
> > omitted before, for example functions env_fat_save(),
> > env_ext4_save(), env_sf_save(), plus a few variables.
>
> As intended for CONFIG_SPL_SAVEENV=y, no?

As you don't bother to mention which exact configuration you are
testing against, it is impossible to tell what exactly you are
doing.

But you give here two versions _with_ and _without_ your patches,
so I _assume_ all the other parameters are kept the same, and with
your patches the BSS size explodes.

Please elucidate,

> With my patches and CONFIG_SPL_SAVEENV=n, those env_fat_save,
> env_ext4_save etc. are compiled, but then discarded (being static, they
> are discarded already at compile-time, but otherwise they would be at
> link-time), instead of being ifdeffed out unconditionally just because
> of CONFIG_SPL_BUILD. I know that you share the opinion that one should
> use IS_ENABLED() in preference to preprocessor conditionals, so I really
> don't understand what you can possibly have against this approach.

Please explain why the memory footprint explodes.

> > It's difficult to comment here as it is not clear to me which exact
> > configuration you reported about,
>
> Huh? I wrote exactly what I used to obtain those numbers for the FAT
> case. Let me quote a bit more

And what exactly is "the FAT case"?  You did not explain what
exactly you are comparing...

> for a wandboard_defconfig modified by
>
> -CONFIG_SPL_FS_EXT4=y
> +CONFIG_SPL_FS_FAT=y
> +CONFIG_SPL_ENV_SUPPORT=y
> +CONFIG_ENV_IS_IN_FAT=y

Is this your test case?  I don't think you mentioned this before.

You your test cases are mainline U-Boot, using wandboard_defconfig
plus these 4 changes, and then either with and without your patches
applied?

> That was the answer to "does it affect the generated code when one
> doesn't enable CONFIG_SPL_SAVEENV". It doesn't, not at all, the code is
> exactly the same. The next part then demonstrated how CONFIG_SPL_SAVEENV
> is currently being ignored because of the ifdeffery in fat.c:

No.  My question was if the code size differs for the same
configurations with and without your patches applied.

> Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my
> patches we get
>
> $ size u-boot spl/u-boot-spl
>    text    data     bss     dec     hex filename
>  407173   45308   98352  550833   867b1 u-boot
>   58298    3360   65860  127518   1f21e spl/u-boot-spl
> $ nm spl/u-boot-spl | grep env_fat
> 0090c6e0 t env_fat_load
> 0090c63c t env_fat_save
>
> but without,
>
> $ size u-boot spl/u-boot-spl
>    text    data     bss     dec     hex filename
>  407173   45308   98352  550833   867b1 u-boot
>   52659    3360     280   56299    dbeb spl/u-boot-spl

OK, here you confirm it again - with your patches the BSS size
explodes from 280 to 65860 bytes, that's a faxtor of more than 200.

Don't you think that looks fishy?


> The .bss increase is simply due to the extra code that no longer gets
> discarded by the linker, more precisely the .map file says there's a

This makes absolutley no sense.  BSS does not include any code, it's
uninitialized data. 

And iff we have the situation, that with your patches any "extra
code ... no longer gets discarded by the linker", then something is
broken with your patches, and this must be fixed.

>  .bss.tmpbuf_cluster
>                 0x0000000000000000    0x10000 fs/built-in.o
>
> that gets discarded without my patches (but with the config options
> chosen so one would _expect_ to have save support in SPL). So yes, of
> course there's a price to pay for enabling environment save support in
> the SPL, with some backends being more expensive (in terms of footprint)
> than others.

A price of more than 64 kB additional memory footprint in the SPL is
a strict no-go.

This must be fixed, and I'm surprised that you did not even spend a
thought about this after I explicitly mentioned it.

This all makes no sense to me, as tmpbuf_cluster[] comes from
fs/fat/fat_write.c, which should not be used for the SPL when you
don't enable both FAT and SAVEENV support together.


> > and it's also not clear if this is
> > a typical result, of if it's the only configuration you ever
> > tested.

You continue to ignore this question.


> > Your patch description sounds as if it was just a #ifdef cleanup
> > without actual impact on the generated code, but the SPL size
> > differences above make it clear that it is not - or that your
> > testing has issues.
>
> There is _no_ change in code size, u-boot or spl, when
> CONFIG_SPL_SAVEENV=n. My patches _only_ affect the case where
> CONFIG_SPL_SAVEENV=y, and only in a way that the developer most likely
> intended, namely actually allowing one to save the environment.

This makes no sense either.

If you compare the SAME configuration with and without your patches
above, then we have this unacceptable BSS explosing, which is
unacceptable.

If you compare two different configurations above, one with
CONFIG_SPL_SAVEENV=n and one with CONFIG_SPL_SAVEENV=y, then the
whole comparion makes no sense.

As long as we stick with the same single board (wandboard_defconfig),
plus the 4 lines changed,  there would be 4 different cases to test:

- CONFIG_SPL_SAVEENV=n without your patches
- CONFIG_SPL_SAVEENV=n with    your patches
- CONFIG_SPL_SAVEENV=y without your patches
- CONFIG_SPL_SAVEENV=y with    your patches

Anything else is comparing apples and bicycles.


> > You also failed to comment on impact on other environment storage
> > configurations (NOR flash, NAND flash, UBI volume, ...).
>
> I don't touch those files at all, so they are not affected. Some still
> fail to honour CONFIG_SPL_SAVEENV (i.e., even if one sets
> CONFIG_SPL_SAVEENV, saving the environment in the SPL is not actually
> supported).

You _say_ they are not affected, and I accept that this is your
intention.  But my question was if you actually _tested_ that your
patches behave as intented?  I think there have been cases before
where code changes had ... let's say unexpected side effects...

You should build a few (if not all!) such boards with and without
your patches applied and _verify_ the the code does not change.
Just guessing is not good enough.


> To repeat myself, for CONFIG_SPL_SAVEENV=n, these patches don't change
> anything, except get rid of a lot of pointless ifdefs. For
> CONFIG_SPL_SAVEENV=y, the patches serve to honour the developer's
> intention of actually being able to save the environment from SPL, at
> least for fat, ext4 and sf.

You continue to fail to prove that.

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
"We don't care.  We don't have to.  We're the Phone Company."

[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[Rasmus Villemoes]

On 26/03/2020 15.31, Wolfgang Denk wrote:
> Dear Rasmus,
> 
>>>
>>> I can see where the increase in text size is coming from - your
>>> removal of #ifdef's now unconditionally includes some code that was
>>> omitted before, for example functions env_fat_save(),
>>> env_ext4_save(), env_sf_save(), plus a few variables.
>>
>> As intended for CONFIG_SPL_SAVEENV=y, no?
> 
> As you don't bother to mention which exact configuration you are
> testing against, it is impossible to tell what exactly you are
> doing.
> 
> But you give here two versions _with_ and _without_ your patches,
> so I _assume_ all the other parameters are kept the same, and with
> your patches the BSS size explodes.
> 
> Please elucidate,
> 
>> With my patches and CONFIG_SPL_SAVEENV=n, those env_fat_save,
>> env_ext4_save etc. are compiled, but then discarded (being static, they
>> are discarded already at compile-time, but otherwise they would be at
>> link-time), instead of being ifdeffed out unconditionally just because
>> of CONFIG_SPL_BUILD. I know that you share the opinion that one should
>> use IS_ENABLED() in preference to preprocessor conditionals, so I really
>> don't understand what you can possibly have against this approach.
> 
> Please explain why the memory footprint explodes.

When CONFIG_SPL_FAT_WRITE is enabled, but nothing actually uses that
functionality, the linker discards those functions, _and_ any data/bss
elements which those discarded functions were the only user of. One of
those elements happen to be a static 64K byte array.

Now, with the current master branch, the logic for whether env/fat.c
defines its own CMD_SAVEENV macro means that whether or not one enables
CONFIG_SPL_SAVEENV, env_fat_save does not get compiled in SPL, so
there's no user of file_fat_write(), and the linker then discards that
along with the 64K static buffer. But of course that also means that
saving the environment is not actually possible.

>>> It's difficult to comment here as it is not clear to me which exact
>>> configuration you reported about,
>>
>> Huh? I wrote exactly what I used to obtain those numbers for the FAT
>> case. Let me quote a bit more
> 
> And what exactly is "the FAT case"?  

The case where the storage backend is FAT, as opposed to ext4 and sf
that are also in the series.

You did not explain what
> exactly you are comparing...
> 
>> for a wandboard_defconfig modified by
>>
>> -CONFIG_SPL_FS_EXT4=y
>> +CONFIG_SPL_FS_FAT=y
>> +CONFIG_SPL_ENV_SUPPORT=y
>> +CONFIG_ENV_IS_IN_FAT=y
> 
> Is this your test case?  I don't think you mentioned this before.

I did, as is totally apparent if you cared to read back a bit in the
thread to verify that I was indeed quoting myself.

> You your test cases are mainline U-Boot, using wandboard_defconfig
> plus these 4 changes, and then either with and without your patches
> applied?

That's one test case, in order to demonstrate how CONFIG_SPL_SAVEENV is
currently vapourware when used with FAT as backend (and for that matter,
a lot of other backends as well).

>> That was the answer to "does it affect the generated code when one
>> doesn't enable CONFIG_SPL_SAVEENV". It doesn't, not at all, the code is
>> exactly the same. The next part then demonstrated how CONFIG_SPL_SAVEENV
>> is currently being ignored because of the ifdeffery in fat.c:
> 
> No.  My question was if the code size differs for the same
> configurations with and without your patches applied.

For a configuration without CONFIG_SPL_SAVEENV, neither SPL or U-Boot
binary size (or the .bss section) differs at all with my patches applied.

For a configuration with CONFIG_SPL_SAVEENV (and SPL_FAT_WRITE, which is
necessary to make it link), the SPL does change with my patches applied,
in that env_fat_safe now gets compiled in, which means file_fat_write
and everything that references no longer gets discarded.

>> Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my
>> patches we get
>>
>> $ size u-boot spl/u-boot-spl
>>    text    data     bss     dec     hex filename
>>  407173   45308   98352  550833   867b1 u-boot
>>   58298    3360   65860  127518   1f21e spl/u-boot-spl
>> $ nm spl/u-boot-spl | grep env_fat
>> 0090c6e0 t env_fat_load
>> 0090c63c t env_fat_save
>>
>> but without,
>>
>> $ size u-boot spl/u-boot-spl
>>    text    data     bss     dec     hex filename
>>  407173   45308   98352  550833   867b1 u-boot
>>   52659    3360     280   56299    dbeb spl/u-boot-spl
> 
> OK, here you confirm it again - with your patches the BSS size
> explodes from 280 to 65860 bytes, that's a faxtor of more than 200.
>
> Don't you think that looks fishy?

No, not at all.

>> The .bss increase is simply due to the extra code that no longer gets
>> discarded by the linker, more precisely the .map file says there's a
> 
> This makes absolutley no sense.  BSS does not include any code, it's
> uninitialized data. 

Read "code and/or data".

> And iff we have the situation, that with your patches any "extra
> code ... no longer gets discarded by the linker", then something is
> broken with your patches, and this must be fixed.

That all works just as usual, it's just that when env/fat.c is patched
to not ignore CONFIG_SPL_SAVEENV, file_fat_write is no longer unreferenced.

>>  .bss.tmpbuf_cluster
>>                 0x0000000000000000    0x10000 fs/built-in.o
>>
>> that gets discarded without my patches (but with the config options
>> chosen so one would _expect_ to have save support in SPL). So yes, of
>> course there's a price to pay for enabling environment save support in
>> the SPL, with some backends being more expensive (in terms of footprint)
>> than others.
> 
> A price of more than 64 kB additional memory footprint in the SPL is
> a strict no-go.

Well, then CONFIG_SPL_FAT_WRITE should be removed, or the code rewritten
to not rely on a statically allocated buffer. Nothing to do with my patches.

> This must be fixed, and I'm surprised that you did not even spend a
> thought about this after I explicitly mentioned it.
> 
> This all makes no sense to me, as tmpbuf_cluster[] comes from
> fs/fat/fat_write.c, which should not be used for the SPL when you
> don't enable both FAT and SAVEENV support together.

Exactly, tmpbuf_cluster[] only gets compiled with CONFIG_SPL_FAT_WRITE.
Without a user, it gets discarded. Enabling CONFIG_SPL_SAVEENV should
create such a user, but it doesn't in current master.

>>> Your patch description sounds as if it was just a #ifdef cleanup
>>> without actual impact on the generated code, but the SPL size
>>> differences above make it clear that it is not - or that your
>>> testing has issues.
>>
>> There is _no_ change in code size, u-boot or spl, when
>> CONFIG_SPL_SAVEENV=n. My patches _only_ affect the case where
>> CONFIG_SPL_SAVEENV=y, and only in a way that the developer most likely
>> intended, namely actually allowing one to save the environment.
> 
> This makes no sense either.
> 
> If you compare the SAME configuration with and without your patches
> above, then we have this unacceptable BSS explosing, which is
> unacceptable.

I do (compare the same configuration with and without my patches).
> 
> As long as we stick with the same single board (wandboard_defconfig),
> plus the 4 lines changed,  there would be 4 different cases to test:
> 
> - CONFIG_SPL_SAVEENV=n without your patches
> - CONFIG_SPL_SAVEENV=n with    your patches
> - CONFIG_SPL_SAVEENV=y without your patches
> - CONFIG_SPL_SAVEENV=y with    your patches
> 
> Anything else is comparing apples and bicycles.

These are exactly the cases I have shown. The first two are covered by
(and I quote this again)

====
With or without these patches, I get

$ size u-boot spl/u-boot-spl
   text    data     bss     dec     hex filename
 407173   45308   98352  550833   867b1 u-boot
  52403    3360     276   56039    dae7 spl/u-boot-spl
$ nm spl/u-boot-spl | grep env_fat
0090c5e8 t env_fat_load
$ nm u-boot | grep env_fat
17826cb4 t env_fat_load
17826c10 t env_fat_save

for a wandboard_defconfig modified by

-CONFIG_SPL_FS_EXT4=y
+CONFIG_SPL_FS_FAT=y
+CONFIG_SPL_ENV_SUPPORT=y
+CONFIG_ENV_IS_IN_FAT=y

So in the "read-only environment access in SPL" case, everything is the
same before and after.
====

Note the "with or without", the sizes shown are for both cases,
everything really is the same.

Then when one enables CONFIG_SPL_SAVEENV and CONFIG_SPL_FAT_WRITE,
without my patches, the result is

$ size u-boot spl/u-boot-spl
   text    data     bss     dec     hex filename
 407173   45308   98352  550833   867b1 u-boot
  52659    3360     280   56299    dbeb spl/u-boot-spl
$ nm spl/u-boot-spl | grep env_fat
0090c5e8 t env_fat_load

So, because env/fat.c never builds env_fat_save for an SPL build,
enabling those options don't add very much to the SPL -
fs/fat/fat_write.o does get built, but most of it gets discarded.

And yes, of course when file_fat_write does have a user, as it has with
my patches applied and SPL_SAVEENV=y,SPL_FAT_WRITE=y, it no longer gets
discarded.

>>> You also failed to comment on impact on other environment storage
>>> configurations (NOR flash, NAND flash, UBI volume, ...).
>>
>> I don't touch those files at all, so they are not affected. Some still
>> fail to honour CONFIG_SPL_SAVEENV (i.e., even if one sets
>> CONFIG_SPL_SAVEENV, saving the environment in the SPL is not actually
>> supported).
> 
> You _say_ they are not affected, and I accept that this is your
> intention.  But my question was if you actually _tested_ that your
> patches behave as intented?  I think there have been cases before
> where code changes had ... let's say unexpected side effects...

Sure, but let's be a little bit reasonable here. I add one CONFIG_*
symbol (CONFIG_SAVEENV, a convenience alias for CONFIG_CMD_SAVEENV), and
one macro, with a hitherto completely unused identifier, in
include/env_internal.h.

> You should build a few (if not all!) such boards with and without
> your patches applied and _verify_ the the code does not change.
> Just guessing is not good enough.

OK, I will do that, though I don't know how to prove that I've done it.

>> To repeat myself, for CONFIG_SPL_SAVEENV=n, these patches don't change
>> anything, except get rid of a lot of pointless ifdefs. For
>> CONFIG_SPL_SAVEENV=y, the patches serve to honour the developer's
>> intention of actually being able to save the environment from SPL, at
>> least for fat, ext4 and sf.
> 
> You continue to fail to prove that.

And how do you expect me carry out such a proof?

I'm getting really tired of this, so I will resend a much simpler
two-part series that only fixes the sf.c case, and without the
ENV_SAVE_PTR macro which doesn't actually provide any value over just
open-coding "CONFIG_IS_ENABLED(SAVEENV) ? env_sf_save : NULL".

Rasmus

[PATCH v2 0/2] allow CONFIG_SPL_SAVEENV to work with ENV_IS_IN_SPI_FLASH

[Rasmus Villemoes]

Currently, CONFIG_SPL_SAVEENV is not very well supported by the
various storage backends, as many of them contain variants of some
logic that end up not compiling the .save method when
CONFIG_SPL_BUILD.

As I need environment save support in SPL for a target that uses
ENV_IS_IN_SPI_FLASH, these patches fix env/sf.c to honour
CONFIG_SPL_SAVEENV (and fixes a build warning in the rare case where
one sets CONFIG_CMD_SAVEENV=n). In order to fix this properly and not
add to the existing maze of preprocessor directives, the first patch
adds a convenience config symbol so the existing CONFIG_IS_ENABLED()
helper can be used - which then, as a bonus, ends up reducing said maze.

Should others need to enable CONFIG_SPL_SAVEENV with one of the
remaining backends that currently ignore it, they can most likely use
a similar approach as done for sf.c here.

Difference from v1 is that patches for ext4.c and fat.c have been
dropped, as well as a patch that introduced a ENV_SAVE_PTR() macro -
and the sf.c patch consequently doesn't use that macro but just uses
'CONFIG_IS_ENABLED(SAVEENV) ? (x) : NULL' directly. Also, the series
is no longer branded as a "cleanup" - it intentionally changes the
generated code for certain configurations, but it does so in way that
happens to reduce ifdeffery.

=== testing ===

This has been run-time tested on a mpc8309-derived board to verify
that saving the environment does indeed work in SPL with these patches
applied.

As far as I can tell, the only in-tree defconfig that sets both
SPL_ENV_IS_IN_SPI_FLASH and SPL_SAVEENV is display5_defconfig, which
also happens to be the only one setting SPL_SAVEENV at all. Let's see
how these patches affect that:

# avoid differences due to different git commit or wallclock time
$ export SOURCE_DATE_EPOCH=1585252702
$ echo 'test' > .scmversion
$ export ARCH=arm
$ export CROSS_COMPILE=arm-linux-gnueabi-
$ git checkout master ; make display5_defconfig ; make -j8
$ cp u-boot u-boot.1 ; cp spl/u-boot-spl u-boot-spl.1
$ git checkout sf-spl-saveenv ; make display5_defconfig ; make -j8
$ cp u-boot u-boot.2 ; cp spl/u-boot-spl u-boot-spl.2
$ size u-boot{,-spl}.{1,2}
   text    data     bss     dec     hex filename
 377468   24620   66164  468252   7251c u-boot.1
 377468   24620   66164  468252   7251c u-boot.2
  58411    2020     116   60547    ec83 u-boot-spl.1
  59976    2028     116   62120    f2a8 u-boot-spl.2

So U-Boot proper is not affected (the files even yield identical
objdump -d output), while the SPL grows by the ~1.5K necessary to
implement saving the environment. Borrowing the bloat-o-meter script
from linux, we can also see the functions/data items that are now
included:

../linux/scripts/bloat-o-meter u-boot-spl.1 u-boot-spl.2
add/remove: 11/0 grow/shrink: 0/1 up/down: 1340/-24 (1316)
Function                                     old     new   delta
hexport_r                                      -     408    +408
env_sf_save                                    -     332    +332
qsort                                          -     144    +144
match_entry                                    -     124    +124
env_export                                     -     100    +100
match_string                                   -      92     +92
strstr                                         -      64     +64
setup_flash_device                             -      56     +56
cmpkey                                         -      12     +12
env_offset                                     -       4      +4
env_new_offset                                 -       4      +4
env_sf_load                                  184     160     -24
Total: Before=52986, After=54302, chg +2.48%

[The difference between 1316 and 62120-60547=1573 is most likely due
to string literals that are referenced from the above functions].

Now, to check that other storage backends are not affected, and also
that nothing (neither U-Boot or SPL) changes for ENV_IS_IN_SPI_FLASH
when CONFIG_SPL_SAVEENV=n, I have repeated the above with
am335x_shc_netboot_defconfig (MMC), pengwyn_defconfig (NAND),
mccmon6_sd_defconfig (FLASH),
ls1046ardb_qspi_spl_defconfig (SPI_FLASH):

$ for c in am335x_shc_netboot_defconfig pengwyn_defconfig mccmon6_sd_defconfig ls1046ardb_qspi_spl_defconfig ; do
  [ $c = "ls1046ardb_qspi_spl_defconfig" ] && CROSS_COMPILE=aarch64-linux-gnu- || CROSS_COMPILE=arm-linux-gnueabi-
  for b in master sf-spl-saveenv ; do
    git checkout $b ;
    make $c && make -j8 ;
    cp u-boot u-boot.$b && cp spl/u-boot-spl u-boot-spl.$b ;
  done ;
  for x in u-boot u-boot-spl ; do
    diff -u <(objdump -d $x.master | sed -e '/file format/d') <(objdump -d $x.sf-spl-saveenv | sed -e '/file format/d') > $c.$x.diff ;
  done ;
done
$ ls -l *.diff
-rw-rw-r-- 1 ravi ravi 0 Mar 26 23:20 am335x_shc_netboot_defconfig.u-boot.diff
-rw-rw-r-- 1 ravi ravi 0 Mar 26 23:20 am335x_shc_netboot_defconfig.u-boot-spl.diff
-rw-rw-r-- 1 ravi ravi 0 Mar 26 23:20 ls1046ardb_qspi_spl_defconfig.u-boot.diff
-rw-rw-r-- 1 ravi ravi 0 Mar 26 23:20 ls1046ardb_qspi_spl_defconfig.u-boot-spl.diff
-rw-rw-r-- 1 ravi ravi 0 Mar 26 23:20 mccmon6_sd_defconfig.u-boot.diff
-rw-rw-r-- 1 ravi ravi 0 Mar 26 23:20 mccmon6_sd_defconfig.u-boot-spl.diff
-rw-rw-r-- 1 ravi ravi 0 Mar 26 23:20 pengwyn_defconfig.u-boot.diff
-rw-rw-r-- 1 ravi ravi 0 Mar 26 23:20 pengwyn_defconfig.u-boot-spl.diff

[the sed -e '/file format/d' is of course just to avoid a false
positive from the

u-boot.master:     file format elf32-littlearm

header line]

Rasmus Villemoes (2):
  env: add SAVEENV as an alias of the CMD_SAVEENV symbol
  env/sf.c: honour CONFIG_SPL_SAVEENV

 env/Kconfig |  3 +++
 env/sf.c    | 12 +-----------
 2 files changed, 4 insertions(+), 11 deletions(-)

-- 
2.23.0

[PATCH v2 1/2] env: add SAVEENV as an alias of the CMD_SAVEENV symbol

[Rasmus Villemoes]

Currently, quite a few storage drivers currently do not honour
SPL_SAVEENV. That is, whether or not one enables CONFIG_SPL_SAVEENV,
the backend drivers do not provide the .save method. Witness

env/fat.c:#ifdef CONFIG_SPL_BUILD
...
env/fat.c-#else
env/fat.c-# define LOADENV
env/fat.c:# if defined(CONFIG_CMD_SAVEENV)
env/fat.c:#  define CMD_SAVEENV
env/fat.c-# endif
env/fat.c-#endif
env/fat.c-
env/fat.c:#ifdef CMD_SAVEENV
env/fat.c-static int env_fat_save(void)

env/flash.c:#ifndef CONFIG_SPL_BUILD
env/flash.c:# if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_FLASH)
env/flash.c:#  define CMD_SAVEENV
...
env/flash.c:#ifdef CMD_SAVEENV
env/flash.c-static int env_flash_save(void)

env/mmc.c:#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
env/mmc.c-static inline int write_env(struct mmc *mmc, unsigned long size,

env/nand.c:#if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && \
env/nand.c:             !defined(CONFIG_SPL_BUILD)
env/nand.c:#define CMD_SAVEENV
...
env/nand.c:#ifdef CMD_SAVEENV
env/nand.c-/*
env/nand.c- * The legacy NAND code saved the environment in the first NAND device i.e.,
env/nand.c- * nand_dev_desc + 0. This is also the behaviour using the new NAND code.
env/nand.c- */
env/nand.c-static int writeenv(size_t offset, u_char *buf)

env/sf.c:#ifndef CONFIG_SPL_BUILD
env/sf.c:#define CMD_SAVEENV
env/sf.c-#define INITENV
env/sf.c-#endif
...
env/sf.c:#ifdef CMD_SAVEENV
env/sf.c-static int env_sf_save(void)

In all these cases, the mere presence of CONFIG_SPL_BUILD means the
save method does not get built.

Now, it is currently a bit awkward to write a proper test for whether
saving the environment is enabled in the current context; something
like

#if (!defined(CONFIG_SPL_BUILD) && defined(CONFIG_CMD_SAVEENV)) || \
    (defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_SAVEENV))

But we already have a rather elegant mechanism that implicitly does
the CONFIG_SPL_BUILD tests, namely CONFIG_IS_ENABLED(). Using that
requires that the controlling config symbols follow a strict pattern:
FOO for U-Boot proper, SPL_FOO for SPL.

This patch introduces CONFIG_SAVEENV as an alias for
CONFIG_CMD_SAVEENV. That way, the above can simply be written

#if CONFIG_IS_ENABLED(SAVEENV)

and moreover, CONFIG_IS_ENABLED(SAVEENV) can also be used in C code,
avoiding ifdeffery and providing more compile testing.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
v2: Expand commit message, explain why one needs a new config symbol
in order to use CONFIG_IS_ENABLED, and demonstrate how many of the
storage drivers don't compile their .save method when
CONFIG_SPL_BUILD. The patch itself is the same.

 env/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/env/Kconfig b/env/Kconfig
index 0d6f559b39..969308fe6c 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -3,6 +3,9 @@ menu "Environment"
 config ENV_SUPPORT
 	def_bool y
 
+config SAVEENV
+	def_bool y if CMD_SAVEENV
+
 config ENV_IS_NOWHERE
 	bool "Environment is not stored"
 	default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \
-- 
2.23.0

[PATCH v2 2/2] env/sf.c: honour CONFIG_SPL_SAVEENV

[Rasmus Villemoes]

Deciding whether to compile the env_sf_save() function based solely on
CONFIG_SPL_BUILD is wrong: For U-Boot proper, it leads to a build
warning in case CONFIG_CMD_SAVEENV=n (because the initialization of
the .save member is guarded by CONFIG_CMD_SAVEENV, while the
env_sf_save() function is built if !CONFIG_SPL_BUILD - and even
without the CONFIG_CMD_SAVEENV guard, the env_save_ptr() macro would
just expand to NULL, with no reference to env_sf_save visible to the
compiler). And for SPL, when one selects CONFIG_SPL_SAVEENV, one
obviously expects to actually be able to save the environment.

The compiler warning can be fixed by using a "<something> ?
env_sf_save : NULL" construction instead of a macro that just eats its
argument and expands to NULL. That way, if <something> is false,
env_sf_save gets eliminated as dead code, but the compiler still sees
the reference to it.

For <something>, we can use CONFIG_IS_ENABLED(SAVEENV), which is true
precisely:

- For U-Boot proper, when CONFIG_CMD_SAVEENV is set (because
  CONFIG_SAVEENV is a hidden config symbol that gets set if and only
  if CONFIG_CMD_SAVEENV is set).
- For SPL, when CONFIG_SPL_SAVEENV is set.

As a bonus, this also removes quite a few preprocessor conditionals.

This has been run-time tested on a mpc8309-derived board to verify
that saving the environment does indeed work in SPL with these patches
applied.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
v2: Use 'CONFIG_IS_ENABLED(SAVEENV) ? env_sf_save : NULL' directly
instead of the dropped ENV_SAVE_PTR macro and expand commit message a
bit.

 env/sf.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/env/sf.c b/env/sf.c
index 5ef4055219..f41a846294 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -21,16 +21,12 @@
 #include <u-boot/crc.h>
 
 #ifndef CONFIG_SPL_BUILD
-#define CMD_SAVEENV
 #define INITENV
 #endif
 
 #ifdef CONFIG_ENV_OFFSET_REDUND
-#ifdef CMD_SAVEENV
 static ulong env_offset		= CONFIG_ENV_OFFSET;
 static ulong env_new_offset	= CONFIG_ENV_OFFSET_REDUND;
-#endif
-
 #endif /* CONFIG_ENV_OFFSET_REDUND */
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -69,7 +65,6 @@ static int setup_flash_device(void)
 }
 
 #if defined(CONFIG_ENV_OFFSET_REDUND)
-#ifdef CMD_SAVEENV
 static int env_sf_save(void)
 {
 	env_t	env_new;
@@ -148,7 +143,6 @@ static int env_sf_save(void)
 
 	return ret;
 }
-#endif /* CMD_SAVEENV */
 
 static int env_sf_load(void)
 {
@@ -187,7 +181,6 @@ out:
 	return ret;
 }
 #else
-#ifdef CMD_SAVEENV
 static int env_sf_save(void)
 {
 	u32	saved_size, saved_offset, sector;
@@ -247,7 +240,6 @@ static int env_sf_save(void)
 
 	return ret;
 }
-#endif /* CMD_SAVEENV */
 
 static int env_sf_load(void)
 {
@@ -313,9 +305,7 @@ U_BOOT_ENV_LOCATION(sf) = {
 	.location	= ENVL_SPI_FLASH,
 	ENV_NAME("SPI Flash")
 	.load		= env_sf_load,
-#ifdef CMD_SAVEENV
-	.save		= env_save_ptr(env_sf_save),
-#endif
+	.save		= CONFIG_IS_ENABLED(SAVEENV) ? env_sf_save : NULL,
 #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
 	.init		= env_sf_init,
 #endif
-- 
2.23.0

[PATCH v2 0/2] allow CONFIG_SPL_SAVEENV to work with ENV_IS_IN_SPI_FLASH

[Wolfgang Denk]

Dear Rasmus,

In message <20200326230200.12617-1-rasmus.villemoes@prevas.dk> you wrote:
> Currently, CONFIG_SPL_SAVEENV is not very well supported by the
> various storage backends, as many of them contain variants of some
> logic that end up not compiling the .save method when
> CONFIG_SPL_BUILD.
...

> As far as I can tell, the only in-tree defconfig that sets both
> SPL_ENV_IS_IN_SPI_FLASH and SPL_SAVEENV is display5_defconfig, which
> also happens to be the only one setting SPL_SAVEENV at all. Let's see
> how these patches affect that:
>
> # avoid differences due to different git commit or wallclock time
> $ export SOURCE_DATE_EPOCH=1585252702
> $ echo 'test' > .scmversion
> $ export ARCH=arm
> $ export CROSS_COMPILE=arm-linux-gnueabi-
> $ git checkout master ; make display5_defconfig ; make -j8
> $ cp u-boot u-boot.1 ; cp spl/u-boot-spl u-boot-spl.1
> $ git checkout sf-spl-saveenv ; make display5_defconfig ; make -j8
> $ cp u-boot u-boot.2 ; cp spl/u-boot-spl u-boot-spl.2
> $ size u-boot{,-spl}.{1,2}
>    text    data     bss     dec     hex filename
>  377468   24620   66164  468252   7251c u-boot.1
>  377468   24620   66164  468252   7251c u-boot.2
>   58411    2020     116   60547    ec83 u-boot-spl.1
>   59976    2028     116   62120    f2a8 u-boot-spl.2

Thanks for the additional testing.  As you can see here, it is
definitely worth the effort.

> So U-Boot proper is not affected (the files even yield identical
> objdump -d output), while the SPL grows by the ~1.5K necessary to
> implement saving the environment. Borrowing the bloat-o-meter script
> from linux, we can also see the functions/data items that are now
> included:

Does this not trigger questions to you?  Why is the code growing?
It had SPL_ENV_IS_IN_SPI_FLASH and SPL_SAVEENV before!

> ../linux/scripts/bloat-o-meter u-boot-spl.1 u-boot-spl.2
> add/remove: 11/0 grow/shrink: 0/1 up/down: 1340/-24 (1316)
> Function                                     old     new   delta
> hexport_r                                      -     408    +408
> env_sf_save                                    -     332    +332
> qsort                                          -     144    +144
> match_entry                                    -     124    +124
> env_export                                     -     100    +100
> match_string                                   -      92     +92
> strstr                                         -      64     +64
> setup_flash_device                             -      56     +56
> cmpkey                                         -      12     +12
> env_offset                                     -       4      +4
> env_new_offset                                 -       4      +4
> env_sf_load                                  184     160     -24
> Total: Before=52986, After=54302, chg +2.48%

To me this triggers at least two questions:

- Why is this code included now, when it was not before?
- Iff the code was not included before, why did this not cause
  problems when trying to save the environment in SPL, which was
  apparently needed by this board?

Adding Lukasz on Cc:, who maintains this board.

After some initial talk to Lukasz it seems your testing indeed
discovered a bug - without your patch SPL_SAVEENV apparently had no
effect, and oard testing did not vdetect this failure, because
requirements changed during the project the the feature that was
once requested got later dropped, but the option was not removed.

Testing is _always_ worth the effort.

> Now, to check that other storage backends are not affected, and also
> that nothing (neither U-Boot or SPL) changes for ENV_IS_IN_SPI_FLASH
> when CONFIG_SPL_SAVEENV=n, I have repeated the above with
> am335x_shc_netboot_defconfig (MMC), pengwyn_defconfig (NAND),
> mccmon6_sd_defconfig (FLASH),
> ls1046ardb_qspi_spl_defconfig (SPI_FLASH):
>
> $ for c in am335x_shc_netboot_defconfig pengwyn_defconfig mccmon6_sd_defconfig ls1046ardb_qspi_spl_defconfig ; do
...

Actually this would have been easier using tbot, and it would have
been possible to cover many more / all boards, but I don't intend to
ask more from you.  Thanks, both for the additional testing and your
patience.


Reviewed-by: Wolfgang Denk <wd@denx.de>

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
I will also, for an appropriate fee, certify that  your  keyboard  is
object-oriented,  and  that  the bits on your hard disk are template-
compatible.            - Jeffrey S. Haemer in <411akr$3ga@cygnus.com>

[PATCH v2 0/2] allow CONFIG_SPL_SAVEENV to work with ENV_IS_IN_SPI_FLASH

[Rasmus Villemoes]

On 27/03/2020 17.31, Wolfgang Denk wrote:
> Dear Rasmus,
> 
>>
>> # avoid differences due to different git commit or wallclock time
>> $ export SOURCE_DATE_EPOCH=1585252702
>> $ echo 'test' > .scmversion
>> $ export ARCH=arm
>> $ export CROSS_COMPILE=arm-linux-gnueabi-
>> $ git checkout master ; make display5_defconfig ; make -j8
>> $ cp u-boot u-boot.1 ; cp spl/u-boot-spl u-boot-spl.1
>> $ git checkout sf-spl-saveenv ; make display5_defconfig ; make -j8
>> $ cp u-boot u-boot.2 ; cp spl/u-boot-spl u-boot-spl.2
>> $ size u-boot{,-spl}.{1,2}
>>    text    data     bss     dec     hex filename
>>  377468   24620   66164  468252   7251c u-boot.1
>>  377468   24620   66164  468252   7251c u-boot.2
>>   58411    2020     116   60547    ec83 u-boot-spl.1
>>   59976    2028     116   62120    f2a8 u-boot-spl.2
> 
> Thanks for the additional testing.  As you can see here, it is
> definitely worth the effort.
> 
>> So U-Boot proper is not affected (the files even yield identical
>> objdump -d output), while the SPL grows by the ~1.5K necessary to
>> implement saving the environment. Borrowing the bloat-o-meter script
>> from linux, we can also see the functions/data items that are now
>> included:
> 
> Does this not trigger questions to you?  Why is the code growing?
> It had SPL_ENV_IS_IN_SPI_FLASH and SPL_SAVEENV before!

No, it does not trigger questions, or at least, none that I can't answer
myself. It's just -ffunction-sections, -fdata-sections,
-Wl,--gc-sections at work, working as intended. When nothing references
env_export (and nothing was referencing it when env_sf_save did not get
compiled in), the linker discards it, along with everything that
env_export was the sole user of.

>> ../linux/scripts/bloat-o-meter u-boot-spl.1 u-boot-spl.2
>> add/remove: 11/0 grow/shrink: 0/1 up/down: 1340/-24 (1316)
>> Function                                     old     new   delta
>> hexport_r                                      -     408    +408
>> env_sf_save                                    -     332    +332
>> qsort                                          -     144    +144
>> match_entry                                    -     124    +124
>> env_export                                     -     100    +100
>> match_string                                   -      92     +92
>> strstr                                         -      64     +64
>> setup_flash_device                             -      56     +56
>> cmpkey                                         -      12     +12
>> env_offset                                     -       4      +4
>> env_new_offset                                 -       4      +4
>> env_sf_load                                  184     160     -24
>> Total: Before=52986, After=54302, chg +2.48%
> 
> To me this triggers at least two questions:
> 
> - Why is this code included now, when it was not before?

See above. It gets compiled, but discarded (in current master, that is).

> - Iff the code was not included before, why did this not cause
>   problems when trying to save the environment in SPL, which was
>   apparently needed by this board?

Yes, that was one thing I did think about, but it wouldn't be the first
time somebody enabled a config option that wasn't actually needed.

> Adding Lukasz on Cc:, who maintains this board.
> 
> After some initial talk to Lukasz it seems your testing indeed
> discovered a bug - without your patch SPL_SAVEENV apparently had no
> effect,

That is exactly what I have been saying (or trying to say) all along.

SPL_SAVEENV is, for many/most backends, completely ignored. The
compiler/linker flags then ensure that the binary doesn't carry a lot of
excess baggage that does get _compiled_ with SPL_SAVEENV, e.g.
env_export(), so while SPL_SAVEENV did not have the intended effect, it
also did not (due to the garbage collection) have any ill effect in
terms of needless bloat.

 and oard testing did not vdetect this failure, because
> requirements changed during the project the the feature that was
> once requested got later dropped, but the option was not removed.

That's what I figured, because once you _do_ try to save the environment
from the SPL, you quickly realize that doesn't work at all. Which is of
course how I discovered the bug in sf.c (and which is repeated in
various forms in the other backends).

> Testing is _always_ worth the effort.
> 
>> Now, to check that other storage backends are not affected, and also
>> that nothing (neither U-Boot or SPL) changes for ENV_IS_IN_SPI_FLASH
>> when CONFIG_SPL_SAVEENV=n, I have repeated the above with
>> am335x_shc_netboot_defconfig (MMC), pengwyn_defconfig (NAND),
>> mccmon6_sd_defconfig (FLASH),
>> ls1046ardb_qspi_spl_defconfig (SPI_FLASH):
>>
>> $ for c in am335x_shc_netboot_defconfig pengwyn_defconfig mccmon6_sd_defconfig ls1046ardb_qspi_spl_defconfig ; do
> ...
> 
> Actually this would have been easier using tbot, 

Can you provide a pointer? Sounds like something I could use going forward.

> Thanks, both for the additional testing and your patience.

Likewise.

> Reviewed-by: Wolfgang Denk <wd@denx.de>

Thanks,
Rasmus

[PATCH v2 0/2] allow CONFIG_SPL_SAVEENV to work with ENV_IS_IN_SPI_FLASH

[Wolfgang Denk]

Dear Rasmus,

In message <893503e2-10a1-2d3e-e7ad-9d24163ade0f@prevas.dk> you wrote:
>
...
> > Actually this would have been easier using tbot, 
>
> Can you provide a pointer? Sounds like something I could use going forward.

See [1]

And/or see the thread "Sharing a hardware lab" [2]


[1] https://tbot.tools/
[2] https://lists.denx.de/pipermail/u-boot/2020-February/399278.html

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
There is a theory which states that if ever anyone discovers  exactly
what  the  Universe is for and why it is here, it will instantly dis-
appear and be replaced by something even more  bizarre  and  inexpli-
cable.  There  is  another  theory which states that this has already
happened.    -- Douglas Adams, "The Hitchhiker's Guide to the Galaxy"

[PATCH 1/5] env: add SAVEENV as an alias of the CMD_SAVEENV symbol

[Tom Rini]

On Wed, Feb 19, 2020 at 09:47:39AM +0000, Rasmus Villemoes wrote:

> Currently, testing whether to compile in support for saving the
> environment is a bit awkward when one needs to take SPL_SAVEENV into
> account, and quite a few storage drivers currently do not honour
> SPL_SAVEENV.
> 
> To make it a bit easier to decide whether environment saving should be
> enabled, introduce SAVEENV as an alias for the CMD_SAVEENV
> symbol. Then one can simply use
> 
>   CONFIG_IS_ENABLED(SAVEENV)
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, 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/20200424/76549e3a/attachment.sig>

[PATCH 2/5] env_internal.h: add alternative ENV_SAVE_PTR macro

[Tom Rini]

On Wed, Feb 19, 2020 at 09:47:40AM +0000, Rasmus Villemoes wrote:

> The current definition of the env_save_ptr does not take SPL_SAVEENV
> into account. Moreover, the way it is implemented means that drivers
> need to guard the definitions of their _save methods with ifdefs to
> avoid "defined but unused" warnings in case CMD_SAVEENV=n.
> 
> The ifdeffery can be avoided by using a "something ? x : NULL"
> construction instead and still have the compiler elide the _save
> method when it is not referenced. Unfortunately we can't just switch
> the existing env_save_ptr macro, since that would give a lot of build
> errors unless all the ifdeffery is removed at the same time.
> Conversely, removing that ifdeffery first would merely lead to the
> "defined but unused" warnings temporarily, but for some storage
> drivers it requires a bit more work than just removing their private
> CMD_SAVEENV logic.
> 
> So introduce an alternative to env_save_ptr, which for lack of a
> better name is simply uppercased, allowing one to update storage
> drivers piecemeal to both reduce their ifdeffery and honour
> CONFIG_SPL_SAVEENV.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, 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/20200424/2d2efd38/attachment.sig>

[PATCH 3/5] env/fat.c: remove private CMD_SAVEENV logic

[Tom Rini]

On Wed, Feb 19, 2020 at 09:47:41AM +0000, Rasmus Villemoes wrote:

> Always compile the env_fat_save() function, and let
> CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether
> it actually ends up being compiled in.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, 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/20200424/34253c9b/attachment.sig>

[PATCH 4/5] env/ext4.c: remove CONFIG_CMD_SAVEENV ifdef

[Tom Rini]

On Wed, Feb 19, 2020 at 09:47:42AM +0000, Rasmus Villemoes wrote:

> Removing this ifdef/endif pair yields a "defined but unused warning"
> for CONFIG_CMD_SAVEENV=n, but that vanishes if we use the ENV_SAVE_PTR
> macro instead. This gives slightly better compile testing, and
> moreover, it's possible to have
> 
>   CONFIG_CMD_SAVEENV=n
>   CONFIG_SPL_SAVEENV=y
>   SPL_ENV_IS_IN_EXT4=y
> 
> in which case env_ext4_save would erroneously not be compiled in.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, 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/20200424/9783cfd8/attachment.sig>

[PATCH 5/5] env/sf.c: drop private CMD_SAVEENV logic

[Tom Rini]

On Wed, Feb 19, 2020 at 09:47:43AM +0000, Rasmus Villemoes wrote:

> Deciding whether to compile the env_sf_save() function based solely on
> CONFIG_SPL_BUILD is wrong: For U-Boot proper, it leads to a build
> warning in case CONFIG_CMD_SAVEENV=n (because the env_save_ptr() macro
> causes the function to indeed not be referenced anywhere). And for
> SPL, when one selects CONFIG_SPL_SAVEENV, one obviously expects to
> actually be able to save the environment.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, 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/20200424/388b9b1b/attachment.sig>

[PATCH v2 1/2] env: add SAVEENV as an alias of the CMD_SAVEENV symbol

[Tom Rini]

On Fri, Mar 27, 2020 at 12:01:59AM +0100, Rasmus Villemoes wrote:

> Currently, quite a few storage drivers currently do not honour
> SPL_SAVEENV. That is, whether or not one enables CONFIG_SPL_SAVEENV,
> the backend drivers do not provide the .save method. Witness
> 
> env/fat.c:#ifdef CONFIG_SPL_BUILD
> ...
> env/fat.c-#else
> env/fat.c-# define LOADENV
> env/fat.c:# if defined(CONFIG_CMD_SAVEENV)
> env/fat.c:#  define CMD_SAVEENV
> env/fat.c-# endif
> env/fat.c-#endif
> env/fat.c-
> env/fat.c:#ifdef CMD_SAVEENV
> env/fat.c-static int env_fat_save(void)
> 
> env/flash.c:#ifndef CONFIG_SPL_BUILD
> env/flash.c:# if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_FLASH)
> env/flash.c:#  define CMD_SAVEENV
> ...
> env/flash.c:#ifdef CMD_SAVEENV
> env/flash.c-static int env_flash_save(void)
> 
> env/mmc.c:#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
> env/mmc.c-static inline int write_env(struct mmc *mmc, unsigned long size,
> 
> env/nand.c:#if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && \
> env/nand.c:             !defined(CONFIG_SPL_BUILD)
> env/nand.c:#define CMD_SAVEENV
> ...
> env/nand.c:#ifdef CMD_SAVEENV
> env/nand.c-/*
> env/nand.c- * The legacy NAND code saved the environment in the first NAND device i.e.,
> env/nand.c- * nand_dev_desc + 0. This is also the behaviour using the new NAND code.
> env/nand.c- */
> env/nand.c-static int writeenv(size_t offset, u_char *buf)
> 
> env/sf.c:#ifndef CONFIG_SPL_BUILD
> env/sf.c:#define CMD_SAVEENV
> env/sf.c-#define INITENV
> env/sf.c-#endif
> ...
> env/sf.c:#ifdef CMD_SAVEENV
> env/sf.c-static int env_sf_save(void)
> 
> In all these cases, the mere presence of CONFIG_SPL_BUILD means the
> save method does not get built.
> 
> Now, it is currently a bit awkward to write a proper test for whether
> saving the environment is enabled in the current context; something
> like
> 
> #if (!defined(CONFIG_SPL_BUILD) && defined(CONFIG_CMD_SAVEENV)) || \
>     (defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_SAVEENV))
> 
> But we already have a rather elegant mechanism that implicitly does
> the CONFIG_SPL_BUILD tests, namely CONFIG_IS_ENABLED(). Using that
> requires that the controlling config symbols follow a strict pattern:
> FOO for U-Boot proper, SPL_FOO for SPL.
> 
> This patch introduces CONFIG_SAVEENV as an alias for
> CONFIG_CMD_SAVEENV. That way, the above can simply be written
> 
> #if CONFIG_IS_ENABLED(SAVEENV)
> 
> and moreover, CONFIG_IS_ENABLED(SAVEENV) can also be used in C code,
> avoiding ifdeffery and providing more compile testing.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, 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/20200508/13fa5b6d/attachment.sig>

[PATCH v2 2/2] env/sf.c: honour CONFIG_SPL_SAVEENV

[Tom Rini]

On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote:

> Deciding whether to compile the env_sf_save() function based solely on
> CONFIG_SPL_BUILD is wrong: For U-Boot proper, it leads to a build
> warning in case CONFIG_CMD_SAVEENV=n (because the initialization of
> the .save member is guarded by CONFIG_CMD_SAVEENV, while the
> env_sf_save() function is built if !CONFIG_SPL_BUILD - and even
> without the CONFIG_CMD_SAVEENV guard, the env_save_ptr() macro would
> just expand to NULL, with no reference to env_sf_save visible to the
> compiler). And for SPL, when one selects CONFIG_SPL_SAVEENV, one
> obviously expects to actually be able to save the environment.
> 
> The compiler warning can be fixed by using a "<something> ?
> env_sf_save : NULL" construction instead of a macro that just eats its
> argument and expands to NULL. That way, if <something> is false,
> env_sf_save gets eliminated as dead code, but the compiler still sees
> the reference to it.
> 
> For <something>, we can use CONFIG_IS_ENABLED(SAVEENV), which is true
> precisely:
> 
> - For U-Boot proper, when CONFIG_CMD_SAVEENV is set (because
>   CONFIG_SAVEENV is a hidden config symbol that gets set if and only
>   if CONFIG_CMD_SAVEENV is set).
> - For SPL, when CONFIG_SPL_SAVEENV is set.
> 
> As a bonus, this also removes quite a few preprocessor conditionals.
> 
> This has been run-time tested on a mpc8309-derived board to verify
> that saving the environment does indeed work in SPL with these patches
> applied.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, 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/20200508/966d2d32/attachment.sig>

[PATCH v2 2/2] env/sf.c: honour CONFIG_SPL_SAVEENV

[Rasmus Villemoes]

On 09/05/2020 00.59, Tom Rini wrote:
> On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote:
> 
> 
> Applied to u-boot/master, thanks!
> 

Eh, thanks, but you already applied v1 consisting of 5 patches. v1 1/5
corresponded to v2 1/2, while v1 5/5 corresponded to v2 2/2 - v1 3/5 and
4/5 were left out of v2. v1 2/5 was a helper macro I decided wasn't that
much of a helper (but it's still needed since v1 3/5 and v1 4/5 were
applied).

This doesn't mean anything needs fixing up - I'm guessing you rebased
the two patches, git saw that v2 1/2 was already applied, and then
either you or git saw that most of v2 2/2 was already applied, so the
only thing commit 6d3524c2ad does is to replace ENV_SAVE_PTR with its
definition.

Rasmus

[PATCH v2 2/2] env/sf.c: honour CONFIG_SPL_SAVEENV

[Tom Rini]

On Sat, May 09, 2020 at 08:56:46PM +0200, Rasmus Villemoes wrote:
> On 09/05/2020 00.59, Tom Rini wrote:
> > On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote:
> > 
> > 
> > Applied to u-boot/master, thanks!
> > 
> 
> Eh, thanks, but you already applied v1 consisting of 5 patches. v1 1/5
> corresponded to v2 1/2, while v1 5/5 corresponded to v2 2/2 - v1 3/5 and
> 4/5 were left out of v2. v1 2/5 was a helper macro I decided wasn't that
> much of a helper (but it's still needed since v1 3/5 and v1 4/5 were
> applied).
> 
> This doesn't mean anything needs fixing up - I'm guessing you rebased
> the two patches, git saw that v2 1/2 was already applied, and then
> either you or git saw that most of v2 2/2 was already applied, so the
> only thing commit 6d3524c2ad does is to replace ENV_SAVE_PTR with its
> definition.

:headdesk:

So, 'git am' went through and applied what could be applied and I didn't
see a "skipping" message.  But at this point, are there any changes that
need to still come in?  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/20200509/84a5ec53/attachment.sig>

[PATCH v2 2/2] env/sf.c: honour CONFIG_SPL_SAVEENV

[Rasmus Villemoes]

On 09/05/2020 22.54, Tom Rini wrote:
> On Sat, May 09, 2020 at 08:56:46PM +0200, Rasmus Villemoes wrote:
>> On 09/05/2020 00.59, Tom Rini wrote:
>>> On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote:
>>>
>>>
>>> Applied to u-boot/master, thanks!
>>>
>>
>> Eh, thanks, but you already applied v1 consisting of 5 patches. v1 1/5
>> corresponded to v2 1/2, while v1 5/5 corresponded to v2 2/2 - v1 3/5 and
>> 4/5 were left out of v2. v1 2/5 was a helper macro I decided wasn't that
>> much of a helper (but it's still needed since v1 3/5 and v1 4/5 were
>> applied).
>>
>> This doesn't mean anything needs fixing up - I'm guessing you rebased
>> the two patches, git saw that v2 1/2 was already applied, and then
>> either you or git saw that most of v2 2/2 was already applied, so the
>> only thing commit 6d3524c2ad does is to replace ENV_SAVE_PTR with its
>> definition.
> 
> :headdesk:
> 
> So, 'git am' went through and applied what could be applied and I didn't
> see a "skipping" message.  But at this point, are there any changes that
> need to still come in?  Thanks!

No, I think the code should all work. The history is a bit misleading as
commit 6d3524c2ad doesn't have any functional change, it was all already
done by the v1 patch which is applied as 9e3c94d11. But there's not much
to be done about that, I guess. One could revert 6d3524c2ad without any
damage and use the commit message to explain things, but I don't know if
that's worth the churn. If you think it is, let me know and I'll try to
draft a revert commit log which you can then edit to taste.

Rasmus

[PATCH v2 2/2] env/sf.c: honour CONFIG_SPL_SAVEENV

[Tom Rini]

On Mon, May 11, 2020 at 08:49:38AM +0200, Rasmus Villemoes wrote:
> On 09/05/2020 22.54, Tom Rini wrote:
> > On Sat, May 09, 2020 at 08:56:46PM +0200, Rasmus Villemoes wrote:
> >> On 09/05/2020 00.59, Tom Rini wrote:
> >>> On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote:
> >>>
> >>>
> >>> Applied to u-boot/master, thanks!
> >>>
> >>
> >> Eh, thanks, but you already applied v1 consisting of 5 patches. v1 1/5
> >> corresponded to v2 1/2, while v1 5/5 corresponded to v2 2/2 - v1 3/5 and
> >> 4/5 were left out of v2. v1 2/5 was a helper macro I decided wasn't that
> >> much of a helper (but it's still needed since v1 3/5 and v1 4/5 were
> >> applied).
> >>
> >> This doesn't mean anything needs fixing up - I'm guessing you rebased
> >> the two patches, git saw that v2 1/2 was already applied, and then
> >> either you or git saw that most of v2 2/2 was already applied, so the
> >> only thing commit 6d3524c2ad does is to replace ENV_SAVE_PTR with its
> >> definition.
> > 
> > :headdesk:
> > 
> > So, 'git am' went through and applied what could be applied and I didn't
> > see a "skipping" message.  But at this point, are there any changes that
> > need to still come in?  Thanks!
> 
> No, I think the code should all work. The history is a bit misleading as
> commit 6d3524c2ad doesn't have any functional change, it was all already
> done by the v1 patch which is applied as 9e3c94d11. But there's not much
> to be done about that, I guess. One could revert 6d3524c2ad without any
> damage and use the commit message to explain things, but I don't know if
> that's worth the churn. If you think it is, let me know and I'll try to
> draft a revert commit log which you can then edit to taste.

I guess we should just leave it be then for now, thanks and sorry for
the confusion.

-- 
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/20200511/c11baa65/attachment.sig>