[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