[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, Aarya.
Edward O'Callaghan 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 54:
(9 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/65879/comment/bbb0fb7b_ab60580a PS54, Line 11: So after use all the layouts as well as : the list itself should be freed. The free_erase_layout function does : that. this part should be documented above the function in a Doxygen style comment. https://github.com/flashrom/flashrom/blob/master/flashrom.c#L555 is a example of Doxygen style comments.
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/65879/comment/3d1ba57e_cd2db943 PS54, Line 24: int `unsigned int`
https://review.coreboot.org/c/flashrom/+/65879/comment/bab9c917_b22c99fe PS54, Line 34: create_erase_layout The O-time complexity of this function seems extremely large for something that says it just creates a layout? Can you add some Doxygen comments above the function and punctuate the function with some comments inside to allow for understanding the phases your algorithm is taking here please?
https://review.coreboot.org/c/flashrom/+/65879/comment/c9fe84b0_3e392952 PS54, Line 55: ( trivial: `e (`
https://review.coreboot.org/c/flashrom/+/65879/comment/6e8fdcd1_3efdca57 PS54, Line 64: ) : { trivial: `) {` and `if (!ptr)` is canonical.
https://review.coreboot.org/c/flashrom/+/65879/comment/219fe8bf_68878343 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 the next iteration of the outer loop?
https://review.coreboot.org/c/flashrom/+/65879/comment/3f0a0f51_2411aa78 PS54, Line 72: chipoff_t end_addr = 0; delete and also don't unnecessarily initialise variables as you obscure compiler warnings.
https://review.coreboot.org/c/flashrom/+/65879/comment/4e5d6042_6cd915ef PS54, Line 74: block_num < block_count since `block_num` is initialised to zero above doesn't this predicate become `0 < block_count` and the block_count never decrements so this is a infinite loop?
https://review.coreboot.org/c/flashrom/+/65879/comment/e03cb7c1_fe1ecba0 PS54, Line 79: e Keep minimal scope to variables so declare here, `chipoff_t end_addr = start_addr + block->size - 1;`
-
Edward O'Callaghan (Code Review)