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: enforce write protection for W25Q128FV
......................................................................
Patch Set 12:
(1 comment)
Patchset:
PS12:
If this patch completes wp emulation in dummyflasher, can it be tested from command line? I mean doing read, write, wp enable, disable etc etc from command line with dummy programmer. Dummy exists for much longer than unit tests, and it has been used from command line even without tests.
If yes and you can test this from command line, if you can add commands you tested into commit description - that would be perfect!
--
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: 12
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: Mon, 06 Dec 2021 01:56:31 +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/+/59074 )
Change subject: [RFC][WPTST] dummyflasher: enforce write protection for W25Q128FV
......................................................................
Patch Set 12: Code-Review+1
(1 comment)
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/59074/comment/76eed054_bd56d542
PS10, Line 264: /* XXX: should data->erase_to_zero be taken into account here? */
> It's a question to anyone who'll read the code. It just looks like a bug, but I'm not entirely sure.
It does look like a bug to me, thanks!
You can mark the comment as FIXME (instead of XXX).
I would say bug fix needs to go as a separate patch, outside of this chain, unless other people disagree.
--
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: 12
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: Mon, 06 Dec 2021 01:52:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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, Paul Menzel, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58738 )
Change subject: cli_classic: add writeprotect CLI
......................................................................
Patch Set 19: Code-Review+1
(3 comments)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/58738/comment/f3d35a15_740f5b89
PS15, Line 66: " --wp-range=<start>,<len> set write protect range\n"
: " --wp-region <region> set write protect region\n"
> Cool thanks! Your explanation: […]
Looks like it is done in the latest patchset. I mark this comment resolved.
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/58738/comment/9286318b_1f121bdc
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;
> We could print messages from libflashrom, but it's generally undesirable and should be removed as mu […]
Few days later, and after re-reading the discussion, I think I understand. Especially if there are potential cases where only 1,2 needed, not necessarily followed by 3.
Existing printing of messages in libflashrom seems like a separate conversation.
Re: tests for API directly: actually yes, and Sergii already added a bunch of those in CB:59075 :)
https://review.coreboot.org/c/flashrom/+/58738/comment/0e16eaef_ad100ab8
PS17, Line 236: ret = flashrom_wp_set_mode(&cfg, mode);
: if (ret) {
: msg_gerr("The chip does not support the requested mode.\n");
: return ret;
: }
:
: /* Write mode before other operations */
: ret = write_wp_config(flash, &cfg);
: if (ret)
: return ret;
> Same comment, seems like an operation consists of two steps.
closing as dup
--
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: 19
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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Mon, 06 Dec 2021 00:41:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment