[U-Boot] CONFIG_BOOTDELAY and env_default.h

[U-Boot] CONFIG_BOOTDELAY and env_default.h

[Alex Kiernan]

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)

?

-- 
Alex Kiernan

[U-Boot] CONFIG_BOOTDELAY and env_default.h

[Simon Glass]

+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

[U-Boot] CONFIG_BOOTDELAY and env_default.h

[Tom Rini]

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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180701/2aa90bbe/attachment.sig>

[U-Boot] CONFIG_BOOTDELAY and env_default.h

[Alex Kiernan]

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

[U-Boot] CONFIG_BOOTDELAY and env_default.h

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180703/de41d452/attachment.sig>