Attention is currently required from: Edward O'Callaghan, 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 23:
(13 comments)
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58481/comment/edc2be39_ebfc32a3 PS23, Line 137: #define FLASHROM_WP_MAX_RANGES 128 Should we use dynamic allocation instead?
https://review.coreboot.org/c/flashrom/+/58481/comment/4a270989_115931b3 PS23, Line 143: size_t Technically `size_t` is supposed to specify a byte count. It's often accepted as a short hand for unsigned int, but I don't think we should do that in the public API.
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58481/comment/91ee1087_a5ad6241 PS23, Line 183: }; That this is actually useful (to search for a configuration) is only visible in a later commit. Please mention such things in the commit message.
https://review.coreboot.org/c/flashrom/+/58481/comment/f4f84209_da0278a2 PS23, Line 187: preferred What is preferred and why?
https://review.coreboot.org/c/flashrom/+/58481/comment/66979be6_a770a7f8 PS23, Line 233: TB in a OTP register Please mention the chip where you have seen something like this. Sometimes these things are hard to believe and you can spare future readers the pain of having to search for such a chip.
https://review.coreboot.org/c/flashrom/+/58481/comment/748ce95a_a39f399e PS23, Line 234: perserved p*re*served
https://review.coreboot.org/c/flashrom/+/58481/comment/2920f0e1_17ca99ca PS23, Line 237: 20 ARRAY_SIZE(bits->bp) + 1 /* TB */ + 1 /* SEC */ + 1 /* CMP */ ?
https://review.coreboot.org/c/flashrom/+/58481/comment/b22142db_0361f08d PS23, Line 240: can_write_bit(bits->bp[i]) This would stop at the first not writable bit and skip further writable ones, right?
https://review.coreboot.org/c/flashrom/+/58481/comment/5c3164d5_90c55d22 PS23, Line 254: (1 << bit_count) Nit, no parentheses needed.
https://review.coreboot.org/c/flashrom/+/58481/comment/61cfdedb_56d6effb PS23, Line 255: 32 Why 32?
https://review.coreboot.org/c/flashrom/+/58481/comment/4417747c_54c7ec49 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 be necessary if they were allocated with `*count` elements).
https://review.coreboot.org/c/flashrom/+/58481/comment/cb298bd0_6d007222 PS23, Line 301: Not sure if we should use tabs here.
https://review.coreboot.org/c/flashrom/+/58481/comment/0219ab50_4c4151d1 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.