[PATCH 0/5] CMD_SAVEENV ifdef cleanup
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(-)
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 && \
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!
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 */
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!
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), };
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
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
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
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.
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
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!
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), };
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!
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
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!
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-Bootext4.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
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-Bootext4.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:
- 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.]
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
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
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
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
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
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
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(-)
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 && \
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!
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
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!
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
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!
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
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.
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
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
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
participants (3)
-
Rasmus Villemoes -
Tom Rini -
Wolfgang Denk