[PATCH v3 02/32] bootstd: Correct dependencies on CMDLINE

[Tom Rini]

On Tue, Oct 17, 2023 at 09:30:30PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 17 Oct 2023 at 07:32, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Mon, Oct 16, 2023 at 04:27:53PM -0600, Simon Glass wrote:
> >
> > > With recent changes over the last few years in boot/Kconfig it is
> > > no-longer possible to disable CMDLINE. It results in various link
> > > errors because some options which require CMDLINE are enabled
> > > regardless of whether it is available.
> > >
> > > Add the necessary conditions to fix this.
> > >
> > > Note that it would be better to have all commands depend on CMDLINE,
> > > but that is extremely difficult at present, since some functions use
> > > CMD_xxx to enable feature xxx. For example networking and environment
> > > have a number of problems to tease apart.
> > >
> > > Signed-off-by: Simon Glass <sjg at chromium.org>
> >
> > Since you later rework things to tease apart most, if not all of this,
> > please split out the patches which tease things apart in to their own
> > series so that the rest of this becomes reviewable and also reasonable
> > looking.
> 
> At this point I fear I have lost track of things, since there is just
> too much going on.

Yes, there is too much going on, which is why I wanted you to split this
up further.  I think we both agree that's a problem with this series.

> Just in this version I have dealt with:
> 
> - some EFI stuff (which needs tweaking)
> - teasing apart the CLI handling (which apparently doesn't go far enough?)

I don't think it's fair to call out EFI here, it's fairly well behaved
and no worse / oddly using CONFIG symbols than video or networking or
just about everything else is.

> Before this version I already looked at environment (which you say
> doesn't go far enough, and fair enough, but please review the code as
> sent, not what you wish it was).

Well, that's just it.  I did, and I wasn't entirely sure how you were
splitting things was right.  There was more stuff that either didn't
belong or should also have been moved, it wasn't clear.  The split I'm
suggesting there should make it clear.

> Perhaps you could apply whatever seems reasonable and then I can take
> another look? Or suggest some small changes that would allow progress
> to be made.

I guess I'll see what I can make some sense out of, yes.

> But do note that supporting booting without CONFIG_CMDLINE is a large
> effort, the subject of 5-10 series, perhaps more. It is not my
> intention to undertake that in this series, or before this series
> lands

Well, one of my issues here, especially with the idea of cycling back
and cleaning things up later is that I kind of assume you had intended
to do that in some places already, having done the good and important
task of breaking up the old common directory.

> The problem, also, is that without this series, it isn't possible to
> start that work. With this series, one can look at what code is
> missing and find a way to call boot functions (for example) bypassing
> the command line. Once you get it booting you are good. Without this
> series, you don't even know where to look, since calls to the command
> line happen silently.

That's where I disagree.  Step one is working towards being able to
build with CMDLINE disabled, and doing it in small reviewable chunks.
This is one big series that gives you "can build with CMDLINE disabled",
but while I'm usually not a fan of the Linux kernel's strict "break it
down per subsystem", I'd be a lot happier here, and we'd have quicker
progress too I bet, if instead of 1 30 part series we had 5 or 6 smaller
series.

> So I think this series is a big step forward and urge another look, please.

That's what I did with v3 yesterday and why I had a bunch of other
feedback.

But, yes, I'll take some parts of v3 and see what I can come up with.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231018/10d8314c/attachment.sig>