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

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

[Masahiro Yamada]

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.

Moreover, CONFIG_IS_ENABLED(SPL_CRC32_SUPPORT) looks weird.
It should be either CONFIG_IS_ENABLED(CRC32_SUPPORT) or
IS_ENABLED(CONFIG_SPL_CRC32_SUPPORT).  But, I see no define for
CONFIG_SPL_CRC32_SUPPORT anywhere.  Likewise for the other three.

The logic of IMAGE_OF_BOARD_SETUP and IMAGE_OF_SYSTEM_SETUP were
also changed for SPL.  This can be a problem for boards defining
CONFIG_SPL_OF_LIBFDT.  I guess it should have been changed to
IS_ENABLED(CONFIG_OF_BOARD_SETUP).

In the first place, if we replace the references in C code,
the macros IMAGE_* will go away.

  if (IS_ENABLED(CONFIG_OF_BOARD_SETUP) {
          ...
  }

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 include/image.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 12 deletions(-)

diff --git a/include/image.h b/include/image.h
index 80a4454..a8f6bd1 100644
--- a/include/image.h
+++ b/include/image.h
@@ -52,15 +52,19 @@ struct lmb;
 #include <hash.h>
 #include <libfdt.h>
 #include <fdt_support.h>
-# ifdef CONFIG_FIT_DISABLE_SHA256
-#  undef CONFIG_SHA256
-#  undef IMAGE_ENABLE_SHA256
-# endif
 # ifdef CONFIG_SPL_BUILD
-#  define IMAGE_ENABLE_CRC32	CONFIG_IS_ENABLED(SPL_CRC32_SUPPORT)
-#  define IMAGE_ENABLE_MD5	CONFIG_IS_ENABLED(SPL_MD5_SUPPORT)
-#  define IMAGE_ENABLE_SHA1	CONFIG_IS_ENABLED(SPL_SHA1_SUPPORT)
-#  define IMAGE_ENABLE_SHA256	CONFIG_IS_ENABLED(SPL_SHA256_SUPPORT)
+#  ifdef CONFIG_SPL_CRC32_SUPPORT
+#   define IMAGE_ENABLE_CRC32	1
+#  endif
+#  ifdef CONFIG_SPL_MD5_SUPPORT
+#   define IMAGE_ENABLE_MD5	1
+#  endif
+#  ifdef CONFIG_SPL_SHA1_SUPPORT
+#   define IMAGE_ENABLE_SHA1	1
+#  endif
+#  ifdef CONFIG_SPL_SHA256_SUPPORT
+#   define IMAGE_ENABLE_SHA256	1
+#  endif
 # else
 #  define CONFIG_CRC32		/* FIT images need CRC32 support */
 #  define CONFIG_MD5		/* and MD5 */
@@ -71,12 +75,53 @@ struct lmb;
 #  define IMAGE_ENABLE_SHA1	1
 #  define IMAGE_ENABLE_SHA256	1
 # endif
+
+#ifdef CONFIG_FIT_DISABLE_SHA256
+#undef CONFIG_SHA256
+#undef IMAGE_ENABLE_SHA256
+#endif
+
+#ifndef IMAGE_ENABLE_CRC32
+#define IMAGE_ENABLE_CRC32	0
+#endif
+
+#ifndef IMAGE_ENABLE_MD5
+#define IMAGE_ENABLE_MD5	0
+#endif
+
+#ifndef IMAGE_ENABLE_SHA1
+#define IMAGE_ENABLE_SHA1	0
+#endif
+
+#ifndef IMAGE_ENABLE_SHA256
+#define IMAGE_ENABLE_SHA256	0
+#endif
+
 #endif /* IMAGE_ENABLE_FIT */
 
-#define IMAGE_ENABLE_RAMDISK_HIGH	CONFIG_IS_ENABLED(SYS_BOOT_RAMDISK_HIGH)
-#define IMAGE_BOOT_GET_CMDLINE		CONFIG_IS_ENABLED(SYS_BOOT_GET_CMDLINE)
-#define IMAGE_OF_BOARD_SETUP		CONFIG_IS_ENABLED(OF_BOARD_SETUP)
-#define IMAGE_OF_SYSTEM_SETUP		CONFIG_IS_ENABLED(OF_SYSTEM_SETUP)
+#ifdef CONFIG_SYS_BOOT_RAMDISK_HIGH
+# define IMAGE_ENABLE_RAMDISK_HIGH	1
+#else
+# define IMAGE_ENABLE_RAMDISK_HIGH	0
+#endif
+
+#ifdef CONFIG_SYS_BOOT_GET_CMDLINE
+# define IMAGE_BOOT_GET_CMDLINE		1
+#else
+# define IMAGE_BOOT_GET_CMDLINE		0
+#endif
+
+#ifdef CONFIG_OF_BOARD_SETUP
+# define IMAGE_OF_BOARD_SETUP		1
+#else
+# define IMAGE_OF_BOARD_SETUP		0
+#endif
+
+#ifdef CONFIG_OF_SYSTEM_SETUP
+# define IMAGE_OF_SYSTEM_SETUP	1
+#else
+# define IMAGE_OF_SYSTEM_SETUP	0
+#endif
 
 /*
  * Operating System Codes
-- 
1.9.1

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

[Robert P. J. Day]

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.

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

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

[Robert P. J. Day]

On Tue, 31 May 2016, Masahiro Yamada wrote:

  ... lengthy explanation snipped ...

thanks for that explanation; clearly, i had no idea what i was getting
into here and, again, i apologize for submitting a patch that was not
even remotely close to correct.

  le *sigh*.

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] [PATCH] Revert "image.h: Tighten up content using handy CONFIG_IS_ENABLED() macro."

[Tom Rini]

On Tue, May 31, 2016 at 08:41:54PM +0900, 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.
> 
> Moreover, CONFIG_IS_ENABLED(SPL_CRC32_SUPPORT) looks weird.
> It should be either CONFIG_IS_ENABLED(CRC32_SUPPORT) or
> IS_ENABLED(CONFIG_SPL_CRC32_SUPPORT).  But, I see no define for
> CONFIG_SPL_CRC32_SUPPORT anywhere.  Likewise for the other three.
> 
> The logic of IMAGE_OF_BOARD_SETUP and IMAGE_OF_SYSTEM_SETUP were
> also changed for SPL.  This can be a problem for boards defining
> CONFIG_SPL_OF_LIBFDT.  I guess it should have been changed to
> IS_ENABLED(CONFIG_OF_BOARD_SETUP).
> 
> In the first place, if we replace the references in C code,
> the macros IMAGE_* will go away.
> 
>   if (IS_ENABLED(CONFIG_OF_BOARD_SETUP) {
>           ...
>   }
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Applied to u-boot/master, thanks!

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