Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine.
Anastasia Klimchuk has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82393?usp=email )
Change subject: erasure_layout: Fix get_flash_region bug ......................................................................
Patch Set 9:
(12 comments)
Patchset:
PS9: Thank you so much for combining all the work in one patch!
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/82393/comment/6c9f690b_7d0b2429?usp... : PS5, Line 240:
I'm a little hesitant to, because `chipoff_t` implies inclusive endpoints. […]
I forgot that `chipoff_t` has its own comment. Now I agree with you.
https://review.coreboot.org/c/flashrom/+/82393/comment/efeaeea0_4087bd38?usp... : PS5, Line 369: len = min(region_end, region.end - 1) - addr + 1;
I added a new parent change that fixes get_flash_region's incorrect use of chipoff_t (now the upper […]
This is great, thank you!
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/82393/comment/ec18619d_d61c83fa?usp... : PS9, Line 766: 16 To be consistent with your previous patch, and also I think tests are partially a documentation (a special kind of it), this needs to be 15, right?
https://review.coreboot.org/c/flashrom/+/82393/comment/d03af7c3_02c698ba?usp... : PS9, Line 951: get_erase_protected_region_algo_tests I think these ones can be also run through write operation, and verifying the memory state after write. The change in flashrom.c affects write as well. But this can be in a separate patch.
https://review.coreboot.org/c/flashrom/+/82393/comment/e4e82405_a99070ef?usp... : PS9, Line 953: const size_t num_unparameterized = 1; Do you think this is needed? It's the same name and the same function. If you change it to 2, it will run the same test twice. Maybe remove this and allocate one test?
https://review.coreboot.org/c/flashrom/+/82393/comment/d42614b2_b3d5edc6?usp... : PS9, Line 973: .name = "erase failure for unskipped unwritable regions", I had the trouble with test names, because I wanted an index (a test case #) in the name. This one is just one, so a name without an index is fine.
I looked into the test `test_erase_fails_for_unwritable_region` I think it's only flag FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS that prevents in from being in the list? setup and teardown seem innocent, won't hurt to have them here. So it's not being unparameterized , but FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS=false is what makes this test different?
Anyway, this can be changed later.
https://review.coreboot.org/c/flashrom/+/82393/comment/80c8e648_8638f708?usp... : PS9, Line 1103: 16 This is `END_PROTECTED_REGION + 1`, just for perfection (with my other comment about END_PROTECTED_REGION)
https://review.coreboot.org/c/flashrom/+/82393/comment/7d1ee9a4_154eadc2?usp... : PS9, Line 1138: < <= if you agree that it should be inclusive
https://review.coreboot.org/c/flashrom/+/82393/comment/8c05f3fd_11e1d2f6?usp... : PS9, Line 1140: < also here <=
File tests/tests.h:
https://review.coreboot.org/c/flashrom/+/82393/comment/310596a6_375a8f10?usp... : PS9, Line 113: void erase_unwritable_regions_skipflag_off_test_success(void **state); this also can be removed from this patch
https://review.coreboot.org/c/flashrom/+/82393/comment/f5a48954_05c164c1?usp... : PS9, Line 114: erase_unwritable_regions_skipflag_on_test_success I am wondering maybe this is also not needed. Because the function which is called in tests.c is `get_erase_func_algo_tests` but not this one. I added this line, but then also tests ended up having a special invocation and I think it's not needed anymore.
-
Anastasia Klimchuk (Code Review)