[U-Boot] [PATCH] Revert "image.h: Tighten up content using handy CONFIG_IS_ENABLED() macro."

[Masahiro Yamada]

Hi.

2016-05-31 21:05 GMT+09:00 Robert P. J. Day <rpjday at crashcourse.ca>:
> On Tue, 31 May 2016, Masahiro Yamada wrote:
>
>> This reverts commit 56adbb38727320375b2f695bd04600d766d8a1b3.
>>
>> Since commit 56adbb387273 ("image.h: Tighten up content using handy
>> CONFIG_IS_ENABLED() macro."), I found my boards fail to boot Linux
>> because the commit changed the logic of macros it touched.  Now,
>> IMAGE_ENABLE_RAMDISK_HIGH and IMAGE_BOOT_GET_CMDLINE are 0 for all
>> the boards.
>>
>> As you can see in include/linux/kconfig.h, CONFIG_IS_ENABLE() (and
>> IS_ENABLED() as well) can only take a macro that is either defined
>> as 1 or undefined.  This is met for boolean options defined in
>> Kconfig.  On the other hand, CONFIG_SYS_BOOT_RAMDISK_HIGH and
>> CONFIG_SYS_BOOT_GET_CMDLINE are defined without any value in
>> arch/*/include/asm/config.h .  This kind of clean-up is welcome,
>> but the options should be moved to Kconfig beforehand.
>
> ... snip ...
>
>   whoops, that would be my fault, i never considered that possibility,
> i thought this was a fairly straightforward (and mostly aesthetic)
> change.
>
>   it seems that there is a fair amount of inconsistent usage of CONFIG
> settings, as in, if one wants to test only if a setting is defined:
>
>   #ifdef CONFIG_FOO
>
> then it's sufficient to manually set:
>
>   #define CONFIG_FOO
>
> however, in the above, it doesn't hurt to also write:
>
>   #define CONFIG_FOO 1
>
> but the instant you do that, you can *also* then test:
>
>   #if CONFIG_FOO
>
> and i'm wondering how much there is of mixing both tests; that is,
> once you write this:
>
>   #define CONFIG_FOO 1
>
> you have a tendency to start using both tests:
>
>   #ifdef CONFIG_FOO
>   #if CONFIG_FOO
>
> which is definitely messy. anyway, my fault for not looking at the
> above carefully enough before submitting.
>


IS_ENABLED() (and include/linux/kernel.h) came from Linux Kernel.

So, you should understand things in Linux side.

Is Linux, all CONFIG options are defined in Kconfig.
(there is only one exception, CONFIG_SHELL, though.)

As you see in include/generated/autoconf.h,
all the boolean options are either defined as 1
or not defined at all.

So,
>   #define CONFIG_FOO
this case (definition without any value) never happens in Linux Kernel.


#if CONFIG_FOO
is error when CONFIG_FOO is not defined.
(notice, boolean CONFIG options are defined as 1 or undefined.
no case for defined as 0.)


I know two benefits of IS_ENABLED().

[1]
If CONFIG_FOO=y in Kconfig,
CONFIG_FOO is defined in include/generated/autoconf.h

If CONFIG_FOO=m in Kconfig,
CONFIG_FOO_MODULE is defined in include/generated/autoconf.h

So, before IS_ENABLED() was invented, we used to write like follows

#if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)
      ...
#end


Now, we can use it as a shorthand

#if IS_ENABLED(CONFIG_FOO)
      ...
#end


[2]
IS_ENABLED() can be used in C context.

    if (CONFIG_FOO) {
             printk("CONFIG_FOO is defined\n");
    }

causes build error if CONFIG_FOO is not defined at all.
(again, no possibilit for  #define CONFIG_FOO   0)

    if (IS_ENABLED(CONFIG_FOO)) {
             printk("CONFIG_FOO is defined\n");
    }

works as expected because IS_ENABLED(CONFIG_FOO) is evaluated to 0
when CONFIG_FOO is not defined.



In U-Boot, things are much more complicated.
Historically, all CONFIGs were defined in C headers
(and still many are defined there)

For those, both style
#define CONFIG_FOO
#define CONFIG_FOO  1
exist because it did not matter at all.

Since Kconfig was introduced to U-Boot,
we have moved many options to Kconfig, but the migration is still under way.


#define CONFIG_FOO  1
is only used in Kconfig.


What is more complex about U-Boot is
it supports multiple image generation from one config.


It is too painful to write

#if  (!defined(CONFIG_SPL_BUILD) && defined(CONFIG_FOO)) ||
      (defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_FOO))

so, a shorthand

#if CONFIG_IS_ENABLED(CONFIG_FOO)

was added.


They are handy, but we have to take care of their correct usage.


-- 
Best Regards
Masahiro Yamada