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