Antoine, > -----Original Message----- > From: linux-crypto-owner@xxxxxxxxxxxxxxx <linux-crypto-owner@xxxxxxxxxxxxxxx> On Behalf Of Antoine Tenart > Sent: Friday, July 26, 2019 2:20 PM > To: Pascal van Leeuwen <pascalvanl@xxxxxxxxx> > Cc: linux-crypto@xxxxxxxxxxxxxxx; antoine.tenart@xxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; Pascal Van > Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx> > Subject: Re: [PATCH 1/3] crypto: inside-secure - add support for authenc(hmac(sha1),cbc(des3_ede)) > > Hi Pascal, > > On Fri, Jul 05, 2019 at 08:49:22AM +0200, Pascal van Leeuwen wrote: > > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx> > > 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? > > @@ -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 ... > > +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 ... > > + .alg.aead = { > > + .setkey = safexcel_aead_setkey, > > + .encrypt = safexcel_aead_encrypt_3des, > > + .decrypt = safexcel_aead_decrypt_3des, > > + .ivsize = DES3_EDE_BLOCK_SIZE, > > + .maxauthsize = SHA1_DIGEST_SIZE, > > + .base = { > > + .cra_name = "authenc(hmac(sha1),cbc(des3_ede))", > > + .cra_driver_name = "safexcel-authenc-hmac-sha1-cbc-des3_ede", > > You could drop "_ede" here, or s/_/-/. > Agree the underscore should not be there. Our HW does not support any other form of 3DES so EDE doesn't really add much here, therefore I will just remove "_ede" entirely. > Apart from those small comments, the patch looks good. > > Thanks! > Antoine > > -- > Antoine Ténart, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com