On Wed, Sep 04, 2019 at 05:40:22PM +0000, Kalyani Akula wrote: > Hi Corentin, > > Thanks for the review comments. > Please find my response/queries inline. > > > -----Original Message----- > > From: Corentin Labbe <clabbe.montjoie@xxxxxxxxx> > > Sent: Monday, September 2, 2019 12:29 PM > > To: Kalyani Akula <kalyania@xxxxxxxxxx> > > Cc: herbert@xxxxxxxxxxxxxxxxxxx; kstewart@xxxxxxxxxxxxxxxxxxx; > > gregkh@xxxxxxxxxxxxxxxxxxx; tglx@xxxxxxxxxxxxx; pombredanne@xxxxxxxx; > > linux-crypto@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > netdev@xxxxxxxxxxxxxxx; Kalyani Akula <kalyania@xxxxxxxxxx> > > Subject: Re: [PATCH V2 4/4] crypto: Add Xilinx AES driver > > > > On Sun, Sep 01, 2019 at 07:24:58PM +0530, Kalyani Akula wrote: > > > This patch adds AES driver support for the Xilinx ZynqMP SoC. > > > > > > Signed-off-by: Kalyani Akula <kalyani.akula@xxxxxxxxxx> > > > --- > > > > Hello > > > > I have some comment below > > > > > drivers/crypto/Kconfig | 11 ++ > > > drivers/crypto/Makefile | 1 + > > > drivers/crypto/zynqmp-aes-gcm.c | 297 > > > ++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 309 insertions(+) > > > create mode 100644 drivers/crypto/zynqmp-aes-gcm.c > > > > > > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index > > > 603413f..a0d058a 100644 > > > --- a/drivers/crypto/Kconfig > > > +++ b/drivers/crypto/Kconfig > > > @@ -677,6 +677,17 @@ config CRYPTO_DEV_ROCKCHIP > > > This driver interfaces with the hardware crypto accelerator. > > > Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode. > > > > > > +config CRYPTO_DEV_ZYNQMP_AES > > > + tristate "Support for Xilinx ZynqMP AES hw accelerator" > > > + depends on ARCH_ZYNQMP || COMPILE_TEST > > > + select CRYPTO_AES > > > + select CRYPTO_SKCIPHER > > > + help > > > + Xilinx ZynqMP has AES-GCM engine used for symmetric key > > > + encryption and decryption. This driver interfaces with AES hw > > > + accelerator. Select this if you want to use the ZynqMP module > > > + for AES algorithms. > > > + > > > config CRYPTO_DEV_MEDIATEK > > > tristate "MediaTek's EIP97 Cryptographic Engine driver" > > > depends on (ARM && ARCH_MEDIATEK) || COMPILE_TEST diff --git > > > a/drivers/crypto/Makefile b/drivers/crypto/Makefile index > > > afc4753..c99663a 100644 > > > --- a/drivers/crypto/Makefile > > > +++ b/drivers/crypto/Makefile > > > @@ -48,3 +48,4 @@ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/ > > > obj-$(CONFIG_CRYPTO_DEV_SAFEXCEL) += inside-secure/ > > > obj-$(CONFIG_CRYPTO_DEV_ARTPEC6) += axis/ obj-y += hisilicon/ > > > +obj-$(CONFIG_CRYPTO_DEV_ZYNQMP_AES) += zynqmp-aes-gcm.o > > > diff --git a/drivers/crypto/zynqmp-aes-gcm.c > > > b/drivers/crypto/zynqmp-aes-gcm.c new file mode 100644 index > > > 0000000..d65f038 > > > --- /dev/null > > > +++ b/drivers/crypto/zynqmp-aes-gcm.c > > > @@ -0,0 +1,297 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Xilinx ZynqMP AES Driver. > > > + * Copyright (c) 2019 Xilinx Inc. > > > + */ > > > + > > > +#include <crypto/aes.h> > > > +#include <crypto/scatterwalk.h> > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/of_device.h> > > > +#include <linux/scatterlist.h> > > > +#include <linux/firmware/xlnx-zynqmp.h> > > > + > > > +#define ZYNQMP_AES_IV_SIZE 12 > > > +#define ZYNQMP_AES_GCM_SIZE 16 > > > +#define ZYNQMP_AES_KEY_SIZE 32 > > > + > > > +#define ZYNQMP_AES_DECRYPT 0 > > > +#define ZYNQMP_AES_ENCRYPT 1 > > > + > > > +#define ZYNQMP_AES_KUP_KEY 0 > > > +#define ZYNQMP_AES_DEVICE_KEY 1 > > > +#define ZYNQMP_AES_PUF_KEY 2 > > > + > > > +#define ZYNQMP_AES_GCM_TAG_MISMATCH_ERR 0x01 > > > +#define ZYNQMP_AES_SIZE_ERR 0x06 > > > +#define ZYNQMP_AES_WRONG_KEY_SRC_ERR 0x13 > > > +#define ZYNQMP_AES_PUF_NOT_PROGRAMMED 0xE300 > > > + > > > +#define ZYNQMP_AES_BLOCKSIZE 0x04 > > > + > > > +static const struct zynqmp_eemi_ops *eemi_ops; struct zynqmp_aes_dev > > > +*aes_dd; > > > > I still think that using a global variable for storing device driver data is bad. > > I think storing the list of dd's would solve up the issue with global variable, but there is only one AES instance here. > Please suggest > Look what I do for amlogic driver https://patchwork.kernel.org/patch/11059633/. I store the device driver in the instatiation of a crypto template. [...] > > > +static int zynqmp_setkey_blk(struct crypto_tfm *tfm, const u8 *key, > > > + unsigned int len) > > > +{ > > > + struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm); > > > + > > > + if (((len != 1) && (len != ZYNQMP_AES_KEY_SIZE)) || (!key)) > > > > typo, two space > > Will fix in the next version > > > > > > + return -EINVAL; > > > + > > > + if (len == 1) { > > > + op->keytype = *key; > > > + > > > + if ((op->keytype < ZYNQMP_AES_KUP_KEY) || > > > + (op->keytype > ZYNQMP_AES_PUF_KEY)) > > > + return -EINVAL; > > > + > > > + } else if (len == ZYNQMP_AES_KEY_SIZE) { > > > + op->keytype = ZYNQMP_AES_KUP_KEY; > > > + op->keylen = len; > > > + memcpy(op->key, key, len); > > > + } > > > + > > > + return 0; > > > +} > > > > It seems your driver does not support AES keysize of 128/196, you need to > > fallback in that case. > > [Kalyani] In case of 128/196 keysize, returning the error would suffice ? > Or still algorithm need to work ? > If error is enough, it is taken care by this condition > if (((len != 1) && (len != ZYNQMP_AES_KEY_SIZE)) || (!key)) I think this problem just simply show us that your driver is not tested against crypto selftest. I have tried to refuse 128/196 in my driver and I get: alg: skcipher: cbc-aes-sun8i-ce setkey failed on test vector 0; expected_error=0, actual_error=-22, flags=0x1 So if your hardware lack 128/196 keys support, you must fallback to a software version. Anyway please test your driver with crypto selftest enabled (and also CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y) Regards
