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

[Bin Meng]

Hi Kyungmin,

On Mon, Aug 6, 2018 at 1:56 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
> 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.

Since UBI seems to be the only user of the Linux CRC32 implementation
but the header files are a mess, I would expect some comments or plan
from you.

Regards,
Bin