[U-Boot] CONFIG_BOOTDELAY and env_default.h
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" #endif
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)
?
+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" #endif
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.
Regards, Simon
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" #endif
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?
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
On Mon, Jul 02, 2018 at 11:00:05AM +0100, Alex Kiernan wrote:
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).
OK. Please put out a patch and I'll pick it up after the release, thanks!
participants (3)
-
Alex Kiernan -
Simon Glass -
Tom Rini