| Message ID | 20200326230200.12617-1-rasmus.villemoes@prevas.dk |
|---|---|
| Headers | show |
| Series | allow CONFIG_SPL_SAVEENV to work with ENV_IS_IN_SPI_FLASH | expand |
Dear Rasmus, In message <20200326230200.12617-1-rasmus.villemoes at 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 at 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 at denx.de> Thanks, Rasmus
Dear Rasmus, In message <893503e2-10a1-2d3e-e7ad-9d24163ade0f at 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
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(-)