* [U-Boot] Two CRC32 implementation in U-Boot, why?
@ 2018-08-01 12:13 Bin Meng
2018-08-01 13:50 ` Heinrich Schuchardt
0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2018-08-01 12:13 UTC (permalink / raw)
To: u-boot
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?
Regards,
Bin
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] Two CRC32 implementation in U-Boot, why?
2018-08-01 12:13 [U-Boot] Two CRC32 implementation in U-Boot, why? Bin Meng
@ 2018-08-01 13:50 ` Heinrich Schuchardt
2018-08-06 5:56 ` Bin Meng
0 siblings, 1 reply; 5+ messages in thread
From: Heinrich Schuchardt @ 2018-08-01 13:50 UTC (permalink / raw)
To: u-boot
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.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] Two CRC32 implementation in U-Boot, why?
2018-08-01 13:50 ` Heinrich Schuchardt
@ 2018-08-06 5:56 ` Bin Meng
2018-08-09 8:02 ` Bin Meng
0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2018-08-06 5:56 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] Two CRC32 implementation in U-Boot, why?
2018-08-06 5:56 ` Bin Meng
@ 2018-08-09 8:02 ` Bin Meng
2018-09-06 7:23 ` Bin Meng
0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2018-08-09 8:02 UTC (permalink / raw)
To: u-boot
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.
Regards,
Bin
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] Two CRC32 implementation in U-Boot, why?
2018-08-09 8:02 ` Bin Meng
@ 2018-09-06 7:23 ` Bin Meng
0 siblings, 0 replies; 5+ messages in thread
From: Bin Meng @ 2018-09-06 7:23 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-09-06 7:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-01 12:13 [U-Boot] Two CRC32 implementation in U-Boot, why? Bin Meng
2018-08-01 13:50 ` Heinrich Schuchardt
2018-08-06 5:56 ` Bin Meng
2018-08-09 8:02 ` Bin Meng
2018-09-06 7:23 ` Bin Meng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.