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