Re: [U-Boot] CONFIG_BOOTDELAY and env_default.h
On Mon, Jul 02, 2018 at 11:00:05AM +0100, Alex Kiernan wrote: > On Mon, Jul 2, 2018 at 3:25 AM Tom Rini 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 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 > > > > + 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! -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] CONFIG_BOOTDELAY and env_default.h
On Mon, Jul 2, 2018 at 3:25 AM Tom Rini 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 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 > > > + 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] CONFIG_BOOTDELAY and env_default.h
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 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 > > + 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? -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] CONFIG_BOOTDELAY and env_default.h
+Tom Hi Alex, On 29 June 2018 at 02:31, Alex Kiernan 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 > + 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[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 + 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) ? -- Alex Kiernan ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot