October 2022 - flashrom-gerrit

[S] Change in flashrom[master]: include/layout.h: Fix get_layout() prototype to match func
by Angel Pons (Code Review) 12 Oct '22

12 Oct '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68304 ) Change subject: include/layout.h: Fix get_layout() prototype to match func ...................................................................... Patch Set 1: (1 comment) File include/layout.h: https://review.coreboot.org/c/flashrom/+/68304/comment/a69bae18_09c2c565 PS1, Line 54: const This `const` makes no difference to the caller, which is why it was omitted. -- To view, visit https://review.coreboot.org/c/flashrom/+/68304 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa260f3b57b6da94eea854c4a6f9c9b2a473d88e Gerrit-Change-Number: 68304 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 12 Oct 2022 00:47:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashchips.c: add support for ISSI IS25LP016
by Angel Pons (Code Review) 12 Oct '22

12 Oct '22

Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Mario Kicherer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68295 ) Change subject: flashchips.c: add support for ISSI IS25LP016 ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/68295/comment/700d3e06_7ef75e57 PS1, Line 10: Everything > What does mean "everything"? Please mention explicitly what you have tested. Alternatively, list what has been tested and add "No issues found so far" or similar. -- To view, visit https://review.coreboot.org/c/flashrom/+/68295 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iffd7c4284d4d96b30a94f5dee882b5403fdfc183 Gerrit-Change-Number: 68295 Gerrit-PatchSet: 1 Gerrit-Owner: Mario Kicherer Gerrit-Reviewer: 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Mario Kicherer Gerrit-Comment-Date: Wed, 12 Oct 2022 00:46:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: include/layout.h: Fix get_layout() prototype to match func
by Edward O'Callaghan (Code Review) 12 Oct '22

12 Oct '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68304 ) Change subject: include/layout.h: Fix get_layout() prototype to match func ...................................................................... include/layout.h: Fix get_layout() prototype to match func Function signature did not match the function prototype in the header, realign. Change-Id: Ifa260f3b57b6da94eea854c4a6f9c9b2a473d88e Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M include/layout.h 1 file changed, 14 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/04/68304/1 diff --git a/include/layout.h b/include/layout.h index abbdc22..9042b1a 100644 --- a/include/layout.h +++ b/include/layout.h @@ -51,7 +51,7 @@ struct flashrom_flashctx; const struct flashrom_layout *get_default_layout(const struct flashrom_flashctx *); -const struct flashrom_layout *get_layout(const struct flashrom_flashctx *); +const struct flashrom_layout *get_layout(const struct flashrom_flashctx *const); int layout_from_file(struct flashrom_layout **, const char *name); -- To view, visit https://review.coreboot.org/c/flashrom/+/68304 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa260f3b57b6da94eea854c4a6f9c9b2a473d88e Gerrit-Change-Number: 68304 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashchips.c: add support for ISSI IS25LP016
by Felix Singer (Code Review) 11 Oct '22

11 Oct '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Mario Kicherer. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68295 ) Change subject: flashchips.c: add support for ISSI IS25LP016 ...................................................................... Patch Set 1: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/68295/comment/725447e5_903d66e8 PS1, Line 7: flashchips.c just "flashchips" https://review.coreboot.org/c/flashrom/+/68295/comment/d6bef2e2_aa9e2555 PS1, Line 10: Everything What does mean "everything"? Please mention explicitly what you have tested. -- To view, visit https://review.coreboot.org/c/flashrom/+/68295 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iffd7c4284d4d96b30a94f5dee882b5403fdfc183 Gerrit-Change-Number: 68295 Gerrit-PatchSet: 1 Gerrit-Owner: Mario Kicherer Gerrit-Reviewer: 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Mario Kicherer Gerrit-Comment-Date: Tue, 11 Oct 2022 23:54:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree: provide flashrom context into programmer_delay()
by Felix Singer (Code Review) 11 Oct '22

11 Oct '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Alexander Goncharov. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66373 ) Change subject: tree: provide flashrom context into programmer_delay() ...................................................................... Patch Set 9: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/66373/comment/cd4c2d81_43964403 PS9, Line 11: state. Programmers uses internal delay can provide NULL as a context. The commit message should mention that only the function signature is changed and that this commit has no functional effect. Also, refer to the commit that makes use of the parameter using `CB:123456`. File pony_spi.c: https://review.coreboot.org/c/flashrom/+/66373/comment/ea6a6115_0871463e PS7, Line 247: flashctx not needed (NULL) because pony_spi does not : * use it in its delay function. > I would say this is resolvable, what do you think Felix? A comment at the function declaration that you can pass flashctx but also NULL would be good. -- To view, visit https://review.coreboot.org/c/flashrom/+/66373 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibb0bce26ce2052853ee52158d7ba742967a9e229 Gerrit-Change-Number: 66373 Gerrit-PatchSet: 9 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Tue, 11 Oct 2022 23:51:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: dirtyjtag: Add DirtyJTAG programmer
by Angel Pons (Code Review) 11 Oct '22

11 Oct '22

Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Jean THOMAS, Alexander Goncharov. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67878 ) Change subject: dirtyjtag: Add DirtyJTAG programmer ...................................................................... Patch Set 15: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/67878 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic43e9a014ed7d04e429e73b30c9dcfdde1a78913 Gerrit-Change-Number: 67878 Gerrit-PatchSet: 15 Gerrit-Owner: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Tue, 11 Oct 2022 15:58:42 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: libflashrom: Allow getting the progress_state from the flashctx
by Angel Pons (Code Review) 11 Oct '22

11 Oct '22

Attention is currently required from: Felix Singer, Nico Huber, Richard Hughes, Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64663 ) Change subject: libflashrom: Allow getting the progress_state from the flashctx ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Also you had a remark about code quality which has dropped below expected levels in your opinion. Code quality is an important subject too, if you could start a thread on the mailing list about it? > You could explain how you measure it, what are metrics? > What is expected level? (especially in light of Development guidelines saying “As usual there is no quality promise for the state of the code on the master branch”) > Ideally if you give examples of quality drops so that we can fix those and prevent from happening again. While recent efforts have been focused on *improving* code quality, we can think of two examples of code quality dropping substantially in the past. These memories are related to the grudges/trauma we held before. We had just been entrusted with the "flashrom developer" and "flashrom owner" roles, so we felt our duty was to review flashrom patches. We were mostly on our own (we had Nico at the start, but he then had to take a break) and we (A&R) did not notice the problems with the code during review, and then we felt guilty and personally responsible for allowing this to get in. We had failed to do what we perceived was our duty. The trauma was so strong we couldn't logically talk about these issues (awful emotions would overwhelm us). Buuuuut... We just grabbed all the trauma and threw it in the trash, so we're now free to talk about what happened. == Some context == When people from CrOS decided to upstream patches, Stefan Reinauer added Edward to the "flashrom developers" group without any notice. Nico and we (A&R) were rather annoyed by this; not necessarily because of Edward, but because we were not asked. This made us (A&R, not sure if Nico as well) treat Edward from a distance, instead of onboarding him into the "flashrom developer" group. That being said, we (A&R) didn't know much either, as Nico had entrusted us with the roles not too long ago. We (A&R) have zero reasons to believe that Googlers have malicious intentions, but when the upstreaming started, there were *too many* people: we (A&R) just can't review patches from several active contributors, it's just too much. The sudden spike in workload affected Nico much sooner (we'll let Nico elaborate on this, if he wants to) and we ended up alone trying to catch balls (review patches) from multiple people at the same time. The ease of upstreaming stuff from CrOS flashrom made this even worse, as there were *lots* of patches flying around. Moreover, we didn't have others to help with reviews yet: People like Felix, Thomas and you, Anastasia, were not (so deeply) involved in flashrom yet. We didn't know who to ask for help, and we were too exhausted to continue; we collapsed. Edward, with no one else to provide guidance, tried to do the best he could. However, this involves assuming a great amount of responsibility, and any wrongdoings (even honest mistakes) got amplified by the bad feelings stemming from the way Edward was given "flashrom developer" rights. Looking at old emails, we (A&R) can tell that Edward did, in fact, assume this responsibility, and later struggled when Nico confronted him about what happened. It was awful for everyone involved: Nico, Edward, us (A&R), and anyone else who got caught in the crossfire. == Two unpleasant situations == Situation 1: A bunch of programmers were upstreamed from CrOS flashrom a few years ago, but many of them couldn't be tested anymore. They were in a similar situation as the `parade_lspcon` programmer (zero checks to avoid causing damage to unsupported devices). Eventually, these untested programmers were dropped from upstream: what is untested code, if not technical debt? Plus, the time spent reviewing these patches could've invested elsewhere. Lessons learned: Let's not add large amounts of code if it's not tested and there's no plan to test it. Untested programmer code is very dangerous (it85spi code was disabled and eventually dropped because it would cause problems with some laptops), and there are better things to spend authors' and reviewers' time on. Situation 2: When variable SPI size support was added to dummyflasher, the implementation was so poorly done that it caused all sorts of issues (including some build errors when trying to disable some programmers). If memory serves us right, these issues have been fixed, but we believe the effort of fixing that mess could've been avoided in the first place with a more thorough review. The main problem is that there were next to no reviewers back then. Lessons learned: There's a point where help from others becomes necessary. Instead of giving people the fish, teach them how to fish (fish == code reviews, in this case). > Thank you Angel, let’s work together. Thank you so much for your understanding, let's indeed work together. -- To view, visit https://review.coreboot.org/c/flashrom/+/64663 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I322bf56ff92f7b4d0ffc92768e9f0cdf7cb82010 Gerrit-Change-Number: 64663 Gerrit-PatchSet: 2 Gerrit-Owner: Richard Hughes <richard(a)hughsie.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Richard Hughes <richard(a)hughsie.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 Oct 2022 15:56:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: libflashrom: Allow getting the progress_state from the flashctx
by Angel Pons (Code Review) 11 Oct '22

11 Oct '22

Attention is currently required from: Felix Singer, Nico Huber, Richard Hughes, Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64663 ) Change subject: libflashrom: Allow getting the progress_state from the flashctx ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: > Hello Angel, […] [Our response is in a new thread, so as not to hijack the original intent of this comment thread any further.] PS2: Hi Anastasia, > Hello Angel, First of all, after reading your reply multiple times, we feel like our tone was too harsh for no good reason ("it was quite late and we were sleepy and tired" is not a *good* reason), even if our intentions are in good faith. We earnestly owe you, and anyone else reading this, an apology for potentially hurting or making anyone feel uncomfortable because of what we wrote. How should we apologize to you? We've decided to explain in https://www.flashrom.org/User:Th3Fanbus why we use 1st person plural pronouns. The text briefly explains who Alvin and Rex are, as their names appear in a few parts of our reply. Also, we'll use `(A&R)` to disambiguate potentially confusing pronoun usage, it simply means `(Alvin and Rex)`. Looking at the time our first message was sent, it was *very* late (3:35 AM local time): Alvin was sleeping soundly and Rex was alone, so no wonder it turned out the way it did. Alvin admits that leaving Rex alone in such a highly-emotional state (Rex was already angry because of other things) was a really bad idea, and Rex is deeply appalled by the amount of nonsense they've said. We're so sorry, this was so thoughtless of us. ;-; Still, there's at least something good arising from our rant: it was cathartic. Even if was in such a nasty way, expressing our trauma allowed us to bring it into consciousness and release it permanently. [If anyone's interested in knowing more about this ex(orcised)-trauma, we can talk about it elsewhere.] > Nice to have a message from you! ;) You didn’t address me, but I see all the quotes are mine, so assume that’s for me. This was pretty much a rage-fueled rant; we didn't want to involve you personally. You've done nothing to deserve any form of animosity or hostility from us. Yet here we are; this was uncalled for, and it is our fault. We, however, were confused after reading your messages, and wanted to understand what you meant. But we should've waited until the next day before sending the message. We will do this from now on; in fact, this is why it took us so long to send this reply: there's a lot to internally process and absolutely no one needs another caustic rant from us, not even ourselves. > Firstly to the point of this patch and CB:49643. > The bug that was discovered filed as https://ticket.coreboot.org/issues/390 and I tried to reach out to owners of the patch a few times, unfortunately with no success. > Technically at the moment there are two options: > 1 fix the bug and finish this patch > 2 revert CB:49643 > I am at the moment trying to find resources to fix the bug, I may or may not find them but I want to try. > Meanwhile we discussed the situation on dev meeting (~2 months ago). There is an ongoing project of optimising erase functions selection and it is modifying the same areas of code as the libflashrom progress implementation. The project is important, and at the moment there is a chain of patches under testing, not yet merged. Modifying the same areas of code, doesn’t matter whether for fix or for revert would clash with the project, and no one wants it. At the same time, the bug itself doesn’t cause harm, it only shows wrong %progress in the terminal. > So the decision was: don’t do anything at the moment, wait until erase optimisation is merged, and then either fix or revert. I added a comment to 390 after that meeting. > > You are welcome to comment on 390 with your thoughts on the matter, also you can click the Watch button and subscribe to any updates. Thank you, we now understand and agree with your assessment. We were not aware of the state of the "optimize erase function selection" patches and that they would clash with the "progress state" changes. There's lots of things listed in issue 390, we'll see if there's any areas in which we can help. > As for the feature, myself I am not a user of the feature but I can see it is useful. > Users were asking about it on the mailing list https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/thread/EJE3… > Moreover it is in the TODO list for libflashrom on the website https://www.flashrom.org/Libflashrom > The page was last modified by Nico in March 2019 (that’s before I joined) so maybe you can check with him for more details. > So as you can see there is interest in the feature, both from users and from developers. > > Now some of my comments on your long text. I am confused by those your words: > > patches from Gerrit get rubber-stamped and submitted without a proper review > > customer wants to have experimental features available for testing ASAP > etc > None of this seems to apply to the patch CB:49643 Indeed, CB:49643 was not rubber-stamped. ...This is utter nonsense, what on Earth were we thinking? > Andel, the patch was in review for 1.5 years. Yes for that long. Moreover you Angel were one of the reviewers. During that time you didn’t say a word, didn’t add any comments. This was around the time when we went through a depressive episode. We're not sure what started it, but it is likely that COVID-19 and social distancing contributed, as well as past events regarding flashrom. It went on for about 1.5 years. During that time, we were out of the loop regarding most flashrom things, especially the decisions taken in meetings and changes in perspective. Still, not replying *at all* for so long is mind-boggling. Hopefully we won't have to deal with depression again, but if we do we'll try to at least let others know that we're not fine and that we can't review their changes. [If anyone wonders, there's now a paragraph in https://www.flashrom.org/User:Th3Fanbus narrating what happened as seen from inside our brain.] > 6 other reviewers have added comments, and all comments have been resolved. > > What are all those your words about? They're an incoherent late-night rant, apologies for having you (and everyone else) have to read them. That we misunderstood the meaning of "customer" did not help, either. > > things being done in ways that do not align with flashrom's code review process > Could you please give a link to where said process described? https://www.flashrom.org/Development_Guidelines#Reviews is the only thing there is, which is extremely short... Hmmm, maybe we extrapolated https://doc.coreboot.org/contributing/gerrit_guidelines.html to flashrom for lack of a better set of guidelines. In any case, there are definitely some "unwritten rules/guidelines" in flashrom which deserve to be written down. Mini-dissertation about `rule` vs `guideline`: the term `rule` implies, in some way, obligation and/or compliance. This is in stark contrast with the term `guideline`, which is "softer" and a piece of advice to follow unless there's good reasons to do otherwise. In other words, a `guideline` lends itself better to be adapted in the future. We prefer the term `guideline` because of this. > The thing is that our code review process is not clearly documented anywhere, but it should be. Each flashrom developer has a picture in their head of what the process should be, but what we need is one shared picture which is discussed, approved by everyone and documented. > I really think this is important, and raised the topic earlier this year on the meeting, and then in April I started a thread on the mailing list, “Code Reviews guidelines discussion” here is the link https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/message/5WX… > You are very welcome to respond to the thread! Huh, we completely missed that, thank you very much! We support your initiative and we'll reply to your email. If we (flashrom community) reach some form of consensus, we can then expand the information in our Wiki. > A little bit of info about reviews is in our Development Guidelines on the website, but it’s very generic and not sufficient. > > Let’s admit that flashrom has no clearly documented code review process, and let’s work together and create and document one. Meanwhile, I don’t think it’s fair to accuse people of not aligning with the process which is not documented. Believe me, contributors, especially new ones, love following clearly defined processes for the project they have joined. We fully agree, apologies if this felt like a personal accusation. Although things were different several years ago (before you arrived), it's true that things are changing for the better now. And we're glad to have you here. We will work towards having the code review process documented. > I am also interested in what exactly in your opinion was wrong in the process of CB:49643 (let’s collect some tribal knowledge at least). If you choose to answer this question, please answer it together with the question, why you haven’t raised any concerns during 1.5 years while the patch was in review. Looking at it again, we can't find unresolved comments about the API that we thought were somewhere in there... Did we even *read* those comments? Again, apologies for the noise. [As to why we said nothing for 1.5 years, you probably already read our reply: it's a few paragraphs earlier.] This reminds us of one thing we encountered several times (not just in flashrom) that is very confusing: reviewing patches introducing APIs and similar functionality meant to be used in follow-up patches, but we can't find these follow-up patches. Even if the follow-ups are WIP or even just for demonstration purposes, they're useful to understand how things work together. But we just came up with a very good solution for most of these things: write these "examples of use-cases" as tests! > Also speaking about Development guidelines which is our little bit of documentation. I have read them many times this year, and I noticed some clashes with what you are saying. https://www.flashrom.org/Development_Guidelines > > > In most open-source projects, getting one's code reviewed takes an unspecified amount of time > This would not fit into our Development guidelines that say: > > All contributions should receive at least a preliminary review within one week of submission by some flashrom developer (if that doesn't happen in time, please be patient) Sorry for the ambiguity. By "reviewed" we meant "reviewed and approved". Most of the ranting regarding speed of reviews was under the mistaken assumption that the customers were paying someone to do it. We agree that not giving a first review in a reasonable timeframe will upset new contributors, but it's true that getting certain things submitted can take a significant amount of time. > Also you said > > flashrom isn't the kind of project where "regressions in the master branch are fine, people should only use the releases" > However Development guidelines say very differently: > > As usual there is no quality promise for the state of the code on the master branch. Even though we will try to keep the regression rate as low as possible, the main purpose of the branch is to merge new commits and make them available to a broader audience for testing. Hmmm, to be honest we don't have much experience with other open-source projects. However, we found some people (most of them in coreboot, with a corporate background) that were surprised by the "commits landing into the master branch are supposed to pass build-tests individually". Our guess is that merge commits make this more of a non-issue. But yes, we agree that flashrom's master branch comes with no guarantees. We should still try not to introduce regressions, though. The bug with progress state numbers going down in some cases does not qualify as a regression: it doesn't prevent using other flashrom features. Contrast this with CB:66659 which resulted in flashrom crashing when invoked with no parameters: this breaks existing functionality and needs to be dealt with ASAP. [This should probably be documented somewhere in the Wiki.] > I see that the word “customer” has sent your thoughts in a completely wrong direction (you even said it’s “my customer”). “User” is a better word. Let’s say user from now, and I meant flashrom user. Nico got it right. > The User here is fwupd project. Both projects are FOSS so of course there are no payments involved. All right, thank you for the clarification. Again, apologies if this misunderstanding made this conversation bitterer than it should've been. > In any case, collaboration between fwupd and flashrom started before I joined. So you know the story better than me, you’ve been working on flashrom already at that time. > > > the result is no longer flashrom > Speaking about what flashrom is and what it is not. > I think flashrom is what we collectively create, we as the current group of people who are giving their time, skills, effort and a piece of their heart into running the project. > Specifically on your statement of “if such and such mistakes have been done, then it’s not flashrom anymore”. It is still flashrom, with a room for improvement. *audible blinking; then, the sound of a facepalm followed by noises of someone falling of a chair* There aren't enough swear-words in the English language, so now we'll have to call ourselves "perkeleen vittupää" (a very not-nice expression in Finnish) just to express our disgust and frustration with our own crap. [This is derived from what Angry Linus Torvalds once wrote on LKML. We don't have any words to describe how infinitely stupid that statement is.] > IMO it is the responsibility of the group of maintainers, or “core developers” to analyse the mistakes and think how to prevent them from happening again. Flashrom is what we create as a result of our actions and words. Let's do this. > You mentioned “historically”, by the way flashrom is ~20years old. Neither you nor me are here for that long. We can ask the founders what were their vision and goals, and whether flashrom now is still flashrom? Sounds good! It would be interesting to (maybe?) chat about this in a voice call or similar. > > TL;DR: Let's work on finding solutions together. > Your tldr is amazing! Yes please! Yes let’s do it, especially given that “let’s work together” is what I am repeating all this year! > I agree, of course. But I want to ask you to speak honestly, without attempts to mislead the readers. Alright? *energetic nodding* Definitely! It will be very easy now, if not trivial: that nonsensical rant purged all the nastiness buried inside us that precluded proper collaboration and improvement. Thanks for your understanding and forgiveness. > So: > > For code reviews process/guidelines here is the thread, please share your ideas: https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/message/5WX… > > For the bug in showing progress in libflashrom, here it is: https://ticket.coreboot.org/issues/390 Will take a look, thank you. [Apparently this response is so long that it exceeds Gerrit's limit... Will send part 2 right afterwards.] -- To view, visit https://review.coreboot.org/c/flashrom/+/64663 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I322bf56ff92f7b4d0ffc92768e9f0cdf7cb82010 Gerrit-Change-Number: 64663 Gerrit-PatchSet: 2 Gerrit-Owner: Richard Hughes <richard(a)hughsie.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Richard Hughes <richard(a)hughsie.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 Oct 2022 15:55:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: dirtyjtag: Add DirtyJTAG programmer
by Jean THOMAS (Code Review) 11 Oct '22

11 Oct '22

Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Angel Pons, Alexander Goncharov. Jean THOMAS has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67878 ) Change subject: dirtyjtag: Add DirtyJTAG programmer ...................................................................... Patch Set 15: (2 comments) File flashrom.8.tmpl: https://review.coreboot.org/c/flashrom/+/67878/comment/ca778702_7afda29a PS13, Line 1580: .SS : .BR "mediatek_i2c_spi " programmer : .IP : This programmer supports SPI flash programming for chips attached to some : Mediatek display controllers, themselves accessed through I2C (tunneling : SPI flash commands through an I2C connection with the host). : .sp : The programmer is designed to support the TSUMOP82JUQ integrated display driver : and scaler as used in the Google Meet Series One Desk 27 (which runs a version : of ChromeOS and uses flashrom in its \fBtsum-scaler-updater\fP scripts that : ship with the OS). Other chips may use compatible protocols but have not been : tested with this programmer, and external chip IOs may need to be controlled : through other non-flashrom means to configure the chip in order for it to : operate as expected. : .sp : \fBdevpath\fP and \fBbus\fP options select the I2C bus to use, as described : previously. \fBallow_brick\fP defaults to no, and must explicitly be set to : "yes" in order for the programmer to operate. This is required because there is : no mechanism in the driver to positively identify that a given I2C bus is : actually connected to a supported device. > Yep, definitely a bad rebase. Yup, sorry for the noise. https://review.coreboot.org/c/flashrom/+/67878/comment/00366f5e_1163d00e PS13, Line 1528: .BR "realtek_mst_i2c_spi " and " parade_lspcon " programmers > Bad rebase? Why are docs about `mediatek_i2c_spi` gone? Done -- To view, visit https://review.coreboot.org/c/flashrom/+/67878 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic43e9a014ed7d04e429e73b30c9dcfdde1a78913 Gerrit-Change-Number: 67878 Gerrit-PatchSet: 15 Gerrit-Owner: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Tue, 11 Oct 2022 15:55:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: dirtyjtag: Add DirtyJTAG programmer
by Jean THOMAS (Code Review) 11 Oct '22

11 Oct '22

Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Jean THOMAS, Alexander Goncharov. Hello Felix Singer, build bot (Jenkins), Nico Huber, Thomas Heijligen, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67878 to look at the new patch set (#15). Change subject: dirtyjtag: Add DirtyJTAG programmer ...................................................................... dirtyjtag: Add DirtyJTAG programmer Add a new programmer driver for the DirtyJTAG project (a USB-JTAG firmware for STM32 MCUs). Successfully tested with DirtyJTAG 1.4 running on an Olimex STM32-H103 development board and a SST25VF020B SPI flash chip. Change-Id: Ic43e9a014ed7d04e429e73b30c9dcfdde1a78913 Signed-off-by: Jean THOMAS <virgule(a)jeanthomas.me> --- M Makefile A dirtyjtag_spi.c M flashrom.8.tmpl M include/programmer.h M meson.build M meson_options.txt M programmer_table.c M test_build.sh 8 files changed, 379 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/67878/15 -- To view, visit https://review.coreboot.org/c/flashrom/+/67878 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic43e9a014ed7d04e429e73b30c9dcfdde1a78913 Gerrit-Change-Number: 67878 Gerrit-PatchSet: 15 Gerrit-Owner: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2022