Attention is currently required from: Arthur Heymans, Jonathan Zhang, Johnny Lin, David Hendricks, Stefan Reinauer, Christian Walter, Deomid "rojer" Ryabkov, Tim Chu. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 )
Change subject: Revert "Add support for Intel Emmitsburg PCH" ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1: David, I wrote my response below first, than had another look at it. It seems to me that you simply underestimate everything, especially the work that is put on reviewers. If I had to derive a rule about -2 from this it would be like this:
Make sure that you see the full picture and carefully weigh alternatives before giving a -2 score.
But I don't think we should have rules like this. I think -2 privileges should be limited to few people that can learn to make their own rules and act appropriately. That includes you David, and I hope you learn from this incident (eventually; I'm not saying that my answer concludes anything).
While aggravating, a -2 is not a personal attack, and in this case I am pretty sure that it was an attempt to spark a conversation.
Indeed, and this is the crux of the matter. Nico's reaction when he saw something broken was to revert, which is understandable, while mine was to fix the code so it works as intended (which obviously did not include breaking other things). In this situation one of the possible fixes was literally 1 line of code (2 if you count the comment) in CB:57581. Other fixes proposed were a few lines longer. Simple enough, right?
Simple to write. But you ignore the review it seems. CB:57581 does two things and makes a lot of assumptions. One needs to find two SPI Guides to review it. I guess which alone might take multiple hours. It also lives from the assump- tion that the obvious regression is the only thing that is wrong with the ori- ginal commit. I'd estimate about 4 to 6 hours of review to clear the whole matter up and breaking the circle of having to revert things. This review would have been forced by you with your -2. And that with time pressure. Because you didn't allow the master branch to be fixed before the whole matter is settled.
My rationale for wanting to push those changes instead was because they're so tiny and got both chipsets working again so we could all move on to solve other problems. To me that seems a lot simpler, faster, and easier to follow in the git log than reverting CB:54965 with this patch, fixing the bad assumption introduced in CB:55651 using CB:57633, and then reverting the revert (this patch) to add Emittsburg back in. We could have had everything working within a day taking timezone differences into account.
I know you had somewhat good intentions, but if any of that is true, why are you not done yet? You could have followed the flow, instead you stopped the whole current with your -2. I'd estimate the same "within a day" if you hadn't given your -2. We can never tell if that had worked out, because we didn't take that path. You took the -2 path and, given that it's not Monday anymore, proved that the one-day assumption is wrong for your path.
And as per the gerrit guidelines, when giving the -2 I provided a concrete recommendation for how to fix things.
Let's see, here is what you wrote, IMHO not less cryptic than my later comments:
Ibex Peak isn't "detected",
This is true for the current master state and supports the revert not your -2. But there are these quotes so you probably meant something else.
it's the default when `content->ISL != 16`.
Is that supposed to explain the quotes? Let's assume yes, but why care about the ISL != 16 case? The working code detected Ibex Peak if ISL == 16 and warned when not.
The way that if-statement works also assumes no FD will have content->ISL > 16, unless `FLMAP2 == 0` (which isn't true in all cases).
The only if statement in this patch: `if (content->ISL <= 80)`. Who can follow?
We need to fix Ibex Peak detection
Again this is what the revert achieves.
or come up with a better handle these ISLs,
Absolutely an option. But if you have to force other people to help you find a solution, isn't there something wrong?
Also, I hope this is a boolean `or`. Otherwise you would have suggested that not fixing Ibex Peak detection is an option.
not prevent other platforms from getting added.
There is nothing that prevents adding other platforms. The easiest way is to ask for it on the mailing list; people in this community are incredibly helpful. In this one instance it's not much work actually, as the existing code seems to support most aspects of the new platform already. One just needs to compare the existing code to the datasheet to find the right path, I guess.
Alas, for the commit in question, it seems instead of looking into the datasheet, assumptions were made. These assumptions were wrong and ignored during review, and on top something broke due to a program- ming error.
That was the original -2 comment so far, you continued then with this:
See comments in CB:54965
This is what made me really angry because you forced me to look into another review (not a fix) which I had no time for. I do not think that forcing people to review can justify a -2. It's some- thing people really shouldn't do to each other.
My recommendation actually solved the problem for both sides. I didn't just throw a tantrum and spew unhelpful and condescending remarks at the other side.
See above, how was that cryptic -2 justification helpful? Neither do I see any clear recommendation nor is anything solved, not even by now.
Reviewing a <5 line patch that actually fixes things would have taken far less time, too.
Please prove that. If a patch makes additional assumptions and is based on assumptions made by a patch that is broken, how can that be easy to review? The number of lines really doesn't matter, you can break any program with a single line.
And when Edward stepped in to clarify that "we should revert and re-land with it fixed correctly" I took my -2 off. Perhaps that should be in the flashrom development guidelines to automatically resolve these sorts of questions: https://www.flashrom.org/Development_Guidelines
And what exactly do you suggest to add there? My biggest concern about your -2 was that it would force people to review (and fix because you wrote them rather hastily) your patches. Isn't that implicit that we don't use -2 to force other people like that?