[PATCH 00/10] efi: Move some efi-loader code into a new shared dir

Mon May 26 20:27:32 CEST 2025

On Sat, May 24, 2025 at 04:35:03PM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Sat, 24 May 2025 at 15:41, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Sat, May 24, 2025 at 03:20:17PM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 23 May 2025 at 23:17, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Fri, May 23, 2025 at 09:23:15PM +0100, Simon Glass wrote:
> > > > > Hi Mark, Tom,
> > > > >
> > > > > On Fri, 23 May 2025 at 18:12, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> > > > > >
> > > > > > > Date: Fri, 23 May 2025 10:49:09 -0600
> > > > > > > From: Tom Rini <trini at konsulko.com>
> > > > > > >
> > > > > > > On Fri, May 23, 2025 at 05:36:52PM +0100, Simon Glass wrote:
> > > > > > > > Hi Heinrich,
> > > > > > > >
> > > > > > > > On Fri, May 23, 2025, 15:19 Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > > > > > > >
> > > > > > > > > On 23.05.25 15:06, Simon Glass wrote:
> > > > > > > > > > Some functions provided in lib/efi_loader are actually useful for the
> > > > > > > > > > app as well. This series refactors the Kconfig and directories a little
> > > > > > > > > > so that code is easier to share.
> > > > > > > > > >
> > > > > > > > > > As a starting point, it moves some filename and device-path functions to
> > > > > > > > > > the new directory.
> > > > > > > > > >
> > > > > > > > > > The next step would be to move device-path code over, but this will need
> > > > > > > > > > some discussion.
> > > > > > > > >
> > > > > > > > > Hello Simon,
> > > > > > > > >
> > > > > > > > > Overall the ideas in this series look fine to me. But this series does
> > > > > > > > > not apply to origin/next.
> > > > > > > > >
> > > > > > > > > Applying: efi_loader: Separate device path into its own header
> > > > > > > > > Patch failed at 0001 efi_loader: Separate device path into its own header
> > > > > > > > > error: patch failed: cmd/efidebug.c:8
> > > > > > > > > error: cmd/efidebug.c: patch does not apply
> > > > > > > > > error: patch failed: include/efi_loader.h:967
> > > > > > > > > error: include/efi_loader.h: patch does not apply
> > > > > > > > > error: patch failed: lib/efi_loader/efi_bootmgr.c:12
> > > > > > > > > error: lib/efi_loader/efi_bootmgr.c: patch does not apply
> > > > > > > > > error: patch failed: lib/efi_loader/efi_device_path.c:10
> > > > > > > > > error: lib/efi_loader/efi_device_path.c: patch does not apply
> > > > > > > > > error: patch failed: lib/efi_loader/efi_helper.c:6
> > > > > > > > > error: lib/efi_loader/efi_helper.c: patch does not apply
> > > > > > > > >
> > > > > > > > > Please, resend the series based on origin/next.
> > > > > > > > >
> > > > > > > > > Patches that are not based on upstream U-Boot cannot be reviewed via
> > > > > > > > > this mailing list.
> > > > > > > >
> > > > > > > > The app is quite behind in Tom's tree due to rejected series.
> > > > > > >
> > > > > > > It was not "Rejected", it was "Changes Requested", please stop
> > > > > > > mis-representing things.
> > > > > > >
> > > > > > > > In fact
> > > > > > > > the app is pretty limited on x86 and there is no Arm app at all.
> > > > > > > >
> > > > > > > > My current plan is to move forward and eventually Tom might take it
> > > > > > > > via a pull request.
> > > > > > > >
> > > > > > > > Do you have any other ideas?
> > > > > > > >
> > > > > > > > Perhaps this is something we could put on the agenda for a future call.
> > > > > > >
> > > > > > > There's nothing to discuss in a future call as step one is "post patches
> > > > > > > against mainline".
> > > > > >
> > > > > > I agree with Tom here.  I did look at some of the patches in this
> > > > > > series and now that I've learnt it doesn't even apply on top of
> > > > > > mainline it feels like I've wasted my time.
> > > > >
> > > > > There is a base commit in the cover letter, so you could look at that
> > > > > if you like.
> > > >
> > > > Yes, it's on a commit that's not in mainline. No one wants to look at
> > > > your personal tree.
> > > >
> > > >
> > > > > I plan to apply this series at some point, so any review is not a
> > > > > waste of time. I do respond to review feedback.
> > > >
> > > > And since it's not based on mainline, this is why people have told you
> > > > they don't want to review your changes. Since you aren't working on
> > > > mainline it's unclear if it will ever be in mainline.
> > >
> > > From my side I would suggest that you avoid bringing up this point on
> > > each patch. It doesn't seem to be working towards resolution. I am
> > > doing what I need to, for now, for the project to make progress.
> >
> > No, you're hindering the project with your insistence on posting patches
> > for your personal tree. I am trying to make it clear to the casual
> > developers of the project that it is your personal tree and not
> > mainline.
> >
> > > > Since this series in fact looks to be addressing some of the feedback
> > > > that was provided to one of the series you applied yourself (the other
> > > > being part of Casey's RFC), it would be much more effective to start on
> > > > the next branch now with the leg work on factoring the common EFI things
> > > > to be usable by EFI_APP and EFI_LOADER. This would lead to being able to
> > > > rebase other work on top of it and reduce the delta between your tree
> > > > and mainline. That's the only way it will get in to mainline.
> > >
> > > Yes I am acting on the feedback provided to Matthew's series and will
> > > get there in the end.
> > >
> > > More generally, there is a substantial amount of work needed on the
> > > EFI app and unfortunately this requires refactoring the EFI-loader
> > > code to avoid duplication. Honestly, I'm not even sure that I want to
> > > take that on, since my ability to get things into lib/efi_loader is
> > > pretty limited and it is seldom an enjoyable experience. I am pleased
> > > to see that the ANSI patch [1] is now in your tree, though, so thank
> > > you for that.
> > >
> > > Re the app, I would very much like to send a PR at some point, but
> > > there are other EFI patches not applied, so it is not trivial.
> > >
> > > I'm working with Heinrich on this and agreed with him that I'll take
> > > another look...
> >
> > If you intend to get things in to mainline, you MUST stop posting things
> > against your personal tree. You are making everything harder than it
> > needs to be. You do not have some special exemption from the normal
> > process. Iterative development on top of mainline, not some feature
> > branch that branched off ages ago.
> 
> So how about:
> - let me speed up your CI

We last left this argument at you saying it was too much work to
demonstrate speeding up CI on source.denx.de in u-boot-dm and me
not being able to see that as a serious point because it's something
like a dozen mouse clicks at most.

> - accept some of the things that you've previously rejected

That's probably going to continue to be a No because it's not like
people flipped a coin and went with "Nope, lets reject this". People
said you're wrong or going in the wrong direction and you disagree.

> In general, I think it would really help if you could adopt a more
> incremental approach to development. It would also help if we could
> discuss things every now and then, instead of all this email.

I don't know if you're being serious with this statement. I don't think
you can be serious with this statement. Doing things incremental would
mean doing them slowly, and in small reviewable chunks and 30-40-50 part
series followed by more of the same.

And there's a regularly scheduled community call every two weeks. I
would really only ask that someone other than you or I on the call take
the notes for any of these topics, just so it's clear later that any
misrepresentation is accidental.

-- 
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/20250526/2045e3b6/attachment.sig>