[U-Boot] doing anything about "bad" Kbuild configuration options?

[Tom Rini]

On Fri, Apr 12, 2019 at 12:32:12PM -0400, Robert P. J. Day wrote:

> 
>   only a few months ago, i jumped into a discussion regarding Kbuild
> config options that were defined in some Kconfig* file, but were
> unused anywhere in the source tree:
> 
> http://u-boot.10912.n7.nabble.com/policy-regarding-unused-code-td351802.html#a351835
> 
>   many years ago, i wrote a script to locate such entries in the linux
> kernel source tree and, once u-boot adoopted the Kbuild build system,
> i could obviously use the same script on the u-boot source -- so far,
> so good. but i wrote some associated scripts to locate other oddities,
> and one of them was to find what i called "badref" options.
> 
>   now, "unused" options are typically not serious -- a Kbuild option
> is defined that is simply not tested anywhere in the source. normally,
> that's because someone removed some source and just forgot to remove
> the associated Kconfig option.
> 
>   "badref" options, on the other hand, are exactly the opposite --
> options that *are* tested somewhere in the code, but are not defined
> anywhere in a Kconfig file. in short, the code is testing some
> CONFIG_* option that will never, ever succeed as it isn't defined,
> which should typically be treated as a potentially more serious issue.
> 
>   i wrote up an example of this here:
> 
> https://crashcourse.ca/dokuwiki/doku.php?id=u-boot_badref
> 
> where you can see the result of running the script and asking it to
> scan the entire u-boot "drivers" directory once upon a time -- the
> output consists of a string for which the corresponding
> CONFIG_<string> is being tested somewhere in the argument directory,
> but for which there is no apparent Kconfig definition; the output for
> each string consists of a tree-wide search for that string.
> 
>   as an example, note the very first example found:
> 
> >>>>> ACX517AKN
> drivers/video/pxa_lcd.c:201:#ifdef CONFIG_ACX517AKN
> drivers/video/pxa_lcd.c:231:#endif /* CONFIG_ACX517AKN */
> drivers/video/pxa_lcd.c:297:#endif /* CONFIG_ACX517AKN */
> scripts/config_whitelist.txt:15:CONFIG_ACX517AKN
> 
> so CONFIG_ACX517AKN is clearly being tested, while there is no
> definition for that option *anywhere* in the tree. (the script might
> not be perfect, there might be the occasional false negative or
> positive, but one can always verify manually if there's any doubt.)
> 
>   one of the worst culprits appears to be CONFIG_SPL_BUILD, which
> appears all over the tree, but one can see a recent commit that takes
> that into account:
> 
>   commit 0ef692084363f2de8547db93397c6a69123d26ca
>   Author: Baruch Siach <baruch at tkos.co.il>
>   Date:   Thu Feb 7 13:21:16 2019 +0200
> 
>     Kconfig: fix BUILD_TARGET for ARCH_MVEBU
> 
>     Commit dc146ca11187 ("Kconfig: Migrate CONFIG_BUILD_TARGET") made the
>     mvebu default build target depend on CONFIG_SPL_BUILD. Unfortunately,
>     there is no such Kconfig symbol. Use the CONFIG_SPL symbol instead to
>     fix that.
> 
>     Cc: Jagan Teki <jagan at amarulasolutions.com>
>     Signed-off-by: Baruch Siach <baruch at tkos.co.il>
>     Reviewed-by: Stefan Roese <sr at denx.de>
>     Reviewed-by: Jagan Teki <jagan at amarulasolutions.com>
>     Signed-off-by: Stefan Roese <sr at denx.de>

So, we have two different problems here.  One of which, as shown by
CONFIG_ACX517AKN is symbols that need to be migrated to Kconfig but also
likely weren't ever added with something setting the option true (or, if
you dig you might see the board(s) that used that option were dropped,
but that bit of code was not).  The second of which is things like
CONFIG_SPL_BUILD / CONFIG_TPL_BUILD that are not to be used in Kconfig
but are used in Kbuild as we set that as makes sense for the stage of
the build.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190422/c4e64314/attachment.sig>