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

[Patrick DELAUNAY]

Hi,

> From: Wolfgang Denk <wd at denx.de>
> Sent: samedi 7 septembre 2019 13:52
> 
> Dear Patrick,
> 
> In message <1567530547-14331-1-git-send-email-patrick.delaunay at 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 ?

> 
> Best regards,
> 
> Wolfgang Denk
> 
> --

Best regards

Patrick Delaunay