[PATCH 1/2] Fix usage of CONFIG_PREBOOT

[PATCH 1/2] Fix usage of CONFIG_PREBOOT

[Pali Rohár]

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

[PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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 # "
-- 
2.20.1

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2383 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5165 bytes --]

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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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.

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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.

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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.

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2035 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3215 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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.

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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?

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5110 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Nokia RX-51: Remove CONFIG_PREBOOT from defconfig

[Pali Rohár]

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.

Re: [PATCH 1/2] Fix usage of CONFIG_PREBOOT

[Simon Glass]

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

Re: [PATCH 1/2] Fix usage of CONFIG_PREBOOT

[Pali Rohár]

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

Re: [PATCH 1/2] Fix usage of CONFIG_PREBOOT

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]