[U-Boot] [PATCH] env: Add CONFIG_ENV_SUPPORT

Tom Rini
2019-09-10T14:33:14Z

On Tue, Sep 10, 2019 at 11:01:18AM +0000, Patrick DELAUNAY wrote:
> Hi,
>
> > From: Wolfgang Denk <wd@denx.de>
> > Sent: samedi 7 septembre 2019 13:52
> >
> > Dear Patrick,
> >
> > In message <1567530547-14331-1-git-send-email-patrick.delaunay@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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190910/6e39ac19/attachment.sig>