Re: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede))

[Antoine Tenart <antoine.tenart@bootlin.com>]

Hi Pascal,

On Fri, Jul 26, 2019 at 12:57:21PM +0000, Pascal Van Leeuwen wrote:
> > On Fri, Jul 05, 2019 at 08:49:22AM +0200, Pascal van Leeuwen wrote:
> > > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
> > 
> > Could you provide a commit message, explaining briefly what the patch is
> > doing?
> > 
> I initially figured that to be redundant if the subject already covered it completely.
> But now that I think of it, it's possible the subject does not end up in the commit
> at all ... if that is the case, would it work if I just copy-paste the relevant part of the
> subject message? Or do I need to be more verbose?

The subject will be the commit title. I know sometimes the commit
message is trivial or redundant, but it's still a good practice to
always have one (and many maintainers will ask for one). Even if it's
only two lines :)

> > > @@ -199,6 +201,15 @@ static int safexcel_aead_aes_setkey(struct crypto_aead *ctfm, const u8 *key,
> > >  		goto badkey;
> > >
> > >  	/* Encryption key */
> > > +	if (ctx->alg == SAFEXCEL_3DES) {
> > > +		flags = crypto_aead_get_flags(ctfm);
> > > +		err = __des3_verify_key(&flags, keys.enckey);
> > > +		crypto_aead_set_flags(ctfm, flags);
> > 
> > You could use directly des3_verify_key() which does exactly this.
> > 
> Actually, I couldn't due to des3_verify_key expecting a struct crypto_skcipher as input,
> and not a struct crypto_aead, that's why I had to do it this way ...

I see. Maybe a good way would be to provide a function taking
'struct crypto_aead' as an argument so that not every single driver
reimplement the same logic. But this can come later if needed.

> > > +struct safexcel_alg_template safexcel_alg_authenc_hmac_sha1_cbc_des3_ede = {
> > > +	.type = SAFEXCEL_ALG_TYPE_AEAD,
> > 
> > You either missed to fill .engines member of this struct, or this series
> > is based on another one not merged yet.
> > 
> Yes, that happened in the patchset of which v2 did not make it to the mailing list ...

:)

So in general if there's a dependency you should say so in the cover
letter.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com