Hi Heiko > From: Heiko Schocher <h...@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 <h...@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.delau...@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.delau...@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: h...@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: h...@denx.de _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot