[U-Boot] U-Boot: Environment flags broken for U-Boot
Patrick DELAUNAY
patrick.delaunay at st.com
Tue Sep 3 14:04:38 UTC 2019
Hi Heiko
> From: Heiko Schocher <hs at 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 <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.
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: hs at 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: hs at denx.de
More information about the U-Boot
mailing list