[U-Boot] [PATCH v2 4/5] autoboot: fix a bunch of misconversion of CONFIG_BOOTDELAY

[Hans de Goede]

Hi,

On 11-06-16 15:46, Masahiro Yamada wrote:
> Hi.
>
> 2016-06-11 20:31 GMT+09:00 Hans de Goede <hdegoede at redhat.com>:
>> Hi,
>>
>> On 11-06-16 11:44, Masahiro Yamada wrote:
>>>
>>> Commit bb597c0eeb7e ("common: bootdelay: move CONFIG_BOOTDELAY into
>>> a Kconfig option") made a number of misconversion.
>>>
>>> [1] CONFIG_BOOTDELAY=-1 all gone
>>> [2] CONFIG_BOOTDELAY=1 all gone
>>> [3] CONFIG_BOOTDELAY=2 all gone
>>> [4] Conditionally defined CONFIG_BOOTDELAY all gone
>>>
>>> All of the misconverted boards now use the default value,
>>> CONFIG_BOOTDELAY=0, which came from the Kconfig entry.
>>>
>>> I am imagining some reasons for this.
>>>
>>> For [1], due to the bug of tools/scripts/define2mk.sed (now fixed),
>>>   #define CONFIG_BOOTDELAY -1
>>> was converted to
>>>   CONFIG_BOOTDELAY="-1"
>>> in the include/autoconf.mk, so the tools/moveconfig.py regarded it
>>> as a string type option, and failed to move it.
>>>
>>> For [2], as you see in the comment block in the define2mk.sed,
>>>   #define CONFIG_BOOTDELAY 1
>>> is converted to
>>>   CONFIG_BOOTDELAY=y
>>> in the include/autoconf.mk.  This needs a special care because we do
>>> not know whether we are moving a bool option with value y or an
>>> integer option with value 1.  A recently-sent patch fixes this issue.
>>>
>>> I do not understand the reason for [3].
>>>
>>> [4] is another case the current moveconfig cannot handle correctly.
>>> If the define is surrounded by #ifndef CONFIG_BOOTDELAY like follows,
>>> the default value from Kconfig entry beats the define in C header.
>>>
>>>   #ifndef CONFIG_BOOTDELAY
>>>   #define CONFIG_BOOTDELAY      3
>>>   #endif
>>>
>>> Joe's patch can solve this issue.
>>>
>>> Anyway, I ran the newest moveconfig tool based on commit 3191d8408053
>>> (=immediately prior to the bad commit) to generate this patch.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>>> ---
>>>
>>> Changes in v2:
>>>   - Fix case [4]
>>>
>
>>
>>
>> No, just no NACK at least for all the sunxi boards.
>>
>> This madness where everytime we convert something to Kconfig
>> we end up bloating ALL the defconfigs needs to stop and it
>> needs to stop now.
>>
>> I'm 100% done with fixing up things everytime after someone
>> thinks this is a good idea.
>>
>> This is not a good idea, it is a REALLY REALLY bad idea.
>>
>> The defconfig files should contain board specific settings,
>> we had the same nonsense when some of the CONFIG_CMD_FOO
>> things were converted, and all of a sudden we had the
>> same 5 extra lines in every defconfig for every board
>> which used to use config_distro_bootcmd.h
>>
>> Apparently no one was smart enough to realize that if
>> we were moving stuff from a common config header to
>> Kconfig, we would need something like
>>
>> CONFIG_DISTRO_BOOTCMD in Kconfig which would then select
>> all CONFIG_CMD_FOO options which were moved, and then
>> board / soc Kconfig snippets could simply do:
>>
>>         select CONFIG_DISTRO_BOOTCMD
>
> Right.
>
> (I had already noticed this, but I was not picky about it
> just because I was not using distro on my boards.)
>
> Recently, when I decided to try distro,
> the first thing I did was
> "git log include/include/config_distro_defaults.h"
> to find out what has gone to Kconfig.

Ack, I've just done the same so as to add a
new DISTRO_DEFAULTS options as discussed with Tom.

>> I've hacked around this for sunxi:
>>
>> config ARCH_SUNXI
>>         bool "Support sunxi (Allwinner) SoCs"
>>         select CMD_BOOTZ
>>         select CMD_DHCP
>>         select CMD_EXT2
>>         select CMD_EXT4
>>         select CMD_FAT
>>         select CMD_FS_GENERIC
>>         select CMD_GPIO
>>         select CMD_MII
>>         select CMD_MMC if MMC
>>         select CMD_PING
>>         select CMD_USB
>>         ...
>>
>> But I'm sick and tired of this nonsense repeating over
>> and over again.
>>
>> This needs to stop and it needs to stop NOW.
>>
>> If Commit bb597c0eeb7e ("common: bootdelay: move CONFIG_BOOTDELAY into
>> a Kconfig option") breaks things on 436 boards, then clearly it needs
>> to be reverted rather then messing up 436 defconfig files with cruft.
>>
>> Why am I so against filling defconfigs with a couple of innocent lines?
>> Because this makes it much harder for people to add support for new
>> boards. The more cruft we put there, the harder it becomes for people
>> to see the actual board specific things.
>>
>> And clearly 0 is a very very poor default for CONFIG_BOOTDELAY, I can
>> pretty much live with any default for sunxi other then 0, generic distro
>> users  need to be able to easily stop auto-boot, so 0 is just no good.
>>
>> If the new default were 1 or 2, or 3, then that would be fine with me,
>> even if 1 and 3 are a change from the current sunxi setting I can live
>> with this.
>
> Agree.
>
> CONFIG_BOOTDELAY=0 can still have a chance to
> enter the command prompt if CONFIG_ZERO_BOOTDELAY_CHECK is set.
> But, anyway, this is absolutely poor default.
>
>
> When I was fixing the broken boards, I realized that
> really small number of boards set it as 0.
>
> According to my analysis,
>
>  CONFIG_BOOTDELAY is not set:  28
>  CONFIG_BOOTDELAY=-1        :  21
>  CONFIG_BOOTDELAY=0         :  17
>  CONFIG_BOOTDELAY=1         :  76
>  CONFIG_BOOTDELAY=2         :  265
>  CONFIG_BOOTDELAY=3         :  289
>  CONFIG_BOOTDELAY=5         :  158
>  CONFIG_BOOTDELAY=6         :  25
>  CONFIG_BOOTDELAY=10        :  275
>  CONFIG_BOOTDELAY=30        :  1
>
> So, of the 1155 defconfigs, only 17 defconfigs set the value 0.
> This default is poor also from the point of view of
> "the most frequently used value".
>
>
> The commit should have chosen 3 for the default.
>
> As you pointed out, the boards with value 2, 3, 5, 6 would probably
> not care about it very much
> as long as it is greater than 0.

Ack, although I would prefer to not add a whole second to the
boot-time of boards currently using 2, and at least
for boards who were using config_distro_defaults.h I do
not believe that is necessary see the patch I'm about to send.

> I am OK with reverting the bad commit completely.

Not sure if that is best. I'm afraid it will lead only more
churn, anyways I believe the most important thing is to avoid
problems like this in the future and I believe that DISTRO_DEFAULTS
will help there (at least for sunxi).

> Anyway, a big churn of Kconfig re-sync will happen again to change the default,
> and move "config BOOTDELAY" entry to the correct place.
>
> Despite that we already have the "autoboot" menu in cmd/Kconfig,
> the entry for BOOTDELAY has gone to a completely unrelated place.
> We need to fix this too.

Ack (as a follow up patch, unless we revert it first).


>> If we need a way to override this for some boards / SoCs, then
>> we need something which we can do at the Kconfig level, iow something
>> which can be select-ed on e.g. a per SoC basis, so that we do not need
>> to mess up 436 defconfigs.
>
>
> The problem for "select" is that
> it disables users to un'select it.

I consider that a feature, at least for sunxi I do not want users
to disable $random_thing and then file a bug that something does
not work. IOW I want to keep the test-matrix as small as possible,
so not allowing to disable things is a feature in my book.

But DISTRO_DEFAULTS will give the best of both worlds, people like
me can simply select that and in other cases were more fine tuning
is used people can simply chose not to use it.

> We also agreed to not do
>
>     config BOOTDELAY
>            default 3
>
> in board Kconfig.

I know, although I do wonder why, AFAICT, doing something like this:

board/sunxi/Kconfig:

if ARCH_SUNXI

...
sunxi specific Kconfig options
...

endif

config BOOTDELAY
	default 2 if ARCH_SUNXI

Should not lead to any problems, notice the
if on the default and doing it outside of the
standard "if ARCH_FOO ... endif" block.

And this would give board / soc maintainers some more liberty
to set soc specific defaults.

> In the first place, having many defconfigs is painful in terms of
> maintainability.

Agreed.

> I always try to support multiple boards with one defconfig.

Unfortunately that often is not possible.

> The recently-added feature for FIT might be helpful to merge similar defconfigs
> (but, this does not seem to be a perfect solution given that FIT is
> heavy for SPL)
>
> Linux supports multi-platform and barebox as well.
> For example, "tegra_v7_defconfig" in barebox covers all the ARMv7 Tegra SoCs.
>
>
>
> On the other hand, in U-boot, I am worried that the more and more
> defconfigs are being added
> just for a small difference.
>
> For example, the recent commit d8ccbe93b57669 added 7 defconfigs just
> for one board.
>
> And the difference between the following two is just CONFIG_BOOTDELAY.
>
> --- configs/am335x_shc_sdboot_defconfig 2016-06-11 21:42:19.189376179 +0900
> +++ configs/am335x_shc_sdboot_prompt_defconfig 2016-06-11
> 21:42:19.189376179 +0900
> @@ -6,7 +6,7 @@
>  CONFIG_SPL=y
>  CONFIG_SPL_STACK_R=y
>  CONFIG_FIT=y
> -CONFIG_BOOTDELAY=0
> +CONFIG_BOOTDELAY=5
>  CONFIG_SYS_PROMPT="U-Boot# "
>  CONFIG_AUTOBOOT_KEYED=y
>  CONFIG_AUTOBOOT_PROMPT="Enter 'shc' to enter prompt (times out) %d
> \nEnter 'noautoboot' to enter prompt without timeout\n"

I agree that this is a (very) bad example, but when dealing with actual different
boards where hw auto-detect is impossible, there really is no other way, in
this case we want to keep things simple for the user, which means having
a board specific defconfig which will do exactly the right thing for
their board.


Regards,

Hans