[U-Boot] a couple questions about "CONFIG_*" variables and the whitelist

[Masahiro Yamada]

Hi.


2017-01-04 22:55 GMT+09:00 Robert P. J. Day <rpjday at crashcourse.ca>:
>
>   more random questions about stuff ... i see in the top-level README
> the explanation:
>
>   "There are two classes of configuration variables:
>
>   * Configuration _OPTIONS_:
>     These are selectable by the user and have names beginning with
>     "CONFIG_".
>
>   * Configuration _SETTINGS_:
>     These depend on the hardware etc. and should not be meddled with if
>     you don't know what you're doing; they have names beginning with
>     "CONFIG_SYS_".
>
> so, perhaps put another way, regular CONFIG_ variables represent
> things that a user has the right to configure to affect the end
> result, while CONFIG_SYS_ variables represent values tied somehow to
> the underlying hardware that u-boot *needs* to know to function
> properly on that hardware? is that a remotely accurate description?
>
>   but that's not a really hard-and-fast rule, is it? as one example
> from cmd/Kconfig:
>
>   config SYS_PROMPT
>         string "Shell prompt"
>         default "=> "
>         help
>           This string is displayed in the command line to the left of the
>           cursor.
>
> obviously, the user has the right to set the prompt to whatever he or
> she wants, and i'm sure there are other examples. so the distinction
> between those two classes of config variables is not as sharp as the
> README suggests, is it? or am i misreading the explanation?


In my opinion, CONFIG_SYS_ is not very successful.
As you notice, there are various examples of misnaming.
When I move CONFIG options to Kconfig, I sometimes try to fix it.

For example, the option below is an example of user-configurable option.

config HUSH_PARSER
        bool "Use hush shell"
        depends on CMDLINE

        help
          This option enables the "hush" shell (from Busybox) as command line
          interpreter, thus enabling powerful command line syntax like
          if...then...else...fi conditionals or `&&' and '||'
          constructs ("shell scripts").

          If disabled, you get the old, much simpler behaviour with a somewhat
          smaller memory footprint.


However, it was originally CONFIG_SYS_HUSH_PARSER.
(you will see it by "git checkout v2014.10 && git grep HUSH_PARSER")
So, I renamed it when moving it to Kconfig.

In my opinion, it is OK to rename options
because moving them to Kconfig is a big churn anyway.






>   second question -- i'm pretty sure the standard policy is that *any*
> config variables that start with "CONFIG_" are supposed to be defined
> somewhere in a Kconfig file, correct? given the history of u-boot,
> this clearly isn't true, but in a perfect world, it would be, yes? so
> that if one wanted to define some local macro names in a header file
> or something, it should never start with the prefix "CONFIG_" unless
> it comes from the Kbuild structure. at least that's the standard i
> remember from the linux kernel.
>
>   now the whitelist -- i'd never looked at it before, read the first
> part of scripts/build-whitelist.sh, so apparently, the whitelist
> represents all of the "CONFIG_"-prefixed variables found scattered
> throughout the code base, or constructed via CONFIG_SYS_EXTRA_OPTIONS.
> and i can see there are over 8,000 of them. yeesh.
>
>   however, given that CONFIG_SYS_EXTRA_OPTIONS is deprecated, and if
> one follows the rule i mentioned earlier (that CONFIG_ prefixed
> variables should always be defined in a Kconfig file), in a perfect
> world, that whitelist would be empty, would it not?

Right.  The whitelist should be empty in a perfect world.

Now, it is banned to add new CONFIG_ options to anywhere except Kconfig.
(That's why the whitelist.  We are allowed to define the innocent
options in headers during migration.  We are making effort
to decrease the options in the whitelist, in the end, make it empty.)


>   and, finally, i'm currently digging through the mtdparts code, and
> noticed that "CONFIG_MTDPARTS" is in the whitelist file, but there is
> no Kbuild entry "config MTDPARTS", so i did a quick grep to see:
>
> $ grep -r CONFIG_MTDPARTS *
> include/configs/sheevaplug.h:#define CONFIG_MTDPARTS            \
> include/configs/sheevaplug.h:   "=ttyS0,115200 mtdparts="CONFIG_MTDPARTS        \
> include/configs/sheevaplug.h:   "mtdparts="CONFIG_MTDPARTS
> include/configs/iconnect.h:#define CONFIG_MTDPARTS \
> include/configs/iconnect.h:     "mtdparts="CONFIG_MTDPARTS              \
> include/configs/guruplug.h:#define CONFIG_MTDPARTS      \
> include/configs/guruplug.h:     "mtdparts="CONFIG_MTDPARTS                                      \
> include/configs/guruplug.h:     "mtdparts="CONFIG_MTDPARTS
> include/configs/nsa310s.h:#define CONFIG_MTDPARTS \
> include/configs/nsa310s.h:      "mtdparts="CONFIG_MTDPARTS \
> include/configs/ib62x0.h:#define CONFIG_MTDPARTS \
> include/configs/ib62x0.h:       "mtdparts="CONFIG_MTDPARTS                                      \
> include/configs/dockstar.h:#define CONFIG_MTDPARTS              "mtdparts=orion_nand:1m(uboot),-(root)\0"
> include/configs/dockstar.h:     "mtdparts="CONFIG_MTDPARTS \
> include/configs/goflexhome.h:#define CONFIG_MTDPARTS \
> include/configs/goflexhome.h:   "mtdparts="CONFIG_MTDPARTS \
> scripts/config_whitelist.txt:CONFIG_MTDPARTS
> $
>
>   the above suggests that CONFIG_MTDPARTS is an entirely local
> variable in a small number of header files, whose scope is never seen
> outside each of those header files. in that case, it would seem
> inappropriate to have used that name for that variable, since it gives
> the impression of being part of the Kbuild structure. i'm not
> suggesting changing any of that (or maybe i am :-), it just seems
> silly that using that name causes that variable to end up in the
> whitelist file for no obvious reason.


There are 8000 options in the whitelist (=TODO list),
but I suspect it includes many false ones.
If you find CONFIG_ is inappropriate in the first place,
please fix the code (rename or delete, etc.) instead of moving as-is.


-- 
Best Regards
Masahiro Yamada