Hi Heiko
From: Heiko Schocher hs@denx.de Sent: mardi 3 septembre 2019 06:45 Subject: Re: U-Boot: Environment flags broken for U-Boot
Hello Patrick,
Am 02.09.2019 um 17:35 schrieb Patrick DELAUNAY:
Hi Heiko,
From: Heiko Schocher hs@denx.de Sent: lundi 2 septembre 2019 16:03
Hello Patrick,
I am just testing U-Boot Environment flags and they do not work anymore with current mainline U-Boot ... I should have made some tbot test for it, but did not found yet time for it ...
Here log with current mainline:
=> printenv heiko heiko=changed => env flags Available variable type flags (position 0): Flag Variable Type Name ---- ------------------ s - string d - decimal x - hexadecimal b - boolean i - IP address m - MAC address
Available variable access flags (position 1): Flag Variable Access Name ---- -------------------- a - any r - read-only o - write-once c - change-default
Static flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- eth\d*addr MAC address any ipaddr IP address any gatewayip IP address any netmask IP address any serverip IP address any nvlan decimal any vlan decimal any dnsip IP address any heiko string write-once
Active flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- .flags string write-once netmask IP address any serverip IP address any heiko string write-once ipaddr IP address any => setenv heiko foo => print heiko heiko=foo => setenv heiko bar => print heiko heiko=bar
I can change Environment variable "heiko" but flag is write-once !
Ok, digging around and I found, that in env/common.c changed_ok is NULL which results in not checking U-Boot flags:
26 struct hsearch_data env_htab = { 27 #if CONFIG_IS_ENABLED(ENV_SUPPORT) 28 /* defined in flags.c, only compile with ENV_SUPPORT */ 29 .change_ok = env_flags_validate, 30 #endif 31 };
reason is your commit:
commit 7d4776545b0f8a8827e5d061206faf61c9ba6ea9 Author: Patrick Delaunay patrick.delaunay@st.com Date: Thu Apr 18 17:32:49 2019 +0200
env: solve compilation error in SPL Solve compilation issue when cli_simple.o is used in SPL and CONFIG_SPL_ENV_SUPPORT is not defined. env/built-in.o:(.data.env_htab+0xc): undefined reference to`env_flags_validate'
u-boot/scripts/Makefile.spl:384: recipe for target 'spl/u-boot-spl' failed make[2]: *** [spl/u-boot-spl] Error 1 u-boot/Makefile:1649: recipe for target 'spl/u-boot-spl' failed make[1]: *** [spl/u-boot-spl] Error 2 Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>ENV_SUPPORT is only defined for SPL and TPL not for U-Boot, which leads in change_ok always NULL in U-Boot ...
:-(
reverting this commit and it works again as expected ...
Urgs ... since april 2019 nobody tested this feature
:-(
Nevertheless, reverting commit and I see:
=> print heiko heiko=test => setenv heiko foo ## Error inserting "heiko" variable, errno=1 =>
So we should find a solution for this.
Any ideas?
Hi,
Yes I am responsible of the regression, sorry.
When I see the definition CONFIG_SPL_ENV_SUPPORT and
CONFIG_TPL_ENV_SUPPORT, I use the generic macro to check the activation of these TPL/SPL feature in the SPL/TPL builds....
But I don't check the existence of the U-Boot flag CONFIG_ENV_SUPPORT
when I propose my patch... so my patch is incorrect.
As flags.o is always compiled for U-Boot :
ifndef CONFIG_SPL_BUILD obj-y += attr.o obj-y += callback.o obj-y += flags.o ... else obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o endif
I see 2 solutions:
1/ change my patch to check U-boot compilation case with !defined(CONFIG_SPL_BUILD)
26 struct hsearch_data env_htab = { 27 #if !defined(CONFIG_SPL_BUILD) ||
CONFIG_IS_ENABLED(ENV_SUPPORT)
28 /* defined in flags.c, only compile with ENV_SUPPORT for SPL / TPL
*/
29 .change_ok = env_flags_validate, 30 #endif 31 };
Hmmm ... in case of CONFIG_TPL_BUILD it is also active, which your original patch wanted to prevent ... so this seems not a good solution to me.
In fact CONFIG_SPL_BUILD is also activated during TPL build (in scripts/Makefile.autocof:85 for tpl/u-boot.cfg)
So the test !defined(CONFIG_SPL_BUILD) is enough but I can replace by more clear #if (!defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_BUILD)) ||
We need a CONFIG_UBOOT_BUILD define in this case ...
2/ introduce a new Kconfig to env support in U-Boot
Yep, this would be the clean solution!
config ENV_SUPPORT bool "Support an environment features" default y help Enable full environment support in U-Boot. Including attributes, callbacks and flags.
And the Makefile is more simple :
obj-y += common.o env.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o
exact!
ifndef CONFIG_SPL_BUILD obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o extra-$(CONFIG_ENV_IS_EMBEDDED) += embedded.o obj-$(CONFIG_ENV_IS_IN_EEPROM) += embedded.o extra-$(CONFIG_ENV_IS_IN_FLASH) += embedded.o obj-$(CONFIG_ENV_IS_IN_NVRAM) += embedded.o obj-$(CONFIG_ENV_IS_IN_NVRAM) += nvram.o obj-$(CONFIG_ENV_IS_IN_ONENAND) += onenand.o obj-$(CONFIG_ENV_IS_IN_SATA) += sata.o obj-$(CONFIG_ENV_IS_IN_REMOTE) += remote.o obj-$(CONFIG_ENV_IS_IN_UBI) += ubi.o endif
but we have also other impact with hashtable...
obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += hashtable.o .....
Ok...
and I have others issues in cmd/nvedit.c / cmd/nvedit.c
What sort of issues ?
Compilation issue (missing function define in hastable.c) when CONFIG_ENV_SUPPORT is not activated....
=> I don't sure of the side effect if I remove all this part in proper U-Boot.
What do you mean with "remove all this part in proper U-Boot" ?
Remove all the code when the CONFIG is not activated (board overridde the default value) Remove all the code in => attr.o, flags.o callback.o hashtable.
For a short term solution it is O (as default is y), but I prefer introduce a CONFIG which can be deactivated.
So I prefer the solution 1, but I can go deeper with solution 2 (only remove
flags.c) if you prefer.
I prefer variant 2 ... but if it is a patch which has a lot of impacts may solution 1 is also valid as a bugfix before we release 2019.10, and variant 2 patch can be discussed and added in the next merge window?
So please send a patch for variant 2 and if it come out, that it has to much impacts before 2019.10 release, we can apply variant 1 ?
I will sent a patch for proposal 2, today or tomorrow.
Tom? What do you think?
If you are allign with my porposal 1 I propose this patch asap:
struct hsearch_data env_htab = {
- #if CONFIG_IS_ENABLED(ENV_SUPPORT)
/* defined in flags.c, only compile with ENV_SUPPORT */+#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT)
/* defined in flags.c, only compile with ENV_SUPPORT for SPL+/ TPL */ .change_ok = env_flags_validate, #endif };
bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
regards Patrick.
Thanks for looking into it!
Regards
Patrick
bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de