Le 14/06/2014 21:01, Marek Vasut a écrit : > On Tuesday, June 10, 2014 at 02:43:14 PM, LABBE Corentin wrote: >> Add support for the Security System included in Allwinner SoC A20. >> The Security System is a hardware cryptographic accelerator that support >> AES/MD5/SHA1/DES/3DES/PRNG algorithms. >> >> Signed-off-by: LABBE Corentin <clabbe.montjoie@xxxxxxxxx> > > [...] > >> diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-cipher-3des.c >> b/drivers/crypto/sunxi-ss/sunxi-ss-cipher-3des.c new file mode 100644 >> index 0000000..fcdc8a4 >> --- /dev/null >> +++ b/drivers/crypto/sunxi-ss/sunxi-ss-cipher-3des.c >> @@ -0,0 +1,118 @@ >> +/* >> + * sunxi-ss.c - hardware cryptographic accelerator for Allwinner A20 SoC >> + * >> + * Copyright (C) 2013-2014 Corentin LABBE <clabbe.montjoie@xxxxxxxxx> >> + * >> + * Support AES cipher with 128,192,256 bits keysize. >> + * Support MD5 and SHA1 hash algorithms. >> + * Support DES and 3DES >> + * Support PRNG >> + * >> + * You could find the datasheet at >> + * http://dl.linux-sunxi.org/A20/A20%20User%20Manual%202013-03-22.pdf >> + * >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation version 2 of the License > > The license text seems incomplete. > [...] I will replace it with a simplier line "Licensed under the GPL-2." > >> +static int sunxi_des3_cbc_encrypt(struct ablkcipher_request *areq) >> +{ >> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq); >> + struct sunxi_req_ctx *op = crypto_ablkcipher_ctx(tfm); >> + >> + if (areq->info == NULL) { >> + dev_info(ss->dev, "Empty IV\n"); > > dev_err() here. > >> + return -EINVAL; >> + } >> + >> + op->mode |= SS_ENCRYPTION; >> + op->mode |= SS_OP_3DES; >> + op->mode |= SS_CBC; > > You can just op |= (foo | bar | baz) here . > >> + return sunxi_des_poll(areq); >> +} >> + >> +static int sunxi_des3_cbc_decrypt(struct ablkcipher_request *areq) >> +{ >> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq); >> + struct sunxi_req_ctx *op = crypto_ablkcipher_ctx(tfm); >> + >> + if (areq->info == NULL) { >> + dev_info(ss->dev, "Empty IV\n"); > > DTTO. > >> + return -EINVAL; >> + } >> + >> + op->mode |= SS_DECRYPTION; >> + op->mode |= SS_OP_3DES; >> + op->mode |= SS_CBC; > > DTTO. > > [...] >> +static int sunxi_ss_3des_init(void) >> +{ >> + int err = 0; >> + if (ss == NULL) { >> + pr_err("Cannot get Security System structure\n"); >> + return -ENODEV; >> + } >> + err = crypto_register_alg(&sunxi_des3_alg); >> + if (err) >> + dev_err(ss->dev, "crypto_register_alg error for DES3\n"); >> + else >> + dev_dbg(ss->dev, "Registred DES3\n"); >> + return err; >> +} >> + >> +static void __exit sunxi_ss_3des_exit(void) >> +{ >> + crypto_unregister_alg(&sunxi_des3_alg); >> +} >> + >> +module_init(sunxi_ss_3des_init); >> +module_exit(sunxi_ss_3des_exit); > > I really dislike how you have multiple modules accessing the same hardware. That > just seems broken. OK I abort my tries to make things optionnal. > > [...] > >> +static int sunxi_aes_cbc_encrypt(struct ablkcipher_request *areq) >> +{ >> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq); >> + struct sunxi_req_ctx *op = crypto_ablkcipher_ctx(tfm); >> + >> + if (areq->info == NULL) { >> + dev_err(ss->dev, "Empty IV\n"); >> + return -EINVAL; >> + } >> + >> + op->mode |= SS_ENCRYPTION; >> + op->mode |= SS_OP_AES; >> + op->mode |= SS_CBC; > > This looks just like the 3DES implementation. Please make this into a common > code if possible. I think you'd just need to have some switch statement based on > the type of cipher to fill op->mode, that's all. I agree, I will simplify that. > >> + return sunxi_aes_poll(areq); >> +} > > [...] > >> +static int sunxi_des_cbc_encrypt(struct ablkcipher_request *areq) >> +{ >> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq); >> + struct sunxi_req_ctx *op = crypto_ablkcipher_ctx(tfm); >> + >> + if (areq->info == NULL) { >> + dev_info(ss->dev, "Empty IV\n"); >> + return -EINVAL; >> + } >> + >> + op->mode |= SS_ENCRYPTION; >> + op->mode |= SS_OP_DES; >> + op->mode |= SS_CBC; > > Looks similar to 3DES and AES again. > >> + return sunxi_des_poll(areq); >> +} > > [...] > >> diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c >> b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c new file mode 100644 >> index 0000000..a27de49 > [...] >> +int sunxi_cipher_init(struct crypto_tfm *tfm) >> +{ >> + struct sunxi_req_ctx *op = crypto_tfm_ctx(tfm); >> + memset(op, 0, sizeof(struct sunxi_req_ctx)); >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(sunxi_cipher_init); > > This is wrong, you don't want to export this symbol. > >> +void sunxi_cipher_exit(struct crypto_tfm *tfm) >> +{ >> +} >> +EXPORT_SYMBOL_GPL(sunxi_cipher_exit); > > Why do you even need an empty function ? Ok, I will clean that > >> +int sunxi_aes_poll(struct ablkcipher_request *areq) >> +{ >> + u32 tmp; >> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq); >> + struct sunxi_req_ctx *op = crypto_ablkcipher_ctx(tfm); >> + unsigned int ivsize = crypto_ablkcipher_ivsize(tfm); >> + /* when activating SS, the default FIFO space is 32 */ >> + u32 rx_cnt = 32; >> + u32 tx_cnt = 0; >> + u32 v; >> + int i; >> + struct scatterlist *in_sg; >> + struct scatterlist *out_sg; >> + void *src_addr; >> + void *dst_addr; >> + unsigned int ileft = areq->nbytes; >> + unsigned int oleft = areq->nbytes; >> + unsigned int sgileft = areq->src->length; >> + unsigned int sgoleft = areq->dst->length; >> + unsigned int todo; >> + u32 *src32; >> + u32 *dst32; >> + >> + tmp = op->mode; >> + tmp |= SS_ENABLED; > > tmp is not a good name for a variable . renamed to mode > >> + in_sg = areq->src; >> + out_sg = areq->dst; >> + if (areq->src == NULL || areq->dst == NULL) { > > You do a NULL pointer test here, yet you access areq->src->length in sgileft > above . DTTO for areq->dst->length . That would crash much earlier than here. > >> + dev_err(ss->dev, "ERROR: Some SGs are NULL %u\n", areq->nbytes); >> + return -1; >> + } >> + mutex_lock(&ss->lock); >> + if (areq->info != NULL) { >> + for (i = 0; i < op->keylen; i += 4) { >> + v = *(u32 *)(op->key + i); > > Consider that op->key would be magically unaligned ... then you'd trigger > alignment fault here. Make the "op->key" an u32[] and drop those crap casts > here. I Agree > >> + writel(v, ss->base + SS_KEY0 + i); >> + } >> + for (i = 0; i < 4 && i < ivsize / 4; i++) { >> + v = *(u32 *)(areq->info + i * 4); >> + writel(v, ss->base + SS_IV0 + i * 4); >> + } >> + } >> + writel(tmp, ss->base + SS_CTL); >> + >> + /* If we have only one SG, we can use kmap_atomic */ >> + if (sg_next(in_sg) == NULL && sg_next(out_sg) == NULL) { >> + src_addr = kmap_atomic(sg_page(in_sg)) + in_sg->offset; >> + if (src_addr == NULL) { >> + dev_err(ss->dev, "kmap_atomic error for src SG\n"); >> + writel(0, ss->base + SS_CTL); >> + mutex_unlock(&ss->lock); >> + return -1; > > -EINVAL ? > >> + } >> + dst_addr = kmap_atomic(sg_page(out_sg)) + out_sg->offset; >> + if (dst_addr == NULL) { >> + dev_err(ss->dev, "kmap_atomic error for dst SG\n"); >> + writel(0, ss->base + SS_CTL); >> + mutex_unlock(&ss->lock); >> + kunmap_atomic(src_addr); >> + return -1; > > -EINVAL ? > >> + } >> + src32 = (u32 *)src_addr; >> + dst32 = (u32 *)dst_addr; >> + ileft = areq->nbytes / 4; >> + oleft = areq->nbytes / 4; >> + do { >> + if (ileft > 0 && rx_cnt > 0) { >> + todo = min(rx_cnt, ileft); >> + ileft -= todo; >> + do { >> + writel_relaxed(*src32++, >> + ss->base + >> + SS_RXFIFO); >> + todo--; >> + } while (todo > 0); >> + } >> + if (tx_cnt > 0) { >> + todo = min(tx_cnt, oleft); >> + oleft -= todo; >> + do { >> + *dst32++ = readl_relaxed(ss->base + >> + SS_TXFIFO); >> + todo--; >> + } while (todo > 0); >> + } >> + tmp = readl_relaxed(ss->base + SS_FCSR); >> + rx_cnt = SS_RXFIFO_SPACES(tmp); >> + tx_cnt = SS_TXFIFO_SPACES(tmp); >> + } while (oleft > 0); >> + writel(0, ss->base + SS_CTL); >> + mutex_unlock(&ss->lock); >> + kunmap_atomic(src_addr); >> + kunmap_atomic(dst_addr); >> + return 0; >> + } >> + >> + /* If we have more than one SG, we cannot use kmap_atomic since >> + * we hold the mapping too long*/ > > Wrong comment style. > > btw. can't you use generic scatterwalk here ? I will check if it simplify the code. > >> + src_addr = kmap(sg_page(in_sg)) + in_sg->offset; >> + if (src_addr == NULL) { >> + dev_err(ss->dev, "KMAP error for src SG\n"); >> + return -1; >> + } > > Why can't you just use dma_map_sg() or somesuch ? I do not see why I will use a DMA function in a driver without DMA support. Perhaps I do not know some tricks. Can I use writel/readl on address gived by DMA API ? > > [...] >> +EXPORT_SYMBOL_GPL(sunxi_aes_poll); > > Again, don't export the symbol please. > > [...] >> diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-cipher.h >> b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.h new file mode 100644 >> index 0000000..ecfbf9c >> --- /dev/null >> +++ b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.h >> @@ -0,0 +1,8 @@ >> +#include "sunxi-ss.h" >> + >> +extern struct sunxi_ss_ctx *ss; > > Right. Please make it into one module and you won't need all this horror. > [...] > Thanks for your review Regards -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
