February 2022 - flashrom-gerrit

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

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Martin Roth, Subrata Banik, Caveh Jalali, 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 9: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/5e6a2858_62b327e0 PS7, Line 17: Without this synchronisation being implemented, flashrom is 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. > Ack What I'm still missing is how it relates to the change. AIUI, it's only the background story what led to its finding, and doesn't say anything about the change. I think it's generally ok to add such information (even though I don't see any value). But it must be clear how it relates to the change. Confusing information can be harmful, especially in commit messages that can't be changed later on. https://review.coreboot.org/c/flashrom/+/61854/comment/1456ac09_f7c86664 PS7, Line 24: BUG=b:215255210 > Ack I don't see any answer. Please never again mark my comments as resolved. Please make sure all information on that issue tracker is sane, doesn't lead to further wrong assumptions about this change and can't be changed anymore, e.g. close the ticket. Or, just don't reference it. https://review.coreboot.org/c/flashrom/+/61854/comment/250702bc_21c0b6a6 PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout. > Ack Please also test older platforms, not just the newest (also, platforms that are not supported yet don't matter). If you need help with testing you can always ask on the mailing list and IRC. -- 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: 9 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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 <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: Angel Pons <th3fanbus(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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> 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: Sat, 26 Feb 2022 16:10:24 +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> 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) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Nico Huber, Martin Roth, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Hello build bot (Jenkins), Patrick Georgi, Stefan Reinauer, Rizwan Qureshi, Sridhar Siricilla, Alex Levin, YH Lin, Nico Huber, Martin Roth, 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 (#9). 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 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 synchronisation 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. BUG=b:215255210 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/9 -- 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: 9 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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 <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: Angel Pons <th3fanbus(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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> 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 Subrata Banik (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Nico Huber, Martin Roth, Caveh Jalali, 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 7: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/c25e76b4_51fd5a78 PS7, Line 17: Without this synchronisation being implemented, flashrom is 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. > If you want to mention this, please also describe the full setup, […] Ack https://review.coreboot.org/c/flashrom/+/61854/comment/45a29380_8e0b77b6 PS7, Line 24: BUG=b:215255210 > What's said in this issue and what's its status? Ack https://review.coreboot.org/c/flashrom/+/61854/comment/abe13130_17c167bb PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout. > Please also test that things still work on as many platforms as […] Ack File ichspi.c: https://review.coreboot.org/c/flashrom/+/61854/comment/0d273097_b35efddd PS7, Line 1410: Error: SCIP never cleared! > Maybe this would fit better: > > Error: SCIP bit is unexpectedly set. 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: 7 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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 <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: Angel Pons <th3fanbus(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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> 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: Sat, 26 Feb 2022 14:51:30 +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]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Martin Roth, Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Hello build bot (Jenkins), Patrick Georgi, Stefan Reinauer, Rizwan Qureshi, Sridhar Siricilla, Alex Levin, YH Lin, Nico Huber, Martin Roth, 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 (#8). 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 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 synchronisation 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. BUG=b:215255210 TEST=Able to flash coreboot image on Alder Lake Brya varients, Tiger Lake Volteer varients and Comet Lake Hatch varients 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/8 -- 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: 8 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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 <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: Angel Pons <th3fanbus(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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> 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) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Martin Roth, Subrata Banik, Caveh Jalali, 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 7: Code-Review+1 (6 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/298cc045_7944cf75 PS7, Line 17: Without this synchronisation being implemented, flashrom is 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. If you want to mention this, please also describe the full setup, preferably how to reproduce the error. And also that it's not con- sidered a valid use case. https://review.coreboot.org/c/flashrom/+/61854/comment/581d8cbc_84d43e43 PS7, Line 24: BUG=b:215255210 What's said in this issue and what's its status? https://review.coreboot.org/c/flashrom/+/61854/comment/d11e02f3_edbc4c3c PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout. Please also test that things still work on as many platforms as possible starting with ICH9. Patchset: PS2: > > If there is a Hardware Sequencing Cycle In Progress and an attempt is made to program any of the c […] Resolving, discussion seems settled. Patchset: PS6: > > > > A version of this change that takes the applicable context into account > > > > (e.g. other waiting loops in this driver, implications when multiple > > > > instances would reach this point in the code simultaneously) would be > > > > accepted immediately. > > > > > > I understand that would be a comprehensive fix across flashrom. > > > > Doesn't need to be. All I'm asking for is to keep the whole code sound > > and not just the single spot where a check would be added. This could > > for instance be a simple > > > > if (REGREAD8(ICH9_REG_HSFS) & HSFS_SCIP) > > return -1; > > Implemented the same with current patchset. please help to take a look (if possible) Ack File ichspi.c: https://review.coreboot.org/c/flashrom/+/61854/comment/df8badca_6cf17696 PS7, Line 1410: Error: SCIP never cleared! Maybe this would fit better: Error: SCIP bit is unexpectedly set. -- 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: 7 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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 <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: Angel Pons <th3fanbus(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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> 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: Sat, 26 Feb 2022 14:00:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes 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: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pcidev: Move pci_card_find() from internal to canonical place
by Nico Huber (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Edward O'Callaghan. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59279 ) Change subject: pcidev: Move pci_card_find() from internal to canonical place ...................................................................... Patch Set 3: (2 comments) File board_enable.c: https://review.coreboot.org/c/flashrom/+/59279/comment/5006c038_f5f450a6 PS3, Line 2650: board->first_card_device)) Looks like this was manually indented with spaces. Please remove them or fill up. https://review.coreboot.org/c/flashrom/+/59279/comment/40fdb620_8b7bcca0 PS3, Line 2658: board->second_card_device)) Same here. -- To view, visit https://review.coreboot.org/c/flashrom/+/59279 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I026bfbecba114411728d4ad1ed8969b469fa7d2d Gerrit-Change-Number: 59279 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sat, 26 Feb 2022 13:51:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

26 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/3217e7f2_68070642 PS3, Line 1097: dev = pcidev_clonedev(dev); > Done. np Nico, thanks for thinking about the problem. Idk. without anything hinting towards "function" 1 in the function name, The code was easier to follow before this change. If I'd read it now, I'd have to look into another file to figure out what it does. > I agree the code is confusing, generally pacc-related paths are hard to follow. There's one exception but generally I think it's best to think of `pacc` as just the global libpci library handle. Nothing special. > Let's move this forwards to make go in the general right direction. Honestly, I don't know yet in what direction you are pointing and if that's the right one. -- 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: Sat, 26 Feb 2022 13:39:48 +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) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Nico Huber, Martin Roth, Caveh Jalali, 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 7: (1 comment) Patchset: PS6: > > > A version of this change that takes the applicable context into account > > > (e.g. other waiting loops in this driver, implications when multiple > > > instances would reach this point in the code simultaneously) would be > > > accepted immediately. > > > > I understand that would be a comprehensive fix across flashrom. > > Doesn't need to be. All I'm asking for is to keep the whole code sound > and not just the single spot where a check would be added. This could > for instance be a simple > > if (REGREAD8(ICH9_REG_HSFS) & HSFS_SCIP) > return -1; Implemented the same with current patchset. please help to take a look (if possible) > > That would adhere to the datasheet, AFAICT and wouldn't make it look > like we don't know what we are doing (not knowing what we are doing, > for instance, would be writing a program that tries to synchronize > multiple processes but fails to do so properly). > > Another solution would be to check the bit once on programmer initiali- > zation and after each transaction. > > > However that will still not guarantee that another flashrom like program will wait on the same file/mutex to synchronize the access. > > > > This check on H_SCIP OTOH, which should be adhered by all the programs at the least prevents the program from cancelling out another's transaction. So let us make flashrom compliant with the recommendation. > > Then check the bit once in programmer init. That should cover another > previously running program. Multiple, concurrently running programs > are out of the scope of SCIP. -- 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: 7 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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 <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: Angel Pons <th3fanbus(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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> 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: Sat, 26 Feb 2022 06:16:09 +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: Rizwan Qureshi <rizwan.qureshi(a)intel.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) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Martin Roth, Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Hello build bot (Jenkins), Patrick Georgi, Stefan Reinauer, Rizwan Qureshi, Sridhar Siricilla, Alex Levin, YH Lin, Nico Huber, Martin Roth, 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 (#7). 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. Without this synchronisation being implemented, flashrom is 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. BUG=b:215255210 TEST=Concurrent flashrom access is not throwing timeout. 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/7 -- 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: 7 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> 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 <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: Angel Pons <th3fanbus(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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> 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]: pcidev.c: Rename pci_dev_find() -> pcidev_find()
by Edward O'Callaghan (Code Review) 26 Feb '22

26 Feb '22

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59282 to look at the new patch set (#2). Change subject: pcidev.c: Rename pci_dev_find() -> pcidev_find() ...................................................................... pcidev.c: Rename pci_dev_find() -> pcidev_find() Make symbol name consistent with the rest in pcidev.c BUG=none TEST=builds Change-Id: I41e0922d5429b4389367dd98b4241d50b8013183 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M board_enable.c M chipset_enable.c M pcidev.c M programmer.h M sb600spi.c 5 files changed, 21 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/59282/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59282 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I41e0922d5429b4389367dd98b4241d50b8013183 Gerrit-Change-Number: 59282 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2022