Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk, Nikolai Artemiev.
Peter Marheine 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 6:
(7 comments)
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/82393/comment/f4b3418e_02355139 : PS5, Line 240:
May I ask you to also add doc comment here , most importantly to say that region_end is inclusive
I'm a little hesitant to, because `chipoff_t` implies inclusive endpoints. We could just as easily add the same comment to every use of that type.
https://review.coreboot.org/c/flashrom/+/82393/comment/4e8f44f2_39889a85 : 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.
As above, `chipoff_t` implies this.
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!
While working on this I hacked debug logging to be included while running tests, which helped me identify where things were off by one; I'm confident that it's correct now, since the debug logs had consistent addresses.
https://review.coreboot.org/c/flashrom/+/82393/comment/621d1b03_b21976b5 : PS5, Line 369: len = min(region_end, region.end - 1) - addr + 1;
Maybe add a comment above this line that […]
I actually think `ich_get_region` (the only existing implementation of `get_region`) is wrong here, because `flash_region` uses `chipoff_t` but initializes `end` to the size of the flash which is not a valid `chipoff_t`. Both should be inclusive.
On further investigation, it looks like `get_flash_region` has always misused `chipoff_t` so it and all of its users need to be fixed.
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/82393/comment/83609822_7ee3c068 : PS5, Line 1042: _tagged
Maybe without `_tagged` ? the rest of functions have suffix, so there will be no clash
Done
https://review.coreboot.org/c/flashrom/+/82393/comment/c872ca8c_743f90e9 : 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! :)
Acknowledged
https://review.coreboot.org/c/flashrom/+/82393/comment/4aef587f_0b15267e : PS5, Line 1092: void erase_unwritable_regions_skipflag_on_test_success(void **state)
I think this qualifies for a comment: […]
Done
https://review.coreboot.org/c/flashrom/+/82393/comment/52e2344b_0e7c7834 : 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. […]
Done