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

[Bin Meng]

Hi Kyungmin,

On Thu, Aug 9, 2018 at 4:02 PM Bin Meng <bmeng.cn at gmail.com> wrote:
>
> 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.

Any comments?

Regards,
Bin