lib:crc32: Allow setting of the initial crc32 value

PATCH New Lukasz Majewski 4 participants 13 messages May 5, 2014 – May 8, 2014 Library code

Share & tools

Lukasz Majewski

author

2014-05-05 13:07 UTC #0 lore raw

PATCH 1/1 lib:crc32: Allow setting of the initial crc32 value

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

include/hash.h +1 -1

index dc21678..abf704d 100644
@@ -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

include/u-boot/crc.h +2 -1

index 754ac72..7a87911 100644
@@ -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,

lib/crc32.c +1 -1

index 9759212..f6266c7 100644
@@ -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));
 }

Signature · 1 line

1.7.10.4

Wolfgang Denk

2014-05-05 17:47 UTC #1 lore raw

PATCH 1/1 lib:crc32: Allow setting of the initial crc32 value

Dear Lukasz,

Quoted 53 lines · 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(-)
include/hash.h +1 -1
index dc21678..abf704d 100644
@@ -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
include/u-boot/crc.h +2 -1
index 754ac72..7a87911 100644
@@ -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,
lib/crc32.c +0 -0
index 9759212..f6266c7 100644

lib/crc32.c void crc32_wd_buf(const unsigned char *input, unsigned int ilen, · line 262

Quoted 7 lines

lib/crc32.c +1 -1

@@ -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

Signature · 6 lines

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

Marek Vasut

2014-05-05 18:56 UTC #2 lore raw

PATCH 1/1 lib:crc32: Allow setting of the initial crc32 value

Quoted 3 lines · Wolfgang Denk

Dear Lukasz,
In message <[email protected]> you

Quoted 29 lines · wrote:

Quoted 29 lines
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(-)
include/hash.h +0 -0
index dc21678..abf704d 100644
@@ -101,7 +101,7 @@ int hash_command(const char *algo_name, int flags,
 cmd_tbl_t *cmdtp, int flag,
 

Quoted 6 lines

include/hash.h int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag,
Quoted 6 lines
include/hash.h +1 -1
  * @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)

Quoted 53 lines

Quoted 39 lines
* @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
include/u-boot/crc.h +2 -1
index 754ac72..7a87911 100644
@@ -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,
lib/crc32.c +1 -1
index 9759212..f6266c7 100644
@@ -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);
lib/crc32.c void crc32_wd_buf(const unsigned char *input, unsigned int ilen, · line 262
Quoted 2 lines
lib/crc32.c +0 -0
 	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 ?

Quoted 9 lines

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.

Lukasz Majewski

author

2014-05-06 05:54 UTC #3 lore raw

PATCH 1/1 lib:crc32: Allow setting of the initial crc32 value

In reply to Wolfgang Denk

Hi Wolfgang,

Quoted 74 lines

Dear Lukasz,
In message <[email protected]>
Quoted 60 lines · you
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(-)
include/hash.h +1 -1
index dc21678..abf704d 100644
@@ -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
include/u-boot/crc.h +0 -0
@@ -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,
lib/crc32.c +0 -0
index 9759212..f6266c7 100644
lib/crc32.c void crc32_wd_buf(const unsigned char *input, · line 262
Quoted 7 lines
lib/crc32.c +1 -1
@@ -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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 11 lines

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.

Quoted 2 lines

What a mess :-(

Quoted 5 lines

Best regards,
Wolfgang Denk

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Marek Vasut

2014-05-05 17:51 UTC #4 lore raw

PATCH 1/1 lib:crc32: Allow setting of the initial crc32 value

In reply to Lukasz Majewski

Quoted 53 lines · Lukasz Majewski

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(-)
include/hash.h +0 -0
index dc21678..abf704d 100644
@@ -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
include/u-boot/crc.h +0 -0
index 754ac72..7a87911 100644
@@ -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,
lib/crc32.c +0 -0
index 9759212..f6266c7 100644

lib/crc32.c void crc32_wd_buf(const unsigned char *input, unsigned · line 260

Quoted 5 lines

lib/crc32.c +1 -1

@@ -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

Lukasz Majewski

author

2014-05-07 06:10 UTC #5 lore raw

PATCH v2 1/1 lib:crc32: Allow setting of the initial crc32 value

In reply to Lukasz Majewski

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.

Additionally comments were appropriately updated.

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

---
Changes for v2:
- Replace casting from (u8*) to (u32*) with memcpy

---
 include/hash.h       |    2 +-
 include/u-boot/crc.h |    3 ++-
 lib/crc32.c          |    7 +++++--
 3 files changed, 8 insertions(+), 4 deletions(-)

include/hash.h +1 -1

index dc21678..abf704d 100644
@@ -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

include/u-boot/crc.h +2 -1

index 754ac72..7a87911 100644
@@ -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,

lib/crc32.c +5 -2

index 9759212..80e078f 100644
@@ -255,9 +255,12 @@ uint32_t ZEXPORT crc32_wd (uint32_t crc,
 void crc32_wd_buf(const unsigned char *input, unsigned int ilen,
 		unsigned char *output, unsigned int chunk_sz)
 {
-	uint32_t crc;
+	uint32_t crc = 0;
 
-	crc = crc32_wd(0, input, ilen, chunk_sz);
+	if (*output)
+		memcpy(&crc, output, sizeof(crc));
+
+	crc = crc32_wd(crc, input, ilen, chunk_sz);
 	crc = htonl(crc);
 	memcpy(output, &crc, sizeof(crc));
 }

Signature · 1 line

1.7.10.4

Marek Vasut

2014-05-07 08:25 UTC #6 lore raw

PATCH v2 1/1 lib:crc32: Allow setting of the initial crc32 value

On Wednesday, May 07, 2014 at 08:10:20 AM, Lukasz Majewski wrote:

[...]

lib/crc32.c uint32_t ZEXPORT crc32_wd (uint32_t crc, · line 261

Quoted 12 lines

lib/crc32.c +3 -2

@@ -255,9 +255,12 @@ uint32_t ZEXPORT crc32_wd (uint32_t crc,
 void crc32_wd_buf(const unsigned char *input, unsigned int ilen,
 		unsigned char *output, unsigned int chunk_sz)
 {
-	uint32_t crc;
+	uint32_t crc = 0;
 
-	crc = crc32_wd(0, input, ilen, chunk_sz);
+	if (*output)
+		memcpy(&crc, output, sizeof(crc));

Won't some sort of put_unaligned() work here ? The $crc is uint32_t afterall, so it might be a jiff faster. Please correct me if I'm wrong.

lib/crc32.c uint32_t ZEXPORT crc32_wd (uint32_t crc, · line 266

Quoted 5 lines

lib/crc32.c +2 -0

+
+	crc = crc32_wd(crc, input, ilen, chunk_sz);
 	crc = htonl(crc);
 	memcpy(output, &crc, sizeof(crc));
 }

Best regards,
Marek Vasut

Lukasz Majewski

author

2014-05-07 10:17 UTC #7 lore raw

PATCH v2 1/1 lib:crc32: Allow setting of the initial crc32 value

Hi Marek,

Quoted 13 lines · Lukasz Majewski

[...]

Quoted 9 lines

lib/crc32.c +1 -1

@@ -255,9 +255,12 @@ uint32_t ZEXPORT crc32_wd (uint32_t crc,
 void crc32_wd_buf(const unsigned char *input, unsigned int ilen,
 		unsigned char *output, unsigned int chunk_sz)
 {
-	uint32_t crc;
+	uint32_t crc = 0;

Quoted 6 lines

lib/crc32.c uint32_t ZEXPORT crc32_wd (uint32_t crc, · line 261
Quoted 3 lines
lib/crc32.c +2 -1
-	crc = crc32_wd(0, input, ilen, chunk_sz);
+	if (*output)
+		memcpy(&crc, output, sizeof(crc));
Won't some sort of put_unaligned() work here ? The $crc is uint32_t afterall, so it might be a jiff faster.

We are concerned here with the use case of copying 4 bytes from unaligned buffer defined on some architectures.

I suppose, that the performance would be the same for both. However, since memcpy() is already used in this function, I would prefer to use it here.

Quoted 11 lines

Please correct me if I'm
wrong.
lib/crc32.c uint32_t ZEXPORT crc32_wd (uint32_t crc, · line 266
Quoted 5 lines
lib/crc32.c +2 -0
+
+	crc = crc32_wd(crc, input, ilen, chunk_sz);
 	crc = htonl(crc);
 	memcpy(output, &crc, sizeof(crc));
 }
Best regards,
Marek Vasut

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Wolfgang Denk

2014-05-07 10:42 UTC #8 lore raw

PATCH v2 1/1 lib:crc32: Allow setting of the initial crc32 value

In reply to Lukasz Majewski

Dear Lukasz Majewski,

Quoted 2 lines · 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.

...

lib/crc32.c uint32_t ZEXPORT crc32_wd (uint32_t crc, · line 265

Quoted 6 lines

lib/crc32.c +4 -0

+	if (*output)
+		memcpy(&crc, output, sizeof(crc));
+
+	crc = crc32_wd(crc, input, ilen, chunk_sz);
 	crc = htonl(crc);
 	memcpy(output, &crc, sizeof(crc));

You can actually remove the "if (*output)" because output has always to be a non-null pointer, as we're going to store the result there.

Which means that you cannot use this to implicitly initialize crc =0, whichin turn means you MUST add porper initialization to all callers of that function.

Best regards,

Wolfgang Denk

Signature · 6 lines

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
Die Freiheit des Menschen liegt nicht darin, dass er tun kann, was er
will, sondern darin, dass er nicht tun muss, was er nicht will.
                                             -- Jean-Jacques Rousseau

Lukasz Majewski

author

2014-05-07 12:25 UTC #9 lore raw

PATCH v2 1/1 lib:crc32: Allow setting of the initial crc32 value

Hi Wolfgang,

Quoted 8 lines

Dear Lukasz Majewski,
In message <[email protected]>
Quoted 3 lines · you
The current approach set the initial value of crc32 calculation to zero, which is correct for calculating checksum of the whole chunk of data.
...

Quoted 9 lines

lib/crc32.c uint32_t ZEXPORT crc32_wd (uint32_t crc, · line 265
Quoted 6 lines
lib/crc32.c +4 -0
+	if (*output)
+		memcpy(&crc, output, sizeof(crc));
+
+	crc = crc32_wd(crc, input, ilen, chunk_sz);
 	crc = htonl(crc);
 	memcpy(output, &crc, sizeof(crc));
You can actually remove the "if (*output)" because output has always to be a non-null pointer, as we're going to store the result there.

I think, that the above statement would be correct if I had checked the if (output).

The problem here is that *output refers to uint8 and only first/last byte is checked. This is obviously wrong.

You are right that this check is not needed.

Quoted 4 lines

Which means that you cannot use this to implicitly initialize crc =0, whichin turn means you MUST add porper initialization to all callers of that function.

Ok.

Quoted 5 lines

Best regards,
Wolfgang Denk

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Lukasz Majewski

author

2014-05-07 12:57 UTC #10 lore raw

PATCH v3 1/1 lib:crc32:hash: Allow setting of the initial crc32 value

In reply to Lukasz Majewski

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.

Additionally comments were appropriately updated.

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

---
Changes for v2:
- Replace casting from (u8*) to (u32*) with memcpy Changes for v3:
- Remove check on the output pointer
- As precaution, zero out the output buffer

---
 common/hash.c        |    6 ++++++
 include/hash.h       |    2 +-
 include/u-boot/crc.h |    3 ++-
 lib/crc32.c          |    3 ++-
 4 files changed, 11 insertions(+), 3 deletions(-)

common/hash.c +6 -0

index 7627b84..84c0a78 100644
@@ -360,6 +360,12 @@ int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag,
 		u8 vsum[HASH_MAX_DIGEST_SIZE];
 		void *buf;
 
+		/*
+		 * This is a precaution for crc32, which allows output to be
+		 * overloaded to provide initial value of crc32.
+		 */
+		memset(output, 0, sizeof(output));
+
 		if (hash_lookup_algo(algo_name, &algo)) {
 			printf("Unknown hash algorithm '%s'\n", algo_name);
 			return CMD_RET_USAGE;

include/hash.h +1 -1

index dc21678..abf704d 100644
@@ -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

include/u-boot/crc.h +2 -1

index 754ac72..7a87911 100644
@@ -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,

lib/crc32.c +2 -1

index 9759212..f57eb87 100644
@@ -257,7 +257,8 @@ void crc32_wd_buf(const unsigned char *input, unsigned int ilen,
 {
 	uint32_t crc;
 
-	crc = crc32_wd(0, input, ilen, chunk_sz);
+	memcpy(&crc, output, sizeof(crc));
+	crc = crc32_wd(crc, input, ilen, chunk_sz);
 	crc = htonl(crc);
 	memcpy(output, &crc, sizeof(crc));
 }

Signature · 1 line

1.7.10.4

Simon Glass

2014-05-07 23:04 UTC #11 lore raw

PATCH v3 1/1 lib:crc32:hash: Allow setting of the initial crc32 value

Hi Lukasz,

Quoted 4 lines · Lukasz Majewski

The current approach set the initial value of crc32 calculation to zero, which is correct for calculating checksum of the whole chunk of data.

Quoted 8 lines

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.

Maybe I am missing something, but this doesn't seem necessary. In hash.h we have

hash_init()
hash_update()
hash_finish()

which permits you to pass more data through a hash function. Doesn't this already do what you want?

What is missing is probably command-line access to this API. Something like:

hash init <envvar>, <algo>
hash update <envvar>, <data>, <size>
hash finish <envvar>, [*]<result>

or similar.

Regards,
Simon

Lukasz Majewski

author

2014-05-08 08:59 UTC #12 lore raw

PATCH v3 1/1 lib:crc32:hash: Allow setting of the initial crc32 value

Hi Simon,

Quoted 21 lines

Hi Lukasz,
Quoted 8 lines · Lukasz Majewski
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.
Quoted 6 lines
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.
Maybe I am missing something, but this doesn't seem necessary. In hash.h we have

Quoted 6 lines

hash_init()
hash_update()
hash_finish()
which permits you to pass more data through a hash function. Doesn't this already do what you want?

I thought, that I would get away with replacing crc32() function call with similar one - hash_block(). As it was pointed out it doesn't itself provide the same functionality.

However, I will try to implement the solution you suggested. Thanks for tip.

Quoted 12 lines

What is missing is probably command-line access to this API. Something like:
hash init <envvar>, <algo>
hash update <envvar>, <data>, <size>
hash finish <envvar>, [*]<result>
or similar.
Regards,
Simon

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group