[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 at 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 at 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