Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/drivers/spi/tpm/tpm.c@... PS11, Line 621: /* : * Read chunk_size bytes at a time, last chunk will be zero : * padded. : */ : do { I am not sure if firmware version needs to be read in every stage. I see in all the stages, it is read, logged and dropped.
With all the new use-cases in ramstage, we can read the firmware version in ramstage alone.
Also with more than one use-case in ramstage, if the firmware version is already cached we can skip reading the firmware version. I believe it is not going to change within the ramstage.
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/c/coreboot/+/43741/11/src/lib/string.c@170 PS11, Line 170: if (!strncmp(haystack, needle, needle_len))
Why not just strcmp()?
The needle may hit '\0' before the haystack. That will cause strcmp to return *haystack - '\0'.
Also for the use-case here, tokenizing the haystack using a " " delimiter can work slightly better than strstr - eg. https://review.coreboot.org/c/coreboot/+/44277/2/src/drivers/spi/tpm/tpm.c#4...
If we take that approach, then strstr can be dropped.