On Tue, Sep 10, 2019 at 11:01:18AM +0000, Patrick DELAUNAY wrote: > Hi, > > > From: Wolfgang Denk <w...@denx.de> > > Sent: samedi 7 septembre 2019 13:52 > > > > Dear Patrick, > > > > In message <1567530547-14331-1-git-send-email-patrick.delau...@st.com> you > > wrote: > > > Add a new flag CONFIG_ENV_SUPPORT to compile all the environment > > > features in U-Boot (attributes, callbacks and flags). It is the > > > equivalent of the 2 existing flags > > > > I think this is a bda name, as it is misleading. It sounds as if it is > > used to enable > > the support of environment vaiables at all, which it does not - instead it > > only > > enables / disables a few specific extended features. This must be > > reflected in the > > name. > > Yes, I use the name than SPL/TPL to use the macro CONFIG_IS_ENABLED(...) > > And I agree the name seens not perfect. > > > > - CONFIG_SPL_ENV_SUPPORT for SPL > > > - CONFIG_TPL_ENV_SUPPORT for TPL > > These pre-existing name are defined in common/spl/Kconfig > > With the same issue (env/common.o env/env.o are always compiled for SPL/TPL > so it is alo bad names) > > > This scares me. Why are there different settings for SPL, TPL and U-Boot > > proper? This looks conceptually broken to me - if a system is configured > > to use a > > specific set of environment features and extensions, then the exact same > > settings > > must be use in all of SPL, TPL and U-Boot proper. > > > > I understand that it may be desirable for example to reduce the size of the > > SPL by > > omitting some environment extensions, but provide these in U-Boot proper, > > but > > this is broken and dangerous. For example, U-Boot flags are often used to > > enforce a certain level of security (say, by making environment variables > > read- > > only or such). > > I agree, that scare me also. > For me also ENV_SUPPORT should be always enable for U-Boot. > > My preferred proposal was only to solve the regression introduced by my > initial patch and > always set change_ok for U-Boot proper (when CONFIG_SPL_BUILD is not defined): > > 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 > }; > > http://u-boot.10912.n7.nabble.com/U-Boot-Environment-flags-broken-for-U-Boot-tt382673.html#a382688 > > The same test is already done in: > > drivers/reset/reset-socfpga.c:47:#if !defined(CONFIG_SPL_BUILD) || > CONFIG_IS_ENABLED(ENV_SUPPORT) > drivers/input/input.c:656:#if !defined(CONFIG_SPL_BUILD) || > CONFIG_IS_ENABLED(ENV_SUPPORT) > > > The same level of handling and protection must also be maintained in SPL and > > TPL. > > if I understood correctly the proposed dependency is : > TPL ENV_SUPPORT select SPL_ENV_SUPPORT > SPL ENV_SUPPORT select ENV_SUPPORT > > > So please reconsider this whole implementation, and make sure that only a > > single > > macro ise used everywhere to enable these features. > > But, if I use the same CONFIG for the 3 binary SPL,TPL and U-Boot, > l increase the size of TPL/SPL for all the platforms when these additional > features are not needed. > > And I am not the sure of the correct dependency for ENV between binary. > > Heiko what is you feedback on Wolfgang remarks.... > > Do you think that I need to come back to the first/simple proposal ?
My two cents is that I would like to see a regression fix "soon" and for this release, and some corrections / updates to names, what is/isn't possible to enable (keeping in mind what is desirable to allow) for the next release. Thanks all! -- Tom
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot