Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 )
Change subject: soc/intel/common/block/cse: Add boot partition related APIs ......................................................................
Patch Set 25:
(7 comments)
Sorry to jump late into the review. It seems `total_number_of_bp` is ignoring, that concerns me. The rest is just bikeshedding.
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 90: cse_bp_info.bp_entries[BP2].bp_status); Shouldn't we check `total_number_of_bp` and only print valid BPs? A loop would make this easier.
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 93: uint8_t When using 1 for success and 0 for failure, bool and true/false would reflect that better.
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 108: (void *) memset() and memcpy() expect `void *`. but any pointer can be used as such. Hence, no cast is necessary.
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 112: size_t resp_size = sizeof(struct get_boot_partition_info_resp); Nit, `sizeof(info_resp)` would be easier to read, imho.
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 125: sizeof(struct cse_boot_partition_info)); Nit, `sizeof(*cse_bp_info)` would make it clearer that it's the correct size.
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 269: Shouldn't we check `total_number_of_bp` too?
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/25/src/soc/intel/common/block... PS25, Line 155: * Otherwise, it contains only 2 partitions(BP1+BP3) Is `BP3` a typo here? How would it work w/o BP2?