Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58481 )
Change subject: libflashrom,writeprotect: add flashrom_wp_get_ranges() ......................................................................
Patch Set 29:
(10 comments)
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58481/comment/6abe25c9_fa070678 PS23, Line 137: #define FLASHROM_WP_MAX_RANGES 128
Should we use dynamic allocation instead?
Done
https://review.coreboot.org/c/flashrom/+/58481/comment/ea09f326_7547bf65 PS23, Line 143: size_t
Technically `size_t` is supposed to specify a byte count. It's often […]
It's nice that size_t is guaranteed to be large enough to represent the number of elements in any array though. Although size_t's intended use is for representing the size of an object in bytes, I don't think that should keep us from using it for somewhere else where it's well suited.
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58481/comment/07e8e40a_724fb596 PS23, Line 187: preferred
What is preferred and why?
For choosing one range encoding over another it's pretty much arbitrary. Usually the only ranges that have multiple encodings either span the entire flash or are completely empty. E.g. TB and SEC have no affect on them. In that case TB/SEC are just set to 0.
I've removed preferred from the description since it was misleading.
https://review.coreboot.org/c/flashrom/+/58481/comment/9a67f579_b5c10f3c PS23, Line 233: TB in a OTP register
Please mention the chip where you have seen something like this. Sometimes […]
Done
https://review.coreboot.org/c/flashrom/+/58481/comment/54d9ee3d_f5cc0e35 PS23, Line 234: perserved
p*re*served
Done
https://review.coreboot.org/c/flashrom/+/58481/comment/d5ac52ce_86fee205 PS23, Line 237: 20
ARRAY_SIZE(bits->bp) + 1 /* TB */ + 1 /* SEC */ + 1 /* CMP */ ?
Done
https://review.coreboot.org/c/flashrom/+/58481/comment/88d3b7dc_b1ce3d8e PS23, Line 240: can_write_bit(bits->bp[i])
This would stop at the first not writable bit and skip further writable […]
Done
https://review.coreboot.org/c/flashrom/+/58481/comment/bf63877d_b27d14d5 PS23, Line 255: for (uint32_t range_index = 0; range_index < *count; range_index++) {
Please check that the destination arrays don't overflow (wouldn't […]
Done
https://review.coreboot.org/c/flashrom/+/58481/comment/fbb2de56_a1fff68c PS23, Line 255: 32
Why 32?
Changed to size_t.
https://review.coreboot.org/c/flashrom/+/58481/comment/d7afba92_d3026f76 PS23, Line 308: struct wp_range_cfg_pair range_cfg_pairs[FLASHROM_WP_MAX_RANGES];
Not on the stack, please. Flashrom sometimes runs in embedded environments.
Done
-
Nikolai Artemiev (Code Review)