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

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

[Robert P. J. Day]

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

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                         http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

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

[Robert P. J. Day]

  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

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                         http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

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

[Chris Packham]

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.

>

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

[Robert P. J. Day]

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

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                         http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

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

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