Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev.
Hello Felix Singer, build bot (Jenkins), Angel Pons, Nikolai Artemiev,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/70895
to look at the new patch set (#6).
Change subject: cli_classic.c: Allow ctrl of behaviour with WO and RO regions
......................................................................
cli_classic.c: Allow ctrl of behaviour with WO and RO regions
Add two force flags to the flashrom CLI frontend,
--force-skip-nonreadable - skip reading non-readbale regions without failure,
--force-skip-nonwritable - skip writing non-writable regions without failure.
Each flag allows the user to finely control the behaviour of
flashrom read and write operations when flashrom encounters a
known permission problem. This can be useful in cases such as
wanting to write a pre-padded BIOS image across flash where locked
regions just wish to be left untouched without the need to craft a
custom layout file for each case. Alternatively, to read a copy of
flash complete with padding for regions that are known not to be
readable at runtime however the user does not know the layout of
flash or just wishes to have a padded binary read back.
Change-Id: Iff753b748765410ea38b845613c361db7ad16a61
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M cli_classic.c
M flashrom.8.tmpl
2 files changed, 53 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/95/70895/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/70895
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iff753b748765410ea38b845613c361db7ad16a61
Gerrit-Change-Number: 70895
Gerrit-PatchSet: 6
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Simon Buhrow, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk.
Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66104 )
Change subject: flashrom.c: Add wrapper function to use the erase algorithm
......................................................................
Patch Set 92:
(9 comments)
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/66104/comment/5d1239da_3226feb6
PS91, Line 165: s
> `const s`
Done
https://review.coreboot.org/c/flashrom/+/66104/comment/69d274b5_cd3f5364
PS91, Line 170: old_start_buf
> allocation never checked.
Done
https://review.coreboot.org/c/flashrom/+/66104/comment/0c6b856a_48af033c
PS91, Line 182: //
> add some `\n` to help readability of the function.
Done
https://review.coreboot.org/c/flashrom/+/66104/comment/4356146b_96083abd
PS91, Line 188: (
> trivial: ` (`
Done
https://review.coreboot.org/c/flashrom/+/66104/comment/99016884_ade85873
PS91, Line 189: (
> trivial: ` (`
Done
https://review.coreboot.org/c/flashrom/+/66104/comment/392bd159_c6341e53
PS91, Line 194: if(erase_layout[i].layout_list[j].selected) {
> consider identifying more base-cases to early return/continue/break to avoid deep loops and branches […]
Done
https://review.coreboot.org/c/flashrom/+/66104/comment/50998fbf_0c5cfff2
PS91, Line 195: //execute erase
: erasefunc_t *erasefn = lookup_erase_func_ptr(erase_layout[i].eraser);
: ret = erasefn(flashctx, start_addr, block_len);
: if (ret) {
: msg_cerr("Failed to execute erase command for offset %#x to %#x.\n", start_addr, start_addr + block_len);
: ret = -1;
: goto _end;
: }
: //adjust curcontents
: memset(curcontents+start_addr, erased_value, block_len);
: //after erase make it unselected again
: erase_layout[i].layout_list[j].selected = false;
: msg_cdbg("E(%x:%x)", start_addr, start_addr + block_len - 1);
: //verify erase
: ret = check_erased_range(flashctx, start_addr, block_len);
: if (ret) {
: msg_cerr("Verifying flash. Erase failed for range %#x : %#x, Abort.\n", start_addr, start_addr + block_len - 1);
: goto _end;
: }
> integrate CB:70517
??
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/66104/comment/e8e49652_4f8388c6
PS72, Line 1349: static int erase_by_layout
> Yeah sorry for the delay. I'm starting to move the implementation to a separate file now.
Done
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/66104/comment/1cf011ca_364dd6ae
PS78, Line 1422: write_by_layout
> in a separate commit before this one, rename as `write_by_layout_legacy()` and, […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/66104
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I29e3f2bd796759794184b125741a5abaac6f3ce8
Gerrit-Change-Number: 66104
Gerrit-PatchSet: 92
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 23 Dec 2022 17:01:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Simon Buhrow, Edward O'Callaghan.
Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65844 )
Change subject: flashrom.c:Add a function to get list of sectors that need erasing
......................................................................
Patch Set 74:
(6 comments)
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/65844/comment/151a1291_97ff5db6
PS73, Line 127: region_end
> `rend` and Doxygen.
Done
https://review.coreboot.org/c/flashrom/+/65844/comment/0816ab4d_167f38e7
PS73, Line 127: void *
> `void *` in type system speak is mostly like putting on a blind-fold and jumping between a large can […]
Done
https://review.coreboot.org/c/flashrom/+/65844/comment/fdee2ac5_28cd8b26
PS73, Line 127: region_start
> `rstart` and Doxygen.
Done
https://review.coreboot.org/c/flashrom/+/65844/comment/fd0be5ef_5bc380ff
PS73, Line 127: function_index
> `findex` and in the Doxygen comment you can say "function index".
Done
https://review.coreboot.org/c/flashrom/+/65844/comment/a432e29b_bd28e321
PS73, Line 127: struct flashctx *flashctx,
> by convention, keep the `flashctx` as the first argument to the function, it helps keep the code-bas […]
Done
https://review.coreboot.org/c/flashrom/+/65844/comment/75d3e6c3_55578ac9
PS73, Line 130: function_index == 0
> `!foo` is canonical for the 0 or NULL case.
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/65844
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256
Gerrit-Change-Number: 65844
Gerrit-PatchSet: 74
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Fri, 23 Dec 2022 17:01:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Simon Buhrow, Nico Huber, Thomas Heijligen, Paul Menzel, Edward O'Callaghan.
Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65879 )
Change subject: flashrom.c:Add function to get a flattened view of the chip erase blocks
......................................................................
Patch Set 55:
(11 comments)
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/65879/comment/f296e9ed_59988bca
PS54, Line 24: int
> `unsigned int`
Done
https://review.coreboot.org/c/flashrom/+/65879/comment/64923c4e_2eddf29b
PS54, Line 26: if (layout) {
> rewrite your function like this: […]
Done
https://review.coreboot.org/c/flashrom/+/65879/comment/7ee44179_11644b3e
PS54, Line 34: )
> The signature of this function should be, […]
Done
https://review.coreboot.org/c/flashrom/+/65879/comment/73e83c48_5ae2e8ae
PS54, Line 48: ;
> \n
Done
https://review.coreboot.org/c/flashrom/+/65879/comment/1653c5f8_c44e0b67
PS54, Line 54: int j = 0;
: while(addr < chip->total_size * 1024) {
> ok `j` is shadowed below. Rewrite this while-loop construct as a `for` loop construct as: […]
Done
https://review.coreboot.org/c/flashrom/+/65879/comment/4a83b380_8a84abed
PS54, Line 64: )
: {
> trivial: `) {` and `if (!ptr)` is canonical.
Done
https://review.coreboot.org/c/flashrom/+/65879/comment/c7338721_7c338bb5
PS54, Line 71: chipoff_t start_addr = 0;
> does this belong to the outer block_count loop? should it be reset when the inner loop finishes for […]
Yes it belongs to outer loop and resets every iteration.
https://review.coreboot.org/c/flashrom/+/65879/comment/57cc57f6_de7f650a
PS54, Line 72: chipoff_t end_addr = 0;
> delete and also don't unnecessarily initialise variables as you obscure compiler warnings.
Done
https://review.coreboot.org/c/flashrom/+/65879/comment/65606c19_03c74f8f
PS54, Line 74: block_num < block_count
> since `block_num` is initialised to zero above doesn't this predicate become `0 < block_count` and t […]
No, `block_num` increases.
https://review.coreboot.org/c/flashrom/+/65879/comment/d8ac57a9_360f9b7f
PS54, Line 79: e
> Keep minimal scope to variables so declare here, […]
Done
https://review.coreboot.org/c/flashrom/+/65879/comment/4918a0b3_68be5692
PS54, Line 84: if (layout_idx > 0) {
: layout[layout_idx].layout_list[block_num].first_sub_block_index = sub_block_index;
: while (layout[layout_idx-1].layout_list[sub_block_index].start_addr >= start_addr &&
: layout[layout_idx-1].layout_list[sub_block_index].end_addr <= end_addr &&
: sub_block_index < layout[layout_idx-1].block_count) {
: sub_block_index++;
: }
: layout[layout_idx].layout_list[block_num].last_sub_block_index = sub_block_index - 1;
: }
> looks like another function with a base case of `layout_idx == 0 => return`.
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/65879
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iafe78de00daa55f7114bd4ce09465dd88074ece4
Gerrit-Change-Number: 65879
Gerrit-PatchSet: 55
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Fri, 23 Dec 2022 17:01:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Simon Buhrow, Thomas Heijligen, Aarya.
Hello build bot (Jenkins), Simon Buhrow, Thomas Heijligen, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/71173
to look at the new patch set (#7).
Change subject: flashrom.c: Add new erase_by_layout and walk_bylayout implementations
......................................................................
flashrom.c: Add new erase_by_layout and walk_bylayout implementations
Add [erase,walk]_by_layout_new to use optimised implementations of the
erase function selection algorithm.
Change-Id: Id79ae943eb9d6a817da28381db477725834faaf6
Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com>
---
M flashrom.c
1 file changed, 92 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/73/71173/7
--
To view, visit https://review.coreboot.org/c/flashrom/+/71173
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id79ae943eb9d6a817da28381db477725834faaf6
Gerrit-Change-Number: 71173
Gerrit-PatchSet: 7
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Simon Buhrow, Nico Huber, Aarya, Anastasia Klimchuk.
Hello build bot (Jenkins), Simon Buhrow, Thomas Heijligen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/66104
to look at the new patch set (#92).
Change subject: flashrom.c: Add wrapper function to use the erase algorithm
......................................................................
flashrom.c: Add wrapper function to use the erase algorithm
Add a function to call the erase algorithm.
Change-Id: I29e3f2bd796759794184b125741a5abaac6f3ce8
Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com>
---
M erasure_layout.c
M erasure_layout.h
2 files changed, 116 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/04/66104/92
--
To view, visit https://review.coreboot.org/c/flashrom/+/66104
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I29e3f2bd796759794184b125741a5abaac6f3ce8
Gerrit-Change-Number: 66104
Gerrit-PatchSet: 92
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Simon Buhrow, Aarya.
Hello build bot (Jenkins), Simon Buhrow, Thomas Heijligen, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/65844
to look at the new patch set (#74).
Change subject: flashrom.c:Add a function to get list of sectors that need erasing
......................................................................
flashrom.c:Add a function to get list of sectors that need erasing
Add a function that returns a list of sectors (as seen by the first
erase function) that need erasing.
Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256
Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com>
---
M erasure_layout.c
1 file changed, 48 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/65844/74
--
To view, visit https://review.coreboot.org/c/flashrom/+/65844
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256
Gerrit-Change-Number: 65844
Gerrit-PatchSet: 74
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Simon Buhrow, Nico Huber, Thomas Heijligen, Paul Menzel, Aarya.
Hello build bot (Jenkins), Simon Buhrow, Thomas Heijligen, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/65879
to look at the new patch set (#55).
Change subject: flashrom.c:Add function to get a flattened view of the chip erase blocks
......................................................................
flashrom.c:Add function to get a flattened view of the chip erase blocks
Add a function to flatten out the addresses of the flash chip as per the
different erase functions. This function will return a list of layouts
which is dynamically allocated. So after use all the layouts as well as
the list itself should be freed. The free_erase_layout function does
that.
Change-Id: Iafe78de00daa55f7114bd4ce09465dd88074ece4
Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com>
---
A erasure_layout.c
A erasure_layout.h
2 files changed, 158 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/65879/55
--
To view, visit https://review.coreboot.org/c/flashrom/+/65879
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iafe78de00daa55f7114bd4ce09465dd88074ece4
Gerrit-Change-Number: 65879
Gerrit-PatchSet: 55
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-MessageType: newpatchset