Re: [PATCH 03/25] autoboot: Correct dependencies on CMDLINE

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2259 bytes --]

On Tue, Oct 03, 2023 at 08:11:11PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Sun, 24 Sept 2023 at 18:40, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Sep 24, 2023 at 02:39:21PM -0600, Simon Glass wrote:
> >
> > > Make AUTOBOOT depend on CMDLINE since it is mostly meaningless without it.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >  boot/Kconfig | 23 ++++++++++++++---------
> > >  1 file changed, 14 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/boot/Kconfig b/boot/Kconfig
> > > index f74ac7e9cc72..41ec2c34bf74 100644
> > > --- a/boot/Kconfig
> > > +++ b/boot/Kconfig
> > > @@ -1167,14 +1167,16 @@ menu "Autoboot options"
> > >
> > >  config AUTOBOOT
> > >       bool "Autoboot"
> > > +     depends on CMDLINE
> > >       default y
> > >       help
> > >         This enables the autoboot.  See doc/README.autoboot for detail.
> >
> > This is fine and correct.
> >
> > > +if AUTOBOOT
> > > +
> > >  config BOOTDELAY
> > >       int "delay in seconds before automatically booting"
> > >       default 2
> > > -     depends on AUTOBOOT
> > [snip]
> > > @@ -1322,6 +1324,9 @@ config AUTOBOOT_STOP_STR_SHA256
> > >         includes a ":", the portion prior to the ":" will be treated
> > >         as a salt value.
> > >
> > > +endif  # AUTOBOOT_KEYED
> > > +endif  # AUTOBOOT
> >
> > So it's ~200 lines, yes, hiding this under an if, or perhaps a menu
> > makes sense, however...
> >
> > >  config AUTOBOOT_USE_MENUKEY
> > >       bool "Allow a specify key to run a menu from the environment"
> > >       depends on !AUTOBOOT_KEYED
> >
> > It looks like there's more stuff to move under a menu/if here?
> 
> Well this depends on !AUTOBOOT_KEYED so can't be in the 'if
> AUTOBOOT_KEYED'. But yes I can create a new 'if !AUTOBOOT_KEYED' for
> these two items. Normally we want 2-3 options to warrant an 'if', so I
> don't see this as a strong case.

Well, sometimes it seems like we abuse the "if" mechanic too.  It's not
short-hand for "avoid saying depends on" a few times, it's "hide these
things until we turn on another feature". 

But right here it looks like AUTOBOOT_USE_MENUKEY still needs to be
under "if AUTOBOOT" yes?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]