On Wed, Aug 1, 2018 at 9:50 PM, Heinrich Schuchardt xypron.glpk@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

Hi Kyungmin,

On Thu, Aug 9, 2018 at 4:02 PM Bin Meng bmeng.cn@gmail.com wrote:

Hi Kyungmin,

On Mon, Aug 6, 2018 at 1:56 PM, Bin Meng bmeng.cn@gmail.com wrote:

...

On Wed, Aug 1, 2018 at 9:50 PM, Heinrich Schuchardt xypron.glpk@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