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
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
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
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
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/+/59072 )
Change subject: [RFC][WPTST] dummyflasher: support emulation of SR2
......................................................................
Patch Set 10:
(5 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/59072/comment/a0deed55_d3e11612
PS10, Line 11:
You've probably tested this patch, probably by running dummyflasher with some programmer params. It would be great to add this info into a commit message.
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/59072/comment/cfcbe318_025668d1
PS10, Line 47: uint8_t emu_status_len; /* number of emulated status bytes */
Is this a number of emulated status bytes in SR2?
https://review.coreboot.org/c/flashrom/+/59072/comment/d74b80c2_ab9d30aa
PS10, Line 342: /* XXX: Else reset SR2 parts to zero? */
What does it mean? Is it a TODO?
https://review.coreboot.org/c/flashrom/+/59072/comment/b14ba74c_e312078c
PS10, Line 930: data->emu_status_len = 1;
Maybe I missed something, but is this value always initialised to 1?
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/59072/comment/cb05a268_cd367d21
PS10, Line 781: is an 8-bit or 16-bit hexadecimal value.
What does this value mean? I know it was like this before ;) But since you are changing/upgrading this programmer param, you probably know what it is and can add some explanation here. Ideally also examples of 8-bit and 16-bit spi_status values.
--
To view, visit https://review.coreboot.org/c/flashrom/+/59072
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I177ae3f068f03380f5b3941d9996a07205672e59
Gerrit-Change-Number: 59072
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 00:47:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Sergii Dmytruk 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/a217a007_83ae04b8
PS10, Line 7: enable write-protection for W25Q{64,128}.V
> I thought these are real chips, not only "mock chips used for emulation"? It just so happens that du […]
Yes, these are real chips. I've tested code on a W25Q64 chip, W25Q128V has absolutely identical configuration per datasheets. If code is known to work in general, I'm not sure every individual configuration of bits needs to be tested on hardware before enabling the support (and in case of OTP this is even harder).
--
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 01 Dec 2021 00:36:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
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/28b496d0_fd983d75
PS10, Line 7: enable write-protection for W25Q{64,128}.V
I thought these are real chips, not only "mock chips used for emulation"? It just so happens that dummy can emulate one of them.
If what I think is true and these are real chips, then enabling wp on them needs to be tested?
--
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 00:19:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment