Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/58481
to look at the new patch set (#54).
Change subject: libflashrom,writeprotect: add flashrom_wp_get_available_ranges()
......................................................................
libflashrom,writeprotect: add flashrom_wp_get_available_ranges()
Generate list of available ranges by enumerating all possible values
that range bits (BPx, TB, ...) can take and using the chip's range
decoding function to get the range that is selected by each one.
BUG=b:195381327,b:153800563
BRANCH=none
TEST=flashrom --wp-list
Change-Id: Id51f038f03305c8536d80313e52f77d27835f34d
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M libflashrom.c
M libflashrom.h
M writeprotect.c
M writeprotect.h
4 files changed, 282 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/81/58481/54
--
To view, visit https://review.coreboot.org/c/flashrom/+/58481
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id51f038f03305c8536d80313e52f77d27835f34d
Gerrit-Change-Number: 58481
Gerrit-PatchSet: 54
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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.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-MessageType: newpatchset
Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58483 )
Change subject: writeprotect: add {get,set}_wp_mode()
......................................................................
Patch Set 58: Code-Review+2
(1 comment)
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58483/comment/87b8ca29_3a27f0c4
PS58, Line 366: }
General thought: That the positive path comes first in such an if and ?:
can confuse the logical mind, because when enumerating cases usually
(logical) 0 comes first. A table might help, e.g.
static const int wp_mode[][] = {
/* srp=0 srp=1 */
/* srl=0 */ { FLASHROM_WP_MODE_DISABLED, FLASHROM_WP_MODE_HARDWARE },
/* srl=1 */ { FLASHROM_WP_MODE_POWER_CYCLE, FLASHROM_WP_MODE_PERMANENT },
};
--
To view, visit https://review.coreboot.org/c/flashrom/+/58483
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7b68e940f0e1359281806c98e1da119b4caf8405
Gerrit-Change-Number: 58483
Gerrit-PatchSet: 58
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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.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: Sun, 27 Feb 2022 19:39:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, Sergii Dmytruk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58482 )
Change subject: writeprotect: add set_wp_range()
......................................................................
Patch Set 56: Code-Review+2
(1 comment)
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58482/comment/dcc2aa31_6accdbeb
PS56, Line 325: so they it select
> There is some typo here, what does it mean to say?
I guess either "so they select" or "so it selects". The former seems more accurate.
--
To view, visit https://review.coreboot.org/c/flashrom/+/58482
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7d26f43fb05c5828b9839bb57a28fa1088dcd9a0
Gerrit-Change-Number: 58482
Gerrit-PatchSet: 56
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Sun, 27 Feb 2022 19:18:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58481 )
Change subject: libflashrom,writeprotect: add flashrom_wp_get_available_ranges()
......................................................................
Patch Set 53: Code-Review+1
(7 comments)
Patchset:
PS53:
Ah, sorry, apparently I forgot to publish comments the last
time I looked ._.
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58481/comment/fcccc7bd_4ba2a9a3
PS49, Line 153: range_list
Maybe abbreviate here and in the two functions below like above, i.e.
`ranges`?
https://review.coreboot.org/c/flashrom/+/58481/comment/9576828b_e70802af
PS49, Line 154: size_t
I would prefer `unsigned int` as `size_t` is officially for byte counts.
We use it internally sometimes for indices in general, but it seems
better to not have such local style in the API.
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/58481/comment/c53b32cf_e53d5790
PS49, Line 800: }
No need to change this if you don't want to. The pattern that seems
to prevail nowadays is to check for errors first and bail out, so
the actual code is at the end and doesn't need indentation, i.e.
if (index >= list->count)
return FLASHROM_WP_ERR_OTHER;
*start = ...
https://review.coreboot.org/c/flashrom/+/58481/comment/2b0a954f_2f3c1252
PS49, Line 811: free(list->ranges);
Better to check for NULL first, e.g.
if (!list)
return;
Then the caller doesn't need to ensure it's set. free() allows NULL and
it makes error paths easier to handle when one just needs to write
`free(x);` without needing to care if `x` was allocated already.
(flashrom_flash_release() doesn't follow that rule either, I'll push a patch CB:62340)
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58481/comment/2fb0e985_9bbbb4d7
PS49, Line 171: Comparitor
Compar*a*tor
https://review.coreboot.org/c/flashrom/+/58481/comment/d664bd22_eae082f0
PS49, Line 200: size_t j = a->bits.bp_bit_count - i - 1;
Reversing the loop would be more readable, I guess:
for (int i = a->bits.bp_bit_count - 1; i >= 0; --i)
--
To view, visit https://review.coreboot.org/c/flashrom/+/58481
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id51f038f03305c8536d80313e52f77d27835f34d
Gerrit-Change-Number: 58481
Gerrit-PatchSet: 53
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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.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: Sun, 27 Feb 2022 19:12:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment