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

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

[Heiko Schocher]

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?

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

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

[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 };

2/ introduce a new Kconfig to env support in U-Boot

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

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
.....

and I have others issues in cmd/nvedit.c / cmd/nvedit.c

=> I don't sure of the side effect if I 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.

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.

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

[Heiko Schocher]

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

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

[Wolfgang Denk]

Dear Tom,

In message <a78f0b04-c3f7-45d5-e9ac-90522dbefc2e@denx.de> Heiko Schocher wrote:
> 
> I am just testing U-Boot Environment flags and they do not work anymore with
> current mainline U-Boot ... 
...
> 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


Looking into the history of this, I wonder if we could / should
have prevented this.

As far as I can see, Patrick's patch series has not been reviewed by
others, probably because general intetest in STM32 is not that big
at the moment.  I can see no Acked-by:, Reviewed-by: nor Tested-by:
tags - nothing.

The whole patch series was then pulled from the u-boot-stm
repository.


However, there was not only STM related code in there.  There were
changes to common code like the environment handling.  common code
was changed without review and without testing.


Are there ways to prevent this?

Yes, we can appeal to the custodians to be more careful, but I
assume they are already doing their best.

It might have even been better if this had been a sub-system with a
clear maintainer, but there is no such person for the environment
code.

How can we prevent this in the future?

Should we define "interested developers" for such areas that have no
custodian (the "Designated reviewer") entry in the MAINTAINERS file
could be used for this, for example)?

Better ideas?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
To make this work we'd need a patch, as nobody of us tests this.
- L. Poettering in https://bugs.freedesktop.org/show_bug.cgi?id=74589

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

[Patrick DELAUNAY]

Hi Heiko

> From: Heiko Schocher <hs@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@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.

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

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

[Joe Hershberger]

On Tue, Sep 3, 2019 at 3:05 AM Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Tom,
>
> In message <a78f0b04-c3f7-45d5-e9ac-90522dbefc2e@denx.de> Heiko Schocher wrote:
> >
> > I am just testing U-Boot Environment flags and they do not work anymore with
> > current mainline U-Boot ...
> ...
> > 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
>
>
> Looking into the history of this, I wonder if we could / should
> have prevented this.
>
> As far as I can see, Patrick's patch series has not been reviewed by
> others, probably because general intetest in STM32 is not that big
> at the moment.  I can see no Acked-by:, Reviewed-by: nor Tested-by:
> tags - nothing.
>
> The whole patch series was then pulled from the u-boot-stm
> repository.
>
>
> However, there was not only STM related code in there.  There were
> changes to common code like the environment handling.  common code
> was changed without review and without testing.

It seems this should be unacceptable even if it's in the area of
interest. Isn't an Acked-by generally accepted as required?

> Are there ways to prevent this?
>
> Yes, we can appeal to the custodians to be more careful, but I
> assume they are already doing their best.

It seems the diffstat should be a quick way to see this, so I would
think not quite their best. Maybe a reminder / recommendation that it
be reviewed by custodians?

> It might have even been better if this had been a sub-system with a
> clear maintainer, but there is no such person for the environment
> code.
>
> How can we prevent this in the future?

Is there any tooling around the MAINTAINERS file that can be used to
reg-flag PRs that contain changes outside of the tree's area of
effect?

> Should we define "interested developers" for such areas that have no
> custodian (the "Designated reviewer") entry in the MAINTAINERS file
> could be used for this, for example)?

I like that idea, though in this specific case I think there should be
a maintainer for env.

> Better ideas?
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> To make this work we'd need a patch, as nobody of us tests this.
> - L. Poettering in https://bugs.freedesktop.org/show_bug.cgi?id=74589
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

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

[Heiko Schocher]

Hello Joe,

Am 04.09.2019 um 01:03 schrieb Joe Hershberger:
> On Tue, Sep 3, 2019 at 3:05 AM Wolfgang Denk <wd@denx.de> wrote:
>>
>> Dear Tom,
>>
>> In message <a78f0b04-c3f7-45d5-e9ac-90522dbefc2e@denx.de> Heiko Schocher wrote:
>>>
>>> I am just testing U-Boot Environment flags and they do not work anymore with
>>> current mainline U-Boot ...
>> ...
>>> 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
>>
>>
>> Looking into the history of this, I wonder if we could / should
>> have prevented this.
>>
>> As far as I can see, Patrick's patch series has not been reviewed by
>> others, probably because general intetest in STM32 is not that big
>> at the moment.  I can see no Acked-by:, Reviewed-by: nor Tested-by:
>> tags - nothing.
>>
>> The whole patch series was then pulled from the u-boot-stm
>> repository.
>>
>>
>> However, there was not only STM related code in there.  There were
>> changes to common code like the environment handling.  common code
>> was changed without review and without testing.
> 
> It seems this should be unacceptable even if it's in the area of
> interest. Isn't an Acked-by generally accepted as required?

Yes, but it seems we are not strict enough here.

> 
>> Are there ways to prevent this?
>>
>> Yes, we can appeal to the custodians to be more careful, but I
>> assume they are already doing their best.
> 
> It seems the diffstat should be a quick way to see this, so I would
> think not quite their best. Maybe a reminder / recommendation that it
> be reviewed by custodians?

Yes. I recommend to use patman for sending patches, or at least to
do a dry run with it, so you get a cc list (which is sometimes to long)
of people who may interested in the patch.

> 
>> It might have even been better if this had been a sub-system with a
>> clear maintainer, but there is no such person for the environment
>> code.
>>
>> How can we prevent this in the future?
> 
> Is there any tooling around the MAINTAINERS file that can be used to
> reg-flag PRs that contain changes outside of the tree's area of
> effect?

I do not know.

>> Should we define "interested developers" for such areas that have no
>> custodian (the "Designated reviewer") entry in the MAINTAINERS file
>> could be used for this, for example)?
> 
> I like that idea, though in this specific case I think there should be
> a maintainer for env.

Wasn;t aware of the the "Designated reviewer" entry in MAINTAINERS,
I think, this is a good idea. And of course, if someone volunteers
for mainting env, this would be good.

But we should monitor (or find a script which checks this), that
patches not acked by a subsystem custodian not go in outside of a
pull request from the custodian. Problem is here, that we have
parts of code, which have no custodian ...

I can only speak for i2c, which often get patches in patchseries
for other custodians. I try to catch such patches and add a Review
or Acked-by ... but I do not catch all... so using patman would help,
as I get added to cc ...

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

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

[Tom Rini]

On Tue, Sep 03, 2019 at 10:04:42AM +0200, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <a78f0b04-c3f7-45d5-e9ac-90522dbefc2e@denx.de> Heiko Schocher wrote:
> > 
> > I am just testing U-Boot Environment flags and they do not work anymore with
> > current mainline U-Boot ... 
> ...
> > 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
> 
> 
> Looking into the history of this, I wonder if we could / should
> have prevented this.

Looking over my scripts, yes, I overlooked the problem.  The 'edison'
platform shows the same issue that Heiko's platform does but I
overlooked the size change.  I'm modifying my script currently so it
will show more details and this should jump out more rather than the
size noise of "changes in a general area".  Now interesting enough,
sandbox didn't blow up here but does also enable the env flags options.

> As far as I can see, Patrick's patch series has not been reviewed by
> others, probably because general intetest in STM32 is not that big
> at the moment.  I can see no Acked-by:, Reviewed-by: nor Tested-by:
> tags - nothing.
> 
> The whole patch series was then pulled from the u-boot-stm
> repository.
> 
> 
> However, there was not only STM related code in there.  There were
> changes to common code like the environment handling.  common code
> was changed without review and without testing.

To be clear, it was tested, but sadly the environment flags code is not
heavily used / enabled.  More in a moment.

> Are there ways to prevent this?
> 
> Yes, we can appeal to the custodians to be more careful, but I
> assume they are already doing their best.
> 
> It might have even been better if this had been a sub-system with a
> clear maintainer, but there is no such person for the environment
> code.
> 
> How can we prevent this in the future?
> 
> Should we define "interested developers" for such areas that have no
> custodian (the "Designated reviewer") entry in the MAINTAINERS file
> could be used for this, for example)?

This, along with some other environment related patches were things I
was keeping an eye on to see if perhaps Joe would have had time to look
at before it went in (as the env flag stuff came from him).  I also try
and make use of the "Needs Review / ACK" flag in patchwork for things
that stand out.  Looking over the merge contents again, that particular
one would not have.

So, things that would help in the future:
- An explicit environment maintainer
- test.py tests for the environment flags, but only if they're also run
  on some platform(s) that also would have failed here.  Perhaps we need
  to enable more functionality in something like qemu-x86 that is less
  of a special case build than sandbox is?  In fact, since I know we
  have the QEMU targets in for "real" uses and not just testing,
  and while I worry about adding in more complex logic, we might want to
  rework the "build and run test.py in QEMU" parts of CI to first make
  use of scripts/kconfig/merge_config.sh to turn ON a whole bunch of
  testing related options.

-- 
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/20190904/df44c379/attachment.sig>

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

[Joe Hershberger]

On Wed, Sep 4, 2019 at 1:01 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Sep 03, 2019 at 10:04:42AM +0200, Wolfgang Denk wrote:
> > Dear Tom,
> >
> > In message <a78f0b04-c3f7-45d5-e9ac-90522dbefc2e@denx.de> Heiko Schocher wrote:
> > >
> > > I am just testing U-Boot Environment flags and they do not work anymore with
> > > current mainline U-Boot ...
> > ...
> > > 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
> >
> >
> > Looking into the history of this, I wonder if we could / should
> > have prevented this.
>
> Looking over my scripts, yes, I overlooked the problem.  The 'edison'
> platform shows the same issue that Heiko's platform does but I
> overlooked the size change.  I'm modifying my script currently so it
> will show more details and this should jump out more rather than the
> size noise of "changes in a general area".  Now interesting enough,
> sandbox didn't blow up here but does also enable the env flags options.
>
> > As far as I can see, Patrick's patch series has not been reviewed by
> > others, probably because general intetest in STM32 is not that big
> > at the moment.  I can see no Acked-by:, Reviewed-by: nor Tested-by:
> > tags - nothing.
> >
> > The whole patch series was then pulled from the u-boot-stm
> > repository.
> >
> >
> > However, there was not only STM related code in there.  There were
> > changes to common code like the environment handling.  common code
> > was changed without review and without testing.
>
> To be clear, it was tested, but sadly the environment flags code is not
> heavily used / enabled.  More in a moment.
>
> > Are there ways to prevent this?
> >
> > Yes, we can appeal to the custodians to be more careful, but I
> > assume they are already doing their best.
> >
> > It might have even been better if this had been a sub-system with a
> > clear maintainer, but there is no such person for the environment
> > code.
> >
> > How can we prevent this in the future?
> >
> > Should we define "interested developers" for such areas that have no
> > custodian (the "Designated reviewer") entry in the MAINTAINERS file
> > could be used for this, for example)?
>
> This, along with some other environment related patches were things I
> was keeping an eye on to see if perhaps Joe would have had time to look
> at before it went in (as the env flag stuff came from him).  I also try

I wasn't aware of it as I wasn't Cc'ed on this series. I generally
don't have time to troll the list in general, which is a bit of a
problem since I also missed the discussions on the UEFI env changes,
some of which are already in, and are not how I would have implemented
it. I only found out that it was in work from Grant Likely at his talk
in San Diego.

> and make use of the "Needs Review / ACK" flag in patchwork for things
> that stand out.  Looking over the merge contents again, that particular
> one would not have.
>
> So, things that would help in the future:
> - An explicit environment maintainer

I would gladly volunteer for this role if Wolfgang would co-maintain
to keep me in line. He seems to have an uncanny ability to keep all
the cases in his head.

> - test.py tests for the environment flags, but only if they're also run
>   on some platform(s) that also would have failed here.  Perhaps we need
>   to enable more functionality in something like qemu-x86 that is less
>   of a special case build than sandbox is?  In fact, since I know we
>   have the QEMU targets in for "real" uses and not just testing,
>   and while I worry about adding in more complex logic, we might want to
>   rework the "build and run test.py in QEMU" parts of CI to first make
>   use of scripts/kconfig/merge_config.sh to turn ON a whole bunch of
>   testing related options.
> --
> Tom
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

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

[Tom Rini]

On Tue, Sep 03, 2019 at 06:03:40PM -0500, Joe Hershberger wrote:
> On Tue, Sep 3, 2019 at 3:05 AM Wolfgang Denk <wd@denx.de> wrote:
> >
> > Dear Tom,
> >
> > In message <a78f0b04-c3f7-45d5-e9ac-90522dbefc2e@denx.de> Heiko Schocher wrote:
> > >
> > > I am just testing U-Boot Environment flags and they do not work anymore with
> > > current mainline U-Boot ...
> > ...
> > > 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
> >
> >
> > Looking into the history of this, I wonder if we could / should
> > have prevented this.
> >
> > As far as I can see, Patrick's patch series has not been reviewed by
> > others, probably because general intetest in STM32 is not that big
> > at the moment.  I can see no Acked-by:, Reviewed-by: nor Tested-by:
> > tags - nothing.
> >
> > The whole patch series was then pulled from the u-boot-stm
> > repository.
> >
> >
> > However, there was not only STM related code in there.  There were
> > changes to common code like the environment handling.  common code
> > was changed without review and without testing.
> 
> It seems this should be unacceptable even if it's in the area of
> interest. Isn't an Acked-by generally accepted as required?
> 
> > Are there ways to prevent this?
> >
> > Yes, we can appeal to the custodians to be more careful, but I
> > assume they are already doing their best.
> 
> It seems the diffstat should be a quick way to see this, so I would
> think not quite their best. Maybe a reminder / recommendation that it
> be reviewed by custodians?

Part of the problem here is that yes, I need to rework my tooling a bit,
and am.  But another part of the problem is that this exact code area is
not widely used.  So the things that cause me concern didn't trigger.
But looking at the code by itself, I would have acked it.  It would have
then been on noticing the size change and function-removal to see
there's a not-so-obvious problem.

> > It might have even been better if this had been a sub-system with a
> > clear maintainer, but there is no such person for the environment
> > code.
> >
> > How can we prevent this in the future?
> 
> Is there any tooling around the MAINTAINERS file that can be used to
> reg-flag PRs that contain changes outside of the tree's area of
> effect?
> 
> > Should we define "interested developers" for such areas that have no
> > custodian (the "Designated reviewer") entry in the MAINTAINERS file
> > could be used for this, for example)?
> 
> I like that idea, though in this specific case I think there should be
> a maintainer for env.

I do wish we had a dedicated custodian for environment changes, yes.

-- 
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/20190904/e3ad85ca/attachment.sig>

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

[Tom Rini]

On Wed, Sep 04, 2019 at 01:30:02PM -0500, Joe Hershberger wrote:
> On Wed, Sep 4, 2019 at 1:01 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Sep 03, 2019 at 10:04:42AM +0200, Wolfgang Denk wrote:
> > > Dear Tom,
> > >
> > > In message <a78f0b04-c3f7-45d5-e9ac-90522dbefc2e@denx.de> Heiko Schocher wrote:
> > > >
> > > > I am just testing U-Boot Environment flags and they do not work anymore with
> > > > current mainline U-Boot ...
> > > ...
> > > > 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
> > >
> > >
> > > Looking into the history of this, I wonder if we could / should
> > > have prevented this.
> >
> > Looking over my scripts, yes, I overlooked the problem.  The 'edison'
> > platform shows the same issue that Heiko's platform does but I
> > overlooked the size change.  I'm modifying my script currently so it
> > will show more details and this should jump out more rather than the
> > size noise of "changes in a general area".  Now interesting enough,
> > sandbox didn't blow up here but does also enable the env flags options.
> >
> > > As far as I can see, Patrick's patch series has not been reviewed by
> > > others, probably because general intetest in STM32 is not that big
> > > at the moment.  I can see no Acked-by:, Reviewed-by: nor Tested-by:
> > > tags - nothing.
> > >
> > > The whole patch series was then pulled from the u-boot-stm
> > > repository.
> > >
> > >
> > > However, there was not only STM related code in there.  There were
> > > changes to common code like the environment handling.  common code
> > > was changed without review and without testing.
> >
> > To be clear, it was tested, but sadly the environment flags code is not
> > heavily used / enabled.  More in a moment.
> >
> > > Are there ways to prevent this?
> > >
> > > Yes, we can appeal to the custodians to be more careful, but I
> > > assume they are already doing their best.
> > >
> > > It might have even been better if this had been a sub-system with a
> > > clear maintainer, but there is no such person for the environment
> > > code.
> > >
> > > How can we prevent this in the future?
> > >
> > > Should we define "interested developers" for such areas that have no
> > > custodian (the "Designated reviewer") entry in the MAINTAINERS file
> > > could be used for this, for example)?
> >
> > This, along with some other environment related patches were things I
> > was keeping an eye on to see if perhaps Joe would have had time to look
> > at before it went in (as the env flag stuff came from him).  I also try
> 
> I wasn't aware of it as I wasn't Cc'ed on this series. I generally
> don't have time to troll the list in general, which is a bit of a
> problem since I also missed the discussions on the UEFI env changes,
> some of which are already in, and are not how I would have implemented
> it. I only found out that it was in work from Grant Likely at his talk
> in San Diego.
> 
> > and make use of the "Needs Review / ACK" flag in patchwork for things
> > that stand out.  Looking over the merge contents again, that particular
> > one would not have.
> >
> > So, things that would help in the future:
> > - An explicit environment maintainer
> 
> I would gladly volunteer for this role if Wolfgang would co-maintain
> to keep me in line. He seems to have an uncanny ability to keep all
> the cases in his head.

Wolfgang, what do you say?  It's certainly an area we could use a
custodian in.

-- 
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/20190909/e697a64c/attachment.sig>

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

[Wolfgang Denk]

Dear Tom,

In message <20190909210130.GU6927@bill-the-cat> you wrote:
> 
> On Wed, Sep 04, 2019 at 01:30:02PM -0500, Joe Hershberger wrote:
...
> > I would gladly volunteer for this role if Wolfgang would co-maintain
> > to keep me in line. He seems to have an uncanny ability to keep all
> > the cases in his head.
>
> Wolfgang, what do you say?  It's certainly an area we could use a
> custodian in.

I would be happy if someone takes care of this - thanks, Hoe for
volunteering!

As for co-maintaining:  for the last 7 years my available time has
always been considerably smaller than my interest in U-Boot, and it
would be not realistic to believe this would be changing any time
soon, sic!

Yes, I'm willing, but I cant guarrantee any bandwidth nor response
times.  I can just try...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Business is like a wheelbarrow. Nothing ever happens until you  start
pushing.

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

[Tom Rini]

On Tue, Sep 10, 2019 at 10:29:04AM +0200, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20190909210130.GU6927@bill-the-cat> you wrote:
> > 
> > On Wed, Sep 04, 2019 at 01:30:02PM -0500, Joe Hershberger wrote:
> ...
> > > I would gladly volunteer for this role if Wolfgang would co-maintain
> > > to keep me in line. He seems to have an uncanny ability to keep all
> > > the cases in his head.
> >
> > Wolfgang, what do you say?  It's certainly an area we could use a
> > custodian in.
> 
> I would be happy if someone takes care of this - thanks, Hoe for
> volunteering!
> 
> As for co-maintaining:  for the last 7 years my available time has
> always been considerably smaller than my interest in U-Boot, and it
> would be not realistic to believe this would be changing any time
> soon, sic!
> 
> Yes, I'm willing, but I cant guarrantee any bandwidth nor response
> times.  I can just try...

OK, thanks guys.  If I know who to poke when they aren't on CC, I can
refrain from pulling things that haven't been checked out yet.  Can one
of you please post a patch to update MAINTAINERS to grab env/ tools/env/
and the env include files so get_maintainers.pl will know too?  Thanks!

-- 
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/6ced8318/attachment.sig>

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

[Joe Hershberger]

On Tue, Sep 10, 2019 at 7:54 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Sep 10, 2019 at 10:29:04AM +0200, Wolfgang Denk wrote:
> > Dear Tom,
> >
> > In message <20190909210130.GU6927@bill-the-cat> you wrote:
> > >
> > > On Wed, Sep 04, 2019 at 01:30:02PM -0500, Joe Hershberger wrote:
> > ...
> > > > I would gladly volunteer for this role if Wolfgang would co-maintain
> > > > to keep me in line. He seems to have an uncanny ability to keep all
> > > > the cases in his head.
> > >
> > > Wolfgang, what do you say?  It's certainly an area we could use a
> > > custodian in.
> >
> > I would be happy if someone takes care of this - thanks, Hoe for
> > volunteering!
> >
> > As for co-maintaining:  for the last 7 years my available time has
> > always been considerably smaller than my interest in U-Boot, and it
> > would be not realistic to believe this would be changing any time
> > soon, sic!
> >
> > Yes, I'm willing, but I cant guarrantee any bandwidth nor response
> > times.  I can just try...
>
> OK, thanks guys.  If I know who to poke when they aren't on CC, I can
> refrain from pulling things that haven't been checked out yet.  Can one
> of you please post a patch to update MAINTAINERS to grab env/ tools/env/
> and the env include files so get_maintainers.pl will know too?  Thanks!

Sure... I'll send a patch later today.

-Joe