[U-Boot] U-Boot: Environment flags broken for U-Boot
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
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@denx.de
regards Patrick.
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,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@denx.de
regards Patrick.
Thanks for looking into it!
bye, Heiko
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@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@denx.de
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
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 SPLLooking 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@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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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 SPLLooking 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
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 SPLLooking 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.
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 SPLLooking 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.
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 SPLLooking 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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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 SPLLooking 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.
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
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!
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
participants (5)
-
Heiko Schocher -
Joe Hershberger -
Patrick DELAUNAY -
Tom Rini -
Wolfgang Denk