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

3 Sep 2019

      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.
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@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@denx.de