[U-Boot] [PATCH] Revert "image.h: Tighten up content using handy CONFIG_IS_ENABLED() macro."
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
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
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.
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
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!
participants (3)
-
Masahiro Yamada -
Robert P. J. Day -
Tom Rini