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

Sat Dec 7 00:43:47 CET 2024

Hi Tom,

On Fri, 6 Dec 2024 at 13:12, Tom Rini <trini at konsulko.com> wrote:
>
> On Fri, Dec 06, 2024 at 12:19:38PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 4 Dec 2024 at 09:27, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 3 Dec 2024 at 18:29, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
> > > > >
> > > > > > From my side I'd like to change the conversation a little, to how to
> > > > > > land code, rather than why we should bother. "Code needs to land"
> > > > > > should be the motto. If someone has taken the time to create
> > > > > > something, our bias should be towards getting it in, with sufficient
> > > > > > changes to make it fit the project. There are cases where something is
> > > > > > just a bad idea, or should be done another way, but for people working
> > > > > > on major features or changes, biasing towards not landing the code is
> > > > > > just going to make them go elsewhere.
> > > > >
> > > > > This is the wrong approach I believe. The goal has always been and
> > > > > continues to be to have reviewed (whenever possible, our developer
> > > > > community is small) incremental change over time.
> > > >
> > > > Yes, I agree with that, but it isn't what I said.
> > >
> > > I don't know how else to interpret "Code needs to land". I'm not sure
> > > what reviewed patches we have that are outstanding, but also not fairly
> > > new. We do have patches outstanding, in general. Much of those fall in
> > > to the categories of:
> > > - Custodian is active, but also very busy (virtually everyone is a
> > >   volunteer so I have a hard time getting forceful with people that have
> > >   a large queue).
> > > - Custodian isn't active / no direct custodian and the code hasn't been
> > >   reviewed by anyone else.
> >
> > Here's my intention with 'code needs to land': to bias against people
> > saying "I don't like this; I don't care about your use case; please go
> > away"
>
> To me that applies an arbitrary nature to patches being rejected, which
> I do not see as being the case.
>
> > > > > Just because something
> > > > > has been posted a number of times does not mean it's ready to be merged.
> > > >
> > > > I didn't say that either.
> > >
> > > It's an impression you give however when you repost a series less than a
> > > day after last posting, without addressing all of the feedback.
> >
> > I'm going to ignore that comment as it is not helpful and
> > mischaracterises my contributions.
>
> I stand by it, and will let others judge if that's the impression they
> have, or not.

I'll put the relevant threads at [1], ditto.

>
> > > > > Your challenge today is that on patchwork you have over 150 patches
> > > > > covering a wide variety of topics and of which many series have
> > > > > technically-merited feedback that needs to be addressed in a technical
> > > > > manner.
> > > >
> > > > By my count I have about 10 series in progress, with a small number of
> > > > patches (< 10?) with pending feedback
> > >
> > > I'm not sure about that less than 10 number, in part because it's hard
> > > to keep track of which feedback was applied, and which not. And part
> > > because some of those series are places where I told you I'm unsure
> > > about the core concept but you asked and I'm giving you leeway to show
> > > me the end result.
> >
> > OK
> >
> > >
> > > > that isn't effectively just a
> > > > NAK.
> > >
> > > To be clear, "just a NAK" isn't the whole story. Those NAKs come with
> > > technical rationales and requests. But maybe they're just lost in the
> > > volume of emails you have in some cases. I know for example I mentioned
> > > a few times and places that we have tests today for booting the OS, when
> > > you mention that we need to add a test for booting the OS. We need to
> > > expand that test, yes.
> >
> > If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't
> > see any other way to interpret it,. So several boards in my lab have
> > been broken for a year.
>
> To be clear, you want to revert:
> commit 70fe23859437ffe4efe0793423937d8b78ebf9d6
> Author:     Simon Glass <sjg at chromium.org>
> AuthorDate: Wed Jan 3 18:49:19 2024 -0700
> Commit:     Simon Glass <sjg at chromium.org>
> CommitDate: Sun Jan 7 13:45:07 2024 -0700
>
>     fdt: Allow the devicetree to come from a bloblist
>
>     Standard passage provides for a bloblist to be passed from one firmware
>     phase to the next. That can be used to pass the devicetree along as well.
>     Add an option to support this.
>
>     Tests for this will be added as part of the Universal Payload work.
>
>     Signed-off-by: Simon Glass <sjg at chromium.org>
>     Reviewed-by: Tom Rini <trini at konsulko.com>
>     Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>
> Which means that you did not test your changes on your hardware at the
> time? And wish to (seemingly) replace that with an earlier version of
> the patch which was objected to on technical grounds at the time.
> Without explaining why the current code does not function on these
> platforms, only that it does not, for entirely unclear reasons.

It never worked on my board, though. I spent 6 revisions trying to
explain that. After number 6 [2] you got in touch and said that I
needed to rework it, which I dutifully did, in version 7 [3]. I did
that purely to unblock Raymond's series. I even put that in the patch:

"The discussion on this was not resolved and is now important due to the
bloblist series from Raymond. So I am sending it again since I believe
this is a better starting point than building on OF_BOARD"

Are you now saying I should have refused and argued for another few
revisions? Patience does have its limits.

>
> So I do not think I've arbitrarily nak'd the revert you posted, I've
> been asking you to explain why the code doesn't work. And I still don't
> have an answer as to why after at run time we don't find a valid
> bloblist here the fall-back use cases aren't functioning. To me that
> seems like the bug to investigate and explain. Reverting code to find
> when functionality broke is great, I do it all the time. Saying we need
> to revert because something broke *without* saying why it broke is a
> problem.
>
> > > > It isn't a particularly large number, if you add up the patches I
> > > > do in each cycle. It is in the nature of development and code review
> > > > that things are often not right the first time, or someone else has
> > > > another perspective, so I cannot see how this can be reduced.
> > >
> > > The easy way to reduce it would be to go from 10 series in progress to 1
> > > series in progress, and finish that before picking up series number 2,
> > > and so on. And then in the future, stop when you have more than 2 or 3
> > > in progress at once.
> >
> > That's obviously not practical for the amount I am contributing and
> > the length of time it takes us to get things in. I hope it isn't a
> > serious suggestion!
>
> It's extremely serious. For example
> https://patchwork.ozlabs.org/project/uboot/list/?series=435516&state=*
> is a 46 part patch series that touches 3 or 4 different areas and the
> only type of review that's feasible is "Does this cause the binary to
> grow too much? Or in unexpected ways?" And that sets aside that I
> believe I did ask you to rework "bootm: Allow building bootm.c without
> CONFIG_SYS_BOOTM_LEN" to refactor things so that SYS_BOOTM_LEN is always
> defined. That's not even a hard change, I say as someone that had to do
> that as part of Kconfig migration of oddly (ab)used symbol names.
>
> But in this case, your series that touch areas of code actively
> maintained by other people is even more relevant. Whereas areas without
> other active developers, or where you are the main developer, I can find
> some smaller limits on what must be reviewed all of your *EFI* has other
> active developers on it. Who are providing technical feedback, not
> arbitrary or stylistic feedback. Please be explicit about what you are
> suggesting in this case, so I do not mischaracterize your intention.
>
> > > But really, it feels like "Code needs to land" is another way of saying
> > > "Just a NAK should be ignored".
> >
> > Yes, a bias more towards that would be more healthy IMO. A NAK needs
> > to come with a full understanding of the use case, an alternative way
> > to meet the use case, once that doesn't involve boiling the Pacific
> > Ocean.
>
> As it stands, I do not like to, but sometimes feel I *have*to* break one
> board to unbreak another board and keep reminding both parties to
> resolve the issue so no board is broken. There has been, since I have
> taken over the lead of the project, at least once where I have
> over-ruled the relevant custodian and that has resulted in the custodian
> leaving. I both feel that was the right technical call, and regret it
> came to that, still. I do not want to adopt a more broad use of that
> policy. The default for absence of agreement must be to try again.

I see that Peter sent a few indignant emails. You can't win.

Regards,
Simon

>
> --
> Tom

[1]
https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sjg@chromium.org/
https://patchwork.ozlabs.org/project/uboot/patch/20230921015730.1511373-31-sjg@chromium.org/
https://patchwork.ozlabs.org/project/uboot/patch/20230924192536.1812799-37-sjg@chromium.org/
https://patchwork.ozlabs.org/project/uboot/patch/20230926141514.2101787-40-sjg@chromium.org/
https://patchwork.ozlabs.org/project/uboot/patch/20231226094625.221671-1-sjg@chromium.org/
https://patchwork.ozlabs.org/project/uboot/patch/20231228133654.2356023-1-sjg@chromium.org/
https://patchwork.ozlabs.org/project/uboot/patch/20231228194725.2482268-1-sjg@chromium.org/
https://patchwork.ozlabs.org/project/uboot/patch/20240104014919.413568-1-sjg@chromium.org/
https://patchwork.ozlabs.org/project/uboot/patch/20241201144240.1664398-5-sjg@chromium.org/
https://patchwork.ozlabs.org/project/uboot/patch/20241201144240.1664398-6-sjg@chromium.org/

[2] https://patchwork.ozlabs.org/project/uboot/patch/20231228194725.2482268-1-sjg@chromium.org/

[3] https://patchwork.ozlabs.org/project/uboot/patch/20240104014919.413568-1-sjg@chromium.org/