Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36624 )
Change subject: lib: add calculate crc byte by byte ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36624/1/src/include/crc_byte.h File src/include/crc_byte.h:
https://review.coreboot.org/c/coreboot/+/36624/1/src/include/crc_byte.h@33 PS1, Line 33: uint16_t crc16_byte(uint16_t prev_crc, uint8_t data); Please document the exact polynomials implemented by these functions, there exist multiple common versions of these CRCs (e.g. see https://en.wikipedia.org/wiki/Cyclic_redundancy_check).
https://review.coreboot.org/c/coreboot/+/36624/1/src/lib/crc_byte.c File src/lib/crc_byte.c:
https://review.coreboot.org/c/coreboot/+/36624/1/src/lib/crc_byte.c@18 PS1, Line 18: static const uint8_t crc7_table[256] = { Is there a reason these need to be table-based implementations? Can you just implement the CRC algorithm bit by bit (e.g. like in src/security/vboot/vbnv.c:crc8_vbnv())? In a firmware setting, the time you waste loading all these extra bytes from flash is almost certainly worse than the tiny speedup you may gain from it.
https://review.coreboot.org/c/coreboot/+/36624/1/src/lib/crc_byte.c@53 PS1, Line 53: uint8_t crc7_byte(uint8_t crc, uint8_t data) Is this really a CRC-7? Looks to me like it's consuming 8 bits at once, wouldn't that make it a CRC-8?