Applying patches to mainline (Was: Re: [PATCH v2 0/7] efi_loader: Add support for logging to a buffer)

[Tom Rini]

Picking just a few points..

On Thu, Dec 12, 2024 at 07:04:54AM -0700, Simon Glass wrote:
> Hi Ilias,
> 
> On Sat, 7 Dec 2024 at 01:18, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
[snip]
> > I haven't had time to look into that series, but I will. The problem
> > with reverts like that is
> > - I get the feeling you need some special code for some special
> > ancient chromebooks that no one cares or uses apart from you. That's
> > problematic overall. We can't just make a project look weird because
> > some hardware is special. It's the hardware that has to adapt, not the
> > other way around
> > - Reverts need to have a *very solid* explanation.
> >
> > For example, this is your x86 revert patches
> > https://lore.kernel.org/u-boot/20241112131830.576864-1-sjg@chromium.org/
> > and this is what actually got merged
> > https://lore.kernel.org/u-boot/20241129170814.768438-1-ilias.apalodimas@linaro.org/
> > Which one would you rather read a year from now debugging a similar issue?
> 
> Reverts are done to fix regressions. We don't need to justify a revert
> that breaks something. It is just a revert. You can see how this
> should be done at [1]. Tom applied it within a day, without any fuss,
> as he should. No, this is not a core issue either.

You most certainly do need to justify a revert. More often than not,
reverting a change to fix one platform breaks another platform. See for
example the whole discussion around SPI-NOR changes that may or may not
remain in v2025.01 at this point because they enable some platforms and
broke others, and only just about now *may* have fixed the others. The
revert you note in [1] is another good example of how and who needs to
justify a revert. You wrote 2e9313179a84 ("global_data: Drop
spl_handoff") and so it's a lot easier for me to accept a revert from
you. I assume that you tested all of the cases and applied it. I'm
likely to apply
https://patchwork.ozlabs.org/project/uboot/patch/20241209161434.6563-2-clamor95@gmail.com/
as soon as Svyatoslav says it's ready or asks because that's also their
code. And if you want to get picky about it, I do wish your revert in
[1] was more detailed about what was broken afterall, assuming a rework
of the intended change is coming, so it's clear what was wrong, not just
that it was wrong. A revert (and git bisect) are great for finding a
problem, but only sometimes the *fix* for a problem.

[snip]
> > Now I don't know who Kevin and BoB
> > are and I wish them luck, but that's hardly a readable commit message
> > or will mean anything in a year from now.
> 
> chromebook_kevin and chromebook_bob are a rk3399-based Chromebooks
> 
> Again, the commit message is not a core issue. If you found yourself
> in a reflective mood, you might want to consider how I feel about 5-6
> platforms which I care about a lot, being permanently broken in Tom's
> tree.

Commit message are a core issue, because they need to be clear (and not
strictly concise) about what a change is for and what a platform
requires. We're a community and the written word, including commit
messages is how we tell everyone else things. Including ourselves a year
down the line so we can see why we did what we did.

-- 
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/20241212/0bbc530e/attachment.sig>