[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