Attention is currently required from: Dinesh Gehlot, Kapil Porwal, Sridhar Siricilla.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74995?usp=email )
Change subject: soc/intel/cmd/blk/cse: Implement APIs to access FW versions in CMOS ......................................................................
Patch Set 54:
(2 comments)
File src/soc/intel/common/block/cse/cse_lite_cmos.c:
https://review.coreboot.org/c/coreboot/+/74995/comment/e6a6616f_257669cf : PS54, Line 74: /* Update checksum over firmware versions */ : info->crc = get_cse_info_crc(info);
The variable "info" is a temporary variable that stores the data read from the CMOS area.
info is a input which holds the DS that we wished to write into the CMOS. I don't understand why you said this is temp variable ?
We update the required CSE firmware partition version, calculate the CRC checksum of the updated data, and update the CRC field. Finally, we copy the entire "info" variable to the CMOS area.
So the design is to calculate CRC locally and update it to CMOS.
if you are passing the `info` to write into the CMOS, isn't the info->crc already has correct CRC data ? also the purpose of this function is to write into the CMOS, you can't change the input argument `info` in this process. Ideally the input should be `const`.
https://review.coreboot.org/c/coreboot/+/74995/comment/02b9bd07_94b9cf98 : PS54, Line 90: info->crc = get_cse_info_crc(info);
The variable "info" is a temporary variable that stores the data read from the CMOS area. We update the required CSE firmware partition version, calculate the CRC checksum of the updated data, and update the CRC field. Finally, we copy the entire "info" variable to the CMOS area.
https://review.coreboot.org/c/coreboot/+/74995/comment/1c7cf1af_4dbe01d8/