On Fri, Dec 06, 2024 at 04:43:47PM -0700, Simon Glass wrote: > Hi Tom, > > On Fri, 6 Dec 2024 at 13:12, Tom Rini <tr...@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 <tr...@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 <tr...@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 <s...@chromium.org> > > AuthorDate: Wed Jan 3 18:49:19 2024 -0700 > > Commit: Simon Glass <s...@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 <s...@chromium.org> > > Reviewed-by: Tom Rini <tr...@konsulko.com> > > Reviewed-by: Ilias Apalodimas <ilias.apalodi...@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.
I'm saying that I assumed what you pushed functioned on your hardware, which is why I'm surprised now that it doesn't. I'll look through all of the links later, but I think you still haven't explained _why_ this solution doesn't function on your platforms (now or last year) and what your platforms need (as in, can't change) / want (you would need to rework some other part of the firmware stack to get them in agreement on that platform). > > 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. I assume this is about the LED series, and I further assume you forgot / missed where he explained that removing the legacy code is a functional user visible usability regression on the pinephone. -- Tom
signature.asc
Description: PGP signature
