[U-Boot] [PATCH 2/4] malloc_f: fix broken .config caused by CONFIG_SYS_MALLOC_F

[Masahiro YAMADA]

2015-02-19 23:22 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Masahiro,
>
> On 19 February 2015 at 00:47, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
>> Since commit b724bd7d6349 (dm: Kconfig: Move CONFIG_SYS_MALLOC_F_LEN
>> to Kconfig), the ".config" created by the configuration has been
>> wrong.
>>
>> For example, the following is a snippet of the ".config" generated
>> by "make beaver_defconfig":
>>
>>   --------------->8-----------------
>>   CONFIG_CC_OPTIMIZE_FOR_SIZE=y
>>   # CONFIG_SYS_MALLOC_F is not set
>>   CONFIG_SYS_MALLOC_F_LEN=0x1800
>>   # CONFIG_EXPERT is not set
>>   ---------------8<-----------------
>>
>> CONFIG_SYS_MALLOC_F_LEN is supposed to depend on CONFIG_SYS_MALLOC_F
>> (see the top level Kconfig), but the ".config" above is not actually
>> following that dependency.
>>
>> This is caused by two mistakes of commit b724bd7d6349.
>>
>> [1] Wrong default value of CONFIG_SYS_MALLOC_F
>>   CONFIG_SYS_MALLOC_F is a boolean option, but its default value is
>>   set to 0x400.
>
> I sent a patch for this.

OK.  But this change alone could still generate a strange .config file.



>>
>> [2] Missing "if SYS_MALLOC_F" in the default setting in each Kconfig
>>   For example, arch/arm/cpu/armv7/tegra-common/Kconfig has the line
>>   "default 0x1800" for SYS_MALLOC_F_LEN.  It must be described as
>>   "default 0x1800 if SYS_MALLOC_F" to follow the dependency.
>
> Does this actually matter? What does it break?

I left a comment in your patch:
http://patchwork.ozlabs.org/patch/441669/


>>
>> Those two bugs together create such a broken ".config".
>>
>> Unfortunately, even if we correct both [1] and [2], the value of
>> CONFIG_SYS_MALLOC_F_LEN is not set as we expect.
>> The "default 0x1800 if SYS_MALLOC_F" would be simply ignored because
>> the "default 0x400" in the top level Kconfig is parsed first.
>>
>> Notice that if multiple default lines appear for the same CONFIG,
>> the first one takes precedence.
>
> I sent a patch for this also as I don't really think the current
> ordering is useful.

I think this is a good idea.



>>
>> So, this commit correct [1] and [2], also leaves some comments
>> in arch/arm/cpu/armv7/tegra-common/Kconfig and arch/x86/Kconfig
>> to notify not-working default values.
>>
>> If you want to change the default value of CONFIG_SYS_MALLOC_F_LEN,
>> the easiest way would be to specify it in each *_defconfig.
>>
>> It is true that describing SoC-common default values in each Kconfig
>> seems handy, but it often introduces nasty problems.
>> If you do not understand well how Kconfig works, as you see above,
>> you could easily create a broken .config file.
>
> It is actually quite painful to not support this. If you add a new
> board you need to work out what the settings should be for that board.
> As we add more and more settings this is going to get even harder. It
> seems much better for Tegra (for example) to be able to specify
> sensible defaults that will work on most boards.
>
> Otherwise we have no place to record that (for example) Tegra124 needs
> this feature on, or this value set. Before Kconfig we have
> tegra-common.h and tegra124-common.h. We discussed this problem before
> and I thought it was agreed that defaults in Kconfig were the best way
> forward.
>
> If we can't use Kconfig then I think we will need to figure out
> something else - perhaps includes in the defconfig files as previously
> discussed. But that would be a new kconfig feature so I don't think
> anyone is thrilled with the idea.


I would not go as far as say it must be banned.

Please be careful when you add default values in Kconfig.





-- 
Best Regards
Masahiro Yamada