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

[Simon Glass]

Hi,

On Thu, Apr 18, 2013 at 11:43 AM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> Hi Tom,
>
> On Thu, 18 Apr 2013 12:58:54 -0400, Tom Rini <trini at ti.com> wrote:
>
>> On Thu, Apr 18, 2013 at 06:39:29PM +0200, Albert ARIBAUD wrote:
>> > Hi Wolfgang,
>> >
>> > On Thu, 18 Apr 2013 16:39:09 +0200, Wolfgang Denk <wd at denx.de> wrote:
>> >
>> > > > So how about changing the element type of output in the definition of
>> > > > hash_func_ws, adapting the corresponding implementations sha1_csum_wd,
>> > > > sha256_csum_wd and crc32_wd_buf, and adapting the output argument
>> > > > of the sole call to hash_func_ws, that is, the local "u8
>> > > > output[HASH_MAX_DIGEST_SIZE];" in hash.c? Then we'be done with
>> > > > alignment.
>> > >
>> > > OK, but that would be a totally different approach (which appears to
>> > > be a good one, here).
>> >
>> > Indeed; I would suggest splitting this change in two independent ones:
>> >
>> > - fix the endianness issue: change the endianness of crc through the
>> >   use of htonl() but leave the existing memcpy() in place as it is,
>> >   even though it is not speed-optimized. That's what Simon's patch
>> >   does if the HOSTCC part is ignored;
>> >
>> > - fix the unalignment issue by changing parameter 'output' of function
>> >   type 'hash_func_ws' from u8* to u32* and adjusting the rest of the
>> >   code accordingly -- which would lead to replacing the crc32 final
>> >   memcpy() with a single indirect assignment.
>> >
>> > These two changes could be submitted either separately, or as a series.
>>
>> Now so that I'm clear, if we don't do anything about the unaligned issue
>> today, it's just slow, not an unaligned access that leads to abort,
>> right?  So part one today for release, part two next week after release.
>
> Yes, the current code is just slower than it could be, but works, and
> so would it with Simon's patch as long as it keeps the memcpy().

Yes the alignment issue is manufactured IMO by the char * used for the
function signature and I did not feel comfortable declaring that it
must be word aligned. To be it seems that the type should be naturally
aligned. What do people think about that?

Anyway, that's why I ended up with this patch, which I felt was small
enough to be accepted as a bug fix for the release.

Albert's email exactly mirrors my thinking on this - and yes I would
be much more comfortable not having to create part 2 for the release.

Please let me know what I should do with this patch for now.

Regards,
Simon