Name of user not set #1003143 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48557 )
Change subject: tests: Add lib/fmap-test test case ......................................................................
Patch Set 2:
(11 comments)
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... File tests/include/lib/fmap/fmap_config.h:
PS2:
Always put a license line, even on generated files, just to be safe.
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... PS2, Line 7: * FMAP_OFFSET was changed to zero for testing purposes.
Doesn't look like it's zero below?
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... PS2, Line 84: //#define FMAP_SECTION_UNUSED_HOLE_START 0xfff000
??
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... File tests/include/lib/fmap/fmap_data.h:
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... PS2, Line 3: #ifndef TESTS_INCLUDE_LIB_FMAP_FMAP_DATA_H_
I don't think you should use tests/include/lib for this, because then all the headers in there clash […]
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... PS2, Line 145: 1568
nit: just use sizeof()?
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c File tests/lib/fmap-test.c:
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@18 PS2, Line 18: static void *local_mmap(const struct region_device *rdev, size_t offset, size_t size)
I think you could just use a mem_region_device instead of all of this?
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@38 PS2, Line 38: size = MIN(size, flash_buffer_size - offset);
Doesn't the check above make this unnecessary?
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@84 PS2, Line 84: rdev_root = malloc(sizeof(rdev_new_root));
I think you can just write […]
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@146 PS2, Line 146: assert_int_not_equal(-1, fmap_locate_area_as_rdev("RO_VPD", rdev));
Please test for actual offsets, not just != -1.
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@161 PS2, Line 161: }
Test the returned rdev, too? Make sure you cannot rdev_writeat() the RO version?
Done
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@261 PS2, Line 261: FMAP_SECTION_FW_MAIN_A_SIZE
Maybe make it smaller so you can also test that the remaining part of the region gets zeroed on over […]
Done