Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk, Nikolai Artemiev.
Peter Marheine 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 12:
(8 comments)
File tests/erase_func_algo.c:
https://review.coreboot.org/c/flashrom/+/82393/comment/1792f48b_a2cb24cb?usp... : PS9, Line 766: 16
To be consistent with your previous patch, and also I think tests are partially a documentation (a s […]
Done
https://review.coreboot.org/c/flashrom/+/82393/comment/11d54903_58ed26e6?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. […]
I don't understand what you're suggesting as an alternative. All of the tests need to be allocated in one block, and since this value is used in two locations (the size for memcpy below as well as generating the total number of tests) it seemed less error-prone to put the 1 in as a constant: this way you're less likely to forget to increase the allocation size if you add another unparameterized case.
https://review.coreboot.org/c/flashrom/+/82393/comment/98801773_4b44b3a6?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. […]
The key with this test is that it expects `flashrom_flash_erase` to fail, whereas the other tests (with `SKIP_UNWRITABLE_REGIONS` enabled) require it to succeed.
https://review.coreboot.org/c/flashrom/+/82393/comment/381cf412_d9796d9e?usp... : PS9, Line 1103: 16
This is `END_PROTECTED_REGION + 1`, just for perfection […]
Done
https://review.coreboot.org/c/flashrom/+/82393/comment/27425dc9_70d4251d?usp... : PS9, Line 1138: <
<= […]
Done
https://review.coreboot.org/c/flashrom/+/82393/comment/cd5f27f4_e0442904?usp... : PS9, Line 1140: <
also here <=
I realized there was another issue with this comparison: it didn't handle the case where the block to be erased completely contains the protected region but neither endpoint is inside it. That doesn't affect the tests right now, but I added a check for that as well.
File tests/tests.h:
https://review.coreboot.org/c/flashrom/+/82393/comment/40e2456a_0f17e15c?usp... : PS9, Line 113: void erase_unwritable_regions_skipflag_off_test_success(void **state);
this also can be removed from this patch
Done
https://review.coreboot.org/c/flashrom/+/82393/comment/ff54bfe2_4fd50433?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. […]
Done
-
Peter Marheine (Code Review)