picoxcell_crypto: add support for the picoxcell crypto engines
PATCH 1/1 picoxcell_crypto: add support for the picoxcell crypto engines
Picochip picoXcell devices have two crypto engines, one targeted at IPSEC offload and the other at WCDMA layer 2 ciphering.
--- drivers/crypto/Kconfig | 15 + drivers/crypto/Makefile | 2 +- drivers/crypto/picoxcell_crypto.c | 1892 ++++++++++++++++++++++++++++++++ drivers/crypto/picoxcell_crypto_regs.h | 128 +++ 4 files changed, 2036 insertions(+), 1 deletions(-) create mode 100644 drivers/crypto/picoxcell_crypto.c create mode 100644 drivers/crypto/picoxcell_crypto_regs.h
drivers/crypto/Kconfig +15 -0
@@ -252,4 +252,19 @@ config CRYPTO_DEV_OMAP_AES OMAP processors have AES module accelerator. Select this if you want to use the OMAP module for AES algorithms. +config CRYPTO_DEV_PICOXCELL + tristate "Support for picoXcell IPSEC and Layer2 crypto engines" + depends on ARCH_PICOXCELL + select CRYPTO_AES + select CRYPTO_AUTHENC + select CRYPTO_ALGAPI + select CRYPTO_DES + select CRYPTO_CBC + select CRYPTO_ECB + select CRYPTO_SEQIV + help + This option enables support for the hardware offload engines in the + Picochip picoXcell SoC devices. Select this for IPSEC ESP offload + and for 3gpp Layer 2 ciphering support. + endif # CRYPTO_HW
drivers/crypto/Makefile +1 -1
@@ -10,4 +10,4 @@ obj-$(CONFIG_CRYPTO_DEV_IXP4XX) += ixp4xx_crypto.o obj-$(CONFIG_CRYPTO_DEV_PPC4XX) += amcc/ obj-$(CONFIG_CRYPTO_DEV_OMAP_SHAM) += omap-sham.o obj-$(CONFIG_CRYPTO_DEV_OMAP_AES) += omap-aes.o - +obj-$(CONFIG_CRYPTO_DEV_PICOXCELL) += picoxcell_crypto.o
drivers/crypto/picoxcell_crypto.c new file +1892 -0
1,892 lines not on this page · raw message
drivers/crypto/picoxcell_crypto_regs.h new file +128 -0
128 lines not on this page · raw message
Signature · 1 line
1.7.4
PATCH 1/1 picoxcell_crypto: add support for the picoxcell crypto engines
Quoted 6 lines · On Tue, 8 Feb 2011 15:56:16 +0000 Jamie Iles <[email protected]> wrote:
Picochip picoXcell devices have two crypto engines, one targeted at IPSEC offload and the other at WCDMA layer 2 ciphering.
Cc Herbert Xu <[email protected]>Signed-off-by Jamie Iles <[email protected]>---
nice driver ;). Have a couple of comments though.
drivers/crypto/Kconfig config CRYPTO_DEV_OMAP_AES · line 268
Quoted 4 lines
drivers/crypto/Kconfig +4
-0+ help + This option enables support for the hardware offload engines in the + Picochip picoXcell SoC devices. Select this for IPSEC ESP offload + and for 3gpp Layer 2 ciphering support.
it'd be nice to mention what name the module will have.
drivers/crypto/picoxcell_crypto.c line 65
Quoted 3 lines
drivers/crypto/picoxcell_crypto.c new file +3
-0+#define SPACC_CRYPTO_AES_MAX_KEY_LEN 32 +#define SPACC_CRYPTO_AES_IV_LEN 16 +#define SPACC_CRYPTO_DES_IV_LEN 8
these are identical to algorithm-generic symbolic constants AES_MAX_KEY_SIZE, [AD]ES_BLOCK_SIZE - why not use them instead?
Quoted 1 line
+struct
spacc_generic_ctx;
this declaration isn't used prior to its definition, so it's not needed.
drivers/crypto/picoxcell_crypto.c line 82
Quoted 3 lines
drivers/crypto/picoxcell_crypto.c new file +3
-0+/* DDT format. This must match the hardware DDT format exactly. */ +struct spacc_ddt { +u32 p, len;
type-consistency: p should be a dma_addr_t
Quoted 1 line
+ /* AEAD specifc bits. */
specific
drivers/crypto/picoxcell_crypto.c line 190
Quoted 11 lines
drivers/crypto/picoxcell_crypto.c new file +11
-0+static inline struct spacc_ablk_ctx * +to_spacc_ablk_ctx (struct spacc_generic_ctx *ctx) +{ + return ctx ? container_of(ctx, struct spacc_ablk_ctx, generic) : NULL; +} + +static inline struct spacc_aead_ctx * +to_spacc_aead_ctx (struct spacc_generic_ctx *ctx) +{ + return ctx ? container_of(ctx, struct spacc_aead_ctx, generic) : NULL; +}
these aren't being used anywhere.
Quoted 1 line
+static inline struct
spacc_alg*to_spacc_alg(structcrypto_alg*alg);
define it here - forward declarations should only be necessary when dealing with circular dependencies.
Quoted 30 lines
drivers/crypto/picoxcell_crypto.c new file +30
-0+/* +* Take a crypto request and scatterlists for the data and turn them into DDTs +* for passing to the crypto engines. This also DMA maps the data so that the +* crypto engines can DMA to/from them. +*/ +static struct spacc_ddt *spacc_sg_to_ddt (struct spacc_engine *engine, +struct scatterlist *payload, +unsigned nbytes, +enum dma_data_direction dir, +dma_addr_t *ddt_phys) +{ +unsigned nents, mapped_ents; +struct scatterlist *cur; +struct spacc_ddt *ddt; +int i; + + nents =sg_count (payload, nbytes); + mapped_ents =dma_map_sg (engine->dev, payload, nents, dir); + +if (mapped_ents +1 >MAX_DDT_LEN ) { +dma_unmap_sg (engine->dev, payload, nents, dir); +return NULL ; + } + + ddt =dma_pool_alloc (engine->req_pool,GFP_ATOMIC , ddt_phys); +if (ddt) { +for_each_sg (payload, cur, mapped_ents, i) { + ddt[i].p =sg_dma_address (cur); + ddt[i].len =sg_dma_len (cur); + }
drivers/crypto/picoxcell_crypto.c line 324
Quoted 10 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+ + ddt[mapped_ents].p =0 ; + ddt[mapped_ents].len =0 ; + }else { +dma_unmap_sg (engine->dev, payload, nents, dir); + ddt =NULL ; + } + +return ddt; +}
easier to read would be:
if (mapped_ents + 1 > MAX_DDT_LEN)
goto out;
ddt = dma_pool_alloc(engine->req_pool, GFP_ATOMIC, ddt_phys);
if (!ddt)
goto out;
for_each_sg(payload, cur, mapped_ents, i) {
ddt[i].p = sg_dma_address(cur);
ddt[i].len = sg_dma_len(cur);
}
ddt[mapped_ents].p = 0;
ddt[mapped_ents].len = 0;
return ddt;out:
dma_unmap_sg(engine->dev, payload, nents, dir);
return NULL;
}even more so by moving ddt_set() above it, and then using ddt_set() to assign the p, len pairs.
Quoted 1 line
+static inline void ddt_set(struct
spacc_ddt*ddt, unsigned long phys,
phys should be dma_addr_t
drivers/crypto/picoxcell_crypto.c line 340
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c new file +8
-0+static int spacc_aead_make_ddts (struct spacc_req *req,u8 *giv) +{ + struct aead_request *areq = container_of(req->req, struct aead_request, + base); +struct spacc_alg *alg =to_spacc_alg (req->req->tfm->__crt_alg); +struct spacc_engine *engine = req->engine; +struct spacc_ddt *src_ddt, *dst_ddt; +unsigned ivsize = alg->alg.cra_aead.ivsize;
no need to go through all those hoops to get to the ivsize - use helper fns crypto_aead_reqtfm() and crypto_aead_ivsize(), as is done at the callsite, or just pass it in from there.
drivers/crypto/picoxcell_crypto.c line 467
Quoted 10 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+static int spacc_aead_des_setkey (struct crypto_aead *aead,const u8 *key, +unsigned int len) +{ +struct crypto_tfm *tfm =crypto_aead_tfm (aead); +struct spacc_aead_ctx *ctx =crypto_tfm_ctx (tfm); +int err =0 ; +u32 tmp[DES_EXPKEY_WORDS ]; + + err =des_ekey (tmp, key); + if (unlikely(!err) &&
might want to change the name of the variable err here to something like ret or is_weak so as to not mislead the reader.
drivers/crypto/picoxcell_crypto.c line 477
Quoted 10 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+ (crypto_aead_get_flags(aead)) & CRYPTO_TFM_REQ_WEAK_KEY) { + tfm->crt_flags |=CRYPTO_TFM_RES_WEAK_KEY ; +return -EINVAL ; + } + err =0 ; + +memcpy (ctx->cipher_key, key, len); + ctx->cipher_key_len = len; + +return err;
actually, it doesn't look like this fn needs a return variable at all.
Quoted 4 lines
drivers/crypto/picoxcell_crypto.c new file +4
-0+/* Set the key for the AES block cipher component of the AEAD transform. */ +static int spacc_aead_aes_setkey (struct crypto_aead *aead,const u8 *key, +unsigned int len) +{
drivers/crypto/picoxcell_crypto.c line 493
Quoted 10 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+struct crypto_tfm *tfm =crypto_aead_tfm (aead); +struct spacc_aead_ctx *ctx =crypto_tfm_ctx (tfm); +int err; + +/* +* IPSec engine only supports 128 and 256 bit AES keys. If we get a +* request for any other size (192 bits) then we need to do a software +* fallback. +*/ +if (!(16 == len ||32 == len)) {
if (len != AES_KEYSIZE_128 && len != AES_KEYSIZE_256)
drivers/crypto/picoxcell_crypto.c line 500
Quoted 7 lines
drivers/crypto/picoxcell_crypto.c new file +7
-0+/* +* Set the fallback transform to use the same request flags as +* the hardware transform. +*/ + ctx->sw_cipher->base.crt_flags &= ~CRYPTO_TFM_REQ_MASK ; + ctx->sw_cipher->base.crt_flags |= + (tfm->crt_flags &CRYPTO_TFM_REQ_MASK );
parens not needed.
drivers/crypto/picoxcell_crypto.c line 508
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c new file +8
-0+ err =crypto_aead_setkey (ctx->sw_cipher, key, len); + }else { +memcpy (ctx->cipher_key, key, len); + ctx->cipher_key_len = len; + err =0 ; + } + +return err;
return crypto_aead_setkey(ctx->sw_cipher, key, len); }
memcpy(ctx->cipher_key, key, len);ctx->cipher_key_len = len;
return 0;
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c new file +8
-0+static int spacc_aead_setkey (struct crypto_aead *tfm,const u8 *key, +unsigned int keylen) +{ +struct spacc_aead_ctx *ctx =crypto_aead_ctx (tfm); +struct spacc_alg *alg =to_spacc_alg (tfm->base.__crt_alg); +struct rtattr *rta = (void *)key; +struct crypto_authenc_key_param *param; +unsigned int authkeylen, enckeylen;
drivers/crypto/picoxcell_crypto.c line 528
Quoted 10 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+int err = -EINVAL ; + +if (!RTA_OK (rta, keylen)) + goto badkey; + +if (rta->rta_type !=CRYPTO_AUTHENC_KEYA_PARAM ) + goto badkey; + +if (RTA_PAYLOAD (rta) <sizeof (*param)) + goto badkey;
I'm not sure, but it should be safe to remove the above three checks - they cause a false badkey failure if the keys aren't within an rtattr struct, which, e.g., something like testmgr.c wouldn't do.
drivers/crypto/picoxcell_crypto.c line 534
Quoted 5 lines
drivers/crypto/picoxcell_crypto.c new file +5
-0+ param =RTA_DATA (rta); + enckeylen =be32_to_cpu (param->enckeylen); + + key +=RTA_ALIGN (rta->rta_len); + keylen -=RTA_ALIGN (rta->rta_len);
actually, I doubt crypto drivers should be including rtnetlink.h at all...but it's probably ok for now - talitos still does :)
drivers/crypto/picoxcell_crypto.c line 593
Quoted 3 lines
drivers/crypto/picoxcell_crypto.c new file +3
-0+if ((spacc_alg->ctrl_default &SPACC_CRYPTO_ALG_MASK ) == +SPA_CTRL_CIPH_ALG_AES && + !(16 == ctx->cipher_key_len ||32 == ctx->cipher_key_len))
as above, please use symbolic equivalents
drivers/crypto/picoxcell_crypto.c line 627
Quoted 6 lines
drivers/crypto/picoxcell_crypto.c new file +6
-0+static void spacc_aead_complete (struct spacc_req *req) +{ +spacc_aead_free_ddts (req); + +if (req->req->complete) + req->req->complete (req->req, req->result);
when is there not a completion function?
drivers/crypto/picoxcell_crypto.c line 648
Quoted 3 lines
drivers/crypto/picoxcell_crypto.c new file +3
-0+/* Set the source and destination DDT pointers. */ +writel ((u32 )req->src_addr, engine->regs +SPA_SRC_PTR_REG_OFFSET ); +writel ((u32 )req->dst_addr, engine->regs +SPA_DST_PTR_REG_OFFSET );
cast necessary?
drivers/crypto/picoxcell_crypto.c line 683
Quoted 7 lines
drivers/crypto/picoxcell_crypto.c new file +7
-0+ ctrl = spacc_alg->ctrl_default; + ctrl |= ((req->ctx_id <<SPA_CTRL_CTX_IDX ) | + (1 <<SPA_CTRL_ICV_APPEND ) | + (req->is_encrypt ? (1 <<SPA_CTRL_ENCRYPT_IDX ) :0 ) | + (req->is_encrypt ? (1 <<SPA_CTRL_AAD_COPY ) :0 )); +if (!req->is_encrypt) + ctrl |= (1 <<SPA_CTRL_KEY_EXP );
ctrl = spacc_alg->ctrl_default | (req->ctx_id << SPA_CTRL_CTX_IDX) |
(1 << SPA_CTRL_ICV_APPEND);
if (req->is_encrypt)
ctrl |= (1 << SPA_CTRL_ENCRYPT_IDX) | (1 << SPA_CTRL_AAD_COPY); else ctrl |= (1 << SPA_CTRL_KEY_EXP);
drivers/crypto/picoxcell_crypto.c line 841
Quoted 9 lines
drivers/crypto/picoxcell_crypto.c new file +9
-0+static int spacc_des_setkey (struct crypto_ablkcipher *cipher,const u8 *key, +unsigned int len) +{ +struct crypto_tfm *tfm =crypto_ablkcipher_tfm (cipher); +struct spacc_ablk_ctx *ctx =crypto_tfm_ctx (tfm); +int err; +u32 tmp[DES_EXPKEY_WORDS ]; + +if (len >SPACC_CRYPTO_AES_MAX_KEY_LEN ) {
AES left overs in a DES setkey
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c new file +8
-0+static int spacc_aes_setkey (struct crypto_ablkcipher *cipher,const u8 *key, +unsigned int len) +{ +struct crypto_tfm *tfm =crypto_ablkcipher_tfm (cipher); +struct spacc_ablk_ctx *ctx =crypto_tfm_ctx (tfm); +int err =0 ; + +if (len >SPACC_CRYPTO_AES_MAX_KEY_LEN ) {
drivers/crypto/picoxcell_crypto.c line 880
Quoted 10 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+crypto_ablkcipher_set_flags (cipher,CRYPTO_TFM_RES_BAD_KEY_LEN ); +return -EINVAL ; + } + +/* +* IPSec engine only supports 128 and 256 bit AES keys. If we get a +* request for any other size (192 bits) then we need to do a software +* fallback. +*/ +if (!(16 == len ||32 == len) && ctx->sw_cipher) {
symbolic constants
drivers/crypto/picoxcell_crypto.c line 887
Quoted 7 lines
drivers/crypto/picoxcell_crypto.c new file +7
-0+/* +* Set the fallback transform to use the same request flags as +* the hardware transform. +*/ + ctx->sw_cipher->base.crt_flags &= ~CRYPTO_TFM_REQ_MASK ; + ctx->sw_cipher->base.crt_flags |= + (cipher->base.crt_flags &CRYPTO_TFM_REQ_MASK );
parens not necessary
drivers/crypto/picoxcell_crypto.c line 939
Quoted 12 lines
drivers/crypto/picoxcell_crypto.c new file +12
-0+static int spacc_ablk_need_fallback (struct spacc_req *req) +{ +struct spacc_ablk_ctx *ctx; +struct crypto_tfm *tfm = req->req->tfm; +struct crypto_alg *alg = req->req->tfm->__crt_alg; +struct spacc_alg *spacc_alg =to_spacc_alg (alg); + + ctx =crypto_tfm_ctx (tfm); + +return (spacc_alg->ctrl_default &SPACC_CRYPTO_ALG_MASK ) == +SPA_CTRL_CIPH_ALG_AES && + !(16 == ctx->key_len ||32 == ctx->key_len);
symbolic constants
drivers/crypto/picoxcell_crypto.c line 1320
Quoted 6 lines
drivers/crypto/picoxcell_crypto.c new file +6
-0+static ssize_t spacc_stat_irq_thresh_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t len) +{ +struct spacc_engine *engine =spacc_dev_to_engine (dev); +unsigned thresh =simple_strtoul (buf,NULL ,0 );
consider using strict_strtoul (checkpatch)
drivers/crypto/picoxcell_crypto.c line 1345
Quoted 13 lines
drivers/crypto/picoxcell_crypto.c new file +13
-0+static struct spacc_alg ipsec_engine_algs[] = { + { + .ctrl_default =SPA_CTRL_CIPH_ALG_AES |SPA_CTRL_CIPH_MODE_CBC , + .key_offs =0 , + .iv_offs =SPACC_CRYPTO_AES_MAX_KEY_LEN , + .alg = { + .cra_name ="cbc(aes)" , + .cra_driver_name ="cbc-aes-picoxcell" , + .cra_priority =SPACC_CRYPTO_ALG_PRIORITY , + .cra_flags =CRYPTO_ALG_TYPE_ABLKCIPHER | +CRYPTO_ALG_ASYNC | +CRYPTO_ALG_NEED_FALLBACK , + .cra_blocksize =16 ,
symbolic constant, here and throughout the rest of this section.
Thanks,
Kim
PATCH 1/1 picoxcell_crypto: add support for the picoxcell crypto engines
Quoted 11 lines · Kim Phillips
Quoted 6 lines · On Tue, 8 Feb 2011 15:56:16 +0000 Jamie Iles <[email protected]> wrote:
Picochip picoXcell devices have two crypto engines, one targeted at IPSEC offload and the other at WCDMA layer 2 ciphering.
Cc Herbert Xu <[email protected]>Signed-off-by Jamie Iles <[email protected]>---
nice driver ;). Have a couple of comments though.
Hi Kim,
Thanks for the great review!
Quoted 7 lines
drivers/crypto/Kconfig config CRYPTO_DEV_OMAP_AES · line 268
Quoted 5 lines
drivers/crypto/Kconfig +4
-0+ help + This option enables support for the hardware offload engines in the + Picochip picoXcell SoC devices. Select this for IPSEC ESP offload + and for 3gpp Layer 2 ciphering support.it'd be nice to mention what name the module will have.
Ok, will add.
Quoted 7 lines
drivers/crypto/picoxcell_crypto.c line 65
Quoted 4 lines
drivers/crypto/picoxcell_crypto.c new file +3
-0+#define SPACC_CRYPTO_AES_MAX_KEY_LEN 32 +#define SPACC_CRYPTO_AES_IV_LEN 16 +#define SPACC_CRYPTO_DES_IV_LEN 8these are identical to algorithm-generic symbolic constants
AES_MAX_KEY_SIZE, [AD]ES_BLOCK_SIZE - why not use them instead?
I wasn't aware of these constants but yes, that's much better.
Quoted 4 lines
Quoted 2 lines
+struct
spacc_generic_ctx;this declaration isn't used prior to its definition, so it's not needed.
Ok, will remove.
Quoted 5 lines
drivers/crypto/picoxcell_crypto.c line 82
Quoted 3 lines
drivers/crypto/picoxcell_crypto.c new file +3
-0+/* DDT format. This must match the hardware DDT format exactly. */ +struct spacc_ddt { +u32 p, len;type-consistency: p should be a
dma_addr_t
The reason I used a u32 was the the engine descriptor format is two 32 bit words so I was trying to be explicit but I'll change this to dma_addr_t.
Quoted 4 lines
Quoted 2 lines
+ /* AEAD specifc bits. */
specific
Good spot!
Quoted 5 lines
Quoted 5 lines
drivers/crypto/picoxcell_crypto.c new file +5
-0+static inline struct spacc_ablk_ctx * +to_spacc_ablk_ctx (struct spacc_generic_ctx *ctx) +{ + return ctx ? container_of(ctx, struct spacc_ablk_ctx, generic) : NULL; +}
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c line 190
Quoted 6 lines
drivers/crypto/picoxcell_crypto.c new file +6
-0+ +static inline struct spacc_aead_ctx * +to_spacc_aead_ctx (struct spacc_generic_ctx *ctx) +{ + return ctx ? container_of(ctx, struct spacc_aead_ctx, generic) : NULL; +}these aren't being used anywhere.
Ok, will remove.
Quoted 5 lines
Quoted 2 lines
+static inline struct
spacc_alg*to_spacc_alg(structcrypto_alg*alg);define it here - forward declarations should only be necessary when dealing with circular dependencies.
Hmm, not sure why I didn't do that originally! Will change.
Quoted 60 lines
Quoted 31 lines
drivers/crypto/picoxcell_crypto.c new file +30
-0+/* +* Take a crypto request and scatterlists for the data and turn them into DDTs +* for passing to the crypto engines. This also DMA maps the data so that the +* crypto engines can DMA to/from them. +*/ +static struct spacc_ddt *spacc_sg_to_ddt (struct spacc_engine *engine, +struct scatterlist *payload, +unsigned nbytes, +enum dma_data_direction dir, +dma_addr_t *ddt_phys) +{ +unsigned nents, mapped_ents; +struct scatterlist *cur; +struct spacc_ddt *ddt; +int i; + + nents =sg_count (payload, nbytes); + mapped_ents =dma_map_sg (engine->dev, payload, nents, dir); + +if (mapped_ents +1 >MAX_DDT_LEN ) { +dma_unmap_sg (engine->dev, payload, nents, dir); +return NULL ; + } + + ddt =dma_pool_alloc (engine->req_pool,GFP_ATOMIC , ddt_phys); +if (ddt) { +for_each_sg (payload, cur, mapped_ents, i) { + ddt[i].p =sg_dma_address (cur); + ddt[i].len =sg_dma_len (cur); + }drivers/crypto/picoxcell_crypto.c line 324
Quoted 10 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+ + ddt[mapped_ents].p =0 ; + ddt[mapped_ents].len =0 ; + }else { +dma_unmap_sg (engine->dev, payload, nents, dir); + ddt =NULL ; + } + +return ddt; +}easier to read would be:
if (mapped_ents + 1 > MAX_DDT_LEN) goto out; ddt = dma_pool_alloc(engine->req_pool, GFP_ATOMIC, ddt_phys); if (!ddt) goto out; for_each_sg(payload, cur, mapped_ents, i) { ddt[i].p = sg_dma_address(cur); ddt[i].len = sg_dma_len(cur); } ddt[mapped_ents].p = 0; ddt[mapped_ents].len = 0; return ddt;
Quoted 7 lines
out:
dma_unmap_sg(engine->dev, payload, nents, dir); return NULL; }even more so by moving
ddt_set()above it, and then usingddt_set()to assign the p, len pairs.
Yes, that's much cleaner.
Quoted 4 lines
Quoted 2 lines
+static inline void ddt_set(struct
spacc_ddt*ddt, unsigned long phys,phys should be
dma_addr_t
Ok.
Quoted 13 lines
drivers/crypto/picoxcell_crypto.c line 340
Quoted 9 lines
drivers/crypto/picoxcell_crypto.c new file +8
-0+static int spacc_aead_make_ddts (struct spacc_req *req,u8 *giv) +{ + struct aead_request *areq = container_of(req->req, struct aead_request, + base); +struct spacc_alg *alg =to_spacc_alg (req->req->tfm->__crt_alg); +struct spacc_engine *engine = req->engine; +struct spacc_ddt *src_ddt, *dst_ddt; +unsigned ivsize = alg->alg.cra_aead.ivsize;no need to go through all those hoops to get to the ivsize - use helper fns
crypto_aead_reqtfm()andcrypto_aead_ivsize(), as is done at the callsite, or just pass it in from there.
Ok, will do.
Quoted 15 lines
drivers/crypto/picoxcell_crypto.c line 467
Quoted 11 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+static int spacc_aead_des_setkey (struct crypto_aead *aead,const u8 *key, +unsigned int len) +{ +struct crypto_tfm *tfm =crypto_aead_tfm (aead); +struct spacc_aead_ctx *ctx =crypto_tfm_ctx (tfm); +int err =0 ; +u32 tmp[DES_EXPKEY_WORDS ]; + + err =des_ekey (tmp, key); + if (unlikely(!err) &&might want to change the name of the variable err here to something like ret or
is_weakso as to not mislead the reader.
Quoted 13 lines
drivers/crypto/picoxcell_crypto.c line 477
Quoted 10 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+ (crypto_aead_get_flags(aead)) & CRYPTO_TFM_REQ_WEAK_KEY) { + tfm->crt_flags |=CRYPTO_TFM_RES_WEAK_KEY ; +return -EINVAL ; + } + err =0 ; + +memcpy (ctx->cipher_key, key, len); + ctx->cipher_key_len = len; + +return err;actually, it doesn't look like this fn needs a return variable at all.
Ok, I'll get rid of err and put the des_ekey() call into the conditional.
Quoted 9 lines
Quoted 9 lines
+/* Set the key for the AES block cipher component of the AEAD transform. */
drivers/crypto/picoxcell_crypto.c new file +7
-0+static int spacc_aead_aes_setkey (struct crypto_aead *aead,const u8 *key, +unsigned int len) +{ +struct crypto_tfm *tfm =crypto_aead_tfm (aead); +struct spacc_aead_ctx *ctx =crypto_tfm_ctx (tfm); +int err; +
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c line 493
Quoted 6 lines
drivers/crypto/picoxcell_crypto.c new file +6
-0+/* +* IPSec engine only supports 128 and 256 bit AES keys. If we get a +* request for any other size (192 bits) then we need to do a software +* fallback. +*/ +if (!(16 == len ||32 == len)) {if (len !=
AES_KEYSIZE_128&& len !=AES_KEYSIZE_256)
Ok, I'll clean up all of these uses.
Quoted 9 lines
drivers/crypto/picoxcell_crypto.c line 500
Quoted 7 lines
drivers/crypto/picoxcell_crypto.c new file +7
-0+/* +* Set the fallback transform to use the same request flags as +* the hardware transform. +*/ + ctx->sw_cipher->base.crt_flags &= ~CRYPTO_TFM_REQ_MASK ; + ctx->sw_cipher->base.crt_flags |= + (tfm->crt_flags &CRYPTO_TFM_REQ_MASK );parens not needed.
Ok.
Quoted 9 lines
drivers/crypto/picoxcell_crypto.c line 508
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c new file +8
-0+ err =crypto_aead_setkey (ctx->sw_cipher, key, len); + }else { +memcpy (ctx->cipher_key, key, len); + ctx->cipher_key_len = len; + err =0 ; + } + +return err;
Quoted 6 lines
return crypto_aead_setkey(ctx->sw_cipher, key, len); }
memcpy(ctx->cipher_key, key, len);ctx->cipher_key_len= len;return 0;
Ok.
Quoted 23 lines
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c new file +8
-0+static int spacc_aead_setkey (struct crypto_aead *tfm,const u8 *key, +unsigned int keylen) +{ +struct spacc_aead_ctx *ctx =crypto_aead_ctx (tfm); +struct spacc_alg *alg =to_spacc_alg (tfm->base.__crt_alg); +struct rtattr *rta = (void *)key; +struct crypto_authenc_key_param *param; +unsigned int authkeylen, enckeylen;drivers/crypto/picoxcell_crypto.c line 528
Quoted 10 lines
drivers/crypto/picoxcell_crypto.c new file +10
-0+int err = -EINVAL ; + +if (!RTA_OK (rta, keylen)) + goto badkey; + +if (rta->rta_type !=CRYPTO_AUTHENC_KEYA_PARAM ) + goto badkey; + +if (RTA_PAYLOAD (rta) <sizeof (*param)) + goto badkey;I'm not sure, but it should be safe to remove the above three checks - they cause a false badkey failure if the keys aren't within an rtattr struct, which, e.g., something like
testmgr.cwouldn't do.
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c line 534
Quoted 5 lines
drivers/crypto/picoxcell_crypto.c new file +5
-0+ param =RTA_DATA (rta); + enckeylen =be32_to_cpu (param->enckeylen); + + key +=RTA_ALIGN (rta->rta_len); + keylen -=RTA_ALIGN (rta->rta_len);actually, I doubt crypto drivers should be including
rtnetlink.hat all...but it's probably ok for now - talitos still does :)
Yes, it doesn't seem the nicest way to pass the keys. ixp4xx and crypto/authenc.c do the same thing (including the first 3 checks). Perhaps this is worth refactoring into a generic crypto_authenc_get_keys() helper?
Quoted 5 lines
drivers/crypto/picoxcell_crypto.c line 593
Quoted 3 lines
drivers/crypto/picoxcell_crypto.c new file +3
-0+if ((spacc_alg->ctrl_default &SPACC_CRYPTO_ALG_MASK ) == +SPA_CTRL_CIPH_ALG_AES && + !(16 == ctx->cipher_key_len ||32 == ctx->cipher_key_len))as above, please use symbolic equivalents
Ok.
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c line 627
Quoted 6 lines
drivers/crypto/picoxcell_crypto.c new file +6
-0+static void spacc_aead_complete (struct spacc_req *req) +{ +spacc_aead_free_ddts (req); + +if (req->req->complete) + req->req->complete (req->req, req->result);when is there not a completion function?
Ok, I'll remove that check.
Quoted 5 lines
drivers/crypto/picoxcell_crypto.c line 648
Quoted 3 lines
drivers/crypto/picoxcell_crypto.c new file +3
-0+/* Set the source and destination DDT pointers. */ +writel ((u32 )req->src_addr, engine->regs +SPA_SRC_PTR_REG_OFFSET ); +writel ((u32 )req->dst_addr, engine->regs +SPA_DST_PTR_REG_OFFSET );cast necessary?
No, probably not. I'll double check and remove if ok.
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c line 683
Quoted 7 lines
drivers/crypto/picoxcell_crypto.c new file +7
-0+ ctrl = spacc_alg->ctrl_default; + ctrl |= ((req->ctx_id <<SPA_CTRL_CTX_IDX ) | + (1 <<SPA_CTRL_ICV_APPEND ) | + (req->is_encrypt ? (1 <<SPA_CTRL_ENCRYPT_IDX ) :0 ) | + (req->is_encrypt ? (1 <<SPA_CTRL_AAD_COPY ) :0 )); +if (!req->is_encrypt) + ctrl |= (1 <<SPA_CTRL_KEY_EXP );
Quoted 7 lines
ctrl =
spacc_alg->ctrl_default| (req->ctx_id<<SPA_CTRL_CTX_IDX) |(1 << SPA_CTRL_ICV_APPEND);if (
req->is_encrypt)ctrl |= (1 << SPA_CTRL_ENCRYPT_IDX) | (1 << SPA_CTRL_AAD_COPY); else ctrl |= (1 << SPA_CTRL_KEY_EXP);
Yes, that's nicer.
Quoted 11 lines
drivers/crypto/picoxcell_crypto.c line 841
Quoted 9 lines
drivers/crypto/picoxcell_crypto.c new file +9
-0+static int spacc_des_setkey (struct crypto_ablkcipher *cipher,const u8 *key, +unsigned int len) +{ +struct crypto_tfm *tfm =crypto_ablkcipher_tfm (cipher); +struct spacc_ablk_ctx *ctx =crypto_tfm_ctx (tfm); +int err; +u32 tmp[DES_EXPKEY_WORDS ]; + +if (len >SPACC_CRYPTO_AES_MAX_KEY_LEN ) {AES left overs in a DES setkey
Good spot, will fix.
Quoted 12 lines
Quoted 12 lines
drivers/crypto/picoxcell_crypto.c new file +12
-0+static int spacc_aes_setkey (struct crypto_ablkcipher *cipher,const u8 *key, +unsigned int len) +{ +struct crypto_tfm *tfm =crypto_ablkcipher_tfm (cipher); +struct spacc_ablk_ctx *ctx =crypto_tfm_ctx (tfm); +int err =0 ; + +if (len >SPACC_CRYPTO_AES_MAX_KEY_LEN ) { +crypto_ablkcipher_set_flags (cipher,CRYPTO_TFM_RES_BAD_KEY_LEN ); +return -EINVAL ; + } +
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c line 880
Quoted 6 lines
drivers/crypto/picoxcell_crypto.c new file +6
-0+/* +* IPSec engine only supports 128 and 256 bit AES keys. If we get a +* request for any other size (192 bits) then we need to do a software +* fallback. +*/ +if (!(16 == len ||32 == len) && ctx->sw_cipher) {symbolic constants
Ok.
Quoted 9 lines
drivers/crypto/picoxcell_crypto.c line 887
Quoted 7 lines
drivers/crypto/picoxcell_crypto.c new file +7
-0+/* +* Set the fallback transform to use the same request flags as +* the hardware transform. +*/ + ctx->sw_cipher->base.crt_flags &= ~CRYPTO_TFM_REQ_MASK ; + ctx->sw_cipher->base.crt_flags |= + (cipher->base.crt_flags &CRYPTO_TFM_REQ_MASK );parens not necessary
Ok.
Quoted 6 lines
Quoted 6 lines
drivers/crypto/picoxcell_crypto.c new file +6
-0+static int spacc_ablk_need_fallback (struct spacc_req *req) +{ +struct spacc_ablk_ctx *ctx; +struct crypto_tfm *tfm = req->req->tfm; +struct crypto_alg *alg = req->req->tfm->__crt_alg; +struct spacc_alg *spacc_alg =to_spacc_alg (alg);
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c line 939
Quoted 6 lines
drivers/crypto/picoxcell_crypto.c new file +6
-0+ + ctx =crypto_tfm_ctx (tfm); + +return (spacc_alg->ctrl_default &SPACC_CRYPTO_ALG_MASK ) == +SPA_CTRL_CIPH_ALG_AES && + !(16 == ctx->key_len ||32 == ctx->key_len);symbolic constants
Ok.
Quoted 8 lines
drivers/crypto/picoxcell_crypto.c line 1320
Quoted 6 lines
drivers/crypto/picoxcell_crypto.c new file +6
-0+static ssize_t spacc_stat_irq_thresh_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t len) +{ +struct spacc_engine *engine =spacc_dev_to_engine (dev); +unsigned thresh =simple_strtoul (buf,NULL ,0 );consider using
strict_strtoul(checkpatch)
Ok, will change.
Quoted 14 lines
drivers/crypto/picoxcell_crypto.c line 1345
Quoted 13 lines
drivers/crypto/picoxcell_crypto.c new file +13
-0+static struct spacc_alg ipsec_engine_algs[] = { + { + .ctrl_default =SPA_CTRL_CIPH_ALG_AES |SPA_CTRL_CIPH_MODE_CBC , + .key_offs =0 , + .iv_offs =SPACC_CRYPTO_AES_MAX_KEY_LEN , + .alg = { + .cra_name ="cbc(aes)" , + .cra_driver_name ="cbc-aes-picoxcell" , + .cra_priority =SPACC_CRYPTO_ALG_PRIORITY , + .cra_flags =CRYPTO_ALG_TYPE_ABLKCIPHER | +CRYPTO_ALG_ASYNC | +CRYPTO_ALG_NEED_FALLBACK , + .cra_blocksize =16 ,
Quoted 1 line
symbolic constant, here and throughout the rest of this section.
Ok.
Thanks again for taking the time to review Kim!
Jamie