Re: [PATCH v4 1/5] crypto: add blake2b generic implementation

[David Sterba <dsterba@suse.cz>]

On Fri, Oct 11, 2019 at 11:04:40AM -0700, Eric Biggers wrote:
> > +struct blake2b_param
> > +{
> 
> It should be 'struct blake2b_param {'
> 
> checkpatch.pl should warn about this.  Can you fix the checkpatch warnings that
> make sense to fix?

I fixed all the { at the end of line of struct definitions and left all
of type

  ERROR: space prohibited after that open square bracket '['
  #276: FILE: crypto/blake2b_generic.c:200:
  +       v[ 9] = blake2b_IV[1];

and

  WARNING: line over 80 characters
  #304: FILE: crypto/blake2b_generic.c:228:
  +static void blake2b_update(struct blake2b_state *S, const void *pin, size_t inlen)

ie. where the ) or ); is beyond 80 but does not otherwise hurt
readability unlike forced newline and parameters on another line. This
is my prefrence in code I otherwise work on but I'll follow what's
common practice in crypto/.

> > +/* init xors IV with 
> > +static int blake2b_init_param(struct blake2b_state *S,
> > +			      const struct blake2b_param *P)
> > +{
> > +	const u8 *p = (const u8 *)(P);
> > +	size_t i;
> > +
> > +	blake2b_init0(S);
> > +
> > +	/* IV XOR ParamBlock */
> > +	for (i = 0; i < 8; ++i)
> > +		S->h[i] ^= get_unaligned_le64(p + sizeof(S->h[i]) * i);
> > +
> > +	S->outlen = P->digest_length;
> > +	return 0;
> > +}
> 
> No need for this to have a return value anymore.  Same with:
> 
> 	blake2b_init_param()
> 	blake2b_update()
> 	blake2b_init()
> 	blake2b_init_key()
> 	blake2b_final()
> 
> The code would be more readable if they returned void, since otherwise it gives
> the impression that errors can occur.

Make sense.

> > +static int blake2b_update(struct blake2b_state *S, const void *pin, size_t inlen)
> > +{
> > +	const unsigned char *in = (const unsigned char *)pin;
> 
> Convention is to use 'u8', not 'unsigned char'.

Fixed.

> > +MODULE_ALIAS_CRYPTO("blake2b");
> > +MODULE_ALIAS_CRYPTO("blake2b-generic");
> 
> Should remove these module aliases now that the "blake2b" algorithm was removed.

A bit more of typing 'modprobe blake2-512' but I have checked if other
modules use a bare algo name as an alias and found none. So for
consistency I'll remove the 2 lines.