10 comments:
Patch Set #4, 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);
...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.
Patch Set #4, 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.
Patch Set #4, 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).
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?
Patch Set #4, Line 164: assert_int_equal(FMAP_SECTION_COREBOOT_START, ar.offset);
For regions we also have accessors (region_offset()/region_sz()).
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, Line 251: fmap_locate_area(section_name, &ar);
same as above
typo
To view, visit change 48557. To unsubscribe, or for help writing mail filters, visit settings.