February 2022 - flashrom-gerrit

Change in flashrom[master]: writeprotect: add set_wp_range()
by Nico Huber (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58482 ) Change subject: writeprotect: add set_wp_range() ...................................................................... Patch Set 56: Code-Review+2 (1 comment) File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58482/comment/dcc2aa31_6accdbeb PS56, Line 325: so they it select > There is some typo here, what does it mean to say? I guess either "so they select" or "so it selects". The former seems more accurate. -- To view, visit https://review.coreboot.org/c/flashrom/+/58482 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7d26f43fb05c5828b9839bb57a28fa1088dcd9a0 Gerrit-Change-Number: 58482 Gerrit-PatchSet: 56 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sun, 27 Feb 2022 19:18:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_get_available_ranges()
by Nico Huber (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58481 ) Change subject: libflashrom,writeprotect: add flashrom_wp_get_available_ranges() ...................................................................... Patch Set 53: Code-Review+1 (7 comments) Patchset: PS53: Ah, sorry, apparently I forgot to publish comments the last time I looked ._. File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58481/comment/fcccc7bd_4ba2a9a3 PS49, Line 153: range_list Maybe abbreviate here and in the two functions below like above, i.e. `ranges`? https://review.coreboot.org/c/flashrom/+/58481/comment/9576828b_e70802af PS49, Line 154: size_t I would prefer `unsigned int` as `size_t` is officially for byte counts. We use it internally sometimes for indices in general, but it seems better to not have such local style in the API. File libflashrom.c: https://review.coreboot.org/c/flashrom/+/58481/comment/c53b32cf_e53d5790 PS49, Line 800: } No need to change this if you don't want to. The pattern that seems to prevail nowadays is to check for errors first and bail out, so the actual code is at the end and doesn't need indentation, i.e. if (index >= list->count) return FLASHROM_WP_ERR_OTHER; *start = ... https://review.coreboot.org/c/flashrom/+/58481/comment/2b0a954f_2f3c1252 PS49, Line 811: free(list->ranges); Better to check for NULL first, e.g. if (!list) return; Then the caller doesn't need to ensure it's set. free() allows NULL and it makes error paths easier to handle when one just needs to write `free(x);` without needing to care if `x` was allocated already. (flashrom_flash_release() doesn't follow that rule either, I'll push a patch CB:62340) File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58481/comment/2fb0e985_9bbbb4d7 PS49, Line 171: Comparitor Compar*a*tor https://review.coreboot.org/c/flashrom/+/58481/comment/d664bd22_eae082f0 PS49, Line 200: size_t j = a->bits.bp_bit_count - i - 1; Reversing the loop would be more readable, I guess: for (int i = a->bits.bp_bit_count - 1; i >= 0; --i) -- To view, visit https://review.coreboot.org/c/flashrom/+/58481 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id51f038f03305c8536d80313e52f77d27835f34d Gerrit-Change-Number: 58481 Gerrit-PatchSet: 53 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sun, 27 Feb 2022 19:12:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: writeprotect: add get_wp_range() for decoding ranges
by Nico Huber (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59183 ) Change subject: writeprotect: add get_wp_range() for decoding ranges ...................................................................... Patch Set 41: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59183 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5a1dfcf384166b1bac319d286306747e1dcaa000 Gerrit-Change-Number: 59183 Gerrit-PatchSet: 41 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sun, 27 Feb 2022 18:55:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flash.h,flashchips.c: add writeprotect bit layout map to chips
by Nico Huber (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58477 ) Change subject: flash.h,flashchips.c: add writeprotect bit layout map to chips ...................................................................... Patch Set 36: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/58477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id08d77e6d4ca5109c0d698271146d026dbc21284 Gerrit-Change-Number: 58477 Gerrit-PatchSet: 36 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sun, 27 Feb 2022 18:54:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg,flashchips: add SR2 read/write support
by Nico Huber (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58570 ) Change subject: spi25_statusreg,flashchips: add SR2 read/write support ...................................................................... Patch Set 29: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/58570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I34a503b0958e8f2f22a2a993a6ea529eb46b41db Gerrit-Change-Number: 58570 Gerrit-PatchSet: 29 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sun, 27 Feb 2022 18:53:53 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pcidev: Move pci_get_dev() logic into canonical place
by Nico Huber (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Edward O'Callaghan. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59276 ) Change subject: pcidev: Move pci_get_dev() logic into canonical place ...................................................................... Patch Set 4: (1 comment) File board_enable.c: https://review.coreboot.org/c/flashrom/+/59276/comment/63c8f76a_99505f4f PS3, Line 1097: dev = pcidev_clonedev(dev); > Yes `pacc` is a handle scoped to libpci and so lexically scope to pcidev.c which is what this change is doing. I see what your code is doing, but I don't know your reasoning. Are you just following design patterns you have learned or did you think it through? how should I know? That's why we have a mailing list where we can discuss such design choices. If we don't discuss such things and the reasons are not 100% obvious, another developer with a different opinion might accidently work on reverting your changes in the future. I've seen this a lot in coreboot btw. it's not just a figment. Currently `pacc` is global yes. But as you well know we want to get rid of global state. So eventually, `pacc` might just become another, locally known parameter, like `dev` is to pci_read_long() etc. And I don't see (yet), that we'd be wrapping every single libpci function in `pcidev.c`. That's not saying that it's wrong to do so for some functions, I just think our reasoning should be more subtle. Also, after reading above paragraphs together again: Can we tell that this change makes replacing the global `pacc` easier or harder? That's all things a reviewer must consider when the direction is unclear. -- To view, visit https://review.coreboot.org/c/flashrom/+/59276 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id9ce055d5e5d347520ec5002b8c6548e60eaa0a7 Gerrit-Change-Number: 59276 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sun, 27 Feb 2022 14:22:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pcidev: Move pci_get_dev() logic into canonical place
by Nico Huber (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Edward O'Callaghan. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59276 ) Change subject: pcidev: Move pci_get_dev() logic into canonical place ...................................................................... Patch Set 4: (1 comment) File board_enable.c: https://review.coreboot.org/c/flashrom/+/59276/comment/baa6fcc1_6154cef6 PS3, Line 1097: dev = pcidev_clonedev(dev); > The direction should be fairly clear, I rebased the follow on patches to give clarity. You realise every time you change your mind here I need to keep wasting time rebasing. How you organize your patch trains is up to you. If constant rebasing is an issue, then don't rebase constantly. Are you really making reviewers responsible for necessary changes during review here? Can you please tell me in your own words what is the point of reviews? > Give actionable feedback, you wanted it renamed, then you changed your mind but agreed that 1 in the function name will do it now your comment is sort of high-Z. Sorry, your patches look like you are spending little time to write them. So please don't expect me to take much more time to write and repeat endless explanations. I changed my mind because my original suggestion was based on the idea that your original naming choice (pcidev_get_dev()) made sense which it made little. I'll try to do better in the future and not to run into that pitfall again. > The change here is not particularly controversial, it is just isolating libpci stuff into pcidev.c which is pretty logical - look in pcidev.c for libpci stuff. So I don't know what you mean by "I have to look in another file", do you want the whole project in one file? I fear, we are talking past each other. What I wanted to say is that before this patch, when reading the code in `board_status.c` I could see what the code does: it queries function 1 of the same PCI device. Now, after this change, when reading the code in `board_status.c` I can't see that anymore because the fact that it's about function 1 is hidden in another file. Putting `function` in the API name could fix that. Other languages than C might not have that problem, AFAIR you know Ada: There you could just write that parameter name in the call e.g. `pcidev_getdev(dev, function => 1)`. But in C all we can do is to choose good function and variable names. (Or add code comments, but I kind of hate those (not checked by the compiler.)) -- To view, visit https://review.coreboot.org/c/flashrom/+/59276 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id9ce055d5e5d347520ec5002b8c6548e60eaa0a7 Gerrit-Change-Number: 59276 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sun, 27 Feb 2022 14:08:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Angel Pons, Sridhar Siricilla, Alex Levin, YH Lin, Martin Roth - Personal, Caveh Jalali, David Hendricks, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 12: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/4eeb4c35_0620f182 PS11, Line 18: AU > nit: What does this mean? I wouldn't use an acronym here. Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 12 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Martin Roth - Personal <martinroth(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> 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: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Martin Roth - Personal <martinroth(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sun, 27 Feb 2022 13:03:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Alex Levin, YH Lin, Martin Roth - Personal, Subrata Banik, Caveh Jalali, David Hendricks, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Hello build bot (Jenkins), Patrick Georgi, Stefan Reinauer, Rizwan Qureshi, Angel Pons, Sridhar Siricilla, Alex Levin, YH Lin, Nico Huber, Martin Roth - Personal, Caveh Jalali, Tim Wawrzynczak, Edward O'Callaghan, Nick Vaccaro, Boris Mittelberg, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/61854 to look at the new patch set (#12). Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... ichspi.c: Check SPI Cycle In-Progress prior start HW Seq As per EDS, SPI controller sets the HSFSTS.bit5 (SCIP) when software sets the Flash Cycle Go (FGO) bit in the Hardware Sequencing Flash Control register. This bit remains set until the cycle completes on the SPI interface. Hardware automatically sets and clears this bit. Software must initiate the next SPI transaction when this bit is 0. Problem Statement: Evidencing AU (Auto Update) failure while performing firmware update on the Alder Lake based ChromeOS devices. Observation: Based on the initial understanding from the failure log/pattern, it seems like the platform is evidencing multiple `flashrom` access from different source, for example: `futility` accesses flashrom for erase, write and read operation, `crossystem` uses flashrom for updating VBNV, additionally, `set_fw_good` script also uses `crossystem` to update the fw status. Solution: Without this SCIP check being implemented in flashrom, there is no way to ensure multiple instances of flashrom performing different SPI operations are not cancelling each other and running into below error: Erasing and writing flash chip... Timeout error between offset 0x0061c000 and 0x0061c03f (= 0x0061c000 + 63)! FAILED! Uh oh. Erase/write failed. Checking if anything has changed. TEST=Able to flash coreboot image on Alder Lake Brya variants, Tiger Lake Volteer variants and Comet Lake Hatch variants without any failure. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 --- M ichspi.c 1 file changed, 15 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/61854/12 -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 12 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Martin Roth - Personal <martinroth(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> 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: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Martin Roth - Personal <martinroth(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Nico Huber (Code Review) 27 Feb '22

27 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Alex Levin, YH Lin, Martin Roth - Personal, Subrata Banik, Caveh Jalali, David Hendricks, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 11: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/c3add2f4_d114626f PS7, Line 24: BUG=b:215255210 > > For historical context, the "big lock" approach was intended to prevent multiples instances of fla […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 11 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Martin Roth - Personal <martinroth(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> 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: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Martin Roth - Personal <martinroth(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sun, 27 Feb 2022 12:29:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2022