[U-Boot] [PATCH] lib:crc32: Allow setting of the initial crc32 value

[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 at 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 at samsung.com>
> > Cc: Marek Vasut <marex at denx.de>
> > Cc: Simon Glass <sjg at 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.