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 11:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 269: burst = pdata_max_size - sizeof(*params);
This could go negative for weird edge cases, so lets make burst an int or ssize_t to be safe.
Hmm, I suppose it could... ssize_t it is.
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 273: if (burst <= sizeof(struct ec_params_flash_write)) {
This check doesn't make much sense now because 'burst' doesn't include actually the space for the pa […]
Ack
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 280: read_buffer = alloca(bufsize);
nit: not sure why you need that extra variable rather than doing […]
That's fair, it isn't really necessary.
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 300: - noop for a memory region
(Not sure what this means, I think you're thinking of memory-mapped SPI which is explicitly not real […]
Ok, it wasn't familiar to me when I started this :)
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 307: printk(BIOS_ERR, "EC failed flash write command!\n");
nit: maybe print offset?
Done