[U-Boot] [PATCH] crc32: Correct endianness of crc32 result

[Simon Glass]

Hi Wolfgang,

On Tue, Apr 16, 2013 at 10:40 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ2jRVpQ56_EpVKUMH8E1L2LJ0HgzNCs-GrN9bXfOSXz+Q at mail.gmail.com> you wrote:
>>
>> >> +#ifdef USE_HOSTCC
>> >> +     crc = htobe32(crc);
>> >>       memcpy(output, &crc, sizeof(crc));
>> >> +#else
>> >> +     put_unaligned_be32(crc, output);
>> >> +#endif
>> >
>> > Why is this depending on USE_HOSTCC, and not on the endianess?
>>
>> We always want big-endian in this case, since the bytes have to date
>> been written that way by the crc32 command.
>
> Let me rephrase.  Why do we need an #ifdef here, and why depends this
> on USE_HOSTCC?
>
>> > And why do we need the #ifdef?  Can we not always use htobe32() and
>> > put_unaligned_be32() ?
>>
>> Well I don't think put_unaligned_be32 is available to user space,
>> which is the environment that the tools are built under. It is
>> available in the kernel, but that's not our environment.
>
> It appears put_unaligned_be32() is a widely unknown, pretty exotic
> function that so far has been used in ony two source files:
>
>         drivers/usb/gadget/f_mass_storage.c
>         lib/tpm.c
>
> The implementation (in "include/linux/unaligned/generic.h") is ugly
> and pretty expensive in terms of run time and memory footprint.
>
> I would like to avoid it's usage all together.

It ends up producing this on ARMv7:

43e2c1d8: e1a03820 lsr r3, r0, #16
43e2c1dc: e5c40003 strb r0, [r4, #3]
43e2c1e0: e5c43001 strb r3, [r4, #1]
43e2c1e4: e1a02423 lsr r2, r3, #8
43e2c1e8: e7e73450 ubfx r3, r0, #8, #8
43e2c1ec: e5c42000 strb r2, [r4]
43e2c1f0: e5c43002 strb r3, [r4, #2]

In fact with unaligned support we could optimise this.

>
>> I'm not happy with this solution and would be pleased to find a better
>> way, but I'm not sure what it is.
>
> Does the htobe32() + memcpy() approach not work everywhere?

But htobe32() isn't available in U-Boot.

I tried using:

#ifdef USE_HOSTCC
   crc = htobe32(crc);
#else
   crc = cpu_to_be32(crc);
#endif
   memcpy(output, &crc, sizeof(crc));


This is one instruction (4 bytes, 16%) smaller, but I suspect quite a
lot slower due to the overhead of a very small memcpy().

43e2c1d8: e28d1008 add r1, sp, #8
43e2c1dc: e3a02004 mov r2, #4
43e2c1e0: e6bf0f30 rev r0, r0
43e2c1e4: e5210004 str r0, [r1, #-4]!
43e2c1e8: e1a00004 mov r0, r4
43e2c1ec: eb001af7 bl 43e32dd0 <memcpy>

>
>> But this patch does fix a real bug which we should sort out before the
>> release, one way or another.
>
> Agreed.

Let me know what you prefer and I will update the patch.

Regards,
Simon