September 2021 - flashrom-gerrit

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by Nico Huber (Code Review) 18 Sep '21

18 Sep '21

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? -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Sat, 18 Sep 2021 13:00:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Comment-In-Reply-To: David Hendricks Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for Intel Emmitsburg PCH
by Nico Huber (Code Review) 18 Sep '21

18 Sep '21

Attention is currently required from: Arthur Heymans, Jonathan Zhang, David Hendricks, Edward O'Callaghan, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57635 ) Change subject: Add support for Intel Emmitsburg PCH ...................................................................... Patch Set 3: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/57635/comment/632f44c9_3df9f285 PS3, Line 952: Ibex Peak is historically used as the default > So if we add a newer chipset with `ICCRIBA == 0x00` and more straps then it should become the defaul […] I wouldn't say "should" but it definitely could. Personally, as this branch seems complex enough already, I'd first check if there is no easier way to distinguish server descriptors from client ones. Also, please note that everything done about Emmitsburg so far (including what I wrote above) makes the assumption that it's right to return _LEWISBURG for it. I don't know if that is correct. -- To view, visit https://review.coreboot.org/c/flashrom/+/57635 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1edae9163b910c3831adc82c90840fa965479451 Gerrit-Change-Number: 57635 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: David Hendricks Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-Comment-Date: Sat, 18 Sep 2021 10:59:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by David Hendricks (Code Review) 18 Sep '21

18 Sep '21

Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, Johnny Lin, Stefan Reinauer, Christian Walter, David Hendricks, Deomid "rojer" Ryabkov, Tim Chu. David Hendricks 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: > 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? 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. And as per the gerrit guidelines, when giving the -2 I provided a concrete recommendation for how to fix things. 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. Reviewing a <5 line patch that actually fixes things would have taken far less time, too. 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 -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Sat, 18 Sep 2021 05:58:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: David Hendricks Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for Intel Emmitsburg PCH
by David Hendricks (Code Review) 18 Sep '21

18 Sep '21

Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, David Hendricks, Edward O'Callaghan, Angel Pons. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57635 ) Change subject: Add support for Intel Emmitsburg PCH ...................................................................... Patch Set 3: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/57635/comment/8eab8fa1_b5f5ac99 PS1, Line 16: TESTED=tried on a server with Intel Emmitsburg PCH, flash update : was successful. This server, however, does not have flash chip : installed, it instead has em100 emulator connected. > I was able to run another round of tests on Archer City CRB with this patch chain and confirmed that […] Done File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/57635/comment/22c6253a_6634ae01 PS3, Line 952: Ibex Peak is historically used as the default > Just because it was the newest mainstream platform on the `ICCRIBA == 0x00` path. So if we add a newer chipset with `ICCRIBA == 0x00` and more straps then it should become the default? (ignoring the `FLMAP2 == 0` path) -- To view, visit https://review.coreboot.org/c/flashrom/+/57635 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1edae9163b910c3831adc82c90840fa965479451 Gerrit-Change-Number: 57635 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: David Hendricks Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-Comment-Date: Sat, 18 Sep 2021 05:30:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Jonathan Zhang <jonzhang(a)fb.com> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: David Hendricks Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors: Normalize chipset detection
by David Hendricks (Code Review) 18 Sep '21

18 Sep '21

Attention is currently required from: Nico Huber. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55651 ) Change subject: ich_descriptors: Normalize chipset detection ...................................................................... Patch Set 5: (2 comments) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/55651/comment/72a6b325_b5a1d982 PS5, Line 936: if (content->ISL <= 16) : return CHIPSET_5_SERIES_IBEX_PEAK; > Please give some number example, I don't follow. Let's say somebody comes along and wants to add support for NEW_CHIPSET with an ISL value of 80. To do that in a manner consistent with the pattern of this code, they'll probably do something like: if (content->ISL <= 80) return NEW_CHIPSET; With this patch (CB:55651) applied it's unclear if the intent of the default case is to positively detect Ibex Peak, or to simply return Ibex Peak as a reasonable default when nothing else is explicitly detected. In the former case, adding detection for NEW_CHIPSET breaks Ibex Peak detection. In the latter case, adding detection for NEW_CHIPSET makes [11,80] the range for NEW_CHIPSET (assuming FLMAP2 == 0, of course) and [81,255] for Ibex Peak, which seems totally fine since we don't care about ISL values > 80. The original code made this completely unambiguous by using a control statement to enforce an allowable range for Ibex Peak detection. https://review.coreboot.org/c/flashrom/+/55651/comment/5c1ebd23_2ba5ff10 PS5, Line 953: return CHIPSET_8_SERIES_LYNX_POINT; : warn_peculiar_desc(true, "Wildcat Point"); : return CHIPSET_9_SERIES_WILDCAT_POINT; > We don't know any difference in the descriptor between these two. So […] Fair point - after a bit of searching it seems Wildcat Point is a refresh of Lynx Point. A developer with a Wildcat Point system might be confused by matching Lynx Point - perhaps a comment would be helpful in situations like this. -- To view, visit https://review.coreboot.org/c/flashrom/+/55651 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic5a5fee870202173b3a9813b03ec261e8ee45155 Gerrit-Change-Number: 55651 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Sat, 18 Sep 2021 05:21:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by Stefan Reinauer (Code Review) 17 Sep '21

17 Sep '21

Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, Johnny Lin, David Hendricks, Christian Walter, Deomid "rojer" Ryabkov, Tim Chu. Stefan Reinauer 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: > > From an outside perspective without having skin in the game of this specific issue: Forking is not always bad. If there are two opposing trains of thought that can not be unified properly, or if there is a lack in will to make that happen, allowing everybody to move on and make their own version better for themselves and their customer base can even help to overcome differences in the long run (see egcs vs gcc a long time ago). > > I agree and this is why I bring it up from time to time without meaning > it as a threat. At some point it is better to try this out and see if it is what you think it is. For your own peace and that of everybody else. I for one would be excited to see different directions take place at the same time. That's much more energizing than yet another community where people don't get along. > > Nico, you are unhappy with about everybody trying to work with you here, > > "everybody": About 1 out of 10 people and only 1 in total long-term. Admittedly I am bad at keeping lists. It seemed to have been everybody that I have worked with, at some point, but that may very well only be a small portion of the flashrom contributors. I apologize if I managed to become your long-term foe. > > and you are offering to fork the project fairly frequently. > > About once per year since Google tried to converge their fork with brute force. Look, "Google" is not trying to do anything with brute force here. David, for one (who has not been at Google for many years), never really bothered if upstream would like the code contributions that he managed to integrate from the Chrome OS team. He was criticized for letting things diverge, and so another person came along, and another, and none of them were seemingly able to make much progress here, even decade long contributors to coreboot were in this list. That is not a sign of agressive convergence or brute force. Now, Edward came along, and Edward is an assertive guy that follows up and follows through, and that can be both a blessing and a curse. I myself have been a complete jerk to Edward when he tried to merge LLVM support into coreboot at the time many years ago (and I never appropriately apologized), because I felt that he was diverging the direction of the project away from the stable construct that we had at the time to something that is too in love with the tools and programming languages used. I felt at times the same about Rust, go, and Ada. Today I feel that I would have been better off seeing Edward's work as the investment that it was, instead of a destructive intrusion. Luckily by the time Ada came around, I had already learned a lesson there. > > It is unhealthy for you and everybody else involved in this project to keep the threat of breakup over everybody's head. I would encourage you to think about whether that is what you want and if so, I will gladly help you with resources (hosting, etc) to get your fork started and off the ground. I will also gladly pay for a domain for you for the first couple of years. > > That is a nice offer. But what is holding me back is not the lack of > resources, but the people who ask me to continue and get another > release done eventually. You can of course do another release also from a new project. Life does not stop at any single moment but the last one. I hope you will be releasing software for decades to come, no matter what the project. My understanding was also that you have officially resigned from the flashrom project a while ago? > > We have all started out as friends in this project, and it is good to remember that in the end we share 95% of the same goals, even if we like to fight to the last breath over the remaining 5%. Let's not use those 5% disagreement as a reason to create an environment in which none of us wants to be. It's not worth anybody's life time. > > This is one of the misunderstandings in this project. We don't share 95% > of the same goals. And I don't think it's wise to ignore that. On one hand, > there is flashrom, the thing that is packaged in long-term stable OS distri- > butions that is used by humble users and needs to be 100% reliable so they > don't brick their machine (or at least reliable enough so we are not buried > under support requests). On the other hand, there is flashrom, a development > tool. For most developers flashrom is the latter, I think. > > Please don't ignore diversity. As grown-up people we can often hold conflicting goals in mind at the same time. I would still hold up that 95% of our developers are interested in creating a tool that is both capable of stable OS distribution as well as use as a development tool. The degree to which one lets the project encompass both of those goals and juggles the objectives successfully is leadership. I don't think that I ignore diversity, although your line of thought is compelling there and I would like to know more about how you get to that conclusion. > > I am open to suggestions but this way of treating each other needs to stop. > > Ok, then we need to talk about how people are treated. Please, Stefan, advice > us what we can do better. I think - and this goes for all of us - that we will be better off putting us into the other person's shoes before alleging that they are only out to hurt us, or even worse "the project" in a religious assumption that only one side can determine what is good for "the project". > I've been working my ass off last sunday for the project. Partially trying to > identify regressions and to fix them. I tried to revert one change that was > logically wrong and introduced a regression. I got a -2 instantly which was > (allegedly) only set to give priority to the author's own unwritten patches. I really appreciate that you are working hard to accomplish a more stable flashrom. 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. I am fairly certain that I have seen similar behavior from you in the past (and I'm sure the other side was upset too ;). Nico, I also don't think that your code or reviews are the only ones that are infallible or that your work is worth significantly more than those of other folks in the community - and that applies as well to Edward and David and the others that are trying to create a working solution for Emmitsburg here. > Do you already see anything there that needs to stop? Or shall I continue to > describe what happened? Trying to figure out your nuance here, but I'm listening. -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 17 Sep 2021 19:07:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: David Hendricks Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for Intel Emmitsburg PCH
by David Hendricks (Code Review) 17 Sep '21

17 Sep '21

Attention is currently required from: Jonathan Zhang. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54965 ) Change subject: Add support for Intel Emmitsburg PCH ...................................................................... Patch Set 3: (1 comment) File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/54965/comment/314fbd24_d732e80c PS3, Line 2076: enable_flash_c620 > What SPI guide do you mean? 606161 does have chapter 26 for SPI. […] The EBG SPI programming guide is document 619386. -- To view, visit https://review.coreboot.org/c/flashrom/+/54965 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a1bb7467e693d1583aa885fa0e277075edd4a3e Gerrit-Change-Number: 54965 Gerrit-PatchSet: 3 Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Comment-Date: Fri, 17 Sep 2021 18:54:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for Intel Emmitsburg PCH
by Jonathan Zhang (Code Review) 17 Sep '21

17 Sep '21

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54965 ) Change subject: Add support for Intel Emmitsburg PCH ...................................................................... Patch Set 3: (1 comment) File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/54965/comment/0be2607f_d2daff09 PS3, Line 2076: enable_flash_c620 > Looking at the EDS (606161, rev 1.3), it seems closer to 300 series. […] What SPI guide do you mean? 606161 does have chapter 26 for SPI. What specific info are you looking for? -- To view, visit https://review.coreboot.org/c/flashrom/+/54965 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a1bb7467e693d1583aa885fa0e277075edd4a3e Gerrit-Change-Number: 54965 Gerrit-PatchSet: 3 Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Fri, 17 Sep 2021 18:06:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Revise mock chip definition and usage
by Nico Huber (Code Review) 17 Sep '21

17 Sep '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57437 ) Change subject: tests: Revise mock chip definition and usage ...................................................................... Patch Set 4: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/57437/comment/686afcec_dd69fa36 PS4, Line 24: fresh copy on the stack. This made me chuckle a little :) What I always tell people when the commit message is a longer list: This list perfectly tells us what separate commits this could have been. More, smaller commits are usually cheaper. But please don't split this one, as it's almost through review ;) File tests/chip.c: https://review.coreboot.org/c/flashrom/+/57437/comment/00c4c9b2_4c4c4ab2 PS3, Line 24: #define CHIP_TOTAL_SIZE 8388608 > Wow useful macros. I didn't know of them, probably because I was looking into flashchips. […] Makes sense. https://review.coreboot.org/c/flashrom/+/57437/comment/9e296448_9d9a9219 PS3, Line 128: static struct flashchip chip_8MiB = { Looks good. > For example, it is not a deep copy. Hmmm, let's have a look at that. We also use a shallow copy of the entries in the `flashchips.c` list (flashrom.c:787). Looking for pointers among the struct members, they are all pointing to `const` strings or functions. The strings don't have to, but in this case are global i.e. not dynamic memory, same for functions. There's one exception, the `wp` member. But I'm not sure if it is non-const on purpose. It's part of some WIP development, so I would ignore it for now. -- To view, visit https://review.coreboot.org/c/flashrom/+/57437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a Gerrit-Change-Number: 57437 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 17 Sep 2021 14:34:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add tests to read from chip
by Nico Huber (Code Review) 17 Sep '21

17 Sep '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57326 ) Change subject: tests: Add tests to read from chip ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/57326 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia57781ebc670c7bd6197e56fe8a20651a425c756 Gerrit-Change-Number: 57326 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 17 Sep 2021 14:20:13 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2021