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