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 10:
(9 comments)
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/Kconfig@... PS9, Line 230: depends on CHROMEOS
Why? (If anything, I'd say it depends on EC_GOOGLE_CHROMEEC. […]
Ah, I missed that one. That fits better.
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 119: (recalc_requested)
Isn't resp. […]
Sure, I assumed that was the case too, it was to avoid asking for it too quickly, hence it would grab the response the next time around, after the initial delay. In depthcharge, there is no initial delay, but there is here (see line 140). 5 or 10 ms ought to be plenty of time for the EC to switch that status indicator, but fair point, I'll leave it in.
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 113: case EC_VBOOT_HASH_STATUS_NONE:
Like mentioned in the other patch set I think we should keep the recalc_requested unless someone can […]
Yeah thinking about it more, I'll just keep it in.
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 235: if (google_chromeec_get_protocol_info(&resp_proto))
nit: might be worth an error message
Done
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 245: return 0;
This should be an error (right?) and should probably also print something.
Yeah that's an error. Fixed.
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 258: burst -= sizeof(struct ec_host_request);
No, this... […]
Yes, I think I got myself tangled up here. Your'e right about the ec_host_request as well, the lpc/i2c/spi drivers take care of that.
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 261: bufsize = MIN(1024, burst);
...uhhh... […]
Done
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 427: "flag.\n");
I think you misunderstood my comment here, I meant there seems to be no need for a line break in thi […]
Just for the 80-character line limit.
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 428: rv = VB2_ERROR_UNKNOWN;
(I think the immediate return here was fine btw)
Ack