Re: [PATCH v2] crypto: xxhash - Implement xxhash support

[Nikolay Borisov <nborisov@suse.com>]

On 28.05.19 г. 18:22 ч., Eric Biggers wrote:
> On Tue, May 28, 2019 at 03:14:51PM +0300, Nikolay Borisov wrote:
>> xxhash is currently implemented as a self-contained module in /lib.
>> This patch enables that module to be used as part of the generic kernel
>> crypto framework. It adds a simple wrapper to the 64bit version.
>>
> 
> Thanks, this looks a lot better.  A couple minor comments below.
> 
>> +static int xxhash64_init(struct shash_desc *desc)
>> +{
>> +	struct xxhash64_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
>> +	struct xxhash64_desc_ctx *dctx = shash_desc_ctx(desc);
>> +
>> +	xxh64_reset(&dctx->xxhstate, tctx->seed);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xxhash64_setkey(struct crypto_shash *tfm, const u8 *key,
>> +			 unsigned int keylen)
>> +{
>> +	struct xxhash64_tfm_ctx *tctx = crypto_shash_ctx(tfm);
>> +
>> +	if (keylen != sizeof(tctx->seed)) {
>> +		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
>> +		return -EINVAL;
>> +	}
>> +	tctx->seed = get_unaligned_le64(key);
>> +	return 0;
>> +}
> 
> Can you please move xxhash64_setkey() to before xxhash64_init() to match the
> order in which the functions get called?
> 
> Sometimes people get confused and think that crypto_shash_init() comes before
> crypto_shash_setkey(), so it's helpful to keep definitions in order.
> 
>> +module_init(xxhash_mod_init);
> 
> Can you change this to subsys_initcall?  We're using subsys_initcall for the
> generic implementations of crypto algorithms now, so that when other
> implementations (e.g. assembly language implementations) are added, the crypto
> self-tests can compare them to the generic implementations.

Will fix those but wanted to ask you whether it's really necessary to
use get_unaligned in setkey given generic code guarantees the buffer is
going to be aligned. E.g. wouldn't cpu_to_le64(*(u64 *)key)) be
"cheaper"? In any case this is a minor point but just want to be sure I
have correctly understood the generic code?

> 
> Thanks,
> 
> - Eric
>