Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48557 )
Change subject: tests: Add lib/fmap-test test case ......................................................................
Patch Set 4:
(10 comments)
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c File tests/lib/fmap-test.c:
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@94 PS4, Line 94: mem_rdev_fmap_ro.rdev.root = &mem_rdev_ro.rdev; What are you trying to do here? This isn't legal. MEM_REGION_DEV_RO_INIT already constructs a fully valid (root) rdev, don't muck around in the internals afterwards. The rdev API maintains some tricky internal invariants, you're only supposed to interact through it with the official accessors.
If you want a device that is chained off mem_rdev_ro instead, you can just do
struct region_device rdev; rdev_chain(&rdev, &mem_rdev_ro, FMAP_SECTION_FMAP_START, FMAP_SECTION_FMAP_SIZE);
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@97 PS4, Line 97: rdev ...and then here you immediately overwrite it here again... no, this is not how this API was designed. 'rdev' is an OUT-parameter to fmap_locate_area_as_rdev(). You don't need to initialize it at all, just allocate it on the stack. The function will fill it in for you. The way this is meant to be called is just
struct region_device rdev; fmap_locate_area_as_rdev("RO_VPD", &rdev);
You don't need to make any connection from mem_rdev_ro to your rdev yourself, the whole point is that the FMAP API is supposed to do that for you.
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@98 PS4, Line 98: assert_int_equal(FMAP_SECTION_RO_VPD_START, rdev->region.offset); Please use the region_device_offset() and region_device_sz() accessors instead of accessing rdev members directly.
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@118 PS4, Line 118: /* Function fmap_locate_area_as_rdev is not tested with NULL See comment style I mentioned in the other patch (or on https://doc.coreboot.org/coding_style.html#commenting).
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@154 PS4, Line 154: */ How about actually writing to it here, and then maybe looking it up as an RO rdev again to confirm your newly written data is visible?
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@164 PS4, Line 164: assert_int_equal(FMAP_SECTION_COREBOOT_START, ar.offset); For regions we also have accessors (region_offset()/region_sz()).
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@204 PS4, Line 204: ar.offset = 2142037; Would be good to test both an out-of-bounds area (like you're doing here) and an incorrect in-bounds area. Maybe also areas where either the start or the end offset is correct, but the size isn't.
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@221 PS4, Line 221: fmap_locate_area("RW_SECTION_A", &ar); Why is this call here? It does nothing and has nothing to do with the rest of the test. fmap_read_area() already does a lookup for the name you give it internally.
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@251 PS4, Line 251: fmap_locate_area(section_name, &ar); same as above
https://review.coreboot.org/c/coreboot/+/48557/4/tests/lib/fmap-test.c@261 PS4, Line 261: war typo