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

Change in flashrom[master]: libflashrom,linux_mtd: add linux_mtd writeprotect support
by Anastasia Klimchuk (Code Review) 04 Apr '22

04 Apr '22

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61897 ) Change subject: libflashrom,linux_mtd: add linux_mtd writeprotect support ...................................................................... Patch Set 36: Code-Review+1 (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61897/comment/f5c43651_83aa0d96 PS36, Line 10: and uses it support Maybe instead "and uses it support" -> "and uses the framework to support". File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/61897/comment/b0973450_a51127f7 PS36, Line 380: if ((cfg->range.len == 0) != (cfg->mode != FLASHROM_WP_MODE_DISABLED)) { : return FLASHROM_WP_ERR_OTHER; : } Please help me, I spent some time staring at the code, also reading the comment above and commit message. It seems to me that comment matches the commit message, but the code does not? what I am missing? Testing info from commit message, first case: flashrom --wp-enable --wp-range <non-empty> succeeds non-empty range makes `cfg->range.len == 0` -> false --wp-enable makes `cfg->mode != FLASHROM_WP_MODE_DISABLED` -> true So the condition becomes `false != true` -> true -> so it returns error? But commit messages says it succeeds? What I am missing? :) -- To view, visit https://review.coreboot.org/c/flashrom/+/61897 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5c86e28cdec44bec49ba1d36f8ab62241b9b01da Gerrit-Change-Number: 61897 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 04 Apr 2022 02:31:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: use MOCK_FD instead of NON_ZERO
by Edward O'Callaghan (Code Review) 04 Apr '22

04 Apr '22

Attention is currently required from: Daniel Campello, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63193 ) Change subject: tests: use MOCK_FD instead of NON_ZERO ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/63193/comment/e94e84fb_9b272e60 PS3, Line 9: return a non-negative value for : the file descriptor expected from open operations. > This could not be intentional. […] Well that is exactly why I was wondering if it was intentional? i.e., to fail if there is no `open()` mocked in a test. -- To view, visit https://review.coreboot.org/c/flashrom/+/63193 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6bac051852aea2465665a6fd669b7f5e3772985 Gerrit-Change-Number: 63193 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 04 Apr 2022 02:00:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: assert pathname and flags when calling open()
by Edward O'Callaghan (Code Review) 04 Apr '22

04 Apr '22

Attention is currently required from: Sam McNally, Daniel Campello. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63227 ) Change subject: tests: assert pathname and flags when calling open() ...................................................................... Patch Set 6: (1 comment) File tests/tests.c: https://review.coreboot.org/c/flashrom/+/63227/comment/fbfe1433_ae7a5435 PS6, Line 77: wrap_open_and_friends > open_state in not used when open != NULL (this function returns from within this if case) That is what I mean by "unexpected behavior" as `open_state` being non-NULL implies a builtin version of `open()` while `open` being non-NULL implies to use an explicit `open()` implementation. This function maybe needs a comment. -- To view, visit https://review.coreboot.org/c/flashrom/+/63227 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb Gerrit-Change-Number: 63227 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Mon, 04 Apr 2022 01:58:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: add writeprotect support for more chips
by Anastasia Klimchuk (Code Review) 04 Apr '22

04 Apr '22

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62214 ) Change subject: flashchips.c: add writeprotect support for more chips ...................................................................... Patch Set 26: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/62214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7f3d4c4148056098a845b5c64308b0333ebda395 Gerrit-Change-Number: 62214 Gerrit-PatchSet: 26 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 04 Apr 2022 01:53:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [WIP] flashchips: Add support for GigaDevice GD25LR256E, GD251R512ME
by Felix Singer (Code Review) 04 Apr '22

04 Apr '22

Attention is currently required from: Ravishankar Sarawadi, Angel Pons. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58025 ) Change subject: [WIP] flashchips: Add support for GigaDevice GD25LR256E, GD251R512ME ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/58025/comment/ea693402_24e38557 PS2, Line 7: [WIP] Also, remove this please -- To view, visit https://review.coreboot.org/c/flashrom/+/58025 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2fe6bc1219cd1ee19b93caabab69de938cfc44b0 Gerrit-Change-Number: 58025 Gerrit-PatchSet: 2 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sukumar Ghorai <sukumar.ghorai(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.corp-partner.google.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Apr 2022 01:13:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [WIP] flashchips: Add support for GigaDevice GD25LR256E, GD251R512ME
by Felix Singer (Code Review) 04 Apr '22

04 Apr '22

Attention is currently required from: Ravishankar Sarawadi, Angel Pons. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58025 ) Change subject: [WIP] flashchips: Add support for GigaDevice GD25LR256E, GD251R512ME ...................................................................... Patch Set 2: (6 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/58025/comment/dd100f43_f8413e4c PS1, Line 9: Tested using Servo: reading and writing works. > Also, mention how you tested it. Done https://review.coreboot.org/c/flashrom/+/58025/comment/ca288ce4_4efd356a PS1, Line 9: Tested using Servo: reading and writing works. > Also, mention how you tested it. Done https://review.coreboot.org/c/flashrom/+/58025/comment/1a5ce964_66e1fed4 PS1, Line 15: Change-Id: I2fe6bc1219cd1ee19b93caabab69de938cfc44b0 > Missing a Signed-off-by line Done Commit Message: https://review.coreboot.org/c/flashrom/+/58025/comment/09cb3fd5_b4217cec PS2, Line 10: Please use two spaces instead of tabs. Also below. File flashchips.c: https://review.coreboot.org/c/flashrom/+/58025/comment/74fe634d_ab6b6f70 PS1, Line 6349: : { : .vendor = "GigaDevice", : .name = "GD25LR256E", : .bustype = BUS_SPI, : .manufacture_id = GIGADEVICE_ID, : .model_id = GIGADEVICE_GD25LR256E, : .total_size = 32768, : .page_size = 256, : .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA_WREN, : .tested = TEST_OK_PREW, : .probe = probe_spi_rdid, : .probe_timing = TIMING_ZERO, : .block_erasers = : { : { : .eraseblocks = { {4 * 1024, 8192} }, : .block_erase = spi_block_erase_21, : }, { : .eraseblocks = { {4 * 1024, 8192} }, : .block_erase = spi_block_erase_20, : }, { : .eraseblocks = { {32 * 1024, 1024} }, : .block_erase = spi_block_erase_5c, : }, { : .eraseblocks = { {32 * 1024, 1024} }, : .block_erase = spi_block_erase_52, : }, { : .eraseblocks = { {64 * 1024, 512} }, : .block_erase = spi_block_erase_dc, : }, { : .eraseblocks = { {64 * 1024, 512} }, : .block_erase = spi_block_erase_d8, : }, { : .eraseblocks = { {32 * 1024 * 1024, 1} }, : .block_erase = spi_block_erase_60, : }, { : .eraseblocks = { {32 * 1024 * 1024, 1} }, : .block_erase = spi_block_erase_c7, : } : }, : .printlock = spi_prettyprint_status_register_bp3_srwd, : .unlock = spi_disable_blockprotect, : .write = spi_chip_write_256, : .read = spi_chip_read, : .voltage = {1600, 2000}, : }, > Please indent with tabs Done File flashchips.c: https://review.coreboot.org/c/flashrom/+/58025/comment/497e73d9_3dbe2610 PS2, Line 6452: }, Please use tabs -- To view, visit https://review.coreboot.org/c/flashrom/+/58025 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2fe6bc1219cd1ee19b93caabab69de938cfc44b0 Gerrit-Change-Number: 58025 Gerrit-PatchSet: 2 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sukumar Ghorai <sukumar.ghorai(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.corp-partner.google.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Apr 2022 00:44:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: add writeprotect support for more chips
by Anastasia Klimchuk (Code Review) 04 Apr '22

04 Apr '22

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62214 ) Change subject: flashchips.c: add writeprotect support for more chips ...................................................................... Patch Set 26: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/62214/comment/9ba5ca29_af337d1e PS26, Line 17: GD25LQ128C/GD25LQ128D/GD25LQ128E This ones seem to be added earlier, not in this patch but before. The table is a good idea, did you mean it to cover everything added in this patch, or everything that supported up to this patch? It is at the moment the latter (unless I am missing something). Which is also fine, just maybe add the title of the table "Summary: everything that supported up to this patch" or something like that. -- To view, visit https://review.coreboot.org/c/flashrom/+/62214 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7f3d4c4148056098a845b5c64308b0333ebda395 Gerrit-Change-Number: 62214 Gerrit-PatchSet: 26 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 04 Apr 2022 00:12:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg: delete spi_read_status_register()
by Anastasia Klimchuk (Code Review) 04 Apr '22

04 Apr '22

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59529 ) Change subject: spi25_statusreg: delete spi_read_status_register() ...................................................................... Patch Set 65: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59529 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I146b4b5439872e66c5d33e156451a729d248c7da Gerrit-Change-Number: 59529 Gerrit-PatchSet: 65 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 03 Apr 2022 23:55:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Introduce HSFC CYCLE READ/WRITE/ERASE macros
by Angel Pons (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62868 ) Change subject: ichspi: Introduce HSFC CYCLE READ/WRITE/ERASE macros ...................................................................... Patch Set 12: (2 comments) File ichspi.c: https://review.coreboot.org/c/flashrom/+/62868/comment/7e897cc5_79e453d7 PS12, Line 163: #define HSFC_FCYCLE(cyc) ((cyc) << HSFC_FCYCLE_OFF) Isn't this equivalent to `HSFC_FCYCLE_MASK`? https://review.coreboot.org/c/flashrom/+/62868/comment/b4d4e309_48e7f2f6 PS12, Line 166: 4K Is it always 4K? -- To view, visit https://review.coreboot.org/c/flashrom/+/62868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ea74b668e5f8d8e4b3da2a8ad8b81f1813e1e80 Gerrit-Change-Number: 62868 Gerrit-PatchSet: 12 Gerrit-Owner: Subrata Banik <subratabanik(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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:56:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Introduce HSFC FDBC(n) macro
by Angel Pons (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Subrata Banik, Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62892 ) Change subject: ichspi: Introduce HSFC FDBC(n) macro ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/62892 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie2512c85de9fc21286234b97f5842ecef1729787 Gerrit-Change-Number: 62892 Gerrit-PatchSet: 6 Gerrit-Owner: Subrata Banik <subratabanik(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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Comment-Date: Sun, 03 Apr 2022 21:54:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2022