Hi,
On Thu, Apr 18, 2013 at 11:43 AM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Tom,
On Thu, 18 Apr 2013 12:58:54 -0400, Tom Rini trini@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@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