[PATCH 0/5] CMD_SAVEENV ifdef cleanup

[Wolfgang Denk]

Dear Rasmus,

In message <51077b65-56c1-464a-8721-77b6a7bf385d at prevas.dk> you wrote:
> > 
> > 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?

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."