Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82393?usp=email )
Change subject: erasure_layout: Fix get_flash_region bug ......................................................................
Patch Set 5:
(8 comments)
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/82393/comment/c57584b4_2953c93e : PS5, Line 240: May I ask you to also add doc comment here , most importantly to say that region_end is inclusive
https://review.coreboot.org/c/flashrom/+/82393/comment/4ed42d95_b8c1c458 : PS5, Line 320: * @param region_end end address of the region I know it wasn't here before, but let's document it here that *end is inclusive*. those 1-offsets are tricky, but also it will be useful for future readers
Also, maybe you can check with your own eyes that region end is inclusive? I checked (by the callsites of erase_write) but just in case!
https://review.coreboot.org/c/flashrom/+/82393/comment/970afeab_ee925839 : PS5, Line 335: region_end - region_start If region end is inclusive, this needs to be +1 byte... am I right? (because the 3rd arg is length) It seems we can add one more test case for this, a test case with protected region of 1 last byte
https://review.coreboot.org/c/flashrom/+/82393/comment/8f01d70f_1f1c50a9 : PS5, Line 369: len = min(region_end, region.end - 1) - addr + 1; Maybe add a comment above this line that region_end is inclusive, but region.end is exclusive
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/82393/comment/992a1efb_d6f8d0e4 : PS5, Line 1042: _tagged Maybe without `_tagged` ? the rest of functions have suffix, so there will be no clash
https://review.coreboot.org/c/flashrom/+/82393/comment/8a96caaa_9405bcde : PS5, Line 1082: #define ERASE_PROTECTED(i) static int block_erase_chip_with_protected_region_ ## i \ That's what I was thinking for the previous patch, a macro loop! :)
https://review.coreboot.org/c/flashrom/+/82393/comment/0f05981b_e52b1ed9 : PS5, Line 1092: void erase_unwritable_regions_skipflag_on_test_success(void **state) I think this qualifies for a comment:
This test that runs through the test cases with protected regions on the chip, and with a flag set to skip (i.e. ignore) protected regions. Ignoring means they are left as is, and no opcodes should be sent to any memory blocks inside protected regions.
feel free to improve if you want!
https://review.coreboot.org/c/flashrom/+/82393/comment/7971c1d9_07e608ab : PS5, Line 1172: void erase_unwritable_regions_skipflag_off_test_success(void **state) : { : // flashrom_flag_set(&flashctx, FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS, false); : } I meant to write this test too, but didn't finish it. Probably better to remove from this patch, and I will add it later