| Message ID | 1399443021-11748-1-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Wednesday, May 07, 2014 at 08:10:20 AM, Lukasz Majewski wrote: [...] > --- a/lib/crc32.c > +++ b/lib/crc32.c > @@ -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. > + > + crc = crc32_wd(crc, input, ilen, chunk_sz); > crc = htonl(crc); > memcpy(output, &crc, sizeof(crc)); > } Best regards, Marek Vasut
Hi Marek, > On Wednesday, May 07, 2014 at 08:10:20 AM, Lukasz Majewski wrote: > > [...] > > > --- a/lib/crc32.c > > +++ b/lib/crc32.c > > @@ -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. 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. > Please correct me if I'm > wrong. > > > + > > + crc = crc32_wd(crc, input, ilen, chunk_sz); > > crc = htonl(crc); > > memcpy(output, &crc, sizeof(crc)); > > } > > Best regards, > Marek Vasut
Dear Lukasz Majewski, In message <1399443021-11748-1-git-send-email-l.majewski@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. ... > + 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
Hi Wolfgang, > Dear Lukasz Majewski, > > In message <1399443021-11748-1-git-send-email-l.majewski@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. > ... > > + 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. > > 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. > > Best regards, > > Wolfgang Denk >
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..80e078f 100644 --- a/lib/crc32.c +++ b/lib/crc32.c @@ -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)); }
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@samsung.com> Cc: Marek Vasut <marex@denx.de> --- 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(-)