Hi Jason, just a few comments: On Fri, Jan 06, 2017 at 09:10:52PM +0100, Jason A. Donenfeld wrote: > +#define SIPHASH_ALIGNMENT __alignof__(u64) > +typedef u64 siphash_key_t[2];
I was confused by all the functions passing siphash_key_t "by value" until I saw that it's actually typedefed to u64[2]. Have you considered making it a struct instead, something like this? typedef struct { u64 v[2]; } siphash_key_t; Then it would be strongly typed and thus harder to misuse, and all the functions would take 'const siphash_key_t *' instead of 'const siphash_key_t' which would make it clear that the key is passed by pointer not by value. > +static inline u64 ___siphash_aligned(const __le64 *data, size_t len, const > siphash_key_t key) > +{ > + if (__builtin_constant_p(len) && len == 4) > + return siphash_1u32(le32_to_cpu(data[0]), key); Small bug here: data[0] is not valid if len is 4. This can be fixed by casting to a le32 pointer: return siphash_1u32(le32_to_cpup((const __le32 *)data), key); > +static int __init siphash_test_init(void) > +{ > + u8 in[64] __aligned(SIPHASH_ALIGNMENT); > + u8 in_unaligned[65]; It seems that in_unaligned+1 is meant to be misaligned, but that's not guaranteed because in_unaligned has no alignment restriction, so it could theoretically be misaligned in a way that makes in_unaligned+1 aligned. So it should be 'in_unaligned[65] __aligned(SIPHASH_ALIGNMENT)'. There are also a lot of checkpatch warnings produced by this patch. It looks like many of them can be ignored, but there may be some that should be addressed. - Eric