| Message ID | 1399295277-28334-1-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
Dear Lukasz, In message <1399295277-28334-1-git-send-email-l.majewski@samsung.com> you wrote: > The current approach set the initial value of crc32 calculation to zero, > which is correct for calculating checksum of the whole chunk of data. > > It however, lacks the flexibility, when one wants to calculate CRC32 of > a file comprised of many smaller parts received separately. > > In the proposed approach the output value is used as a starting condition > for the proper crc32 calculation at crc32_wd function. This behavior is > identical to the one provided by crc32() method implementation. > > Additionally, comments were appropriately updated. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Simon Glass <sjg@chromium.org> > --- > include/hash.h | 2 +- > include/u-boot/crc.h | 3 ++- > lib/crc32.c | 2 +- > 3 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/hash.h b/include/hash.h > index dc21678..abf704d 100644 > --- a/include/hash.h > +++ b/include/hash.h > @@ -101,7 +101,7 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, > * @algo_name: Hash algorithm to use > * @data: Data to hash > * @len: Lengh of data to hash in bytes > - * @output: Place to put hash value > + * @output: Place to put hash value - also the initial value (crc32) > * @output_size: On entry, pointer to the number of bytes available in > * output. On exit, pointer to the number of bytes used. > * If NULL, then it is assumed that the caller has > diff --git a/include/u-boot/crc.h b/include/u-boot/crc.h > index 754ac72..7a87911 100644 > --- a/include/u-boot/crc.h > +++ b/include/u-boot/crc.h > @@ -19,7 +19,8 @@ uint32_t crc32_no_comp (uint32_t, const unsigned char *, uint); > * > * @input: Input buffer > * @ilen: Input buffer length > - * @output: Place to put checksum result (4 bytes) > + * @output: Place to provide initial CRC32 value and afterwards > + * put checksum result (4 bytes) > * @chunk_sz: Trigger watchdog after processing this many bytes > */ > void crc32_wd_buf(const unsigned char *input, uint ilen, > diff --git a/lib/crc32.c b/lib/crc32.c > index 9759212..f6266c7 100644 > --- a/lib/crc32.c > +++ b/lib/crc32.c > @@ -257,7 +257,7 @@ void crc32_wd_buf(const unsigned char *input, unsigned int ilen, > { > uint32_t crc; > > - crc = crc32_wd(0, input, ilen, chunk_sz); > + crc = crc32_wd(*(uint32_t *)output, input, ilen, chunk_sz); > crc = htonl(crc); > memcpy(output, &crc, sizeof(crc)); This looks wrong to me, in a number of ways. First, the *(uint32_t *)output cast, is likely to trigger unaligned accesses with the resulting problems on some platforms. Never, never ever cast a character pointer into something that requires any alignment! Seconds, where exactly do you now initialize the CRC vlaue to start with 0 ? Finally we should keep in mind (this is nothing caused by your patch, but when touching this area we really should consider it) that we have a number of (slightly) different CRC implementations thare cry for cleanup / unification: in addition to lib/crc32.c we have disk/part_efi.c (which provides efi_crc32()), drivers/mtd/ubi/crc32.c (which provides crc32_le() and crc32_be()), net/eth.c (which uses ether_crc()), we have BZ2_crc32Table[256] in lib/bzlib_private.h / lib/bzlib_crctable.c (which appears to be unsued), and we have tools/pblimage.c (which provides pbl_crc32()). What a mess :-( Best regards, Wolfgang Denk
On Monday, May 05, 2014 at 03:07:57 PM, Lukasz Majewski wrote: > The current approach set the initial value of crc32 calculation to zero, > which is correct for calculating checksum of the whole chunk of data. > > It however, lacks the flexibility, when one wants to calculate CRC32 of > a file comprised of many smaller parts received separately. > > In the proposed approach the output value is used as a starting condition > for the proper crc32 calculation at crc32_wd function. This behavior is > identical to the one provided by crc32() method implementation. > > Additionally, comments were appropriately updated. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Simon Glass <sjg@chromium.org> > --- > include/hash.h | 2 +- > include/u-boot/crc.h | 3 ++- > lib/crc32.c | 2 +- > 3 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/hash.h b/include/hash.h > index dc21678..abf704d 100644 > --- a/include/hash.h > +++ b/include/hash.h > @@ -101,7 +101,7 @@ int hash_command(const char *algo_name, int flags, > cmd_tbl_t *cmdtp, int flag, * @algo_name: Hash algorithm to use > * @data: Data to hash > * @len: Lengh of data to hash in bytes > - * @output: Place to put hash value > + * @output: Place to put hash value - also the initial value (crc32) > * @output_size: On entry, pointer to the number of bytes available in > * output. On exit, pointer to the number of bytes used. > * If NULL, then it is assumed that the caller has > diff --git a/include/u-boot/crc.h b/include/u-boot/crc.h > index 754ac72..7a87911 100644 > --- a/include/u-boot/crc.h > +++ b/include/u-boot/crc.h > @@ -19,7 +19,8 @@ uint32_t crc32_no_comp (uint32_t, const unsigned char *, > uint); * > * @input: Input buffer > * @ilen: Input buffer length > - * @output: Place to put checksum result (4 bytes) > + * @output: Place to provide initial CRC32 value and afterwards > + * put checksum result (4 bytes) > * @chunk_sz: Trigger watchdog after processing this many bytes > */ > void crc32_wd_buf(const unsigned char *input, uint ilen, > diff --git a/lib/crc32.c b/lib/crc32.c > index 9759212..f6266c7 100644 > --- a/lib/crc32.c > +++ b/lib/crc32.c > @@ -257,7 +257,7 @@ void crc32_wd_buf(const unsigned char *input, unsigned > int ilen, { > uint32_t crc; > > - crc = crc32_wd(0, input, ilen, chunk_sz); > + crc = crc32_wd(*(uint32_t *)output, input, ilen, chunk_sz); I sense alignment fault here. [...] Best regards, Marek Vasut
On Monday, May 05, 2014 at 07:47:30 PM, Wolfgang Denk wrote: > Dear Lukasz, > > In message <1399295277-28334-1-git-send-email-l.majewski@samsung.com> you wrote: > > The current approach set the initial value of crc32 calculation to zero, > > which is correct for calculating checksum of the whole chunk of data. > > > > It however, lacks the flexibility, when one wants to calculate CRC32 of > > a file comprised of many smaller parts received separately. > > > > In the proposed approach the output value is used as a starting condition > > for the proper crc32 calculation at crc32_wd function. This behavior is > > identical to the one provided by crc32() method implementation. > > > > Additionally, comments were appropriately updated. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Cc: Marek Vasut <marex@denx.de> > > Cc: Simon Glass <sjg@chromium.org> > > --- > > > > include/hash.h | 2 +- > > include/u-boot/crc.h | 3 ++- > > lib/crc32.c | 2 +- > > 3 files changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/include/hash.h b/include/hash.h > > index dc21678..abf704d 100644 > > --- a/include/hash.h > > +++ b/include/hash.h > > @@ -101,7 +101,7 @@ int hash_command(const char *algo_name, int flags, > > cmd_tbl_t *cmdtp, int flag, > > > > * @algo_name: Hash algorithm to use > > * @data: Data to hash > > * @len: Lengh of data to hash in bytes > > > > - * @output: Place to put hash value > > + * @output: Place to put hash value - also the initial value (crc32) > > > > * @output_size: On entry, pointer to the number of bytes available in > > * output. On exit, pointer to the number of bytes used. > > * If NULL, then it is assumed that the caller has > > > > diff --git a/include/u-boot/crc.h b/include/u-boot/crc.h > > index 754ac72..7a87911 100644 > > --- a/include/u-boot/crc.h > > +++ b/include/u-boot/crc.h > > @@ -19,7 +19,8 @@ uint32_t crc32_no_comp (uint32_t, const unsigned char > > *, uint); > > > > * > > * @input: Input buffer > > * @ilen: Input buffer length > > > > - * @output: Place to put checksum result (4 bytes) > > + * @output: Place to provide initial CRC32 value and afterwards > > + * put checksum result (4 bytes) > > > > * @chunk_sz: Trigger watchdog after processing this many bytes > > */ > > > > void crc32_wd_buf(const unsigned char *input, uint ilen, > > > > diff --git a/lib/crc32.c b/lib/crc32.c > > index 9759212..f6266c7 100644 > > --- a/lib/crc32.c > > +++ b/lib/crc32.c > > @@ -257,7 +257,7 @@ void crc32_wd_buf(const unsigned char *input, > > unsigned int ilen, > > > > { > > > > uint32_t crc; > > > > - crc = crc32_wd(0, input, ilen, chunk_sz); > > + crc = crc32_wd(*(uint32_t *)output, input, ilen, chunk_sz); > > > > crc = htonl(crc); > > memcpy(output, &crc, sizeof(crc)); > > This looks wrong to me, in a number of ways. > > First, the *(uint32_t *)output cast, is likely to trigger unaligned > accesses with the resulting problems on some platforms. Never, never > ever cast a character pointer into something that requires any > alignment! > > Seconds, where exactly do you now initialize the CRC vlaue to start > with 0 ? > > > Finally we should keep in mind (this is nothing caused by your patch, > but when touching this area we really should consider it) that we have > a number of (slightly) different CRC implementations thare cry for > cleanup / unification: in addition to lib/crc32.c we have > disk/part_efi.c (which provides efi_crc32()), drivers/mtd/ubi/crc32.c > (which provides crc32_le() and crc32_be()), net/eth.c (which uses > ether_crc()), we have BZ2_crc32Table[256] in lib/bzlib_private.h / > lib/bzlib_crctable.c (which appears to be unsued), and we have > tools/pblimage.c (which provides pbl_crc32()). Just an addition ... All those different implementations should use the hash_*() calls and this one central implementation. Thanks for pointing this part out.
Hi Wolfgang, > Dear Lukasz, > > In message <1399295277-28334-1-git-send-email-l.majewski@samsung.com> > you wrote: > > The current approach set the initial value of crc32 calculation to > > zero, which is correct for calculating checksum of the whole chunk > > of data. > > > > It however, lacks the flexibility, when one wants to calculate > > CRC32 of a file comprised of many smaller parts received separately. > > > > In the proposed approach the output value is used as a starting > > condition for the proper crc32 calculation at crc32_wd function. > > This behavior is identical to the one provided by crc32() method > > implementation. > > > > Additionally, comments were appropriately updated. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Cc: Marek Vasut <marex@denx.de> > > Cc: Simon Glass <sjg@chromium.org> > > --- > > include/hash.h | 2 +- > > include/u-boot/crc.h | 3 ++- > > lib/crc32.c | 2 +- > > 3 files changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/include/hash.h b/include/hash.h > > index dc21678..abf704d 100644 > > --- a/include/hash.h > > +++ b/include/hash.h > > @@ -101,7 +101,7 @@ int hash_command(const char *algo_name, int > > flags, cmd_tbl_t *cmdtp, int flag, > > * @algo_name: Hash algorithm to use > > * @data: Data to hash > > * @len: Lengh of data to hash in bytes > > - * @output: Place to put hash value > > + * @output: Place to put hash value - also the > > initial value (crc32) > > * @output_size: On entry, pointer to the number of bytes > > available in > > * output. On exit, pointer to the number > > of bytes used. > > * If NULL, then it is assumed that the > > caller has diff --git a/include/u-boot/crc.h b/include/u-boot/crc.h > > index 754ac72..7a87911 100644 > > --- a/include/u-boot/crc.h > > +++ b/include/u-boot/crc.h > > @@ -19,7 +19,8 @@ uint32_t crc32_no_comp (uint32_t, const unsigned > > char *, uint); * > > * @input: Input buffer > > * @ilen: Input buffer length > > - * @output: Place to put checksum result (4 bytes) > > + * @output: Place to provide initial CRC32 value and > > afterwards > > + * put checksum result (4 bytes) > > * @chunk_sz: Trigger watchdog after processing this many > > bytes */ > > void crc32_wd_buf(const unsigned char *input, uint ilen, > > diff --git a/lib/crc32.c b/lib/crc32.c > > index 9759212..f6266c7 100644 > > --- a/lib/crc32.c > > +++ b/lib/crc32.c > > @@ -257,7 +257,7 @@ void crc32_wd_buf(const unsigned char *input, > > unsigned int ilen, { > > uint32_t crc; > > > > - crc = crc32_wd(0, input, ilen, chunk_sz); > > + crc = crc32_wd(*(uint32_t *)output, input, ilen, chunk_sz); > > crc = htonl(crc); > > memcpy(output, &crc, sizeof(crc)); > > This looks wrong to me, in a number of ways. > > First, the *(uint32_t *)output cast, is likely to trigger unaligned > accesses with the resulting problems on some platforms. Never, never > ever cast a character pointer into something that requires any > alignment! I admit that I was thinking about my (particular) platform. Instead, I should memcpy the output to crc variable, which is defined as uint32_t, and pass it to the crc32_wd. > > Seconds, where exactly do you now initialize the CRC vlaue to start > with 0 ? The proposed approach (with setting initial value of CRC32) is working fine with crc32() function at least in the DFU. Zeroing out of relevant variable is performed there. The venerable crc32() implementation allows passing initial value of crc. As it is now, the hash_block() doesn't. > > > Finally we should keep in mind (this is nothing caused by your patch, > but when touching this area we really should consider it) that we have > a number of (slightly) different CRC implementations thare cry for > cleanup / unification: in addition to lib/crc32.c we have > disk/part_efi.c (which provides efi_crc32()), drivers/mtd/ubi/crc32.c > (which provides crc32_le() and crc32_be()), net/eth.c (which uses > ether_crc()), we have BZ2_crc32Table[256] in lib/bzlib_private.h / > lib/bzlib_crctable.c (which appears to be unsued), and we have > tools/pblimage.c (which provides pbl_crc32()). As Marek pointed out, we shall start using hash_block (from #include <hash.h>). However, as I pointed out in this patch hash_block (at least for crc32) needs to be modified to preserve the functionality of bare metal crc32() function. To unify, we also may be forced to introduce some flags - like output crc endianess (big, little). But this may wait for the moment. > > What a mess :-( +1 > > Best regards, > > Wolfgang Denk >
diff --git a/include/hash.h b/include/hash.h index dc21678..abf704d 100644 --- a/include/hash.h +++ b/include/hash.h @@ -101,7 +101,7 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag, * @algo_name: Hash algorithm to use * @data: Data to hash * @len: Lengh of data to hash in bytes - * @output: Place to put hash value + * @output: Place to put hash value - also the initial value (crc32) * @output_size: On entry, pointer to the number of bytes available in * output. On exit, pointer to the number of bytes used. * If NULL, then it is assumed that the caller has diff --git a/include/u-boot/crc.h b/include/u-boot/crc.h index 754ac72..7a87911 100644 --- a/include/u-boot/crc.h +++ b/include/u-boot/crc.h @@ -19,7 +19,8 @@ uint32_t crc32_no_comp (uint32_t, const unsigned char *, uint); * * @input: Input buffer * @ilen: Input buffer length - * @output: Place to put checksum result (4 bytes) + * @output: Place to provide initial CRC32 value and afterwards + * put checksum result (4 bytes) * @chunk_sz: Trigger watchdog after processing this many bytes */ void crc32_wd_buf(const unsigned char *input, uint ilen, diff --git a/lib/crc32.c b/lib/crc32.c index 9759212..f6266c7 100644 --- a/lib/crc32.c +++ b/lib/crc32.c @@ -257,7 +257,7 @@ void crc32_wd_buf(const unsigned char *input, unsigned int ilen, { uint32_t crc; - crc = crc32_wd(0, input, ilen, chunk_sz); + crc = crc32_wd(*(uint32_t *)output, input, ilen, chunk_sz); crc = htonl(crc); memcpy(output, &crc, sizeof(crc)); }
The current approach set the initial value of crc32 calculation to zero, which is correct for calculating checksum of the whole chunk of data. It however, lacks the flexibility, when one wants to calculate CRC32 of a file comprised of many smaller parts received separately. In the proposed approach the output value is used as a starting condition for the proper crc32 calculation at crc32_wd function. This behavior is identical to the one provided by crc32() method implementation. Additionally, comments were appropriately updated. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> Cc: Marek Vasut <marex@denx.de> Cc: Simon Glass <sjg@chromium.org> --- include/hash.h | 2 +- include/u-boot/crc.h | 3 ++- lib/crc32.c | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-)