October 2022 - flashrom-gerrit

[S] Change in flashrom[master]: [WIP] test_build.sh: Rework programmer selection using Meson
by Nico Huber (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Felix Singer, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68068 ) Change subject: [WIP] test_build.sh: Rework programmer selection using Meson ...................................................................... Patch Set 3: (2 comments) Patchset: PS3: > Maybe we should just go with the programmer list in test_build.sh. […] Is that file not in a standard format? I wouldn't know, is it python? A completely different thought occured to me: Maybe the problem is that we are trying to write a build system on top of build systems. We already have these lists in a programmable context (make and meson each). The repeated builds are easy enough to write in make, how is it in meson? If it's not possible, maybe that's something we should teach it? File test_build.sh: https://review.coreboot.org/c/flashrom/+/68068/comment/63b04e26_daff42cc PS2, Line 52: programmer_list=$(cat ${meson_logfile} | tail -n 1 | sed 's/^.*auto/auto/' | sed 's/\,//g' | sed 's/\"//g') > If anybody knows a better way how to get a list of available programmers, I am open for suggestions Is there no way to show the available option arguments (on stdout)? How do human users do it? -- To view, visit https://review.coreboot.org/c/flashrom/+/68068 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9f41ce2ff219c70c3c05a90134291b01a084c859 Gerrit-Change-Number: 68068 Gerrit-PatchSet: 3 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 05 Oct 2022 09:45:46 +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]: dummyflasher.c: Remove custom mapper from opaque_master
by Felix Singer (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Thomas Heijligen, Jonathon Hall. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68091 ) Change subject: dummyflasher.c: Remove custom mapper from opaque_master ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I76ae3e0c2e91ecba4fd320941bd1eff038050731 Gerrit-Change-Number: 68091 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> 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: 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: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Comment-Date: Wed, 05 Oct 2022 03:49:42 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ichspi: Do not attempt to map physical memory for hwseq
by Felix Singer (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Thomas Heijligen, Jonathon Hall. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68090 ) Change subject: ichspi: Do not attempt to map physical memory for hwseq ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68090 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie698071c3181e988f10b750b0e50c9700efaa1a3 Gerrit-Change-Number: 68090 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> 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: 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: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Comment-Date: Wed, 05 Oct 2022 03:48:38 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers: Move (un)map_flash_region to par/spi/opaque_master
by Felix Singer (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Jonathon Hall, Nikolai Artemiev. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 ) Change subject: drivers: Move (un)map_flash_region to par/spi/opaque_master ...................................................................... Patch Set 8: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/67695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Gerrit-Change-Number: 67695 Gerrit-PatchSet: 8 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 05 Oct 2022 03:47:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers/: Make 'fallback_{un}map' the default unless defined
by Felix Singer (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Edward O'Callaghan. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67404 ) Change subject: drivers/: Make 'fallback_{un}map' the default unless defined ...................................................................... Patch Set 4: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/67404/comment/2052fcda_f6bfd601 PS4, Line 10: function pointer and also please mention the struct -- To view, visit https://review.coreboot.org/c/flashrom/+/67404 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ea7bd68f7ae2cd4af9902ef07255ab6ce0bfdb3 Gerrit-Change-Number: 67404 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 05 Oct 2022 03:26:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers/: Make 'fallback_{un}map' the default unless defined
by Felix Singer (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Edward O'Callaghan. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67404 ) Change subject: drivers/: Make 'fallback_{un}map' the default unless defined ...................................................................... Patch Set 4: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/67404/comment/da16867c_d867e1fc PS4, Line 11: a select few exceptions Please mention the exceptions. Otherwise it looks good to me! -- To view, visit https://review.coreboot.org/c/flashrom/+/67404 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ea7bd68f7ae2cd4af9902ef07255ab6ce0bfdb3 Gerrit-Change-Number: 67404 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 05 Oct 2022 03:26:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: s25f.c: Fix undefined behaviour on shift
by Felix Singer (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Nikolai Artemiev, Evan Benn. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68127 ) Change subject: s25f.c: Fix undefined behaviour on shift ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68127 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I88188ef2ba2af919eeae9ba08916374d31d8b989 Gerrit-Change-Number: 68127 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Wed, 05 Oct 2022 03:03:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: s25f.c: Fix undefined behaviour on shift
by Angel Pons (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Nikolai Artemiev, Evan Benn. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68127 ) Change subject: s25f.c: Fix undefined behaviour on shift ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Patchset: PS1: > thanks Angel You're welcome! -- To view, visit https://review.coreboot.org/c/flashrom/+/68127 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I88188ef2ba2af919eeae9ba08916374d31d8b989 Gerrit-Change-Number: 68127 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Wed, 05 Oct 2022 01:36:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Evan Benn <evanbenn(a)google.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) 05 Oct '22

05 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: Hi, Apologies if our comment^wrant sounds harsh and/or rude, our head hurts from too much confused head scratching. It is also quite late. We have noticed some problems about things being done in ways that do not align with flashrom's code review process and we would like to find solutions to these problems. Also, clarification about using 1st person plural: it is because of recent discoveries regarding our mind. It does not refer to the flashrom project or any other individual involved or not with the project. Sorry if it sounds confusing, it is. > I can tell you my thoughts. We have a customer for this feature (fwupd), the feature already introduced in the previous patch CB:49643 , and this patch just makes it available for the customer. So we have someone who is waiting this feature, and if we merge this patch they will start using it straight away. Which means, if there are bugs, they will discover them and fix them, because they need the feature to work. Can't said customer just apply patches from Gerrit? Or do they not want to do that because having to fetch changes from Gerrit makes them "less good" for some reason? If patches from Gerrit get rubber-stamped and submitted without a proper review, the code will still be the same thing one would get from Gerrit, but any mistakes (bugs in the code, but also dumb typos) will be immortalized forever in git history. And flashrom isn't the kind of project where "regressions in the master branch are fine, people should only use the releases". It's precisely this kind of attitude that resulted in us being unable to make a new release for years: the master branch got swarmed by many patches that couldn't be reviewed thoroughly, and code quality (there was a lot of new code whose quality was unknown) of the master branch dropped below the expected quality levels to release a new flashrom version. The point of code review is to have other people look at the code to find problems and suggest improvements; it's to collaborate. In most open-source projects, getting one's code reviewed takes an unspecified amount of time because the people doing code reviews do so voluntarily, and most do not get paid for doing so. Trying to "expedite" things by throwing more people into the project usually doesn't work: these people still need to learn how to contribute to the project to be trusted with submit rights, and if the process of gaining the trust of existing contributors is bypassed for some reason, there will be conflicts and everyone involved will feel miserable about it. Moreover, if our understanding is correct, flashrom has always been a rather slow-paced project, mainly because the consequences of regressions could result in many users ending up with bricked hardware. Another very important thing about code reviews is that they allow knowledge sharing, i.e. teaching things to others. When we started contributing to coreboot, we had no clue about things, but others welcomed us helped us get started, and we feel that our duty is to give back this "welcome wagon" to other newcomers. If the customer wants to have experimental features available for testing ASAP, please have them pull patches from Gerrit or provide a branch and/or a downstream fork. We could consider bringing back the "staging" branch or something like this to mess around with work-in-progress features without compromising the stability of the master branch. However, if the customer wants new features with quality on par with existing code, it is fundamental to follow flashrom's review process. Otherwise, the result is no longer flashrom: it may resemble flashrom, but it may not be the same, so it's hard to make new releases due to the fear that users may notice the difference (imagine Coca-Cola that ends up tasting like Pepsi, or like bubbly rainwater). > So, what's the plan: it would be awesome to keep the feature and fix the bug. The feature is already merged and this patch makes it available to external users, it makes it available to the whole world in other words. When the whole world is using the feature, it very important to take the responsibility for known issues. If a flashrom feature is being used by the whole world, it better be something that has been tried and tested. Personally, we do not want to be held liable for issues regarding a flashrom feature that was not properly reviewed. We did not take a look at CB:49643 in detail, but we saw that others commented about API design decisions. Fixing such fundamental issues is best achieved by reverting the problematic implementation and re-introducing the feature so that it can be reviewed using Gerrit as it was meant to be used. Even something simple like CB:66659 was much easier to fix by undoing it and re-doing it again > Is it possible to merge this patch before the bug is fixed? Sorry, we'd rather not. "Upstream first" does not mean "test/review later". The progress percentage glitch is not too bad, but it indicates something else might be wrong with the code. If your customer really wants the buggy feature anyway, they should consider having their own downstream fork of flashrom to deal with things like this. Having a downstream fork is not mutually exclusive with "upstream first": the feature is developed upstream, and cherry-picked downstream for use. While we won't block this change, we'd appreciate if you (or anyone else reading this) could share your thoughts on how to satisfy both your customer and the flashrom project's needs. We're pretty sure there's a way. > Not ideal, but yes it is possible if the bug is clearly assigned, accepted, estimated and started. The thing is, we probably wouldn't need to treat this as a bug had things been tested / reviewed beforehand. The procedures to deal with bugs seem significantly less efficient than the review process, although it might be because we're much more familiar with review processes than with bug report processes. Reverting things is not inherently bad: we've approved or even created reverts of our own commits, because it was the most sensible thing to do. It's admitting that mistakes were made and prepares the code to try again. TL;DR: Let's work on finding solutions together. Best regards, Angel -- 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: Wed, 05 Oct 2022 01:35:42 +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: Daniel Campello <campello(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: s25f.c: Fix undefined behaviour on shift
by Evan Benn (Code Review) 05 Oct '22

05 Oct '22

Attention is currently required from: Nikolai Artemiev. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68127 ) Change subject: s25f.c: Fix undefined behaviour on shift ...................................................................... Patch Set 1: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/68127/comment/c84e697d_220d5b03 PS1, Line 7: UB > There's enough space to use the full form: `undefined behavior` (or `undefined behaviour`, pick your […] Done https://review.coreboot.org/c/flashrom/+/68127/comment/82c2040b_1a60fb76 PS1, Line 7: p > spurious `p` Done https://review.coreboot.org/c/flashrom/+/68127/comment/9ee5e63d_58df48dc PS1, Line 9: 24 > We had a teacher that would say "24 what? Potatoes?". Maybe reword this sentence as follows: […] Done Patchset: PS1: thanks Angel -- To view, visit https://review.coreboot.org/c/flashrom/+/68127 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I88188ef2ba2af919eeae9ba08916374d31d8b989 Gerrit-Change-Number: 68127 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 05 Oct 2022 01:30:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2022