[U-Boot] Two CRC32 implementation in U-Boot, why?

[Bin Meng]

On Wed, Aug 1, 2018 at 9:50 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
> On 08/01/2018 02:13 PM, Bin Meng wrote:
>> Hi,
>>
>> Currently it seems that we have two CRC32 implementation in U-Boot.
>> Two headers files are provided.
>>
>> 1. include/linux/crc32.h
>> The implementation is drivers/mtd/ubi/crc32.c.
>> Codes that use this implementation include:
>>
>> drivers/mtd/ubi/*
>> drivers/mtd/ubispl/*
>> fs/ubifs/*
>>
>> 2. include/u-boot/crc.h
>> The implementation is lib/crc32.c
>> Codes that use this implementation include:
>>
>> fs/btrfs/hash.c
>> tools/*
>> common/hash.c
>> common/image.c
>> common/image-fit.c
>> lib/efi_loader/efi_boottime.c
>>
>> It looks that include/linux/crc32.h was originally imported from Linux
>> kernel's include/linux/crc32.h, but the implementation in Linux
>> kernel's lib/crc32.c was not imported to U-Boot's lib/crc32.c but to
>> drivers/mtd/ubi/crc32.c. Why?
>>
>> Somehow U-Boot lib/crc32.c uses another different implementation from zlib.
>>
>> This is a mess. For example if I include both headers in one C file,
>> it won't compile.
>>
>> Can we clean this up?
>
> Thanks for pointing this out.
>
> The drivers/mtd/ubi/crc32.c is based on an elder version of Linux.
>
> When looking at the function signatures I am not happy with
> include/u-boot/crc.h
>  uint32_t crc32 (uint32_t, const unsigned char *, uint)
> The last parameter should be size_t. Otherwise the CRC may be wrong on
> 64bit systems.
>
> The two crc32 implementations do not have the same result on a
> low-endian system:
>
> crc32_le(0, 'U-Boot', 6) = a289ac17
> crc32(0, 'U-Boot', 6) = 134b0db4.
>
> According to the comments in in include/linux/crc32.h the result of
> crc32_le is in bit reversed order.
>
> Conflicting definitions could be avoided by removing #define crc32() in
> include/linux/crc32.h and adjusting the ubi code accordingly.
>

I would like to see one CRC32 implementation to support all use cases
in U-Boot. Allowing two different implementation just confuses people.

Regards,
Bin