[U-Boot] U-Boot: Environment flags broken for U-Boot

[Patrick DELAUNAY <patrick.delaunay@st.com>]

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 };

2/ introduce a new Kconfig to env support in U-Boot

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

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
.....

and I have others issues in cmd/nvedit.c / cmd/nvedit.c

=> I don't sure of the side effect if I 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.

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.