[PATCH 1/2] Fix usage of CONFIG_PREBOOT
Due to usage of PREBOOT in Kconfig, macro CONFIG_PREBOOT is always defined when CONFIG_USE_PREBOOT is enabled. In case CONFIG_PREBOOT is not explicitly enabled it is set to empty C string and therefore '#ifdef CONFIG_PREBOOT' guard does not work. Fix this issue by introducing a new Kconfig symbol PREBOOT_DEFINED which cause to define new C macro CONFIG_PREBOOT_DEFINED only when CONFIG_PREBOOT is really defined.
Change usage of '#ifdef CONFIG_PREBOOT' by '#ifdef CONFIG_USE_PREBOOT' for code which checks if preboot code would be called and by '#ifdef CONFIG_PREBOOT_DEFINED' for defining preboot code.
Signed-off-by: Pali Rohár pali@kernel.org --- board/boundary/nitrogen6x/nitrogen6x.c | 4 ++-- boot/Kconfig | 4 ++++ include/env_default.h | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c index 83bb445d481a..382c01ddf4e0 100644 --- a/board/boundary/nitrogen6x/nitrogen6x.c +++ b/board/boundary/nitrogen6x/nitrogen6x.c @@ -929,7 +929,7 @@ U_BOOT_CMD( "Returns 0 (true) to shell if key is pressed." );
-#ifdef CONFIG_PREBOOT +#ifdef CONFIG_USE_PREBOOT static char const kbd_magic_prefix[] = "key_magic"; static char const kbd_command_prefix[] = "key_cmd";
@@ -989,7 +989,7 @@ int misc_init_r(void) gpio_request(IMX_GPIO_NR(2, 3), "search"); gpio_request(IMX_GPIO_NR(7, 13), "volup"); gpio_request(IMX_GPIO_NR(4, 5), "voldown"); -#ifdef CONFIG_PREBOOT +#ifdef CONFIG_USE_PREBOOT preboot_keys(); #endif
diff --git a/boot/Kconfig b/boot/Kconfig index 08451c65a56b..5e7ae61d5116 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -1309,6 +1309,10 @@ config PREBOOT help This is the default of "preboot" environment variable.
+config PREBOOT_DEFINED + bool + default y if PREBOOT != "" + config DEFAULT_FDT_FILE string "Default fdt file" help diff --git a/include/env_default.h b/include/env_default.h index 7004a6fef29b..62a73b939cf2 100644 --- a/include/env_default.h +++ b/include/env_default.h @@ -62,7 +62,7 @@ const char default_environment[] = { #ifdef CONFIG_SYS_AUTOLOAD "autoload=" CONFIG_SYS_AUTOLOAD "\0" #endif -#ifdef CONFIG_PREBOOT +#ifdef CONFIG_PREBOOT_DEFINED "preboot=" CONFIG_PREBOOT "\0" #endif #ifdef CONFIG_ROOTPATH
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org --- configs/nokia_rx51_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/nokia_rx51_defconfig b/configs/nokia_rx51_defconfig index 309cf28269c1..46b794f168d9 100644 --- a/configs/nokia_rx51_defconfig +++ b/configs/nokia_rx51_defconfig @@ -24,7 +24,6 @@ CONFIG_AUTOBOOT_MENU_SHOW=y CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run sdboot;run emmcboot;run attachboot;echo" CONFIG_USE_PREBOOT=y -CONFIG_PREBOOT="run preboot" # CONFIG_SYS_DEVICE_NULLDEV is not set CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Nokia RX-51 # "
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
configs/nokia_rx51_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/nokia_rx51_defconfig b/configs/nokia_rx51_defconfig index 309cf28269c1..46b794f168d9 100644 --- a/configs/nokia_rx51_defconfig +++ b/configs/nokia_rx51_defconfig @@ -24,7 +24,6 @@ CONFIG_AUTOBOOT_MENU_SHOW=y CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run sdboot;run emmcboot;run attachboot;echo" CONFIG_USE_PREBOOT=y -CONFIG_PREBOOT="run preboot" # CONFIG_SYS_DEVICE_NULLDEV is not set CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Nokia RX-51 # "
These changes are actually a bit puzzling. There are other platforms that set preboot in their default environment, rather than via CONFIG_PREBOOT, and their final value ends up being the one set in CONFIG_EXTRA_ENV_SETTINGS rather than the empty string that CONFIG_PREBOOT is. I assume you've confirmed that at run-time you end up with preboot="run preboot" being set, and not preboot="long command" ? The difference between nokia_rx51 and the other platforms is, I think, LTO being enabled and maybe that leads to a different final value in the resulting environment.
On Monday 11 July 2022 19:23:26 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
configs/nokia_rx51_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/nokia_rx51_defconfig b/configs/nokia_rx51_defconfig index 309cf28269c1..46b794f168d9 100644 --- a/configs/nokia_rx51_defconfig +++ b/configs/nokia_rx51_defconfig @@ -24,7 +24,6 @@ CONFIG_AUTOBOOT_MENU_SHOW=y CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run sdboot;run emmcboot;run attachboot;echo" CONFIG_USE_PREBOOT=y -CONFIG_PREBOOT="run preboot" # CONFIG_SYS_DEVICE_NULLDEV is not set CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Nokia RX-51 # "
These changes are actually a bit puzzling. There are other platforms that set preboot in their default environment, rather than via CONFIG_PREBOOT, and their final value ends up being the one set in CONFIG_EXTRA_ENV_SETTINGS rather than the empty string that CONFIG_PREBOOT is. I assume you've confirmed that at run-time you end up with preboot="run preboot" being set, and not preboot="long command" ?
At nokia n900 runtime is always "preboot=long command" and not "preboot=run preboot". It also was before these changes.
The difference between nokia_rx51 and the other platforms is, I think, LTO being enabled and maybe that leads to a different final value in the resulting environment.
-- Tom
On Tue, Jul 12, 2022 at 10:11:00AM +0200, Pali Rohár wrote:
On Monday 11 July 2022 19:23:26 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
configs/nokia_rx51_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/nokia_rx51_defconfig b/configs/nokia_rx51_defconfig index 309cf28269c1..46b794f168d9 100644 --- a/configs/nokia_rx51_defconfig +++ b/configs/nokia_rx51_defconfig @@ -24,7 +24,6 @@ CONFIG_AUTOBOOT_MENU_SHOW=y CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run sdboot;run emmcboot;run attachboot;echo" CONFIG_USE_PREBOOT=y -CONFIG_PREBOOT="run preboot" # CONFIG_SYS_DEVICE_NULLDEV is not set CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Nokia RX-51 # "
These changes are actually a bit puzzling. There are other platforms that set preboot in their default environment, rather than via CONFIG_PREBOOT, and their final value ends up being the one set in CONFIG_EXTRA_ENV_SETTINGS rather than the empty string that CONFIG_PREBOOT is. I assume you've confirmed that at run-time you end up with preboot="run preboot" being set, and not preboot="long command" ?
At nokia n900 runtime is always "preboot=long command" and not "preboot=run preboot". It also was before these changes.
OK, so then we really just need this patch, and not the other. And long term figure out how to better handle this along with the other half-dozen places the user may want to configure the default environment a bit more, on top of what's already provided. There are a handful of other environment variables that are handled the same (confusing) way right now.
On Tuesday 12 July 2022 17:39:06 Tom Rini wrote:
On Tue, Jul 12, 2022 at 10:11:00AM +0200, Pali Rohár wrote:
On Monday 11 July 2022 19:23:26 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
configs/nokia_rx51_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/nokia_rx51_defconfig b/configs/nokia_rx51_defconfig index 309cf28269c1..46b794f168d9 100644 --- a/configs/nokia_rx51_defconfig +++ b/configs/nokia_rx51_defconfig @@ -24,7 +24,6 @@ CONFIG_AUTOBOOT_MENU_SHOW=y CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run sdboot;run emmcboot;run attachboot;echo" CONFIG_USE_PREBOOT=y -CONFIG_PREBOOT="run preboot" # CONFIG_SYS_DEVICE_NULLDEV is not set CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Nokia RX-51 # "
These changes are actually a bit puzzling. There are other platforms that set preboot in their default environment, rather than via CONFIG_PREBOOT, and their final value ends up being the one set in CONFIG_EXTRA_ENV_SETTINGS rather than the empty string that CONFIG_PREBOOT is. I assume you've confirmed that at run-time you end up with preboot="run preboot" being set, and not preboot="long command" ?
At nokia n900 runtime is always "preboot=long command" and not "preboot=run preboot". It also was before these changes.
OK, so then we really just need this patch, and not the other.
Note that this is observation and all automated tests depend on this behavior. So if this behavior somehow change (e.g. by changing implementation or hacking LTO to change order, or etc...) then tests start failing and you would see it in failed CI. I hope that n900 test is still running in CI and in case of issues somebody inform me about it...
PATCH 1/2 avoids inclusion of "preboot=CONFIG_PREBOOT" into default environment when CONFIG_PREBOOT is not defined in defconfig file. And PATCH 2/2 then unsets CONFIG_PREBOOT from nokia defconfig file.
And long term figure out how to better handle this along with the other half-dozen places the user may want to configure the default environment a bit more, on top of what's already provided. There are a handful of other environment variables that are handled the same (confusing) way right now.
-- Tom
Apparently this problem appeared by converting config.h options into Kconfig. And continue to grow by conversion of new and new options.
Really Kconfig is not ideal tool for generating environment variables.
For all this stuff is needed some stronger tool/language than Kconfig, e.g. C preprocessor (like it was before in config.h) or any similar stronger macro language (e.g. m4) or script languages (shell / python).
On Tue, Jul 12, 2022 at 11:52:45PM +0200, Pali Rohár wrote:
On Tuesday 12 July 2022 17:39:06 Tom Rini wrote:
On Tue, Jul 12, 2022 at 10:11:00AM +0200, Pali Rohár wrote:
On Monday 11 July 2022 19:23:26 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
configs/nokia_rx51_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/nokia_rx51_defconfig b/configs/nokia_rx51_defconfig index 309cf28269c1..46b794f168d9 100644 --- a/configs/nokia_rx51_defconfig +++ b/configs/nokia_rx51_defconfig @@ -24,7 +24,6 @@ CONFIG_AUTOBOOT_MENU_SHOW=y CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run sdboot;run emmcboot;run attachboot;echo" CONFIG_USE_PREBOOT=y -CONFIG_PREBOOT="run preboot" # CONFIG_SYS_DEVICE_NULLDEV is not set CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Nokia RX-51 # "
These changes are actually a bit puzzling. There are other platforms that set preboot in their default environment, rather than via CONFIG_PREBOOT, and their final value ends up being the one set in CONFIG_EXTRA_ENV_SETTINGS rather than the empty string that CONFIG_PREBOOT is. I assume you've confirmed that at run-time you end up with preboot="run preboot" being set, and not preboot="long command" ?
At nokia n900 runtime is always "preboot=long command" and not "preboot=run preboot". It also was before these changes.
OK, so then we really just need this patch, and not the other.
Note that this is observation and all automated tests depend on this behavior. So if this behavior somehow change (e.g. by changing implementation or hacking LTO to change order, or etc...) then tests
Yes, and n900 is not the only platform and preboot is not the only environment variable that has this issue.
start failing and you would see it in failed CI. I hope that n900 test is still running in CI and in case of issues somebody inform me about it...
I certainly hope it's still doing useful things in CI, but if it doesn't fail when it should fail, I won't know. On the other hand, if it does fail then that means whatever broke has to be fixed.
PATCH 1/2 avoids inclusion of "preboot=CONFIG_PREBOOT" into default environment when CONFIG_PREBOOT is not defined in defconfig file. And
Right, but it doesn't functionally change anything as preboot is set right on this and other platforms all the same. And we would want a similar patch for other variables that have the same issue too. PREBOOT isn't special in this regard.
PATCH 2/2 then unsets CONFIG_PREBOOT from nokia defconfig file.
And long term figure out how to better handle this along with the other half-dozen places the user may want to configure the default environment a bit more, on top of what's already provided. There are a handful of other environment variables that are handled the same (confusing) way right now.
-- Tom
Apparently this problem appeared by converting config.h options into Kconfig. And continue to grow by conversion of new and new options.
I prefer to look at it as a problem shown by Kconfig that a number of things are both "set CONFIG_FOO to enable it as a boolean" and "set CONFIG_FOO to some non-empty value that will be used" rather than as two options, one as a bool and one with the value.
Really Kconfig is not ideal tool for generating environment variables.
It really isn't, that's why the plain text environment stuff is a good step in the right direction. What I'm not sure about how to best handle is making it easy for a user, rather than developer, to override environment values too. What I mean he
For all this stuff is needed some stronger tool/language than Kconfig, e.g. C preprocessor (like it was before in config.h) or any similar stronger macro language (e.g. m4) or script languages (shell / python).
Yes, so, take a look at 5c3f6a320678d64c9fa0c42755488822a033b567 as an example of moving a board away from board config.h and to something else. What I'm not sure on is how to best handle preboot in this case as I'm not quite liking "Enable CONFIG_PREBOOT and then edit .../boardname.env to set preboot to the right value".
On Tuesday 12 July 2022 18:58:31 Tom Rini wrote:
On Tue, Jul 12, 2022 at 11:52:45PM +0200, Pali Rohár wrote:
On Tuesday 12 July 2022 17:39:06 Tom Rini wrote:
On Tue, Jul 12, 2022 at 10:11:00AM +0200, Pali Rohár wrote:
On Monday 11 July 2022 19:23:26 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
configs/nokia_rx51_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/nokia_rx51_defconfig b/configs/nokia_rx51_defconfig index 309cf28269c1..46b794f168d9 100644 --- a/configs/nokia_rx51_defconfig +++ b/configs/nokia_rx51_defconfig @@ -24,7 +24,6 @@ CONFIG_AUTOBOOT_MENU_SHOW=y CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run sdboot;run emmcboot;run attachboot;echo" CONFIG_USE_PREBOOT=y -CONFIG_PREBOOT="run preboot" # CONFIG_SYS_DEVICE_NULLDEV is not set CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="Nokia RX-51 # "
These changes are actually a bit puzzling. There are other platforms that set preboot in their default environment, rather than via CONFIG_PREBOOT, and their final value ends up being the one set in CONFIG_EXTRA_ENV_SETTINGS rather than the empty string that CONFIG_PREBOOT is. I assume you've confirmed that at run-time you end up with preboot="run preboot" being set, and not preboot="long command" ?
At nokia n900 runtime is always "preboot=long command" and not "preboot=run preboot". It also was before these changes.
OK, so then we really just need this patch, and not the other.
Note that this is observation and all automated tests depend on this behavior. So if this behavior somehow change (e.g. by changing implementation or hacking LTO to change order, or etc...) then tests
Yes, and n900 is not the only platform and preboot is not the only environment variable that has this issue.
start failing and you would see it in failed CI. I hope that n900 test is still running in CI and in case of issues somebody inform me about it...
I certainly hope it's still doing useful things in CI, but if it doesn't fail when it should fail, I won't know. On the other hand, if it does fail then that means whatever broke has to be fixed.
PATCH 1/2 avoids inclusion of "preboot=CONFIG_PREBOOT" into default environment when CONFIG_PREBOOT is not defined in defconfig file. And
Right, but it doesn't functionally change anything as preboot is set right on this and other platforms all the same. And we would want a similar patch for other variables that have the same issue too. PREBOOT isn't special in this regard.
You are right.
PATCH 2/2 then unsets CONFIG_PREBOOT from nokia defconfig file.
And long term figure out how to better handle this along with the other half-dozen places the user may want to configure the default environment a bit more, on top of what's already provided. There are a handful of other environment variables that are handled the same (confusing) way right now.
-- Tom
Apparently this problem appeared by converting config.h options into Kconfig. And continue to grow by conversion of new and new options.
I prefer to look at it as a problem shown by Kconfig that a number of things are both "set CONFIG_FOO to enable it as a boolean" and "set CONFIG_FOO to some non-empty value that will be used" rather than as two options, one as a bool and one with the value.
Yes, I agree.
Really Kconfig is not ideal tool for generating environment variables.
It really isn't, that's why the plain text environment stuff is a good step in the right direction. What I'm not sure about how to best handle is making it easy for a user, rather than developer, to override environment values too. What I mean he
Maybe the important question is: Why is needed CONFIG_PREBOOT define at all? Why it is not enough just to set preboot= env variable? And same questions for all other CONFIG_<name_of_env> options.
For all this stuff is needed some stronger tool/language than Kconfig, e.g. C preprocessor (like it was before in config.h) or any similar stronger macro language (e.g. m4) or script languages (shell / python).
Yes, so, take a look at 5c3f6a320678d64c9fa0c42755488822a033b567 as an example of moving a board away from board config.h and to something else. What I'm not sure on is how to best handle preboot in this case as I'm not quite liking "Enable CONFIG_PREBOOT and then edit .../boardname.env to set preboot to the right value".
-- Tom
With that commit, all env variables are moved into separate file. This file can be parsed by other tools and at compile it is possible to extract current value of preboot= env variable or check if preboot= is defined. Cannot be by this logic automatically defined/expanded CONFIG_PREBOOT symbol? Of course, it would not be Kconfig symbol anymore.
On Wed, Jul 13, 2022 at 01:11:35AM +0200, Pali Rohár wrote:
On Tuesday 12 July 2022 18:58:31 Tom Rini wrote:
[snip]
It really isn't, that's why the plain text environment stuff is a good step in the right direction. What I'm not sure about how to best handle is making it easy for a user, rather than developer, to override environment values too. What I mean he
Maybe the important question is: Why is needed CONFIG_PREBOOT define at all? Why it is not enough just to set preboot= env variable? And same questions for all other CONFIG_<name_of_env> options.
I'm sorry if this sounds snarky, I don't intend it to. But, size. Maybe that doesn't matter so much these days for the size savings on run_preboot_environment_command().
For all this stuff is needed some stronger tool/language than Kconfig, e.g. C preprocessor (like it was before in config.h) or any similar stronger macro language (e.g. m4) or script languages (shell / python).
Yes, so, take a look at 5c3f6a320678d64c9fa0c42755488822a033b567 as an example of moving a board away from board config.h and to something else. What I'm not sure on is how to best handle preboot in this case as I'm not quite liking "Enable CONFIG_PREBOOT and then edit .../boardname.env to set preboot to the right value".
-- Tom
With that commit, all env variables are moved into separate file. This file can be parsed by other tools and at compile it is possible to extract current value of preboot= env variable or check if preboot= is defined. Cannot be by this logic automatically defined/expanded CONFIG_PREBOOT symbol? Of course, it would not be Kconfig symbol anymore.
That sounds interesting.
On Tuesday 12 July 2022 19:15:45 Tom Rini wrote:
On Wed, Jul 13, 2022 at 01:11:35AM +0200, Pali Rohár wrote:
On Tuesday 12 July 2022 18:58:31 Tom Rini wrote:
[snip]
It really isn't, that's why the plain text environment stuff is a good step in the right direction. What I'm not sure about how to best handle is making it easy for a user, rather than developer, to override environment values too. What I mean he
Maybe the important question is: Why is needed CONFIG_PREBOOT define at all? Why it is not enough just to set preboot= env variable? And same questions for all other CONFIG_<name_of_env> options.
I'm sorry if this sounds snarky, I don't intend it to. But, size. Maybe that doesn't matter so much these days for the size savings on run_preboot_environment_command().
Size is really a good argument.
For all this stuff is needed some stronger tool/language than Kconfig, e.g. C preprocessor (like it was before in config.h) or any similar stronger macro language (e.g. m4) or script languages (shell / python).
Yes, so, take a look at 5c3f6a320678d64c9fa0c42755488822a033b567 as an example of moving a board away from board config.h and to something else. What I'm not sure on is how to best handle preboot in this case as I'm not quite liking "Enable CONFIG_PREBOOT and then edit .../boardname.env to set preboot to the right value".
-- Tom
With that commit, all env variables are moved into separate file. This file can be parsed by other tools and at compile it is possible to extract current value of preboot= env variable or check if preboot= is defined. Cannot be by this logic automatically defined/expanded CONFIG_PREBOOT symbol? Of course, it would not be Kconfig symbol anymore.
That sounds interesting.
-- Tom
And this solve also issue with size (if we can determinate at compile time if preboot= env is defined or not).
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
Applied to u-boot/master, thanks!
On Monday 25 July 2022 17:21:00 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
Applied to u-boot/master, thanks!
-- Tom
Patch 1/2 is missing.
On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote:
On Monday 25 July 2022 17:21:00 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
Applied to u-boot/master, thanks!
Patch 1/2 is missing.
Sorry if I was unclear enough in the rest of the emails, I'm not applying 1/2. The empty string is fine enough (again, other variables have this issue). Unless I missed something and there's a run-time failure that 1/2 resolves?
On Wednesday 27 July 2022 14:48:23 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote:
On Monday 25 July 2022 17:21:00 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
Applied to u-boot/master, thanks!
Patch 1/2 is missing.
Sorry if I was unclear enough in the rest of the emails, I'm not applying 1/2. The empty string is fine enough (again, other variables have this issue). Unless I missed something and there's a run-time failure that 1/2 resolves?
The issue is that preboot= variable is defined in ENV array two times. Once in Kconfig and second time in regular environment define.
It is just coincidence that the definition from Kconfig is ignored because currently it is put _after_ the regular preboot= definition. But this is fragile and probably some unrelated change/patch could change this behavior.
We really do not want two preboot= definitions in ENV array and pray that the correct one would be used by U-Boot at runtime.
On Wed, Jul 27, 2022 at 08:52:01PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:48:23 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote:
On Monday 25 July 2022 17:21:00 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
Applied to u-boot/master, thanks!
Patch 1/2 is missing.
Sorry if I was unclear enough in the rest of the emails, I'm not applying 1/2. The empty string is fine enough (again, other variables have this issue). Unless I missed something and there's a run-time failure that 1/2 resolves?
The issue is that preboot= variable is defined in ENV array two times. Once in Kconfig and second time in regular environment define.
It is just coincidence that the definition from Kconfig is ignored because currently it is put _after_ the regular preboot= definition. But this is fragile and probably some unrelated change/patch could change this behavior.
We really do not want two preboot= definitions in ENV array and pray that the correct one would be used by U-Boot at runtime.
It's not coincidence, it's down to link order. So it's not going to break anytime soon. And it's not just preboot that's in this situation.
A more generic approach to deal with this would be appreciated.
On Wednesday 27 July 2022 14:58:20 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:52:01PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:48:23 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote:
On Monday 25 July 2022 17:21:00 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does not do anything useful (it is infinite recursion). Config file for this board already contains default preboot= env variable with correct value, which has higher priority than CONFIG_PREBOOT and this is reason why nonsense CONFIG_PREBOOT is ignored.
Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file.
Signed-off-by: Pali Rohár pali@kernel.org
Applied to u-boot/master, thanks!
Patch 1/2 is missing.
Sorry if I was unclear enough in the rest of the emails, I'm not applying 1/2. The empty string is fine enough (again, other variables have this issue). Unless I missed something and there's a run-time failure that 1/2 resolves?
The issue is that preboot= variable is defined in ENV array two times. Once in Kconfig and second time in regular environment define.
It is just coincidence that the definition from Kconfig is ignored because currently it is put _after_ the regular preboot= definition. But this is fragile and probably some unrelated change/patch could change this behavior.
We really do not want two preboot= definitions in ENV array and pray that the correct one would be used by U-Boot at runtime.
It's not coincidence, it's down to link order. So it's not going to break anytime soon. And it's not just preboot that's in this situation.
It is not linker order, but rather order in which are specified preprocessor macros...
And what else is in the same situation as preboot? I just do not see other variable which is defined two times.
A more generic approach to deal with this would be appreciated.
-- Tom
On Wed, Jul 27, 2022 at 09:01:15PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:58:20 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:52:01PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:48:23 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote:
On Monday 25 July 2022 17:21:00 Tom Rini wrote:
On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote:
> CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. > Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does > not do anything useful (it is infinite recursion). Config file for this > board already contains default preboot= env variable with correct value, > which has higher priority than CONFIG_PREBOOT and this is reason why > nonsense CONFIG_PREBOOT is ignored. > > Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file. > > Signed-off-by: Pali Rohár pali@kernel.org
Applied to u-boot/master, thanks!
Patch 1/2 is missing.
Sorry if I was unclear enough in the rest of the emails, I'm not applying 1/2. The empty string is fine enough (again, other variables have this issue). Unless I missed something and there's a run-time failure that 1/2 resolves?
The issue is that preboot= variable is defined in ENV array two times. Once in Kconfig and second time in regular environment define.
It is just coincidence that the definition from Kconfig is ignored because currently it is put _after_ the regular preboot= definition. But this is fragile and probably some unrelated change/patch could change this behavior.
We really do not want two preboot= definitions in ENV array and pray that the correct one would be used by U-Boot at runtime.
It's not coincidence, it's down to link order. So it's not going to break anytime soon. And it's not just preboot that's in this situation.
It is not linker order, but rather order in which are specified preprocessor macros...
Ah right, sorry, I was thinking of something else I found when doing CONFIG_EXTRA_ENV_TEXT / CONFIG_EXTRA_ENV_SETTINGS stuff.
And what else is in the same situation as preboot? I just do not see other variable which is defined two times.
Right so it's all from include/env_default.h yes? CONFIG_EXTRA_ENV_TEXT and then CONFIG_EXTRA_ENV_SETTINGS win. And pretty much everything else in that file that has a CONFIG knob to it either has a different bit of Kconfig "fun" to avoid the situation PREBOOT has or is already in the same boat.
A more generic approach to deal with this would be appreciated.
Which is why I said this. How we're doing this right now isn't great and isn't super user friendly. I don't want to have to repeat the logic you do in 1/2 for all of the CONFIG options in env_default.h that aren't already converted to Kconfig.
On Wednesday 27 July 2022 15:08:05 Tom Rini wrote:
On Wed, Jul 27, 2022 at 09:01:15PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:58:20 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:52:01PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:48:23 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote:
On Monday 25 July 2022 17:21:00 Tom Rini wrote: > On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote: > > > CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. > > Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does > > not do anything useful (it is infinite recursion). Config file for this > > board already contains default preboot= env variable with correct value, > > which has higher priority than CONFIG_PREBOOT and this is reason why > > nonsense CONFIG_PREBOOT is ignored. > > > > Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file. > > > > Signed-off-by: Pali Rohár pali@kernel.org > > Applied to u-boot/master, thanks!
Patch 1/2 is missing.
Sorry if I was unclear enough in the rest of the emails, I'm not applying 1/2. The empty string is fine enough (again, other variables have this issue). Unless I missed something and there's a run-time failure that 1/2 resolves?
The issue is that preboot= variable is defined in ENV array two times. Once in Kconfig and second time in regular environment define.
It is just coincidence that the definition from Kconfig is ignored because currently it is put _after_ the regular preboot= definition. But this is fragile and probably some unrelated change/patch could change this behavior.
We really do not want two preboot= definitions in ENV array and pray that the correct one would be used by U-Boot at runtime.
It's not coincidence, it's down to link order. So it's not going to break anytime soon. And it's not just preboot that's in this situation.
It is not linker order, but rather order in which are specified preprocessor macros...
Ah right, sorry, I was thinking of something else I found when doing CONFIG_EXTRA_ENV_TEXT / CONFIG_EXTRA_ENV_SETTINGS stuff.
And what else is in the same situation as preboot? I just do not see other variable which is defined two times.
Right so it's all from include/env_default.h yes? CONFIG_EXTRA_ENV_TEXT and then CONFIG_EXTRA_ENV_SETTINGS win. And pretty much everything else in that file that has a CONFIG knob to it either has a different bit of Kconfig "fun" to avoid the situation PREBOOT has or is already in the same boat.
A more generic approach to deal with this would be appreciated.
Which is why I said this. How we're doing this right now isn't great and isn't super user friendly. I don't want to have to repeat the logic you do in 1/2 for all of the CONFIG options in env_default.h that aren't already converted to Kconfig.
But this is how it is expressed in Kconfig (for string options). For example in kernel there was recently fixes for specifying -mcpu powerpc gcc option via Kconfig CONFIG_TARGET_CPU string option and there is also corresponding Kconfig CONFIG_TARGET_CPU_BOOL option which has same meaning as my *_DEFINED option. If you do not want to do it then do not use Kconfig for those options. Generic better approach is to move these settings out of the Kconfig, but this is something which you do not want to do it. It is pretty simple, if you are going to use tool which is not ideal or designed for some purpose then you need to deal with it. I just described one example which I saw recently used in kernel.
In my opinion, options which use, call or contains parts of u-boot script should _not_ be in Kconfig; but rather in CONFIG_EXTRA_ENV_SETTINGS because it refers directly to other "run" variables defined in that CONFIG_EXTRA_ENV_SETTINGS.
On Sunday 09 October 2022 15:03:03 Pali Rohár wrote:
On Wednesday 27 July 2022 15:08:05 Tom Rini wrote:
On Wed, Jul 27, 2022 at 09:01:15PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:58:20 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:52:01PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:48:23 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote: > On Monday 25 July 2022 17:21:00 Tom Rini wrote: > > On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote: > > > > > CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. > > > Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does > > > not do anything useful (it is infinite recursion). Config file for this > > > board already contains default preboot= env variable with correct value, > > > which has higher priority than CONFIG_PREBOOT and this is reason why > > > nonsense CONFIG_PREBOOT is ignored. > > > > > > Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file. > > > > > > Signed-off-by: Pali Rohár pali@kernel.org > > > > Applied to u-boot/master, thanks! > > Patch 1/2 is missing.
Sorry if I was unclear enough in the rest of the emails, I'm not applying 1/2. The empty string is fine enough (again, other variables have this issue). Unless I missed something and there's a run-time failure that 1/2 resolves?
The issue is that preboot= variable is defined in ENV array two times. Once in Kconfig and second time in regular environment define.
It is just coincidence that the definition from Kconfig is ignored because currently it is put _after_ the regular preboot= definition. But this is fragile and probably some unrelated change/patch could change this behavior.
We really do not want two preboot= definitions in ENV array and pray that the correct one would be used by U-Boot at runtime.
It's not coincidence, it's down to link order. So it's not going to break anytime soon. And it's not just preboot that's in this situation.
It is not linker order, but rather order in which are specified preprocessor macros...
Ah right, sorry, I was thinking of something else I found when doing CONFIG_EXTRA_ENV_TEXT / CONFIG_EXTRA_ENV_SETTINGS stuff.
And what else is in the same situation as preboot? I just do not see other variable which is defined two times.
Right so it's all from include/env_default.h yes? CONFIG_EXTRA_ENV_TEXT and then CONFIG_EXTRA_ENV_SETTINGS win. And pretty much everything else in that file that has a CONFIG knob to it either has a different bit of Kconfig "fun" to avoid the situation PREBOOT has or is already in the same boat.
A more generic approach to deal with this would be appreciated.
Which is why I said this. How we're doing this right now isn't great and isn't super user friendly. I don't want to have to repeat the logic you do in 1/2 for all of the CONFIG options in env_default.h that aren't already converted to Kconfig.
But this is how it is expressed in Kconfig (for string options). For example in kernel there was recently fixes for specifying -mcpu powerpc gcc option via Kconfig CONFIG_TARGET_CPU string option and there is also corresponding Kconfig CONFIG_TARGET_CPU_BOOL option which has same meaning as my *_DEFINED option. If you do not want to do it then do not use Kconfig for those options. Generic better approach is to move these settings out of the Kconfig, but this is something which you do not want to do it. It is pretty simple, if you are going to use tool which is not ideal or designed for some purpose then you need to deal with it. I just described one example which I saw recently used in kernel.
In my opinion, options which use, call or contains parts of u-boot script should _not_ be in Kconfig; but rather in CONFIG_EXTRA_ENV_SETTINGS because it refers directly to other "run" variables defined in that CONFIG_EXTRA_ENV_SETTINGS.
Any comments here?
On Tue, Nov 01, 2022 at 11:58:48PM +0100, Pali Rohár wrote:
On Sunday 09 October 2022 15:03:03 Pali Rohár wrote:
On Wednesday 27 July 2022 15:08:05 Tom Rini wrote:
On Wed, Jul 27, 2022 at 09:01:15PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:58:20 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:52:01PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:48:23 Tom Rini wrote: > On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote: > > On Monday 25 July 2022 17:21:00 Tom Rini wrote: > > > On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote: > > > > > > > CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. > > > > Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does > > > > not do anything useful (it is infinite recursion). Config file for this > > > > board already contains default preboot= env variable with correct value, > > > > which has higher priority than CONFIG_PREBOOT and this is reason why > > > > nonsense CONFIG_PREBOOT is ignored. > > > > > > > > Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file. > > > > > > > > Signed-off-by: Pali Rohár pali@kernel.org > > > > > > Applied to u-boot/master, thanks! > > > > Patch 1/2 is missing. > > Sorry if I was unclear enough in the rest of the emails, I'm not > applying 1/2. The empty string is fine enough (again, other variables > have this issue). Unless I missed something and there's a run-time > failure that 1/2 resolves?
The issue is that preboot= variable is defined in ENV array two times. Once in Kconfig and second time in regular environment define.
It is just coincidence that the definition from Kconfig is ignored because currently it is put _after_ the regular preboot= definition. But this is fragile and probably some unrelated change/patch could change this behavior.
We really do not want two preboot= definitions in ENV array and pray that the correct one would be used by U-Boot at runtime.
It's not coincidence, it's down to link order. So it's not going to break anytime soon. And it's not just preboot that's in this situation.
It is not linker order, but rather order in which are specified preprocessor macros...
Ah right, sorry, I was thinking of something else I found when doing CONFIG_EXTRA_ENV_TEXT / CONFIG_EXTRA_ENV_SETTINGS stuff.
And what else is in the same situation as preboot? I just do not see other variable which is defined two times.
Right so it's all from include/env_default.h yes? CONFIG_EXTRA_ENV_TEXT and then CONFIG_EXTRA_ENV_SETTINGS win. And pretty much everything else in that file that has a CONFIG knob to it either has a different bit of Kconfig "fun" to avoid the situation PREBOOT has or is already in the same boat.
A more generic approach to deal with this would be appreciated.
Which is why I said this. How we're doing this right now isn't great and isn't super user friendly. I don't want to have to repeat the logic you do in 1/2 for all of the CONFIG options in env_default.h that aren't already converted to Kconfig.
But this is how it is expressed in Kconfig (for string options). For example in kernel there was recently fixes for specifying -mcpu powerpc gcc option via Kconfig CONFIG_TARGET_CPU string option and there is also corresponding Kconfig CONFIG_TARGET_CPU_BOOL option which has same meaning as my *_DEFINED option. If you do not want to do it then do not use Kconfig for those options. Generic better approach is to move these settings out of the Kconfig, but this is something which you do not want to do it. It is pretty simple, if you are going to use tool which is not ideal or designed for some purpose then you need to deal with it. I just described one example which I saw recently used in kernel.
In my opinion, options which use, call or contains parts of u-boot script should _not_ be in Kconfig; but rather in CONFIG_EXTRA_ENV_SETTINGS because it refers directly to other "run" variables defined in that CONFIG_EXTRA_ENV_SETTINGS.
Any comments here?
Yes, what I thought I had said before either above, or in the other thread. The definition of the preboot variable belongs in the text based environment file(s) and not in Kconfig. We no longer have boards defining the value in BOTH Kconfig and their board.h files, which was the problem.
On Tuesday 01 November 2022 19:29:01 Tom Rini wrote:
On Tue, Nov 01, 2022 at 11:58:48PM +0100, Pali Rohár wrote:
On Sunday 09 October 2022 15:03:03 Pali Rohár wrote:
On Wednesday 27 July 2022 15:08:05 Tom Rini wrote:
On Wed, Jul 27, 2022 at 09:01:15PM +0200, Pali Rohár wrote:
On Wednesday 27 July 2022 14:58:20 Tom Rini wrote:
On Wed, Jul 27, 2022 at 08:52:01PM +0200, Pali Rohár wrote: > On Wednesday 27 July 2022 14:48:23 Tom Rini wrote: > > On Wed, Jul 27, 2022 at 08:34:41PM +0200, Pali Rohár wrote: > > > On Monday 25 July 2022 17:21:00 Tom Rini wrote: > > > > On Sun, Jul 10, 2022 at 01:42:56PM +0200, Pali Rohár wrote: > > > > > > > > > CONFIG_PREBOOT just cause putting "preboot=CONFIG_PREBOOT" into env list. > > > > > Value CONFIG_PREBOOT="run preboot" in defconfig is just nonsense and does > > > > > not do anything useful (it is infinite recursion). Config file for this > > > > > board already contains default preboot= env variable with correct value, > > > > > which has higher priority than CONFIG_PREBOOT and this is reason why > > > > > nonsense CONFIG_PREBOOT is ignored. > > > > > > > > > > Remove nonsense and unused CONFIG_PREBOOT from nokia_rx51_defconfig file. > > > > > > > > > > Signed-off-by: Pali Rohár pali@kernel.org > > > > > > > > Applied to u-boot/master, thanks! > > > > > > Patch 1/2 is missing. > > > > Sorry if I was unclear enough in the rest of the emails, I'm not > > applying 1/2. The empty string is fine enough (again, other variables > > have this issue). Unless I missed something and there's a run-time > > failure that 1/2 resolves? > > The issue is that preboot= variable is defined in ENV array two times. > Once in Kconfig and second time in regular environment define. > > It is just coincidence that the definition from Kconfig is ignored > because currently it is put _after_ the regular preboot= definition. But > this is fragile and probably some unrelated change/patch could change > this behavior. > > We really do not want two preboot= definitions in ENV array and pray > that the correct one would be used by U-Boot at runtime.
It's not coincidence, it's down to link order. So it's not going to break anytime soon. And it's not just preboot that's in this situation.
It is not linker order, but rather order in which are specified preprocessor macros...
Ah right, sorry, I was thinking of something else I found when doing CONFIG_EXTRA_ENV_TEXT / CONFIG_EXTRA_ENV_SETTINGS stuff.
And what else is in the same situation as preboot? I just do not see other variable which is defined two times.
Right so it's all from include/env_default.h yes? CONFIG_EXTRA_ENV_TEXT and then CONFIG_EXTRA_ENV_SETTINGS win. And pretty much everything else in that file that has a CONFIG knob to it either has a different bit of Kconfig "fun" to avoid the situation PREBOOT has or is already in the same boat.
A more generic approach to deal with this would be appreciated.
Which is why I said this. How we're doing this right now isn't great and isn't super user friendly. I don't want to have to repeat the logic you do in 1/2 for all of the CONFIG options in env_default.h that aren't already converted to Kconfig.
But this is how it is expressed in Kconfig (for string options). For example in kernel there was recently fixes for specifying -mcpu powerpc gcc option via Kconfig CONFIG_TARGET_CPU string option and there is also corresponding Kconfig CONFIG_TARGET_CPU_BOOL option which has same meaning as my *_DEFINED option. If you do not want to do it then do not use Kconfig for those options. Generic better approach is to move these settings out of the Kconfig, but this is something which you do not want to do it. It is pretty simple, if you are going to use tool which is not ideal or designed for some purpose then you need to deal with it. I just described one example which I saw recently used in kernel.
In my opinion, options which use, call or contains parts of u-boot script should _not_ be in Kconfig; but rather in CONFIG_EXTRA_ENV_SETTINGS because it refers directly to other "run" variables defined in that CONFIG_EXTRA_ENV_SETTINGS.
Any comments here?
Yes, what I thought I had said before either above, or in the other thread. The definition of the preboot variable belongs in the text based environment file(s) and not in Kconfig. We no longer have boards defining the value in BOTH Kconfig and their board.h files, which was the problem.
Well, for N900 problem is still there with u-boot master branch. preboot variable is defined two times in default env array and U-Boot on serial console prints following error:
DELETE CANDIDATE: "preboot" hdelete: DELETE key "preboot" DELETE ERROR ##############################
After applying patch 1/2 this error disappear.
Hi Pali,
On Sun, 10 Jul 2022 at 05:43, Pali Rohár pali@kernel.org wrote:
Due to usage of PREBOOT in Kconfig, macro CONFIG_PREBOOT is always defined when CONFIG_USE_PREBOOT is enabled. In case CONFIG_PREBOOT is not explicitly enabled it is set to empty C string and therefore '#ifdef CONFIG_PREBOOT' guard does not work. Fix this issue by introducing a new Kconfig symbol PREBOOT_DEFINED which cause to define new C macro CONFIG_PREBOOT_DEFINED only when CONFIG_PREBOOT is really defined.
Change usage of '#ifdef CONFIG_PREBOOT' by '#ifdef CONFIG_USE_PREBOOT' for code which checks if preboot code would be called and by '#ifdef CONFIG_PREBOOT_DEFINED' for defining preboot code.
Signed-off-by: Pali Rohár pali@kernel.org
board/boundary/nitrogen6x/nitrogen6x.c | 4 ++-- boot/Kconfig | 4 ++++ include/env_default.h | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-)
Can you not use:
#idef CONFIG_USE_PREBOOT
?
You should not be checking for the existence of a string Kconfig.
Regards, Simon
On Tuesday 12 July 2022 04:58:51 Simon Glass wrote:
Hi Pali,
On Sun, 10 Jul 2022 at 05:43, Pali Rohár pali@kernel.org wrote:
Due to usage of PREBOOT in Kconfig, macro CONFIG_PREBOOT is always defined when CONFIG_USE_PREBOOT is enabled. In case CONFIG_PREBOOT is not explicitly enabled it is set to empty C string and therefore '#ifdef CONFIG_PREBOOT' guard does not work. Fix this issue by introducing a new Kconfig symbol PREBOOT_DEFINED which cause to define new C macro CONFIG_PREBOOT_DEFINED only when CONFIG_PREBOOT is really defined.
Change usage of '#ifdef CONFIG_PREBOOT' by '#ifdef CONFIG_USE_PREBOOT' for code which checks if preboot code would be called and by '#ifdef CONFIG_PREBOOT_DEFINED' for defining preboot code.
Signed-off-by: Pali Rohár pali@kernel.org
board/boundary/nitrogen6x/nitrogen6x.c | 4 ++-- boot/Kconfig | 4 ++++ include/env_default.h | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-)
Can you not use:
#idef CONFIG_USE_PREBOOT
?
Where?
You should not be checking for the existence of a string Kconfig.
I do not see other option, because this is how kconfig is working. When string option is not set then kconfig defines it to empty string.
Regards, Simon
On Sun, Jul 10, 2022 at 01:42:55PM +0200, Pali Rohár wrote:
Due to usage of PREBOOT in Kconfig, macro CONFIG_PREBOOT is always defined when CONFIG_USE_PREBOOT is enabled. In case CONFIG_PREBOOT is not explicitly enabled it is set to empty C string and therefore '#ifdef CONFIG_PREBOOT' guard does not work. Fix this issue by introducing a new Kconfig symbol PREBOOT_DEFINED which cause to define new C macro CONFIG_PREBOOT_DEFINED only when CONFIG_PREBOOT is really defined.
Change usage of '#ifdef CONFIG_PREBOOT' by '#ifdef CONFIG_USE_PREBOOT' for code which checks if preboot code would be called and by '#ifdef CONFIG_PREBOOT_DEFINED' for defining preboot code.
Signed-off-by: Pali Rohár pali@kernel.org
For the record, we need to long term figure out a better solution to user configurable default environment stuff. Doing this via Kconfig is fragile / problematic. However, as this patch does solve a real problem, applied to u-boot/master, thanks!
participants (3)
-
Pali Rohár -
Simon Glass -
Tom Rini