December 2021 - flashrom-gerrit

Change in flashrom[master]: flashrom.8.tmpl: document W25Q128FV is emulated by dummyflasher
by Anastasia Klimchuk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Edward O'Callaghan, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59811 ) Change subject: flashrom.8.tmpl: document W25Q128FV is emulated by dummyflasher ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Patchset: PS2: If you put this patch into the chain with another your patch CB:59408 then they both can be submitted together. -- To view, visit https://review.coreboot.org/c/flashrom/+/59811 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I50f6cd6c5d853d6c70921e8027ada52d27982708 Gerrit-Change-Number: 59811 Gerrit-PatchSet: 2 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 20:58:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.8.tmpl: document W25Q128FV is emulated by dummyflasher
by Sergii Dmytruk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59811 to look at the new patch set (#2). Change subject: flashrom.8.tmpl: document W25Q128FV is emulated by dummyflasher ...................................................................... flashrom.8.tmpl: document W25Q128FV is emulated by dummyflasher It was absent from the list of emulated chips. Change-Id: I50f6cd6c5d853d6c70921e8027ada52d27982708 Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M flashrom.8.tmpl 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/59811/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59811 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I50f6cd6c5d853d6c70921e8027ada52d27982708 Gerrit-Change-Number: 59811 Gerrit-PatchSet: 2 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashrom.8.tmpl: document Winbond W25Q128FV chip
by Sergii Dmytruk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Sergii Dmytruk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59811 ) Change subject: flashrom.8.tmpl: document Winbond W25Q128FV chip ...................................................................... flashrom.8.tmpl: document Winbond W25Q128FV chip It was absent from the list of emulated chips. Change-Id: I50f6cd6c5d853d6c70921e8027ada52d27982708 Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M flashrom.8.tmpl 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/59811/1 diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index 989d677..b113225 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -693,6 +693,8 @@ .sp .RB "* Macronix " MX25L6436 " SPI flash chip (8192 kB, RDID, SFDP)" .sp +.RB "* Winbond " W25Q128FV " SPI flash chip (16384 kB, RDID)" +.sp .RB "* Dummy vendor " VARIABLE_SIZE " SPI flash chip (configurable size, page write)" .sp Example: -- To view, visit https://review.coreboot.org/c/flashrom/+/59811 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I50f6cd6c5d853d6c70921e8027ada52d27982708 Gerrit-Change-Number: 59811 Gerrit-PatchSet: 1 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: cli_classic: add writeprotect CLI
by Nikolai Artemiev (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Nico Huber, Paul Menzel, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58738 ) Change subject: cli_classic: add writeprotect CLI ...................................................................... Patch Set 18: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/58738/comment/05a65f48_b37e9f3a PS17, Line 220: ret = flashrom_wp_set_range(flash, &cfg, &range); : if (ret) { : msg_gerr("The chip does not support the requested range.\n"); : return ret; : } : : /* Write range before other operations (i.e. enabling HW protection) */ : ret = write_wp_config(flash, &cfg); : if (ret) : return ret; > > It's good to know there is no harm, but is there any effect of doing 1,2 without 3? What would be […] We could print messages from libflashrom, but it's generally undesirable and should be removed as much as possible IMO. It indicates that something is important enough that a CLI user should know about it, without providing any way for API callers to know about it (unless they intercept and parse stdout). There are also some potential cases where we want to do operations 1/2 but not 3: - A dry run mode that just prints the config that to be written instead of writing it. - Chip-specific tests that check the config bits being set by the set_range and set_mode. We might be able to use dummyflasher for this but testing the API directly would also be useful. -- To view, visit https://review.coreboot.org/c/flashrom/+/58738 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I499f521781ee8999921996517802c0c0c641d869 Gerrit-Change-Number: 58738 Gerrit-PatchSet: 18 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 07:16:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [FIX ME] tests: Add test for extract operation
by Daniel Campello (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk, Peter Marheine. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59532 ) Change subject: [FIX ME] tests: Add test for extract operation ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > Peter, Daniel, thank you for information, very useful! Now I understand how this situation has happe […] I think all you need to do is to take the first change in flashrom.c from the referenced patch and that should solve the issue: if (filename) ret = write_buf_to_file(buf, size, filename); (see line 1103 in https://review.coreboot.org/c/flashrom/+/52362/19/flashrom.c ) -- To view, visit https://review.coreboot.org/c/flashrom/+/59532 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I13107c0e095d53184e32a41fa72227cf7dc1d449 Gerrit-Change-Number: 59532 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Daniel Campello <campello(a)google.com> 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-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 01 Dec 2021 04:56:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [FIX ME] tests: Add test for extract operation
by Anastasia Klimchuk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Edward O'Callaghan, Daniel Campello, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59532 ) Change subject: [FIX ME] tests: Add test for extract operation ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > I believe Peter is right and there is some issues due to the differences between the Chromium fork a […] Peter, Daniel, thank you for information, very useful! Now I understand how this situation has happened. And thanks for the link to another patch. Just to be precise, that patch is "Make -r/-w/-v argument optional" but here we have do_extract which is supposed to be -x (which is none of -r/-w/-v)? Is it possible to re-write do_extract so that it is only doing what it needs to be doing: prepare_layout_for_extraction() and write_buf_to_include_args() without dealing with filenames? -- To view, visit https://review.coreboot.org/c/flashrom/+/59532 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I13107c0e095d53184e32a41fa72227cf7dc1d449 Gerrit-Change-Number: 59532 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> 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-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 01 Dec 2021 04:10:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC][WPTST] flashchips: enable write-protection for W25Q{64,128}.V
by Anastasia Klimchuk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59071 ) Change subject: [RFC][WPTST] flashchips: enable write-protection for W25Q{64,128}.V ...................................................................... Patch Set 10: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/59071/comment/bcbef773_dbce1c5a PS10, Line 7: enable write-protection for W25Q{64,128}.V > Yes, these are real chips. […] Good, if you tested real chips, could you please mention this in commit message? I am not sure about every configuration of bits, let's check with Nikolai, since he has been testing lots of chips recently. -- To view, visit https://review.coreboot.org/c/flashrom/+/59071 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iccb69a8d3a0dd2192e2c938caddaf07b1889ed35 Gerrit-Change-Number: 59071 Gerrit-PatchSet: 10 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 03:55:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC][WPTST] tests: test write protection
by Anastasia Klimchuk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59075 ) Change subject: [RFC][WPTST] tests: test write protection ...................................................................... Patch Set 10: (10 comments) File tests/chip_wp.c: https://review.coreboot.org/c/flashrom/+/59075/comment/84805f27_24f92c0a PS7, Line 177: /* Check initial mode. */ : assert_int_equal(0, flashrom_wp_get_mode(&cfg, &mode)); : assert_int_equal(WP_MODE_DISABLED, mode); : : /* Switch modes in dummyflasher. */ : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_PERMANENT)); : assert_int_equal(0, flashrom_wp_write_chip_config(&flash, &cfg)); : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_POWER_CYCLE)); : assert_int_equal(0, flashrom_wp_write_chip_config(&flash, &cfg)); : : /* Final mode should be "permanent". */ : assert_int_equal(0, flashrom_wp_read_chip_config(&flash, &cfg)); : assert_int_equal(0, flashrom_wp_get_mode(&cfg, &mode)); : assert_int_equal(WP_MODE_PERMANENT, mode); > I think it's both: […] Yes it's better, I mark this one as resolved. There are two minor comments on this test method, but big question is solved. File tests/chip_wp.c: https://review.coreboot.org/c/flashrom/+/59075/comment/f86a27b3_7648e211 PS10, Line 141: /* Use first 4 KiB for a range. */ : range.len = 0x1000; : assert_int_equal(0, flashrom_wp_set_range(&flash, &cfg, &range)); : : /* Change range to last 4 KiB. */ : range.start = 0x00fff000; : assert_int_equal(0, flashrom_wp_set_range(&flash, &cfg, &range)); This looks like 2 in 1 tests (1 for first 4KiB range, 2 for last 4KiB). Is there a reason to have set range twice in a sequence, or this can be two tests? https://review.coreboot.org/c/flashrom/+/59075/comment/3a021a8e_a5c8f1b3 PS10, Line 159: set_wp_mode_dummyflasher_test_success I think the test is now good, given the comment, but maybe better name is "switch_wp_mode_...." instead of set_wp_mode.... https://review.coreboot.org/c/flashrom/+/59075/comment/d24c30c3_5877f811 PS10, Line 188: /* : * Check that "permanent" mode can't be unset and it's not considered to be : * an error at the level of library calls (verification should be done : * higher). : */ That's a very good comment, thank you! Only need to fix indentation. https://review.coreboot.org/c/flashrom/+/59075/comment/124c5dc2_790dcf91 PS10, Line 220: cfg.srp_bit_count = 1; : assert_int_equal(0, flashrom_wp_set_mode(&cfg, WP_MODE_DISABLED)); I see two parts here: 1) with srp_bit_count = 2, 2) with srp_bit_count = 1 Similar to other tests, a question is whether this is a sequence needed for a test scenario (then just add a comment), or this are two independent scenarios (then split). And the same for 3 more tests below. https://review.coreboot.org/c/flashrom/+/59075/comment/58694d2f_84ec5d58 PS10, Line 290: assert_int_equal(0x004000, range.start); : assert_int_equal(0xffc000, range.len); just to check, do range start/len come from spi_status? the comment above param_dup only mentions register bits. https://review.coreboot.org/c/flashrom/+/59075/comment/7d7621e6_5f6e143e PS10, Line 310: assert_int_equal(0, flashrom_layout_include_region(layout, "head")); : assert_int_equal(0, flashrom_layout_include_region(layout, "tail")); It is not clear in the context of test method where "head" and "tail" come from. I think these two assertions need to be moved to setup_chip. https://review.coreboot.org/c/flashrom/+/59075/comment/d66d3fce_c214348e PS10, Line 313: Write protection takes effect only after changing SRP values. if you can add a bit more for example , so at this stage WP is not enabled and erase completes successfully. or something similar https://review.coreboot.org/c/flashrom/+/59075/comment/dc114c0e_baedfce4 PS10, Line 329: Try erasing the chip again also you can add continue the comment say , however because now wp is enabled, erase operation fails. or something https://review.coreboot.org/c/flashrom/+/59075/comment/d7d233eb_7225cb46 PS10, Line 364: printf("Erase chip operation started.\n"); : assert_int_equal(0, do_erase(&flash)); : printf("Erase chip operation done.\n"); Oh now I compare this test scenario and previous, and looks like first 4 KiB are special? Because in this scenario, erase completes successfully even with wp enabled? Let's add more comments to explain what these two tests are testing, and what is expected behaviour. The code itself looks good, just need to be more documented. -- To view, visit https://review.coreboot.org/c/flashrom/+/59075 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I49af7f6d173eb4c56c22d80b01a473b8c499c0f8 Gerrit-Change-Number: 59075 Gerrit-PatchSet: 10 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 03:52:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC][WPTST] dummyflasher: write protection for W25Q128FV
by Anastasia Klimchuk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59074 ) Change subject: [RFC][WPTST] dummyflasher: write protection for W25Q128FV ...................................................................... Patch Set 10: (7 comments) Patchset: PS10: d thi File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/59074/comment/58829516_cad538d8 PS10, Line 63: bool wp; /* state of hardware write protection */ And this name also can be hwwp https://review.coreboot.org/c/flashrom/+/59074/comment/ce4ca40d_245a88be PS10, Line 244: write_flash_part The name is a bit confusing, what "part" means here? And same for erase_flash_part https://review.coreboot.org/c/flashrom/+/59074/comment/a3e866b5_1043ac6b PS10, Line 264: /* XXX: should data->erase_to_zero be taken into account here? */ Is it a TODO, or work-in-progress note? https://review.coreboot.org/c/flashrom/+/59074/comment/f2a6c347_80b280ba PS10, Line 967: will have WP enabled Hardware WP? File flashrom.8.tmpl: https://review.coreboot.org/c/flashrom/+/59074/comment/1bbcd0b2_09fb5294 PS10, Line 696: Winbond " W25Q128FV " SPI flash chip (16384 kB, RDID)" Oh wow this was missing in the man for all the time?! Thank you for spotting! I think, just this one line, can be a separate patch outside of WP chain, and can be submitted independently without waiting for everything else to get through reviews. https://review.coreboot.org/c/flashrom/+/59074/comment/ab0561e7_0c5e1108 PS10, Line 792: wp=state I remember we discussed that in the patch with test, yes now I see that "hwwp" would be a better name. Especially that description explains what it is exactly. -- To view, visit https://review.coreboot.org/c/flashrom/+/59074 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9fd1417f941186391bd213bd355530143c8f04a0 Gerrit-Change-Number: 59074 Gerrit-PatchSet: 10 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 02:17:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC][WPTST] dummyflasher: SR2 for W25Q128FV
by Anastasia Klimchuk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59073 ) Change subject: [RFC][WPTST] dummyflasher: SR2 for W25Q128FV ...................................................................... Patch Set 10: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/59073/comment/4a6f2b8a_d2d91567 PS10, Line 8: : By the way, W25Q128FV has three status registers, but no plans to use : the third one yet. If you remove "by the way", it is a very good second paragraph for this commit message! The first paragraph needs to describe more precisely what this commit is doing. A wider question is about this and previous patch, what exactly each of them is doing. Maybe the work between them can be re-distributed (for example, previous patch has branch with emu_status_len == 2 , but the value set to 2 only here). I am saying "maybe" because it is a question, let's look closely. After the work is distributed between two patches, then commit description and commit message for each of them need to reflect that. At the moment commit descriptions are: "support emulation of SR2" "SR2 for W25Q128FV" Imagine looking at commit history a year after. It would not be easy to guess what each patch is doing. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/59073/comment/70006fff_8952b246 PS10, Line 922: data->emu_status_len = 2; I added a comment to previous patch about emu_status_len always 1, I see that it becomes 2 in this patch. However, in the previous patch it is always 1. Maybe some of the parts of previous patch should be here? -- To view, visit https://review.coreboot.org/c/flashrom/+/59073 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I79f9b4a0b604663d3288ad70dcbe3ea4075dede5 Gerrit-Change-Number: 59073 Gerrit-PatchSet: 10 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 01:16:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0