On Mon, Jul 2, 2018 at 3:25 AM Tom Rini trini@konsulko.com wrote:
On Fri, Jun 29, 2018 at 09:19:34PM -0700, Simon Glass wrote:
+Tom
Hi Alex,
On 29 June 2018 at 02:31, Alex Kiernan alex.kiernan@gmail.com wrote:
I've just been digging into a problem where I've got both CONFIG_ENV_IS_NOWHERE set and CONFIG_BOOTDELAY set to -2 and it turns out in env_default.h we have:
#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) "bootdelay=" __stringify(CONFIG_BOOTDELAY) "\0" #endifreason
So the -ve values never make it into the default environment, which means I don't have it at all when CONFIG_ENV_IS_NOWHERE.
The (CONFIG_BOOTDELAY >= 0) seems to have been there forever (c609719b8d1b2dca590e0ed499016d041203e403, Sun Nov 3 00:24:07 2002 +0000 is as far back as I've gone), but we've then changed the behaviours of the bootdelay values in (the commit I was looking at was 2fbb8462b0e18893b4b739705db047ffda82d4fc from Mon Jun 27 16:23:01 2016 +0900, but I'm not sure that's really the right one)
I think we should change the code to a simple #if defined(CONFIG_BOOTDELAY)
?
I don't know what the check was supposed to do and the comment on the 'bootdelay' env variable just says 'see CONFIG_BOOTDELAY'. Your solution sounds reasonable to me but perhaps Tom or Wolfgang have more insight.
It seems like a historical oversight. But.. what happens before and after when you have a negative bootdelay value in the default environment?
It's a bit convoluted...
There's bootmenu, where (CONFIG_BOOTDELAY >= 0) supplies a compile time default, which can be overridden by options to the command, or the bootmenu_delay environment variable. This never looks at the bootdelay environment variable, so I think we can ignore this one.
Then there's autoboot, which reads the environment for bootdelay, but if that's not set then uses CONFIG_BOOTDELAY as a default.
So whilst you end up with the default in all cases, it's only stored in the environment if it's >= 0.
Looking at the only place bootdelay is read from the environment (in bootdelay_process):
s = env_get("bootdelay"); bootdelay = s ? (int)simple_strtol(s, NULL, 10) : CONFIG_BOOTDELAY;
So if you have a compiled in -ve CONFIG_BOOTDELAY, there won't be a value for bootdelay in the default environment and we'll use the compile time default. Swap to storing all values of CONFIG_BOOTDELAY and you'll fetch it from the environment, rather than using the compile time default.
So long as we don't remove the CONFIG_BOOTDELAY in this ternary expression, I think you end up with the same behaviour in every circumstance.
The reason I tripped over it was some scripting in CONFIG_PREBOOT which was common between two configurations, but was expecting to differentiate between them based on bootdelay (which broke when it turned out to not be set).
-- Alex Kiernan