Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk, Nikolai Artemiev.
7 comments:
File erasure_layout.c:
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.
Patch Set #5, 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.
Patch Set #5, 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:
Patch Set #5, Line 1042: _tagged
Maybe without `_tagged` ? the rest of functions have suffix, so there will be no clash
Done
Patch Set #5, 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
Patch Set #5, Line 1092: void erase_unwritable_regions_skipflag_on_test_success(void **state)
I think this qualifies for a comment: […]
Done
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
To view, visit change 82393. To unsubscribe, or for help writing mail filters, visit settings.