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

[Wolfgang Denk]

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()).

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: wd at denx.de
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