Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37753
to review the following change.
Change subject: lib/crc_byte: Add CRC32 implementation ......................................................................
lib/crc_byte: Add CRC32 implementation
* Add CRC32 using polynomial 0x04C11DB7 + Add macro to caculate CRC of a buffer
Change-Id: If98e4e12bb53a6e5123e94e8cdffde1eb3bc4b4b Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/crc_byte.h M src/lib/crc_byte.c 2 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/37753/1
diff --git a/src/include/crc_byte.h b/src/include/crc_byte.h index 9315277..3740b4a 100644 --- a/src/include/crc_byte.h +++ b/src/include/crc_byte.h @@ -36,5 +36,24 @@ */ uint16_t crc16_byte(uint16_t prev_crc, uint8_t data);
+/* This function is used to calculate crc32 byte by byte, with polynomial + * x^32 + x^26 + x^23 + x^22 + x^16 + x^12 + x^11 + x^10 + x^8 + x^7 + + * x^5 + x^4 + x^2 + x + 1 + * + * prev_crc: old crc result (0 for first) + * data: new byte + * return value: new crc result + */ +uint32_t crc32_byte(uint32_t prev_crc, uint8_t data); + +#define CRC(buf, size, crc_func) ({ \ + const uint8_t *_crc_local_buf = (const uint8_t *)buf; \ + size_t _crc_local_size = size; \ + __typeof__(crc_func(0, 0)) _crc_local_result = ~0; \ + while (_crc_local_size--) { \ + _crc_local_result = crc_func(_crc_local_result, *_crc_local_buf++); \ + } \ + ~_crc_local_result; \ +})
#endif /* CRC_BYTE_H */ diff --git a/src/lib/crc_byte.c b/src/lib/crc_byte.c index 0ac0063..c04449d 100644 --- a/src/lib/crc_byte.c +++ b/src/lib/crc_byte.c @@ -36,3 +36,17 @@ prev_crc ^= ((prev_crc & 0xff) << 4) << 1; return prev_crc; } + +uint32_t crc32_byte(uint32_t prev_crc, uint8_t data) +{ + prev_crc ^= (uint32_t)data << 24; + + for (int i = 0; i < 8; i++) { + if ((prev_crc & 0x80000000UL) != 0) + prev_crc = ((prev_crc << 1) ^ 0x04C11DB7UL); + else + prev_crc <<= 1; + } + + return prev_crc; +}
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37753 )
Change subject: lib/crc_byte: Add CRC32 implementation ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
I don't really understand the double negation thing you're doing there, but assuming you have a good reason for it, LGTM.
https://review.coreboot.org/c/coreboot/+/37753/1/src/include/crc_byte.h File src/include/crc_byte.h:
https://review.coreboot.org/c/coreboot/+/37753/1/src/include/crc_byte.h@52 PS1, Line 52: ~0; Why the negation here and for the result? Does it not work when you just start with 0? If so, do we need to update the documentation above that suggests you should start with 0?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37753 )
Change subject: lib/crc_byte: Add CRC32 implementation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37753/1/src/include/crc_byte.h File src/include/crc_byte.h:
https://review.coreboot.org/c/coreboot/+/37753/1/src/include/crc_byte.h@52 PS1, Line 52: ~0;
Why the negation here and for the result? Does it not work when you just start with 0? If so, do we […]
That's described in ITU I.363.5 /ITU V.42. Starting with 0 is also possible, but would give different results.
Patrick Rudolph has uploaded a new patch set (#2) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/37753 )
Change subject: lib/crc_byte: Add CRC32 implementation ......................................................................
lib/crc_byte: Add CRC32 implementation
* Add CRC32 using polynomial 0x04C11DB7 + Add macro to caculate CRC of a buffer
Change-Id: If98e4e12bb53a6e5123e94e8cdffde1eb3bc4b4b Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/include/crc_byte.h M src/lib/crc_byte.c 2 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/37753/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37753 )
Change subject: lib/crc_byte: Add CRC32 implementation ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37753/1/src/include/crc_byte.h File src/include/crc_byte.h:
https://review.coreboot.org/c/coreboot/+/37753/1/src/include/crc_byte.h@52 PS1, Line 52: ~0;
That's described in ITU I.363.5 /ITU V.42. […]
Using 0 for now.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37753 )
Change subject: lib/crc_byte: Add CRC32 implementation ......................................................................
Patch Set 6: Code-Review+2
Patrick Rudolph has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37753 )
Change subject: lib/crc_byte: Add CRC32 implementation ......................................................................
lib/crc_byte: Add CRC32 implementation
* Add CRC32 using polynomial 0x04C11DB7 + Add macro to caculate CRC of a buffer
Change-Id: If98e4e12bb53a6e5123e94e8cdffde1eb3bc4b4b Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37753 Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/crc_byte.h M src/lib/crc_byte.c 2 files changed, 33 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved
diff --git a/src/include/crc_byte.h b/src/include/crc_byte.h index 9315277..c0df5b0 100644 --- a/src/include/crc_byte.h +++ b/src/include/crc_byte.h @@ -36,5 +36,24 @@ */ uint16_t crc16_byte(uint16_t prev_crc, uint8_t data);
+/* This function is used to calculate crc32 byte by byte, with polynomial + * x^32 + x^26 + x^23 + x^22 + x^16 + x^12 + x^11 + x^10 + x^8 + x^7 + + * x^5 + x^4 + x^2 + x + 1 + * + * prev_crc: old crc result (0 for first) + * data: new byte + * return value: new crc result + */ +uint32_t crc32_byte(uint32_t prev_crc, uint8_t data); + +#define CRC(buf, size, crc_func) ({ \ + const uint8_t *_crc_local_buf = (const uint8_t *)buf; \ + size_t _crc_local_size = size; \ + __typeof__(crc_func(0, 0)) _crc_local_result = 0; \ + while (_crc_local_size--) { \ + _crc_local_result = crc_func(_crc_local_result, *_crc_local_buf++); \ + } \ + _crc_local_result; \ +})
#endif /* CRC_BYTE_H */ diff --git a/src/lib/crc_byte.c b/src/lib/crc_byte.c index 0ac0063..c04449d 100644 --- a/src/lib/crc_byte.c +++ b/src/lib/crc_byte.c @@ -36,3 +36,17 @@ prev_crc ^= ((prev_crc & 0xff) << 4) << 1; return prev_crc; } + +uint32_t crc32_byte(uint32_t prev_crc, uint8_t data) +{ + prev_crc ^= (uint32_t)data << 24; + + for (int i = 0; i < 8; i++) { + if ((prev_crc & 0x80000000UL) != 0) + prev_crc = ((prev_crc << 1) ^ 0x04C11DB7UL); + else + prev_crc <<= 1; + } + + return prev_crc; +}
-
Julius Werner (Code Review) -
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)