On Monday, May 05, 2014 at 07:47:30 PM, Wolfgang Denk wrote: > Dear Lukasz, > > In message <[email protected]> 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 <[email protected]> > > Cc: Marek Vasut <[email protected]> > > Cc: Simon Glass <[email protected]> > > --- > > > > 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. _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
