Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36208 )
Change subject: security/vboot: Add vboot callbacks to support EC software sync ......................................................................
Patch Set 8:
(12 comments)
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 14: #include <2api.h>
vb2_api. […]
Done
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 37: LIMIT_POWER_WAIT_TIMEOUT
Please suffix this with a unit (e.g. _MS).
Done
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 39: #define LIMIT_POWER_POLL_SLEEP 10
This too
Done
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 99: static
I think it is static because the address of hash_digest buffer from the response is returned/assigne […]
Exactly, there is no information in the API about the required lifetime of the "returned" buffer (resp.hash_digest), so this way there is no concern.
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 119: (recalc_requested)
Since we have set EC_VBOOT_HASH_STATUS_BUSY and checking against that explicitly, at what point is t […]
Excellent point, this was ported straight from depthcharge w/ as few changes as possible. But that looks completely unnecessary. Will eliminate it.
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 232: int
I think it makes sense to be consistent per-file. […]
Done
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 236: MIN(1024
I think we may need to move this into google_chromeec_flash_write_burst_size(), because we still nee […]
I've removed that function in the next patch and integrated the logic into ec_flash_write().
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 244: -1
VB2_ERROR_UNKNOWN
Done
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 270: -1
VB2_ERROR_UNKNOWN
Done
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 273: 0
VB2_SUCCESS
Done
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 398: "flag.\n");
No need to break?
Was a return next line, see next CL for a refactor.
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 416: limit_power_wait_time += LIMIT_POWER_POLL_SLEEP;
nit: Would be better to solve this with the stopwatch API
Done