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

[Simon Glass]

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
- accept some of the things that you've previously rejected

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.

>
> And please note, *I* did not take the ANSI patch. Heinrich did. He and
> Ilias are the EFI_LOADER custodians. That's why it's in mainline.

Well I'm grateful in any case. It has been *infuriating* to have
random ANSI characters spewing into my terminal, to no useful purpose.

Regards,
Simon