Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 8: -Code-Review
(2 comments)
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h File src/include/mrc_cache.h:
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@38 PS8, Line 38: so it is up to the : * caller to unmap if necessary.
I don't think the caller can really unmap since it does not have pointer to the rdev structure used […]
Technically you could do rdev_unmap(boot_device_ro(), mapping) since rdev ops are always only tied to the root device anyway. We could make an mrc_cache_unmap() to encapsulate that so the caller wouldn't have to guess which root device mrc_cache.c uses. But considering that this is only meant to be used by platforms that know mapping is a no-op for them anyway, I don't think that would be worth it.
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@43 PS8, Line 43: data_size
There is at least one path which calls this function with data_size set to NULL. […]
I think I would prefer keeping this consistent with the underlying rdev API to be honest. rdev_mmap() returns a pointer or NULL on error, and rdev_read() returns < 0 on error. mrc_cache_current_mmap_leak() is the higher level MRC-cache-specific equivalent of rdev_mmap() and mrc_cache_load_current() of rdev_read(), so I think it's good they follow the same return code conventions.