March 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: flashrom.c: Add 64KiB write granularity
by Marina Michelle (Code Review) 02 Mar '22

02 Mar '22

Attention is currently required from: Michał Żygowski, Angel Pons. Marina Michelle has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55713 ) Change subject: flashrom.c: Add 64KiB write granularity ...................................................................... Patch Set 13: (1 comment) Patchset: PS13: Run -- To view, visit https://review.coreboot.org/c/flashrom/+/55713 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I69801f581d0cb9b85f6596de7e1267ef9317673f Gerrit-Change-Number: 55713 Gerrit-PatchSet: 13 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Marina Michelle <marinamichelle100(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 02 Mar 2022 12:45:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Add FTDI FT4233H
by Angel Pons (Code Review) 02 Mar '22

02 Mar '22

Attention is currently required from: Charles Parent. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62480 ) Change subject: ft2232_spi.c: Add FTDI FT4233H ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Hello Angel, […] Thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/62480 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73cee8fd2a6613a8fbc26508d99bbe67da2b4f72 Gerrit-Change-Number: 62480 Gerrit-PatchSet: 2 Gerrit-Owner: Charles Parent <cha.parent(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Charles Parent <cha.parent(a)gmail.com> Gerrit-Comment-Date: Wed, 02 Mar 2022 11:03:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Charles Parent <cha.parent(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Add FTDI FT4233H
by Angel Pons (Code Review) 02 Mar '22

02 Mar '22

Attention is currently required from: Charles Parent. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62480 ) Change subject: ft2232_spi.c: Add FTDI FT4233H ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/62480 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73cee8fd2a6613a8fbc26508d99bbe67da2b4f72 Gerrit-Change-Number: 62480 Gerrit-PatchSet: 2 Gerrit-Owner: Charles Parent <cha.parent(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Charles Parent <cha.parent(a)gmail.com> Gerrit-Comment-Date: Wed, 02 Mar 2022 11:02:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Add Alder Lake support
by EricR Lai (Code Review) 02 Mar '22

02 Mar '22

Attention is currently required from: Rizwan Qureshi, Edward O'Callaghan, Angel Pons. EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62251 ) Change subject: ichspi: Add Alder Lake support ...................................................................... Patch Set 12: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/62251 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie66cf519df13f3391c41f5016b16a81ef3dfd4bf Gerrit-Change-Number: 62251 Gerrit-PatchSet: 12 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 02 Mar 2022 06:37:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests/linux_spi: Validate param file path
by Anastasia Klimchuk (Code Review) 02 Mar '22

02 Mar '22

Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62319 ) Change subject: tests/linux_spi: Validate param file path ...................................................................... Patch Set 4: (3 comments) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/62319/comment/e7c75e52_9263d45e PS4, Line 41: default_io_mock_state > The two implementations should be homogenised however there is too much code duplication in tests/ s […] Ok, maybe multipath is not great name if it has a meaning already, let's think of other name. open_validate_paths? I want to have more self-descriptive name, that's my main point. Just "default" doesn't explain anything. And self-descriptive name is even more important if such a function is defined somewhere in a different file and used throughout tests. https://review.coreboot.org/c/flashrom/+/62319/comment/60a8337a_70175492 PS4, Line 43: 2 > This could be made as a define, it's just the current max paths supported. Yes, great idea to have it as define! https://review.coreboot.org/c/flashrom/+/62319/comment/e7e1b75a_8a183176 PS4, Line 305: data > I am not so sure about this pattern, it looks very Java'ish with long variable names. […] Ok, this is existing pattern that was reviewed and approved by everyone, that's why I suggested it. Maybe it's not perfect, alright. But then it needs to be changed in all tests. What I was usually doing in this situation was first patch to change existing tests, and then all the next can use "new style" (data as name, in this example). -- To view, visit https://review.coreboot.org/c/flashrom/+/62319 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If5d24c65f291c53a35509fea5d2f5b3fdb51c306 Gerrit-Change-Number: 62319 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 02 Mar 2022 05:27:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(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) 02 Mar '22

02 Mar '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Angel Pons, Sridhar Siricilla, Alex Levin, YH Lin, Nico Huber, Martin Roth, 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 13: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/6dd68125_6c5723cc 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. > > > Please let me know if anything we need to help for moving this CL? > > > > What was said in the first message of the comment thread. > > I have reattempted to update the commit msg with setup details and replication steps. > > Please suggest if you need better wordings. Ping! https://review.coreboot.org/c/flashrom/+/61854/comment/f76d4ec5_df819f77 PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout. > > > Please let me know if anything we need to help for moving this CL? > > > > What was said in the first message of the comment thread. > > I'm able to verify this change on eve device. Updated the same in the commit msg. Ping! -- 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: 13 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 <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: 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: 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: Wed, 02 Mar 2022 05:04:36 +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

Change in flashrom[master]: tests/linux_spi: Validate param file path
by Edward O'Callaghan (Code Review) 02 Mar '22

02 Mar '22

Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62319 ) Change subject: tests/linux_spi: Validate param file path ...................................................................... Patch Set 4: (3 comments) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/62319/comment/d3172fe8_3989b0ad PS4, Line 41: default_io_mock_state > I would like to rename this, I have two reasons in mind. […] The two implementations should be homogenised however there is too much code duplication in tests/ so it is unclear where to put the canonical implementation. It was named 'default' as it provides a default implementation two a otherwise step-stage construction of the state machine behind open(). I am not sure multipath is a good name as that has a very specific meaning already in Linux, unrelated to this, open() is not a singleton function and should respect the path it was feed which it currently does not hence the need for so many patches to resolve the conflict with adding another call to open() somewhere else in flashrom. https://review.coreboot.org/c/flashrom/+/62319/comment/ec98cb10_d378c194 PS4, Line 43: 2 > Just wondering, why 2 ? This could be made as a define, it's just the current max paths supported. https://review.coreboot.org/c/flashrom/+/62319/comment/c65ea8eb_bd134d5b PS4, Line 305: data > I would keep the naming to indicate this piece of data belongs to linux_spi test, "linux_spi_io_stat […] I am not so sure about this pattern, it looks very Java'ish with long variable names. C already has scoping rules that make it clear about its life-time on the stack. I would actually rather we didn't have prefixes everywhere in stack-local scoped variables. -- To view, visit https://review.coreboot.org/c/flashrom/+/62319 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If5d24c65f291c53a35509fea5d2f5b3fdb51c306 Gerrit-Change-Number: 62319 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 02 Mar 2022 03:25:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests/: wrap flock() and ftruncate()
by Anastasia Klimchuk (Code Review) 02 Mar '22

02 Mar '22

Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62312 ) Change subject: tests/: wrap flock() and ftruncate() ...................................................................... Patch Set 6: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/62312/comment/c69dbc6e_521735ca PS6, Line 7: wrap flock() and ftruncate() Let's mention in commit description that the code which uses these mocks is introduced further in the chain (in the next patch). Otherwise it's not clear why they are added. -- To view, visit https://review.coreboot.org/c/flashrom/+/62312 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I57e7843a4d94c0a7200690d4c1d9e4194b9f83c5 Gerrit-Change-Number: 62312 Gerrit-PatchSet: 6 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 02 Mar 2022 02:42:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests/chips.c: Mock out open() for lock file
by Anastasia Klimchuk (Code Review) 02 Mar '22

02 Mar '22

Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62324 ) Change subject: tests/chips.c: Mock out open() for lock file ...................................................................... Patch Set 3: (5 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/62324/comment/9bc059c6_eaac502c PS3, Line 7: out You can remove "out". Just "Mock open() for all chip tests". And as a commit description, I would add info that it's a lock file which needs to be opened. File tests/chip.c: https://review.coreboot.org/c/flashrom/+/62324/comment/588c6807_dd48e1d3 PS3, Line 189: #define DEFAULT_MOCK_FD 0xC0FE Whatever will be end result in CB:62319 ideally it can be reused here https://review.coreboot.org/c/flashrom/+/62324/comment/7aa3ab48_97db7277 PS3, Line 212: data Similarly as in previous patch, this can be `erase_chip_io_state` and same for the rest of the patch. https://review.coreboot.org/c/flashrom/+/62324/comment/8ff49ced_507f66ac PS3, Line 213: "/run/lock/flashrom_lock" If this is exactly identical path for all the chip tests, can be a macro in this file? https://review.coreboot.org/c/flashrom/+/62324/comment/906bbe86_c6520308 PS3, Line 222: io_mock_register(&chip_io); You need to do `io_mock_register(NULL);` at the end of test, like verify tests do. -- To view, visit https://review.coreboot.org/c/flashrom/+/62324 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I527a266233648b0eef11a89108e82d0a008eeb8d Gerrit-Change-Number: 62324 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 02 Mar 2022 02:39:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests/: Allow for file path validation
by Anastasia Klimchuk (Code Review) 02 Mar '22

02 Mar '22

Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62320 ) Change subject: tests/: Allow for file path validation ...................................................................... Patch Set 4: (5 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/62320/comment/503a8650_cf8f8ace PS4, Line 7: Allow for file path validation I added some comments about commit messages in previous patches, this one would need to be aligned as well. I prefer something like "Add support for multiple calls to open". Also would be great to have some description, I would mention all tests except ABC are using the same mock function introduced earlier in the chain. File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/62320/comment/5d5b50ab_a88b57e4 PS4, Line 61: data Why dummy needs mocks? This is unexpected [to me]. https://review.coreboot.org/c/flashrom/+/62320/comment/565aed66_c1b1d7d5 PS4, Line 80: data Same comment as in previous patches: let's be more specific with naming and call this `nicrealtek_io_state`, and same for the rest of tests touched in the patch. https://review.coreboot.org/c/flashrom/+/62320/comment/31fb95b7_a6100fe8 PS4, Line 89: Lets add new line before and after `io_mock_register` https://review.coreboot.org/c/flashrom/+/62320/comment/d1ae56bf_e0baee43 PS4, Line 90: Since this test now registering io mock, it also needs to unregister, which is io_mock_register(NULL); after running lifecycle. And same for all other tests touched by this patch. Dediprog test is a good examples, it has all new lines and unregister. -- To view, visit https://review.coreboot.org/c/flashrom/+/62320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I892fa1ecee26ebce9640893edbb228fa9aa7b0b6 Gerrit-Change-Number: 62320 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 02 Mar 2022 02:27:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2022