[M] Change in flashrom[master]: flashrom.c:Add function to get a flattened view of the chip erase blocks
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
-
Aarya (Code Review)