[U-Boot] doing anything about "bad" Kbuild configuration options?
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...
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@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@amarulasolutions.com Signed-off-by: Baruch Siach baruch@tkos.co.il Reviewed-by: Stefan Roese sr@denx.de Reviewed-by: Jagan Teki jagan@amarulasolutions.com Signed-off-by: Stefan Roese sr@denx.de
the output for that example in my wiki page is a bit overwhelming, which is why my badref script takes an optional directory to search, so you can focus your attention as finely as you want. as an example, check only drivers/mmc, which appears to find exactly four bad references:
$ find_badref_configs.sh drivers/mmc
HSMMC3_8BIT
drivers/mmc/omap_hsmmc.c:1577:#if defined(CONFIG_DRA7XX) && defined(CONFIG_HSMMC3_8BIT)
MMC_RPMB_TRACE
drivers/mmc/rpmb.c:98:#ifdef CONFIG_MMC_RPMB_TRACE drivers/mmc/rpmb.c:115:#ifdef CONFIG_MMC_RPMB_TRACE drivers/mmc/rpmb.c:131:#ifdef CONFIG_MMC_RPMB_TRACE drivers/mmc/rpmb.c:147:#ifdef CONFIG_MMC_RPMB_TRACE drivers/mmc/rpmb.c:154:#ifdef CONFIG_MMC_RPMB_TRACE scripts/config_whitelist.txt:1225:CONFIG_MMC_RPMB_TRACE
MMC_SPI_CRC_ON
drivers/mmc/mmc_spi.c:94:#ifdef CONFIG_MMC_SPI_CRC_ON drivers/mmc/mmc_spi.c:123:#ifdef CONFIG_MMC_SPI_CRC_ON drivers/mmc/mmc.c:2259:#ifdef CONFIG_MMC_SPI_CRC_ON scripts/config_whitelist.txt:1228:CONFIG_MMC_SPI_CRC_ON arch/arm/include/asm/arch-pxa/regs-mmc.h:66:#define MMC_SPI_CRC_ON (1 << 1)
ZYNQ_HISPD_BROKEN
drivers/mmc/zynq_sdhci.c:263:#ifdef CONFIG_ZYNQ_HISPD_BROKEN scripts/config_whitelist.txt:4637:CONFIG_ZYNQ_HISPD_BROKEN $
in any event, i am not planning to do anything about this, i just thought others might want to check whatever part of the code base for which they are a maintainer, and see if any such things exist. and as forrest gump once said, "that's all i have to say about that."
rday
at risk of boring, i'll mention a couple more scripts i have for locating oddities or inconsistencies in the Kbuild structure, which people are welcome to play with.
the first is called "find_badref_selects.sh", which specifically locates "select" directives in Kconfig files that are selecting non-existing Kbuild options, which makes them pointless.
example (focusing attention on arch/arm directory):
$ find_badref_selects.sh arch/arm
CPU_ARM926EJS1
arch/arm/mach-imx/mx2/Kconfig:20: select CPU_ARM926EJS1
SPL_DISABLE_OF_CONTROL
arch/arm/mach-exynos/Kconfig:119: select SPL_DISABLE_OF_CONTROL arch/arm/mach-exynos/Kconfig:153: select SPL_DISABLE_OF_CONTROL $
and a second script called "find_badref_make_configs.sh" specifically finds Kconfig references in Makefiles that point to non-existent Kconfig options. for example:
$ find_badref_make_configs.sh drivers/gpio
ADI_GPIO2
./drivers/gpio/Makefile:obj-$(CONFIG_ADI_GPIO2) += adi_gpio2.o
DB8500_GPIO
./drivers/gpio/Makefile:obj-$(CONFIG_DB8500_GPIO) += db8500_gpio.o
DM644X_GPIO
./drivers/gpio/Makefile:obj-$(CONFIG_DM644X_GPIO) += da8xx_gpio.o $
if anyone's interested, i can post those scripts on a couple more wiki pages this weekend, with an example or two. and on that note, i will shut up about this now.
rday
On Sat, 13 Apr 2019, 4:56 AM Robert P. J. Day, rpjday@crashcourse.ca wrote:
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:
That's not quite right. CONFIG_SPL_BUILD is defined for the Makefile just not in Kconfig (I'm on a tablet right now so I can't point you to the source).
commit 0ef692084363f2de8547db93397c6a69123d26ca
Author: Baruch Siach baruch@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@amarulasolutions.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> Reviewed-by: Stefan Roese <sr@denx.de> Reviewed-by: Jagan Teki <jagan@amarulasolutions.com> Signed-off-by: Stefan Roese <sr@denx.de>
This commit addresses the fact that CONFIG_SPL_BUILD is not available to use in Kconfig. It is available to Makefiles, I can't remember if it gets defined for source files.
On Sat, 13 Apr 2019, Chris Packham wrote:
On Sat, 13 Apr 2019, 4:56 AM Robert P. J. Day, rpjday@crashcourse.ca wrote: 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:
That's not quite right. CONFIG_SPL_BUILD is defined for the Makefile just not in Kconfig (I'm on a tablet right now so I can't point you to the source).
commit 0ef692084363f2de8547db93397c6a69123d26ca Author: Baruch Siach <baruch@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@amarulasolutions.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> Reviewed-by: Stefan Roese <sr@denx.de> Reviewed-by: Jagan Teki <jagan@amarulasolutions.com> Signed-off-by: Stefan Roese <sr@denx.de>This commit addresses the fact that CONFIG_SPL_BUILD is not available to use in Kconfig. It is available to Makefiles, I can't remember if it gets defined for source files.
i realize there are such things; however, the standard is that any options/variables prefixed with "CONFIG_" are typically defined as part of the Kbuild infrastructure (with the exception of prefixes of "CONFIG_SYS_").
i think what you're referring to is in scripts/Makefile.spl:
scripts/Makefile.spl:KBUILD_CPPFLAGS += -DCONFIG_SPL_BUILD
i'm not going to make recommendations one way or the other; merely pointing out that i have scripts that display such things, and people are free to do what they wish with the results.
rday
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...
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@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@amarulasolutions.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> Reviewed-by: Stefan Roese <sr@denx.de> Reviewed-by: Jagan Teki <jagan@amarulasolutions.com> Signed-off-by: Stefan Roese <sr@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.
participants (3)
-
Chris Packham -
Robert P. J. Day -
Tom Rini