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()). What a mess :-( Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [email protected] What is tolerance? -- it is the consequence of humanity. We are all formed of frailty and error; let us pardon reciprocally each other's folly -- that is the first law of nature. - Voltaire _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
