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 20:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 54: /* Set up context and work buffer */
nit: They're already set up you're just fetching them. […]
Ack
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 58: ctx->flags &= ~VB2_CONTEXT_EC_SYNC_SLOW;
There should be no way this could be set here, so no need to clear it.
Paranoia, I guess. Will remove.
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 61: vbnv_init(ctx->nvdata);
No, persistent context takes care of that already now.
Because it's already done in verstage, correct?
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 66: printk(BIOS_ERR, "EC software sync failed (%#x)\n", retval);
We actually need to reboot into recovery mode on error, not just print and move on. […]
Gotcha, same logic as in vboot_logic.c. I'll add a refactor for the data saving. +1 to the assert.
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 68: return retval;
Probably doesn't need a return value, then.
I suppose not.
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 156:
nit: maybe personal preference but I tend to not make a space between number and unit. […]
Doesn't matter to me, I can remove the spaces.