Attention is currently required from: Julius Werner. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59494 )
Change subject: libpayload/libc/fmap: Implement new FlashMap API ......................................................................
Patch Set 6:
(9 comments)
File payloads/libpayload/libc/fmap.c:
https://review.coreboot.org/c/coreboot/+/59494/comment/3306c814_01f9ca8c PS6, Line 34: #include <fmap_serialized.h>
While you're here would you mind also getting rid of this version of the file and have libpayload us […]
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/64109664_0d09cfef PS6, Line 41: int
This should then also be cb_err_t
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/37767eb2_f6274076 PS6, Line 50: 0
CB_SUCCESS
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/8a94e3e2_cb6def6c PS6, Line 53: -1
Probably not worth making a new type for this, so maybe just reuse CB_CBFS_NOT_FOUND here?
CB_CBFS_NOT_FOUND has description: "File not found in directory", which indicates, that we are looking for a CBFS file. I think, it will be misleading for user/developer and might suggest, that an error is in a different place in the code.
https://review.coreboot.org/c/coreboot/+/59494/comment/de06c7f1_bb46d937 PS6, Line 73: int
I'm actually surprised the compiler doesn't complain, but this should match the declaration (cb_err_ […]
It does not complain, because cb_err_t is:
typedef int32_t cb_err_t;
File payloads/libpayload/tests/libc/fmap_locate_area-test.c:
https://review.coreboot.org/c/coreboot/+/59494/comment/32ab4b6e_369b7b33 PS6, Line 14: extern struct fmap *_fmap_cache;
This no longer belongs here
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/bbca9f3c_bd8f9d9b PS6, Line 98: _fmap_cache = NULL;
Why not use reset_fmap_cache() for these?
Because _fmap_cache will be pointing to fmap_buffer, which is allocated on the stack.
https://review.coreboot.org/c/coreboot/+/59494/comment/9b10267d_788f2e28 PS6, Line 100: /* Different number of areas in fmap_cache and fmap_offset */
Okay so now the comment is correct but I'm still not sure why we want to test this? I would say the […]
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/7643efaa_025c0135 PS6, Line 101: lib_sysinfo.fmap_offset = (uint64_t)fmap_buffer;
This is super weird anyway. […]
Done