Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 21:
(10 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 441: version[0]
TPM_BOARD_CFG is supported from the version 0.{5,6}.5. […]
Thanks, I have updated the logic to only assume that the BOARD_CFG is supported on 0.5.5, and 0.6.5 and onwards. (As well as 1+.x.x, though that probably does not matter.)
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 628: char
Is stack space tight? We are not using this version beyond the invocation of this method, so what is […]
I assume this suggestion has been withdrawn.
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 645: sizeof(version_str)
You are right, thanks for paying attention.
Actually, you were not right, but I had forgotten the reason my first version was correct. What the code was doing (due chunk_count being previously incremented, and then adding one more) was to verify that the buffer has at least 50 bytes left, which has not been touched yet. On the last iteration, there will be only one byte left. This was to guard against version_str not being one more than a multiple of chunk_size.
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 440: (version[1] >= 5 && version[2] >= 4)) {
nit: Probably fits on the previous line within the 96-column limit.
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 444: " TPM_BOARD_CFG, version: %d.%d.%d\n",
This can go on the previous line. […]
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 459: &board_cfg_value, sizeof(board_cfg_value))) {
nit: This probably fits on the previous line within the 96-column limit?
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 484: &board_cfg_value, sizeof(board_cfg_value)))
nit: Fits on previous line.
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 498: &board_cfg_value, sizeof(board_cfg_value))) {
nit: This probably fits on the previous line within the 96-column limit.
Done
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 622: ;
= { 0 };
The logic does not depend on the buffer being pre-initialized to zero.
(And the explicit zero-termination after the while loop cannot be safely removed, even if we pre-fill the buffer with zeroes. In the case of version_str being re-declared to e.g. 300 bytes, or some other multiple of chunk_size, the last byte could be written to non-zero during the last iteration of the loop.)
https://review.coreboot.org/c/coreboot/+/43741/20/src/drivers/spi/tpm/tpm.c@... PS20, Line 638: + 1
This is not correct. chunk_count is already incremented in line 637. […]
You are right that after my previous revision, this is no longer correct. I have changed the logic somewhat, to separate and document the two exit conditions.