[U-Boot] U-Boot: Environment flags broken for U-Boot
Heiko Schocher
hs at denx.de
Tue Sep 3 04:44:42 UTC 2019
Hello Patrick,
Am 02.09.2019 um 17:35 schrieb Patrick DELAUNAY:
> Hi Heiko,
>
>
>> From: Heiko Schocher <hs at 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 at 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 at 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.
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 ?
> => 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" ?
> 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 ?
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 at denx.de
>
> regards
> Patrick.
Thanks for looking into it!
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 at denx.de
More information about the U-Boot
mailing list