Re: CAAM: kernel BUG at drivers/crypto/caam/jr.c:230! (and dma-coherent query)
On Wed, Mar 03, 2021 at 12:26:32PM +0200, Horia Geantă wrote: > Adding some people in the loop, maybe they could help in understanding > why lack of "dma-coherent" property for a HW-coherent device could lead to > unexpected / strange side effects. > > On 3/1/2021 5:22 PM, Sascha Hauer wrote: > > Hi All, > > > > I am on a Layerscape LS1046a using Linux-5.11. The CAAM driver sometimes > > crashes during the run-time self tests with: > > > >> kernel BUG at drivers/crypto/caam/jr.c:247! > >> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP > >> Modules linked in: > >> CPU: 0 PID: 0 Comm: swapper/0 Not tainted > >> 5.11.0-20210225-3-00039-g434215968816-dirty #12 > >> Hardware name: TQ TQMLS1046A SoM on Arkona AT1130 (C300) board (DT) > >> pstate: 6005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > >> pc : caam_jr_dequeue+0x98/0x57c > >> lr : caam_jr_dequeue+0x98/0x57c > >> sp : 800010003d50 > >> x29: 800010003d50 x28: 8000118d4000 > >> x27: 8000118d4328 x26: 01f0 > >> x25: 0008022be480 x24: 0008022c6410 > >> x23: 01f1 x22: 8000118d4329 > >> x21: 4d80 x20: 01f1 > >> x19: 0001 x18: 0020 > >> x17: x16: 0015 > >> x15: 800011690230 x14: 2e2e2e2e2e2e2e2e > >> x13: 2e2e2e2e2e2e2020 x12: 3030303030303030 > >> x11: 800011700a38 x10: f000 > >> x9 : 8000100ada30 x8 : 8000116a8a38 > >> x7 : 0001 x6 : > >> x5 : x4 : > >> x3 : x2 : > >> x1 : x0 : 1800 > >> Call trace: > >> caam_jr_dequeue+0x98/0x57c > >> tasklet_action_common.constprop.0+0x164/0x18c > >> tasklet_action+0x44/0x54 > >> __do_softirq+0x160/0x454 > >> __irq_exit_rcu+0x164/0x16c > >> irq_exit+0x1c/0x30 > >> __handle_domain_irq+0xc0/0x13c > >> gic_handle_irq+0x5c/0xf0 > >> el1_irq+0xb4/0x180 > >> arch_cpu_idle+0x18/0x30 > >> default_idle_call+0x3c/0x1c0 > >> do_idle+0x23c/0x274 > >> cpu_startup_entry+0x34/0x70 > >> rest_init+0xdc/0xec > >> arch_call_rest_init+0x1c/0x28 > >> start_kernel+0x4ac/0x4e4 > >> Code: 91392021 912c2000 d377d8c6 97f24d96 (d421) > > > > The driver iterates over the descriptors in the output ring and matches them > > with the ones it has previously queued. If it doesn't find a matching > > descriptor it complains with the BUG_ON() seen above. What I see sometimes > > is > > that the address in the output ring is 0x0, the job status in this case is > > 0x4006 (meaning DECO Invalid KEY command). It seems that the CAAM > > doesn't > > write the descriptor address to the output ring at least in some error > > cases. > > When we don't have the descriptor address of the failed descriptor we have > > no > > way to find it in the list of queued descriptors, thus we also can't find > > the > > callback for that descriptor. This looks very unfortunate, anyone else seen > > this or has an idea what to do about it? > > > > I haven't investigated yet which job actually fails and why. Of course that > > would > > be my ultimate goal to find that out. > > > This looks very similar to an earlier report from Greg. > He confirmed that adding "dma-coherent" property to the "crypto" DT node > fixes the issue: > https://lore.kernel.org/linux-crypto/74f664f5-5433-d322-4789-3c78bdb81...@kernel.org > Patch rebased on v5.11 is at the bottom. Does it work for you too? Indeed this seems to solve it for me as well, you can add my Tested-by: Sascha Hauer However, there seem to be two problems: First that "DECO Invalid KEY command" actually occurs and second that the deqeueue code currently can't handle a NULL pointer in the output ring. Do you think that the occurence of a NULL pointer is also a coherency issue? Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
CAAM: kernel BUG at drivers/crypto/caam/jr.c:230!
Hi All, I am on a Layerscape LS1046a using Linux-5.11. The CAAM driver sometimes crashes during the run-time self tests with: > kernel BUG at drivers/crypto/caam/jr.c:247! > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP > Modules linked in: > CPU: 0 PID: 0 Comm: swapper/0 Not tainted > 5.11.0-20210225-3-00039-g434215968816-dirty #12 > Hardware name: TQ TQMLS1046A SoM on Arkona AT1130 (C300) board (DT) > pstate: 6005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > pc : caam_jr_dequeue+0x98/0x57c > lr : caam_jr_dequeue+0x98/0x57c > sp : 800010003d50 > x29: 800010003d50 x28: 8000118d4000 > x27: 8000118d4328 x26: 01f0 > x25: 0008022be480 x24: 0008022c6410 > x23: 01f1 x22: 8000118d4329 > x21: 4d80 x20: 01f1 > x19: 0001 x18: 0020 > x17: x16: 0015 > x15: 800011690230 x14: 2e2e2e2e2e2e2e2e > x13: 2e2e2e2e2e2e2020 x12: 3030303030303030 > x11: 800011700a38 x10: f000 > x9 : 8000100ada30 x8 : 8000116a8a38 > x7 : 0001 x6 : > x5 : x4 : > x3 : x2 : > x1 : x0 : 1800 > Call trace: > caam_jr_dequeue+0x98/0x57c > tasklet_action_common.constprop.0+0x164/0x18c > tasklet_action+0x44/0x54 > __do_softirq+0x160/0x454 > __irq_exit_rcu+0x164/0x16c > irq_exit+0x1c/0x30 > __handle_domain_irq+0xc0/0x13c > gic_handle_irq+0x5c/0xf0 > el1_irq+0xb4/0x180 > arch_cpu_idle+0x18/0x30 > default_idle_call+0x3c/0x1c0 > do_idle+0x23c/0x274 > cpu_startup_entry+0x34/0x70 > rest_init+0xdc/0xec > arch_call_rest_init+0x1c/0x28 > start_kernel+0x4ac/0x4e4 > Code: 91392021 912c2000 d377d8c6 97f24d96 (d421) The driver iterates over the descriptors in the output ring and matches them with the ones it has previously queued. If it doesn't find a matching descriptor it complains with the BUG_ON() seen above. What I see sometimes is that the address in the output ring is 0x0, the job status in this case is 0x4006 (meaning DECO Invalid KEY command). It seems that the CAAM doesn't write the descriptor address to the output ring at least in some error cases. When we don't have the descriptor address of the failed descriptor we have no way to find it in the list of queued descriptors, thus we also can't find the callback for that descriptor. This looks very unfortunate, anyone else seen this or has an idea what to do about it? I haven't investigated yet which job actually fails and why. Of course that would be my ultimate goal to find that out. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] ubifs: fix wrong use of crypto_shash_descsize()
On Fri, May 01, 2020 at 10:59:45PM -0700, Eric Biggers wrote:
> From: Eric Biggers
>
> crypto_shash_descsize() returns the size of the shash_desc context
> needed to compute the hash, not the size of the hash itself.
>
> crypto_shash_digestsize() would be correct, or alternatively using
> c->hash_len and c->hmac_desc_len which already store the correct values.
> But actually it's simpler to just use stack arrays, so do that instead.
>
> Fixes: 49525e5eecca ("ubifs: Add helper functions for authentication support")
> Fixes: da8ef65f9573 ("ubifs: Authenticate replayed journal")
> Cc: # v4.20+
> Cc: Sascha Hauer
> Signed-off-by: Eric Biggers
Looks better that way, thanks.
Acked-by: Sascha Hauer
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: ctr(aes) broken in CAAM driver
On Wed, Jun 12, 2019 at 01:35:36PM +0200, Sascha Hauer wrote: > On Wed, Jun 12, 2019 at 10:33:56AM +, Horia Geanta wrote: > > On 6/12/2019 12:40 PM, Sascha Hauer wrote: > > > Hi Horia, > > > > > > On Wed, May 15, 2019 at 01:35:16PM +, Horia Geanta wrote: > > >> For talitos, the problem is the lack of IV update. > > >> > > >> For caam, the problem is incorrect IV update (output IV is equal to last > > >> ciphertext block, which is correect for cbc, but not for ctr mode). > > >> > > >> I am working at a fix, but it takes longer since I would like to program > > >> the > > >> accelerator to the save the IV (and not do counter increment in SW, which > > >> created problems for many other implementations). > > > > > > Any news here? With the fix Ard provided gcm(aes) now works again, but > > > only as long as the crypto self tests are disabled. > > > > > I've recently submitted support for IV update done in HW (caam engine), > > which fixes this issue: > > https://patchwork.kernel.org/cover/10984927/ > > Thanks, I haven't seen this. I'll give it a try. This works here, thanks I don't have the original patch mails, so I'm adding it here: Tested-by: Sascha Hauer Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: ctr(aes) broken in CAAM driver
On Wed, Jun 12, 2019 at 10:33:56AM +, Horia Geanta wrote: > On 6/12/2019 12:40 PM, Sascha Hauer wrote: > > Hi Horia, > > > > On Wed, May 15, 2019 at 01:35:16PM +, Horia Geanta wrote: > >> For talitos, the problem is the lack of IV update. > >> > >> For caam, the problem is incorrect IV update (output IV is equal to last > >> ciphertext block, which is correect for cbc, but not for ctr mode). > >> > >> I am working at a fix, but it takes longer since I would like to program > >> the > >> accelerator to the save the IV (and not do counter increment in SW, which > >> created problems for many other implementations). > > > > Any news here? With the fix Ard provided gcm(aes) now works again, but > > only as long as the crypto self tests are disabled. > > > I've recently submitted support for IV update done in HW (caam engine), > which fixes this issue: > https://patchwork.kernel.org/cover/10984927/ Thanks, I haven't seen this. I'll give it a try. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: ctr(aes) broken in CAAM driver
Hi Horia, On Wed, May 15, 2019 at 01:35:16PM +, Horia Geanta wrote: > For talitos, the problem is the lack of IV update. > > For caam, the problem is incorrect IV update (output IV is equal to last > ciphertext block, which is correect for cbc, but not for ctr mode). > > I am working at a fix, but it takes longer since I would like to program the > accelerator to the save the IV (and not do counter increment in SW, which > created problems for many other implementations). Any news here? With the fix Ard provided gcm(aes) now works again, but only as long as the crypto self tests are disabled. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v2 0/4] crypto: CAAM: Print debug messages at debug level
The CAAM driver has most of its debug messages inside #ifdef DEBUG and then prints them at KERN_ERR level. Do this properly and print the messages at DEBUG_LEVEL as they are supposed to. With this we can get rid of a lot of ifdefs in the code. Sascha Changes since v1: - Fix alignment on following lines when converting print_hex_dump to print_hex_dump_debug - Add 1/4 to avoid crash when debugging is enabled Sascha Hauer (4): crypto: caam: print IV only when non NULL crypto: caam: remove unused defines crypto: caam: print debug messages at debug level crypto: caam: print messages in caam_dump_sg at debug level Sascha Hauer (4): crypto: caam: print IV only when non NULL crypto: caam: remove unused defines crypto: caam: print debug messages at debug level crypto: caam: print messages in caam_dump_sg at debug level drivers/crypto/caam/caamalg.c | 161 drivers/crypto/caam/caamalg_desc.c | 116 ++ drivers/crypto/caam/caamalg_qi.c | 56 +++ drivers/crypto/caam/caamalg_qi2.c | 4 +- drivers/crypto/caam/caamhash.c | 233 - drivers/crypto/caam/caamrng.c | 22 ++- drivers/crypto/caam/error.c| 8 +- drivers/crypto/caam/error.h| 2 +- drivers/crypto/caam/key_gen.c | 28 ++-- drivers/crypto/caam/sg_sw_sec4.h | 8 +- 10 files changed, 253 insertions(+), 385 deletions(-) -- 2.20.1
[PATCH 1/4] crypto: caam: print IV only when non NULL
Since eaed71a44ad9 ("crypto: caam - add ecb(*) support") the IV can be
NULL, so only dump it when it's non NULL as designated by the ivsize
variable.
Fixes: eaed71a44ad9 ("crypto: caam - add ecb(*) support")
Signed-off-by: Sascha Hauer
---
drivers/crypto/caam/caamalg.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index c0ece44f303b..b5415c74 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -1011,9 +1011,10 @@ static void skcipher_encrypt_done(struct device *jrdev,
u32 *desc, u32 err,
caam_jr_strstatus(jrdev, err);
#ifdef DEBUG
- print_hex_dump(KERN_ERR, "dstiv @"__stringify(__LINE__)": ",
- DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
- edesc->src_nents > 1 ? 100 : ivsize, 1);
+ if (ivsize)
+ print_hex_dump(KERN_ERR, "dstiv @"__stringify(__LINE__)": ",
+ DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
+ edesc->src_nents > 1 ? 100 : ivsize, 1);
#endif
caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
--
2.20.1
[PATCH 3/4] crypto: caam: print debug messages at debug level
The CAAM driver used to put its debug messages inside #ifdef DEBUG and
then prints the messages at KERN_ERR level. Replace this with proper
functions printing at KERN_DEBUG level. The #ifdef DEBUG gets
unnecessary when the right functions are used.
This replaces:
- print_hex_dump(KERN_ERR ...) inside #ifdef DEBUG with
print_hex_dump_debug(...)
- dev_err() inside #ifdef DEBUG with dev_dbg()
- printk(KERN_ERR ...) inside #ifdef DEBUG with dev_dbg()
Some parts of the driver use these functions already, so it is only
consequent to use the debug function consistently.
Signed-off-by: Sascha Hauer
---
drivers/crypto/caam/caamalg.c | 145 ---
drivers/crypto/caam/caamalg_desc.c | 116 ++-
drivers/crypto/caam/caamalg_qi.c | 54 +++
drivers/crypto/caam/caamhash.c | 225 -
drivers/crypto/caam/caamrng.c | 22 ++-
drivers/crypto/caam/key_gen.c | 28 ++--
drivers/crypto/caam/sg_sw_sec4.h | 8 +-
7 files changed, 240 insertions(+), 358 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index fb2994f3b18f..30fed464fb3f 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -576,13 +576,11 @@ static int aead_setkey(struct crypto_aead *aead,
if (crypto_authenc_extractkeys(&keys, key, keylen) != 0)
goto badkey;
-#ifdef DEBUG
- printk(KERN_ERR "keylen %d enckeylen %d authkeylen %d\n",
+ dev_dbg(jrdev, "keylen %d enckeylen %d authkeylen %d\n",
keys.authkeylen + keys.enckeylen, keys.enckeylen,
keys.authkeylen);
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
- DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#endif
+ print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
+DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
/*
* If DKP is supported, use it in the shared descriptor to generate
@@ -616,11 +614,10 @@ static int aead_setkey(struct crypto_aead *aead,
memcpy(ctx->key + ctx->adata.keylen_pad, keys.enckey, keys.enckeylen);
dma_sync_single_for_device(jrdev, ctx->key_dma, ctx->adata.keylen_pad +
keys.enckeylen, ctx->dir);
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "ctx.key@"__stringify(__LINE__)": ",
- DUMP_PREFIX_ADDRESS, 16, 4, ctx->key,
- ctx->adata.keylen_pad + keys.enckeylen, 1);
-#endif
+
+ print_hex_dump_debug("ctx.key@"__stringify(__LINE__)": ",
+DUMP_PREFIX_ADDRESS, 16, 4, ctx->key,
+ctx->adata.keylen_pad + keys.enckeylen, 1);
skip_split_key:
ctx->cdata.keylen = keys.enckeylen;
@@ -671,10 +668,8 @@ static int gcm_setkey(struct crypto_aead *aead,
struct caam_ctx *ctx = crypto_aead_ctx(aead);
struct device *jrdev = ctx->jrdev;
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
- DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#endif
+ print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
+DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
memcpy(ctx->key, key, keylen);
dma_sync_single_for_device(jrdev, ctx->key_dma, keylen, ctx->dir);
@@ -692,10 +687,8 @@ static int rfc4106_setkey(struct crypto_aead *aead,
if (keylen < 4)
return -EINVAL;
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
- DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#endif
+ print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
+DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
memcpy(ctx->key, key, keylen);
@@ -718,10 +711,8 @@ static int rfc4543_setkey(struct crypto_aead *aead,
if (keylen < 4)
return -EINVAL;
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
- DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#endif
+ print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
+DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
memcpy(ctx->key, key, keylen);
@@ -750,10 +741,8 @@ static int skcipher_setkey(struct crypto_skcipher
*skcipher, const u8 *key,
OP_ALG_AAI_CTR_MOD128);
const bool is_rfc3686 = alg->caam.rfc3686;
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
- DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#en
[PATCH 4/4] crypto: caam: print messages in caam_dump_sg at debug level
caam_dump_sg() is only compiled in when DEBUG is defined, hence the
messages are debug messages. Remove the @level argument from
caam_dump_sg() and print all messages at debug level.
Signed-off-by: Sascha Hauer
---
drivers/crypto/caam/caamalg.c | 8
drivers/crypto/caam/caamalg_qi.c | 2 +-
drivers/crypto/caam/caamalg_qi2.c | 4 ++--
drivers/crypto/caam/error.c | 8
drivers/crypto/caam/error.h | 2 +-
5 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 30fed464fb3f..73c0627c265d 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -991,7 +991,7 @@ static void skcipher_encrypt_done(struct device *jrdev, u32
*desc, u32 err,
DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
edesc->src_nents > 1 ? 100 : ivsize, 1);
- caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
@@ -1026,7 +1026,7 @@ static void skcipher_decrypt_done(struct device *jrdev,
u32 *desc, u32 err,
print_hex_dump_debug("dstiv @"__stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->iv, ivsize, 1);
- caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
@@ -1234,7 +1234,7 @@ static void init_skcipher_job(struct skcipher_request
*req,
dev_dbg(jrdev, "asked=%d, cryptlen%d\n",
(int)edesc->src_nents > 1 ? 100 : req->cryptlen, req->cryptlen);
- caam_dump_sg(KERN_ERR, "src@" __stringify(__LINE__)": ",
+ caam_dump_sg("src@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->src,
edesc->src_nents > 1 ? 100 : req->cryptlen, 1);
@@ -1596,7 +1596,7 @@ static int aead_decrypt(struct aead_request *req)
u32 *desc;
int ret = 0;
- caam_dump_sg(KERN_ERR, "dec src@" __stringify(__LINE__)": ",
+ caam_dump_sg("dec src@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->src,
req->assoclen + req->cryptlen, 1);
diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c
index 44642058b5ac..621bdf1a3b9d 100644
--- a/drivers/crypto/caam/caamalg_qi.c
+++ b/drivers/crypto/caam/caamalg_qi.c
@@ -1182,7 +1182,7 @@ static void skcipher_done(struct caam_drv_req *drv_req,
u32 status)
print_hex_dump_debug("dstiv @" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
edesc->src_nents > 1 ? 100 : ivsize, 1);
- caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
diff --git a/drivers/crypto/caam/caamalg_qi2.c
b/drivers/crypto/caam/caamalg_qi2.c
index 2b2980a8a9b9..600948ba4c71 100644
--- a/drivers/crypto/caam/caamalg_qi2.c
+++ b/drivers/crypto/caam/caamalg_qi2.c
@@ -1324,7 +1324,7 @@ static void skcipher_encrypt_done(void *cbk_ctx, u32
status)
print_hex_dump_debug("dstiv @" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
edesc->src_nents > 1 ? 100 : ivsize, 1);
- caam_dump_sg(KERN_DEBUG, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
@@ -1362,7 +1362,7 @@ static void skcipher_decrypt_done(void *cbk_ctx, u32
status)
print_hex_dump_debug("dstiv @" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
edesc->src_nents > 1 ? 100 : ivsize, 1);
- caam_dump_sg(KERN_DEBUG, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req-&g
[PATCH 2/4] crypto: caam: remove unused defines
The CAAM driver defines its own debug() macro, but it is unused. Remove
it.
Signed-off-by: Sascha Hauer
Reviewed-by: Horia Geantă
---
drivers/crypto/caam/caamalg.c | 7 ---
drivers/crypto/caam/caamhash.c | 8
2 files changed, 15 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index b5415c74..fb2994f3b18f 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -77,13 +77,6 @@
#define DESC_MAX_USED_BYTES(CAAM_DESC_BYTES_MAX - DESC_JOB_IO_LEN)
#define DESC_MAX_USED_LEN (DESC_MAX_USED_BYTES / CAAM_CMD_SZ)
-#ifdef DEBUG
-/* for print_hex_dumps with line references */
-#define debug(format, arg...) printk(format, arg)
-#else
-#define debug(format, arg...)
-#endif
-
struct caam_alg_entry {
int class1_alg_type;
int class2_alg_type;
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 7205d9f4029e..8aee7c9bf862 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -82,14 +82,6 @@
#define HASH_MSG_LEN 8
#define MAX_CTX_LEN(HASH_MSG_LEN + SHA512_DIGEST_SIZE)
-#ifdef DEBUG
-/* for print_hex_dumps with line references */
-#define debug(format, arg...) printk(format, arg)
-#else
-#define debug(format, arg...)
-#endif
-
-
static struct list_head hash_list;
/* ahash per-session context */
--
2.20.1
Re: [PATCH 2/3] crypto: caam: print debug messages at debug level
On Fri, May 17, 2019 at 11:29:04AM +0200, Sascha Hauer wrote:
> The CAAM driver used to put its debug messages inside #ifdef DEBUG and
> then prints the messages at KERN_ERR level. Replace this with proper
> functions printing at KERN_DEBUG level. The #ifdef DEBUG gets
> unnecessary when the right functions are used.
>
> This replaces:
>
> - print_hex_dump(KERN_ERR ...) inside #ifdef DEBUG with
> print_hex_dump_debug(...)
> - dev_err() inside #ifdef DEBUG with dev_dbg()
> - printk(KERN_ERR ...) inside #ifdef DEBUG with dev_dbg()
>
> Some parts of the driver use these functions already, so it is only
> consequent to use the debug function consistently.
>
> @@ -993,20 +978,17 @@ static void skcipher_encrypt_done(struct device *jrdev,
> u32 *desc, u32 err,
> struct crypto_skcipher *skcipher = crypto_skcipher_reqtfm(req);
> int ivsize = crypto_skcipher_ivsize(skcipher);
>
> -#ifdef DEBUG
> - print_hex_dump(KERN_ERR, "dstiv @"__stringify(__LINE__)": ",
> + print_hex_dump_debug("dstiv @"__stringify(__LINE__)": ",
> DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
> edesc->src_nents > 1 ? 100 : ivsize, 1);
> -#endif
> +
I just realized that this print_hex_dump_debug() needs to be inside if (ivsize)
because since eaed71a44ad9 ("crypto: caam - add ecb(*) support") req->iv
can be NULL. This is broken with or without this patch, I can include a
patch fixing this up when doing a v2.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH 2/3] crypto: caam: print debug messages at debug level
The CAAM driver used to put its debug messages inside #ifdef DEBUG and
then prints the messages at KERN_ERR level. Replace this with proper
functions printing at KERN_DEBUG level. The #ifdef DEBUG gets
unnecessary when the right functions are used.
This replaces:
- print_hex_dump(KERN_ERR ...) inside #ifdef DEBUG with
print_hex_dump_debug(...)
- dev_err() inside #ifdef DEBUG with dev_dbg()
- printk(KERN_ERR ...) inside #ifdef DEBUG with dev_dbg()
Some parts of the driver use these functions already, so it is only
consequent to use the debug function consistently.
Signed-off-by: Sascha Hauer
---
drivers/crypto/caam/caamalg.c | 95 --
drivers/crypto/caam/caamalg_desc.c | 71
drivers/crypto/caam/caamalg_qi.c | 36 +++--
drivers/crypto/caam/caamhash.c | 126 +
drivers/crypto/caam/caamrng.c | 16 ++--
drivers/crypto/caam/key_gen.c | 19 ++---
drivers/crypto/caam/sg_sw_sec4.h | 5 +-
7 files changed, 113 insertions(+), 255 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 007c35cfc670..1395b4860f23 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -575,13 +575,11 @@ static int aead_setkey(struct crypto_aead *aead,
if (crypto_authenc_extractkeys(&keys, key, keylen) != 0)
goto badkey;
-#ifdef DEBUG
- printk(KERN_ERR "keylen %d enckeylen %d authkeylen %d\n",
+ dev_dbg(jrdev, "keylen %d enckeylen %d authkeylen %d\n",
keys.authkeylen + keys.enckeylen, keys.enckeylen,
keys.authkeylen);
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
+ print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#endif
/*
* If DKP is supported, use it in the shared descriptor to generate
@@ -615,11 +613,10 @@ static int aead_setkey(struct crypto_aead *aead,
memcpy(ctx->key + ctx->adata.keylen_pad, keys.enckey, keys.enckeylen);
dma_sync_single_for_device(jrdev, ctx->key_dma, ctx->adata.keylen_pad +
keys.enckeylen, ctx->dir);
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "ctx.key@"__stringify(__LINE__)": ",
+
+ print_hex_dump_debug("ctx.key@"__stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, ctx->key,
ctx->adata.keylen_pad + keys.enckeylen, 1);
-#endif
skip_split_key:
ctx->cdata.keylen = keys.enckeylen;
@@ -670,10 +667,8 @@ static int gcm_setkey(struct crypto_aead *aead,
struct caam_ctx *ctx = crypto_aead_ctx(aead);
struct device *jrdev = ctx->jrdev;
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
+ print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#endif
memcpy(ctx->key, key, keylen);
dma_sync_single_for_device(jrdev, ctx->key_dma, keylen, ctx->dir);
@@ -691,10 +686,8 @@ static int rfc4106_setkey(struct crypto_aead *aead,
if (keylen < 4)
return -EINVAL;
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
+ print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#endif
memcpy(ctx->key, key, keylen);
@@ -717,10 +710,8 @@ static int rfc4543_setkey(struct crypto_aead *aead,
if (keylen < 4)
return -EINVAL;
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
+ print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#endif
memcpy(ctx->key, key, keylen);
@@ -749,10 +740,8 @@ static int skcipher_setkey(struct crypto_skcipher
*skcipher, const u8 *key,
OP_ALG_AAI_CTR_MOD128);
const bool is_rfc3686 = alg->caam.rfc3686;
-#ifdef DEBUG
- print_hex_dump(KERN_ERR, "key in @"__stringify(__LINE__)": ",
+ print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
-#endif
/*
* AES-CTR needs to load IV in CONTEXT1 reg
* at an offset of 128bits (16bytes)
@@ -941,9 +930,7 @@ static void aead_encrypt_done(struct device *jrdev, u32
*desc, u32 err,
struct aead_request *req = context;
struct aead_edesc *edesc;
-#ifdef DEBUG
- dev_err(jrdev, "%s %d: err
[PATCH 3/3] crypto: caam: print messages in caam_dump_sg at debug level
caam_dump_sg() is only compiled in when DEBUG is defined, hence the
messages are debug messages. Remove the @level argument from
caam_dump_sg() and print all messages at debug level.
Signed-off-by: Sascha Hauer
---
drivers/crypto/caam/caamalg.c | 8
drivers/crypto/caam/caamalg_qi.c | 2 +-
drivers/crypto/caam/caamalg_qi2.c | 4 ++--
drivers/crypto/caam/error.c | 6 +++---
drivers/crypto/caam/error.h | 2 +-
5 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 1395b4860f23..af2047fd9b30 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -989,7 +989,7 @@ static void skcipher_encrypt_done(struct device *jrdev, u32
*desc, u32 err,
DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
edesc->src_nents > 1 ? 100 : ivsize, 1);
- caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
@@ -1024,7 +1024,7 @@ static void skcipher_decrypt_done(struct device *jrdev,
u32 *desc, u32 err,
print_hex_dump_debug("dstiv @"__stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->iv, ivsize, 1);
- caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
@@ -1232,7 +1232,7 @@ static void init_skcipher_job(struct skcipher_request
*req,
dev_dbg(jrdev, "asked=%d, cryptlen%d\n",
(int)edesc->src_nents > 1 ? 100 : req->cryptlen, req->cryptlen);
- caam_dump_sg(KERN_ERR, "src@" __stringify(__LINE__)": ",
+ caam_dump_sg("src@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->src,
edesc->src_nents > 1 ? 100 : req->cryptlen, 1);
@@ -1594,7 +1594,7 @@ static int aead_decrypt(struct aead_request *req)
u32 *desc;
int ret = 0;
- caam_dump_sg(KERN_ERR, "dec src@" __stringify(__LINE__)": ",
+ caam_dump_sg("dec src@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->src,
req->assoclen + req->cryptlen, 1);
diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c
index 66df91c5a6eb..146bb06e6075 100644
--- a/drivers/crypto/caam/caamalg_qi.c
+++ b/drivers/crypto/caam/caamalg_qi.c
@@ -1181,7 +1181,7 @@ static void skcipher_done(struct caam_drv_req *drv_req,
u32 status)
print_hex_dump_debug("dstiv @" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
edesc->src_nents > 1 ? 100 : ivsize, 1);
- caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
diff --git a/drivers/crypto/caam/caamalg_qi2.c
b/drivers/crypto/caam/caamalg_qi2.c
index 33a4df6b81de..b897b1ad1218 100644
--- a/drivers/crypto/caam/caamalg_qi2.c
+++ b/drivers/crypto/caam/caamalg_qi2.c
@@ -1323,7 +1323,7 @@ static void skcipher_encrypt_done(void *cbk_ctx, u32
status)
print_hex_dump_debug("dstiv @" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
edesc->src_nents > 1 ? 100 : ivsize, 1);
- caam_dump_sg(KERN_DEBUG, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
@@ -1361,7 +1361,7 @@ static void skcipher_decrypt_done(void *cbk_ctx, u32
status)
print_hex_dump_debug("dstiv @" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
edesc->src_nents > 1 ? 100 : ivsize, 1);
- caam_dump_sg(KERN_DEBUG, "dst@" __stringify(__LINE__)": ",
+ caam_dump_sg("dst@" __stringify(__LINE__)": ",
DUMP_PREFIX_ADDRESS, 16, 4, req->dst,
edesc->dst_nents >
[PATCH 1/3] crypto: caam: remove unused defines
The CAAM driver defines its own debug() macro, but it is unused. Remove
it.
Signed-off-by: Sascha Hauer
---
drivers/crypto/caam/caamalg.c | 7 ---
drivers/crypto/caam/caamhash.c | 8
2 files changed, 15 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 3e23d4b2cce2..007c35cfc670 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -77,13 +77,6 @@
#define DESC_MAX_USED_BYTES(CAAM_DESC_BYTES_MAX - DESC_JOB_IO_LEN)
#define DESC_MAX_USED_LEN (DESC_MAX_USED_BYTES / CAAM_CMD_SZ)
-#ifdef DEBUG
-/* for print_hex_dumps with line references */
-#define debug(format, arg...) printk(format, arg)
-#else
-#define debug(format, arg...)
-#endif
-
struct caam_alg_entry {
int class1_alg_type;
int class2_alg_type;
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 7205d9f4029e..8aee7c9bf862 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -82,14 +82,6 @@
#define HASH_MSG_LEN 8
#define MAX_CTX_LEN(HASH_MSG_LEN + SHA512_DIGEST_SIZE)
-#ifdef DEBUG
-/* for print_hex_dumps with line references */
-#define debug(format, arg...) printk(format, arg)
-#else
-#define debug(format, arg...)
-#endif
-
-
static struct list_head hash_list;
/* ahash per-session context */
--
2.20.1
[PATCH 0/3] crypto: CAAM: Print debug messages at debug level
The CAAM driver has most of its debug messages inside #ifdef DEBUG and then prints them at KERN_ERR level. Do this properly and print the messages at DEBUG_LEVEL as they are supposed to. With this we can get rid of a lot of ifdefs in the code. Sascha Sascha Hauer (3): crypto: caam: remove unused defines crypto: caam: print debug messages at debug level crypto: caam: print messages in caam_dump_sg at debug level drivers/crypto/caam/caamalg.c | 110 --- drivers/crypto/caam/caamalg_desc.c | 71 --- drivers/crypto/caam/caamalg_qi.c | 38 +++- drivers/crypto/caam/caamalg_qi2.c | 4 +- drivers/crypto/caam/caamhash.c | 134 + drivers/crypto/caam/caamrng.c | 16 ++-- drivers/crypto/caam/error.c| 6 +- drivers/crypto/caam/error.h| 2 +- drivers/crypto/caam/key_gen.c | 19 ++-- drivers/crypto/caam/sg_sw_sec4.h | 5 +- 10 files changed, 124 insertions(+), 281 deletions(-) -- 2.20.1
Re: ctr(aes) broken in CAAM driver
On Wed, May 15, 2019 at 01:35:16PM +, Horia Geanta wrote: > On 5/15/2019 4:22 PM, Sascha Hauer wrote: > > Hi Fabio, > > > > On Wed, May 15, 2019 at 10:17:19AM -0300, Fabio Estevam wrote: > >> Hi Sascha, > >> > >> On Wed, May 15, 2019 at 10:09 AM Sascha Hauer > >> wrote: > >>> > >>> Hi, > >>> > >>> ctr(aes) is broken in current kernel (v5.1+). It may have been broken > >>> for longer, but the crypto tests now check for a correct output IV. The > >>> testmgr answers with: > >>> > >>> alg: skcipher: ctr-aes-caam encryption test failed (wrong output IV) on > >>> test vector 0, cfg="in-place" > >>> > >>> output IV is this, which is the last 16 bytes of the encrypted message: > >>> : 1e 03 1d da 2f be 03 d1 79 21 70 a0 f3 00 9c ee > >>> > >>> It should look like this instead, which is input IV + 4: > >>> : f0 f1 f2 f3 f4 f5 f6 f7 f8 f9 fa fb fc fd ff 03 > >>> > >>> I have no idea how to fix this as I don't know how to get the output IV > >>> back from the CAAM. Any ideas? > >> > >> Is this problem similar to this one? > >> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg37512.html > > > > Different algo, different hardware, but yes, it seems to be the same > > type of failure. > > > For talitos, the problem is the lack of IV update. > > For caam, the problem is incorrect IV update (output IV is equal to last > ciphertext block, which is correect for cbc, but not for ctr mode). > > I am working at a fix, but it takes longer since I would like to program the > accelerator to the save the IV (and not do counter increment in SW, which > created problems for many other implementations). Thanks for working on it. I'd be glad to test it once you have something. Thanks Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] crypto: caam: print debugging hex dumps after unmapping
For encryption the destination pointer was still mapped, so the hex dump may be wrong. The IV still contained the input IV while printing instead of the output IV as intended. For decryption the destination pointer was still mapped, so the hex dump may be wrong. The IV dump was correct. Do the hex dumps consistenly after the buffers have been unmapped and in case of IV copied to their final destination. Signed-off-by: Sascha Hauer --- drivers/crypto/caam/caamalg.c | 21 +++-- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c index 3e23d4b2cce2..a992ff56fd15 100644 --- a/drivers/crypto/caam/caamalg.c +++ b/drivers/crypto/caam/caamalg.c @@ -1009,15 +1009,6 @@ static void skcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, if (err) caam_jr_strstatus(jrdev, err); -#ifdef DEBUG - print_hex_dump(KERN_ERR, "dstiv @"__stringify(__LINE__)": ", - DUMP_PREFIX_ADDRESS, 16, 4, req->iv, - edesc->src_nents > 1 ? 100 : ivsize, 1); -#endif - caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ", -DUMP_PREFIX_ADDRESS, 16, 4, req->dst, -edesc->dst_nents > 1 ? 100 : req->cryptlen, 1); - skcipher_unmap(jrdev, edesc, req); /* @@ -1028,6 +1019,15 @@ static void skcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, scatterwalk_map_and_copy(req->iv, req->dst, req->cryptlen - ivsize, ivsize, 0); +#ifdef DEBUG + print_hex_dump(KERN_ERR, "dstiv @"__stringify(__LINE__)": ", + DUMP_PREFIX_ADDRESS, 16, 4, req->iv, + edesc->src_nents > 1 ? 100 : ivsize, 1); +#endif + caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ", +DUMP_PREFIX_ADDRESS, 16, 4, req->dst, +edesc->dst_nents > 1 ? 100 : req->cryptlen, 1); + kfree(edesc); skcipher_request_complete(req, err); @@ -1049,6 +1049,8 @@ static void skcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, if (err) caam_jr_strstatus(jrdev, err); + skcipher_unmap(jrdev, edesc, req); + #ifdef DEBUG print_hex_dump(KERN_ERR, "dstiv @"__stringify(__LINE__)": ", DUMP_PREFIX_ADDRESS, 16, 4, req->iv, ivsize, 1); @@ -1057,7 +1059,6 @@ static void skcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, DUMP_PREFIX_ADDRESS, 16, 4, req->dst, edesc->dst_nents > 1 ? 100 : req->cryptlen, 1); - skcipher_unmap(jrdev, edesc, req); kfree(edesc); skcipher_request_complete(req, err); -- 2.20.1
Re: [PATCH] crypto: caam: print debugging hex dumps after unmapping
On Wed, May 15, 2019 at 04:27:51PM +, Horia Geanta wrote:
> On 5/15/2019 4:13 PM, Sascha Hauer wrote:
> > The debugging hex dumps in skcipher_encrypt_done() and
> > skcipher_decrypt_done() are printed while the request is still DMA
> > mapped. This results in bogus hex dumps with things like mixtures
> > between plain text and cipher text. Unmap first before printing.
> >
> The description is not accurate.
> req->iv is no longer DMA mapped since commit 115957bb3e59 ("crypto: caam - fix
> IV DMA mapping and updating"), so this is not related to IV DMA unmapping vs.
> print order.
>
> Currently:
> -for encryption, printed req->iv contains the input IV; copy of output IV into
> req->iv is done further below
> -for decryption, printed req->iv should be correct, since output IV is copied
> into req->iv in skcipher_decrypt(), before accelerator runs
>
> Could you please resubmit with updated message?
Just did that.
Thanks
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: ctr(aes) broken in CAAM driver
Hi Fabio, On Wed, May 15, 2019 at 10:17:19AM -0300, Fabio Estevam wrote: > Hi Sascha, > > On Wed, May 15, 2019 at 10:09 AM Sascha Hauer wrote: > > > > Hi, > > > > ctr(aes) is broken in current kernel (v5.1+). It may have been broken > > for longer, but the crypto tests now check for a correct output IV. The > > testmgr answers with: > > > > alg: skcipher: ctr-aes-caam encryption test failed (wrong output IV) on > > test vector 0, cfg="in-place" > > > > output IV is this, which is the last 16 bytes of the encrypted message: > > : 1e 03 1d da 2f be 03 d1 79 21 70 a0 f3 00 9c ee > > > > It should look like this instead, which is input IV + 4: > > : f0 f1 f2 f3 f4 f5 f6 f7 f8 f9 fa fb fc fd ff 03 > > > > I have no idea how to fix this as I don't know how to get the output IV > > back from the CAAM. Any ideas? > > Is this problem similar to this one? > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg37512.html Different algo, different hardware, but yes, it seems to be the same type of failure. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] crypto: caam: print debugging hex dumps after unmapping
The debugging hex dumps in skcipher_encrypt_done() and skcipher_decrypt_done() are printed while the request is still DMA mapped. This results in bogus hex dumps with things like mixtures between plain text and cipher text. Unmap first before printing. Signed-off-by: Sascha Hauer --- drivers/crypto/caam/caamalg.c | 21 +++-- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c index 3e23d4b2cce2..a992ff56fd15 100644 --- a/drivers/crypto/caam/caamalg.c +++ b/drivers/crypto/caam/caamalg.c @@ -1009,15 +1009,6 @@ static void skcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, if (err) caam_jr_strstatus(jrdev, err); -#ifdef DEBUG - print_hex_dump(KERN_ERR, "dstiv @"__stringify(__LINE__)": ", - DUMP_PREFIX_ADDRESS, 16, 4, req->iv, - edesc->src_nents > 1 ? 100 : ivsize, 1); -#endif - caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ", -DUMP_PREFIX_ADDRESS, 16, 4, req->dst, -edesc->dst_nents > 1 ? 100 : req->cryptlen, 1); - skcipher_unmap(jrdev, edesc, req); /* @@ -1028,6 +1019,15 @@ static void skcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err, scatterwalk_map_and_copy(req->iv, req->dst, req->cryptlen - ivsize, ivsize, 0); +#ifdef DEBUG + print_hex_dump(KERN_ERR, "dstiv @"__stringify(__LINE__)": ", + DUMP_PREFIX_ADDRESS, 16, 4, req->iv, + edesc->src_nents > 1 ? 100 : ivsize, 1); +#endif + caam_dump_sg(KERN_ERR, "dst@" __stringify(__LINE__)": ", +DUMP_PREFIX_ADDRESS, 16, 4, req->dst, +edesc->dst_nents > 1 ? 100 : req->cryptlen, 1); + kfree(edesc); skcipher_request_complete(req, err); @@ -1049,6 +1049,8 @@ static void skcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, if (err) caam_jr_strstatus(jrdev, err); + skcipher_unmap(jrdev, edesc, req); + #ifdef DEBUG print_hex_dump(KERN_ERR, "dstiv @"__stringify(__LINE__)": ", DUMP_PREFIX_ADDRESS, 16, 4, req->iv, ivsize, 1); @@ -1057,7 +1059,6 @@ static void skcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, DUMP_PREFIX_ADDRESS, 16, 4, req->dst, edesc->dst_nents > 1 ? 100 : req->cryptlen, 1); - skcipher_unmap(jrdev, edesc, req); kfree(edesc); skcipher_request_complete(req, err); -- 2.20.1
ctr(aes) broken in CAAM driver
Hi, ctr(aes) is broken in current kernel (v5.1+). It may have been broken for longer, but the crypto tests now check for a correct output IV. The testmgr answers with: alg: skcipher: ctr-aes-caam encryption test failed (wrong output IV) on test vector 0, cfg="in-place" output IV is this, which is the last 16 bytes of the encrypted message: : 1e 03 1d da 2f be 03 d1 79 21 70 a0 f3 00 9c ee It should look like this instead, which is input IV + 4: : f0 f1 f2 f3 f4 f5 f6 f7 f8 f9 fa fb fc fd ff 03 I have no idea how to fix this as I don't know how to get the output IV back from the CAAM. Any ideas? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] crypto: caam - Do not overwrite IV
Hi Horia,
On Mon, Feb 04, 2019 at 12:26:26PM +, Horia Geanta wrote:
> On 1/31/2019 8:12 AM, Sascha Hauer wrote:
> > In skcipher_decrypt() the IV passed in by the caller is overwritten and
> > the tcrypt module fails with:
> >
> > alg: aead: decryption failed on test 1 for
> > gcm_base(ctr-aes-caam,ghash-generic): ret=74
> > alg: aead: Failed to load transform for gcm(aes): -2
> >
> > With this patch tcrypt runs without errors.
> >
> This doesn't mean the patch is correct.
> crypto API requires skcipher implementations to update the IV with the last
> ciphertext block.
>
> The root cause of the issue is cache line sharing.
>
> struct crypto_gcm_req_priv_ctx {
> u8 iv[16];
> u8 auth_tag[16];
> [...]
> };
>
> Since caam does not support ghash on i.MX6, only ctr skcipher part of the gcm
> is
> offloaded.
> The skcipher request received by caam has req->src pointing to auth_tag[16]
> (1st
> S/G entry) and req->iv pointing to iv[16].
> caam driver:
> 1-DMA maps req->src
> 2-copies original req->iv to internal buffer
> 3-updates req->iv (scatterwalk_map_and_copy from last block in req->src)
> 4-sends job to crypto engine
>
> Problem is that operation 3 above is writing iv[16], which is on the same
> cache
> line as auth_tag[16] that was previously DMA mapped.
>
> I've checked that forcing auth_tag and iv to be on separate cache lines
> - u8 auth_tag[16];
> + u8 auth_tag[16] cacheline_aligned;
> solves the issue.
I can confirm that this solves it here on my side aswell.
I have no idea what's best to do here, but I'd like to have that fixed.
Is there some easy to reproduce testcase to show the issues that arise
with my patch? Apparently tcrypt doesn't do chaining, right?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] crypto: caam - Do not overwrite IV
In skcipher_decrypt() the IV passed in by the caller is overwritten and
the tcrypt module fails with:
alg: aead: decryption failed on test 1 for
gcm_base(ctr-aes-caam,ghash-generic): ret=74
alg: aead: Failed to load transform for gcm(aes): -2
With this patch tcrypt runs without errors.
Fixes: 115957bb3e59 ("crypto: caam - fix IV DMA mapping and updating")
Signed-off-by: Sascha Hauer
---
drivers/crypto/caam/caamalg.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 80ae69f906fb..493fa4169382 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -1735,7 +1735,6 @@ static int skcipher_decrypt(struct skcipher_request *req)
struct skcipher_edesc *edesc;
struct crypto_skcipher *skcipher = crypto_skcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_skcipher_ctx(skcipher);
- int ivsize = crypto_skcipher_ivsize(skcipher);
struct device *jrdev = ctx->jrdev;
u32 *desc;
int ret = 0;
@@ -1745,13 +1744,6 @@ static int skcipher_decrypt(struct skcipher_request *req)
if (IS_ERR(edesc))
return PTR_ERR(edesc);
- /*
-* The crypto API expects us to set the IV (req->iv) to the last
-* ciphertext block.
-*/
- scatterwalk_map_and_copy(req->iv, req->src, req->cryptlen - ivsize,
-ivsize, 0);
-
/* Create and submit job descriptor*/
init_skcipher_job(req, edesc, false);
desc = edesc->hw_desc;
--
2.20.1
Re: [PATCH] crypto: caam - fix setting IV after decrypt
Horia,
On Fri, Dec 07, 2018 at 12:31:23PM +0100, Sascha Hauer wrote:
> The crypto API wants the updated IV in req->info after decryption. The
> updated IV used to be copied correctly to req->info after running the
> decryption job. Since 115957bb3e59 this is done before running the job
> so instead of the updated IV only the unmodified input IV is given back
> to the crypto API.
>
> This was observed running the gcm(aes) selftest which internally uses
> ctr(aes) implemented by the CAAM engine.
>
> Fixes: 115957bb3e59 ("crypto: caam - fix IV DMA mapping and updating")
>
> Signed-off-by: Sascha Hauer
> Cc: sta...@vger.kernel.org
> ---
> drivers/crypto/caam/caamalg.c | 17 +
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 869f092432de..c05c7938439c 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -937,6 +937,14 @@ static void skcipher_decrypt_done(struct device *jrdev,
> u32 *desc, u32 err,
>edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
>
> skcipher_unmap(jrdev, edesc, req);
> +
> + /*
> + * The crypto API expects us to set the IV (req->iv) to the last
> + * ciphertext block.
> + */
> + scatterwalk_map_and_copy(req->iv, req->src, req->cryptlen - ivsize,
> + ivsize, 0);
> +
I was wrong. It's not adding the scatterwalk_map_and_copy() here which
fixes gcm(aes) selftest. In fact, this has not to be done.
> @@ -1588,13 +1596,6 @@ static int skcipher_decrypt(struct skcipher_request
> *req)
> if (IS_ERR(edesc))
> return PTR_ERR(edesc);
>
> - /*
> - * The crypto API expects us to set the IV (req->iv) to the last
> - * ciphertext block.
> - */
> - scatterwalk_map_and_copy(req->iv, req->src, req->cryptlen - ivsize,
> - ivsize, 0);
> -
It's the removal of the scatterwalk_map_and_copy() here which fixes
things. With the above the initialization vector which gets passed in is
overwritten.
Now I don't know enough of the crypto stuff to judge if overwriting the IV
always has to be removed or just in some cases, but as a matter of fact
removing these lines fixes the gcm(aes) selftest on i.MX6. From
115957bb3e59 ("crypto: caam - fix IV DMA mapping and updating")
insmodding tcrypt fails with:
alg: aead: decryption failed on test 1 for
gcm_base(ctr-aes-caam,ghash-generic): ret=74
alg: aead: Failed to load transform for gcm(aes): -2
alg: aead: Failed to load transform for rfc4106(gcm(aes)): -2
alg: aead: Failed to load transform for rfc4543(gcm(aes)): -2
With the overwriting removed it works again.
Horia, does this make sense to you or is there more that is wrong here?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] crypto: caam - fix setting IV after decrypt
The crypto API wants the updated IV in req->info after decryption. The
updated IV used to be copied correctly to req->info after running the
decryption job. Since 115957bb3e59 this is done before running the job
so instead of the updated IV only the unmodified input IV is given back
to the crypto API.
This was observed running the gcm(aes) selftest which internally uses
ctr(aes) implemented by the CAAM engine.
Fixes: 115957bb3e59 ("crypto: caam - fix IV DMA mapping and updating")
Signed-off-by: Sascha Hauer
Cc: sta...@vger.kernel.org
---
drivers/crypto/caam/caamalg.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 869f092432de..c05c7938439c 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -917,10 +917,10 @@ static void skcipher_decrypt_done(struct device *jrdev,
u32 *desc, u32 err,
{
struct skcipher_request *req = context;
struct skcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_skcipher *skcipher = crypto_skcipher_reqtfm(req);
int ivsize = crypto_skcipher_ivsize(skcipher);
+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
#endif
@@ -937,6 +937,14 @@ static void skcipher_decrypt_done(struct device *jrdev,
u32 *desc, u32 err,
edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
skcipher_unmap(jrdev, edesc, req);
+
+ /*
+* The crypto API expects us to set the IV (req->iv) to the last
+* ciphertext block.
+*/
+ scatterwalk_map_and_copy(req->iv, req->src, req->cryptlen - ivsize,
+ivsize, 0);
+
kfree(edesc);
skcipher_request_complete(req, err);
@@ -1588,13 +1596,6 @@ static int skcipher_decrypt(struct skcipher_request *req)
if (IS_ERR(edesc))
return PTR_ERR(edesc);
- /*
-* The crypto API expects us to set the IV (req->iv) to the last
-* ciphertext block.
-*/
- scatterwalk_map_and_copy(req->iv, req->src, req->cryptlen - ivsize,
-ivsize, 0);
-
/* Create and submit job descriptor*/
init_skcipher_job(req, edesc, false);
desc = edesc->hw_desc;
--
2.19.1
Re: [PATCH 6/6] crypto: vf-crc - Add new driver for Freescale Vybrid CRC
On Fri, Aug 31, 2018 at 01:07:39PM +0200, Krzysztof Kozlowski wrote:
> On Fri, 31 Aug 2018 at 09:39, Sascha Hauer wrote:
> >
> > Hi Krzysztof,
> >
> > Some comments inline.
> >
> > On Thu, Aug 30, 2018 at 07:15:39PM +0200, Krzysztof Kozlowski wrote:
> > > Add driver for using the Freescale/NXP Vybrid processor CRC block for
> > > CRC16 and CRC32 offloading. The driver implements shash_alg and was
> > > tested using internal testmgr tests and libkcapi.
> > >
> > > Signed-off-by: Krzysztof Kozlowski
> > > ---
> > > MAINTAINERS | 7 +
> > > drivers/crypto/Kconfig| 10 ++
> > > drivers/crypto/Makefile | 1 +
> > > drivers/crypto/vf-crc.c | 387
> > > ++
> > > include/linux/crc32poly.h | 7 +
> > > 5 files changed, 412 insertions(+)
> > > create mode 100644 drivers/crypto/vf-crc.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 0a340f680230..e84fa829a4e4 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -15388,6 +15388,13 @@ S: Maintained
> > > F: Documentation/fb/uvesafb.txt
> > > F: drivers/video/fbdev/uvesafb.*
> > >
> > > +VF500/VF610 HW CRC DRIVER
> > > +M: Krzysztof Kozlowski
> > > +L: linux-crypto@vger.kernel.org
> > > +S: Maintained
> > > +F: drivers/crypto/vf-crc.c
> > > +F: Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt
> > > +
> > > VF610 NAND DRIVER
> > > M: Stefan Agner
> > > L: linux-...@lists.infradead.org
> > > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > > index 20314d7a7b58..0ade940ac79c 100644
> > > --- a/drivers/crypto/Kconfig
> > > +++ b/drivers/crypto/Kconfig
> > > @@ -418,6 +418,16 @@ config CRYPTO_DEV_MXS_DCP
> > > To compile this driver as a module, choose M here: the module
> > > will be called mxs-dcp.
> > >
> > > +config CRYPTO_DEV_VF_CRC
> > > + tristate "Support for Freescale/NXP Vybrid CRC HW accelerator"
> > > + select CRYPTO_HASH
> > > + help
> > > + This option enables support for the CRC16/32 hardware accelerator
> > > + on Freescale/NXP Vybrid VF500/VF610 SoCs.
> > > +
> > > + To compile this driver as a module, choose M here: the module
> > > + will be called vf-crc.
> > > +
> > > config CRYPTO_DEV_EXYNOS_RNG
> > > tristate "EXYNOS HW pseudo random number generator support"
> > > depends on ARCH_EXYNOS || COMPILE_TEST
> > > diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> > > index c23396f32c8a..418c08bdc19c 100644
> > > --- a/drivers/crypto/Makefile
> > > +++ b/drivers/crypto/Makefile
> > > @@ -41,6 +41,7 @@ obj-$(CONFIG_ARCH_STM32) += stm32/
> > > obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> > > obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
> > > obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
> > > +obj-$(CONFIG_CRYPTO_DEV_VF_CRC) += vf-crc.o
> > > obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
> > > obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> > > obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
> > > +static int vf_crc_update_prepare(struct vf_crc_tfm_ctx *mctx,
> > > + struct vf_crc_desc_ctx *desc_ctx)
> > > +{
> > > + struct vf_crc *crc = desc_ctx->crc;
> > > + int ret;
> > > +
> > > + ret = clk_prepare_enable(crc->clk);
> > > + if (ret) {
> > > + dev_err(crc->dev, "Failed to enable clock\n");
> > > + return ret;
> > > + }
> >
> > Generally have you measured the performance of this driver? Is it faster
> > than the software implementation?
>
> I wanted to replace our in-house out-of-tree, hacky ioctl-based driver
> with something more upstreamable. I run few simple user-space
> performance tests and in fact SW implementation is faster. Around 5x
> faster for this version of driver. However it depends highly on size
> of message (buffer) because there is big overhead of libkcapi.
Well, I meant comparing the hardware vs. software implementation directly
in the kernel. Of course when a userspace API is involved the comparison
is not fair.
>
> The typical SW implementation (with lookup tables) is just fetching of
> data from memory and computing. Usage of libkcapi i
Re: [PATCH 6/6] crypto: vf-crc - Add new driver for Freescale Vybrid CRC
Hi Krzysztof,
Some comments inline.
On Thu, Aug 30, 2018 at 07:15:39PM +0200, Krzysztof Kozlowski wrote:
> Add driver for using the Freescale/NXP Vybrid processor CRC block for
> CRC16 and CRC32 offloading. The driver implements shash_alg and was
> tested using internal testmgr tests and libkcapi.
>
> Signed-off-by: Krzysztof Kozlowski
> ---
> MAINTAINERS | 7 +
> drivers/crypto/Kconfig| 10 ++
> drivers/crypto/Makefile | 1 +
> drivers/crypto/vf-crc.c | 387
> ++
> include/linux/crc32poly.h | 7 +
> 5 files changed, 412 insertions(+)
> create mode 100644 drivers/crypto/vf-crc.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0a340f680230..e84fa829a4e4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15388,6 +15388,13 @@ S: Maintained
> F: Documentation/fb/uvesafb.txt
> F: drivers/video/fbdev/uvesafb.*
>
> +VF500/VF610 HW CRC DRIVER
> +M: Krzysztof Kozlowski
> +L: linux-crypto@vger.kernel.org
> +S: Maintained
> +F: drivers/crypto/vf-crc.c
> +F: Documentation/devicetree/bindings/crypto/fsl-vf610-crc.txt
> +
> VF610 NAND DRIVER
> M: Stefan Agner
> L: linux-...@lists.infradead.org
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 20314d7a7b58..0ade940ac79c 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -418,6 +418,16 @@ config CRYPTO_DEV_MXS_DCP
> To compile this driver as a module, choose M here: the module
> will be called mxs-dcp.
>
> +config CRYPTO_DEV_VF_CRC
> + tristate "Support for Freescale/NXP Vybrid CRC HW accelerator"
> + select CRYPTO_HASH
> + help
> + This option enables support for the CRC16/32 hardware accelerator
> + on Freescale/NXP Vybrid VF500/VF610 SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called vf-crc.
> +
> config CRYPTO_DEV_EXYNOS_RNG
> tristate "EXYNOS HW pseudo random number generator support"
> depends on ARCH_EXYNOS || COMPILE_TEST
> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> index c23396f32c8a..418c08bdc19c 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_ARCH_STM32) += stm32/
> obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
> obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
> +obj-$(CONFIG_CRYPTO_DEV_VF_CRC) += vf-crc.o
> obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
> obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
> +static int vf_crc_update_prepare(struct vf_crc_tfm_ctx *mctx,
> + struct vf_crc_desc_ctx *desc_ctx)
> +{
> + struct vf_crc *crc = desc_ctx->crc;
> + int ret;
> +
> + ret = clk_prepare_enable(crc->clk);
> + if (ret) {
> + dev_err(crc->dev, "Failed to enable clock\n");
> + return ret;
> + }
Generally have you measured the performance of this driver? Is it faster
than the software implementation?
Under certain circumstances a clk_prepare_enable might become expensive,
so it could happen that all this clk enabling/disabling takes longer
than the action you do in between. Using pm_runtime might help here.
> +
> + mutex_lock(&crc->lock);
> +
> + /*
> + * Check if we are continuing to process request already configured
> + * in HW. HW has to be re-initialized only on first update() for given
> + * request or if new request was processed after last call to update().
> + */
> + if (crc->processed_desc == desc_ctx)
> + return 0;
You never set crc->processed_desc to anything, so this optimization
never triggers.
Unless properly implementing this skip-to-reinitialize-hardware really
brings a measurerable performance gain I would just drop this
optimization. In the end you only save a few register writes, but it
makes the driver more complicated.
> +
> + vf_crc_initialize_regs(mctx, desc_ctx);
> +
> + return 0;
> +}
> +
> +static int vf_crc_finup(struct shash_desc *desc, const u8 *data,
> + unsigned int len, u8 *out)
> +{
> + return vf_crc_update(desc, data, len) ?:
> +vf_crc_final(desc, out);
> +}
> +
> +static int vf_crc_digest(struct shash_desc *desc, const u8 *data,
> + unsigned int leng, u8 *out)
> +{
> + return vf_crc_init(desc) ?: vf_crc_finup(desc, data, leng, out);
> +}
These seem unnecessary. The crypto core will set these with similar
wrappers if unspecified.
> +static int vf_crc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct vf_crc *crc;
> + int ret;
> +
> + if (vf_crc_data) {
> + dev_err(dev, "Device already registered (only one instance
> allowed)\n");
> + return -EINVAL;
> + }
> +
> + crc = devm_kzalloc(dev, sizeof(*crc), GFP_KERN
Re: [PATCH 03/12] crypto: caam - Enable and disable clocks on Freescale i.MX platforms
On Thu, Jul 30, 2015 at 11:32:44PM -0700, Victoria Milhoan wrote:
> Hi Sascha,
>
> Thank you for the responses. Comments inline. Changes will be
> in the next version of the patch set.
>
> -Victoria
>
> On Thu, 30 Jul 2015 08:02:14 +0200
> Sascha Hauer wrote:
>
> > Hi Victoria,
> >
> > comments inline.
> >
> > On Wed, Jul 29, 2015 at 08:58:20PM -0700, Victoria Milhoan wrote:
> > > ARM-based systems may disable clocking to the CAAM device on the
> > > Freescale i.MX platform for power management purposes. This patch
> > > enables the required clocks when the CAAM module is initialized and
> > > disables the required clocks when the CAAM module is shut down.
> > >
> > > Signed-off-by: Victoria Milhoan
> > > ---
> > > drivers/crypto/caam/compat.h | 1 +
> > > drivers/crypto/caam/ctrl.c | 191
> > > +++
> > > drivers/crypto/caam/intern.h | 5 ++
> > > 3 files changed, 197 insertions(+)
> > >
> > > diff --git a/drivers/crypto/caam/compat.h b/drivers/crypto/caam/compat.h
> > > index f57f395..b6955ec 100644
> > > --- a/drivers/crypto/caam/compat.h
> > > +++ b/drivers/crypto/caam/compat.h
> > > @@ -23,6 +23,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > >
> > > #include
> > > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> > > index 660cc3e..cfd8c9e 100644
> > > --- a/drivers/crypto/caam/ctrl.c
> > > +++ b/drivers/crypto/caam/ctrl.c
> > > @@ -16,6 +16,121 @@
> > > #include "error.h"
> > >
> > > /*
> > > + * ARM targets tend to have clock control subsystems that can
> > > + * enable/disable clocking to our device. Support clocking
> > > + * with the following functions.
> > > + */
> > > +#ifdef CONFIG_ARM
> > > +static inline struct clk *caam_drv_get_clk_ipg(struct caam_drv_private
> > > *drv)
> > > +{
> > > + return drv->caam_ipg;
> > > +}
> >
> > You return drv->caam_ipg on ARM and NULL on powerpc. drv->caam_ipg is
> > NULL on powerpc anyway which makes this different implementations for
> > ARM and powerpc unnecessary. Just access drv->caam_ipg directly where
> > needed.
>
> Agreed. I've reworked the patch to use direct references to the clocks.
>
> >
> > > +
> > > +static inline struct clk *caam_drv_get_clk_mem(struct caam_drv_private
> > > *drv)
> > > +{
> > > + return drv->caam_mem;
> > > +}
> > > +
> > > +static inline struct clk *caam_drv_get_clk_aclk(struct caam_drv_private
> > > *drv)
> > > +{
> > > + return drv->caam_aclk;
> > > +}
> > > +
> > > +static inline struct clk *caam_drv_get_clk_emislow(struct
> > > caam_drv_private *drv)
> > > +{
> > > + return drv->caam_emi_slow;
> > > +}
> > > +
> > > +static inline void caam_drv_set_clk_ipg(struct caam_drv_private *drv,
> > > + struct clk *clk)
> > > +{
> > > + drv->caam_ipg = clk;
> > > +}
> >
> > Ditto, just access drv->caam_ipg when needed.
> >
> > > +
> > > +static inline void caam_drv_set_clk_mem(struct caam_drv_private *drv,
> > > + struct clk *clk)
> > > +{
> > > + drv->caam_mem = clk;
> > > +}
> > > +
> > > +static inline void caam_drv_set_clk_aclk(struct caam_drv_private *drv,
> > > + struct clk *clk)
> > > +{
> > > + drv->caam_aclk = clk;
> > > +}
> > > +
> > > +static inline void caam_drv_set_clk_emislow(struct caam_drv_private *drv,
> > > + struct clk *clk)
> > > +{
> > > + drv->caam_emi_slow = clk;
> > > +}
> > > +
> > > +static inline struct clk *caam_drv_identify_clk(struct device *dev,
> > > + char *clk_name)
> > > +{
> > > + return devm_clk_get(dev, clk_name);
> > > +}
> >
> > devm_clk_get() returns NULL when the architecture does not have clk
> > support, so it also seems unnecessary to have architecture specific
> > implementations for this.
> >
>
> Some of the QorIQ architectures have clock support enabled but do not
> support clocking to CAAM. This causes devm_clk_get() to return an error
> for these clocks instead of NULL. So, the only architecture-specific code
> left in the reworked patch is caam_drv_identify_clk().
I see. Wouldn't it be better to add CAAM clk support for QorIQ then?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/12] ARM: clk-imx6q: Add CAAM clock support
On Wed, Jul 29, 2015 at 08:58:25PM -0700, Victoria Milhoan wrote:
> Add CAAM clock support to the i.MX6 clocking infrastructure.
>
> Signed-off-by: Victoria Milhoan
> ---
> drivers/clk/imx/clk-imx6q.c | 3 +++
> include/dt-bindings/clock/imx6qdl-clock.h | 5 -
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
> index d046f8e..4de4943 100644
> --- a/drivers/clk/imx/clk-imx6q.c
> +++ b/drivers/clk/imx/clk-imx6q.c
> @@ -381,6 +381,9 @@ static void __init imx6q_clocks_init(struct device_node
> *ccm_node)
> clk[IMX6QDL_CLK_ASRC] = imx_clk_gate2_shared("asrc",
> "asrc_podf", base + 0x68, 6, &share_count_asrc);
> clk[IMX6QDL_CLK_ASRC_IPG] = imx_clk_gate2_shared("asrc_ipg",
> "ahb", base + 0x68, 6, &share_count_asrc);
> clk[IMX6QDL_CLK_ASRC_MEM] = imx_clk_gate2_shared("asrc_mem",
> "ahb", base + 0x68, 6, &share_count_asrc);
> + clk[IMX6QDL_CAAM_MEM] = imx_clk_gate2("caam_mem", "ahb",
> base + 0x68, 8);
> + clk[IMX6QDL_CAAM_ACLK]= imx_clk_gate2("caam_aclk", "ahb",
> base + 0x68, 10);
> + clk[IMX6QDL_CAAM_IPG] = imx_clk_gate2("caam_ipg", "ipg",
> base + 0x68, 12);
> clk[IMX6QDL_CLK_CAN1_IPG] = imx_clk_gate2("can1_ipg", "ipg",
> base + 0x68, 14);
> clk[IMX6QDL_CLK_CAN1_SERIAL] = imx_clk_gate2("can1_serial",
> "can_root", base + 0x68, 16);
> clk[IMX6QDL_CLK_CAN2_IPG] = imx_clk_gate2("can2_ipg", "ipg",
> base + 0x68, 18);
> diff --git a/include/dt-bindings/clock/imx6qdl-clock.h
> b/include/dt-bindings/clock/imx6qdl-clock.h
> index 8780868..f68e925 100644
> --- a/include/dt-bindings/clock/imx6qdl-clock.h
> +++ b/include/dt-bindings/clock/imx6qdl-clock.h
> @@ -251,6 +251,9 @@
> #define IMX6QDL_CLK_VIDEO_27M238
> #define IMX6QDL_CLK_MIPI_CORE_CFG239
> #define IMX6QDL_CLK_MIPI_IPG 240
> -#define IMX6QDL_CLK_END 241
> +#define IMX6QDL_CAAM_MEM 241
> +#define IMX6QDL_CAAM_ACLK242
> +#define IMX6QDL_CAAM_IPG 243
IMX6QDL_CLK_* please.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/12] ARM: dts: mx6qdl: Add CAAM device node
On Wed, Jul 29, 2015 at 08:58:26PM -0700, Victoria Milhoan wrote:
> Add CAAM device node to the i.MX6 device tree.
>
> Signed-off-by: Victoria Milhoan
> ---
> arch/arm/boot/dts/imx6qdl.dtsi | 30 ++
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index e6d1359..4df9f1e 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -836,10 +836,32 @@
> reg = <0x0210 0x10>;
> ranges;
>
> - caam@0210 {
> - reg = <0x0210 0x4>;
> - interrupts = <0 105 IRQ_TYPE_LEVEL_HIGH>,
> - <0 106 IRQ_TYPE_LEVEL_HIGH>;
> + crypto: caam@210 {
> + compatible = "fsl,sec-v4.0";
> + fsl,sec-era = <4>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0x210 0x1>;
> + ranges = <0 0x210 0x1>;
> + interrupt-parent = <&intc>;
> + clocks = <&clks IMX6QDL_CAAM_MEM>,
> + <&clks IMX6QDL_CAAM_ACLK>,
> + <&clks IMX6QDL_CAAM_IPG>,
> + <&clks IMX6QDL_CLK_EIM_SLOW>;
> + clock-names = "caam_mem", "caam_aclk",
> + "caam_ipg", "caam_emi_slow";
The binding document should be updated for these additional properties.
The namespace caam_ is already clear from the context, you can drop
these prefixes.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/12] crypto: caam - Enable and disable clocks on Freescale i.MX platforms
Hi Victoria,
comments inline.
On Wed, Jul 29, 2015 at 08:58:20PM -0700, Victoria Milhoan wrote:
> ARM-based systems may disable clocking to the CAAM device on the
> Freescale i.MX platform for power management purposes. This patch
> enables the required clocks when the CAAM module is initialized and
> disables the required clocks when the CAAM module is shut down.
>
> Signed-off-by: Victoria Milhoan
> ---
> drivers/crypto/caam/compat.h | 1 +
> drivers/crypto/caam/ctrl.c | 191
> +++
> drivers/crypto/caam/intern.h | 5 ++
> 3 files changed, 197 insertions(+)
>
> diff --git a/drivers/crypto/caam/compat.h b/drivers/crypto/caam/compat.h
> index f57f395..b6955ec 100644
> --- a/drivers/crypto/caam/compat.h
> +++ b/drivers/crypto/caam/compat.h
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index 660cc3e..cfd8c9e 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -16,6 +16,121 @@
> #include "error.h"
>
> /*
> + * ARM targets tend to have clock control subsystems that can
> + * enable/disable clocking to our device. Support clocking
> + * with the following functions.
> + */
> +#ifdef CONFIG_ARM
> +static inline struct clk *caam_drv_get_clk_ipg(struct caam_drv_private *drv)
> +{
> + return drv->caam_ipg;
> +}
You return drv->caam_ipg on ARM and NULL on powerpc. drv->caam_ipg is
NULL on powerpc anyway which makes this different implementations for
ARM and powerpc unnecessary. Just access drv->caam_ipg directly where
needed.
> +
> +static inline struct clk *caam_drv_get_clk_mem(struct caam_drv_private *drv)
> +{
> + return drv->caam_mem;
> +}
> +
> +static inline struct clk *caam_drv_get_clk_aclk(struct caam_drv_private *drv)
> +{
> + return drv->caam_aclk;
> +}
> +
> +static inline struct clk *caam_drv_get_clk_emislow(struct caam_drv_private
> *drv)
> +{
> + return drv->caam_emi_slow;
> +}
> +
> +static inline void caam_drv_set_clk_ipg(struct caam_drv_private *drv,
> + struct clk *clk)
> +{
> + drv->caam_ipg = clk;
> +}
Ditto, just access drv->caam_ipg when needed.
> +
> +static inline void caam_drv_set_clk_mem(struct caam_drv_private *drv,
> + struct clk *clk)
> +{
> + drv->caam_mem = clk;
> +}
> +
> +static inline void caam_drv_set_clk_aclk(struct caam_drv_private *drv,
> + struct clk *clk)
> +{
> + drv->caam_aclk = clk;
> +}
> +
> +static inline void caam_drv_set_clk_emislow(struct caam_drv_private *drv,
> + struct clk *clk)
> +{
> + drv->caam_emi_slow = clk;
> +}
> +
> +static inline struct clk *caam_drv_identify_clk(struct device *dev,
> + char *clk_name)
> +{
> + return devm_clk_get(dev, clk_name);
> +}
devm_clk_get() returns NULL when the architecture does not have clk
support, so it also seems unnecessary to have architecture specific
implementations for this.
> +
> +static inline void caam_drv_show_clk(struct device *dev, struct clk *clk,
> + char *clk_name)
> +{
> + dev_info(dev, "%s clock:%d\n", clk_name, (int)clk_get_rate(clk));
> +}
The correct format specifier for unsigned long is "%lu", no need to cast
it to int. Besides, this information is not generally interesting, you
can drop this.
> +
> +#else
> +static inline struct clk *caam_drv_get_clk_ipg(struct caam_drv_private *drv)
> +{
> + return NULL;
> +}
> +
> +static inline struct clk *caam_drv_get_clk_mem(struct caam_drv_private *drv)
> +{
> + return NULL;
> +}
> +
> +static inline struct clk *caam_drv_get_clk_aclk(struct caam_drv_private *drv)
> +{
> + return NULL;
> +}
> +
> +static inline struct clk *caam_drv_get_clk_emislow(struct caam_drv_private
> *drv)
> +{
> + return NULL;
> +}
> +
> +static inline void caam_drv_set_clk_ipg(struct caam_drv_private *drv,
> + struct clk *clk)
> +{
> +}
> +
> +static inline void caam_drv_set_clk_mem(struct caam_drv_private *drv,
> + struct clk *clk)
> +{
> +}
> +
> +static inline void caam_drv_set_clk_aclk(struct caam_drv_private *drv,
> + struct clk *clk)
> +{
> +}
> +
> +static inline void caam_drv_set_clk_emislow(struct caam_drv_private *drv,
> + struct clk *clk)
> +{
> +}
> +
> +static inline struct clk *caam_drv_identify_clk(struct device *dev,
> + char *clk_name)
> +{
> + return 0;
> +}
> +
> +static inline void caam_drv_show_clk(struct device *dev, struct clk *clk,
> + char *clk_name)
> +{
> +}
> +#endif
> +
> +/*
> * Descriptor to instantiate RNG State Handle 0 in normal mode
Re: [PATCH v3 1/2] i.MX27: Add clock support for SAHARA2.
On Mon, Mar 11, 2013 at 09:19:26AM +0800, Herbert Xu wrote: > On Mon, Mar 11, 2013 at 12:08:56AM +0100, Sascha Hauer wrote: > > On Sun, Mar 10, 2013 at 04:34:01PM +0800, Herbert Xu wrote: > > > > > > > > > > https://patchwork.kernel.org/patch/1817741/ > > > > > > > > > > So the change above becomes unnecessary > > > > > > > > Very good. Then this patch can be safely dropped. > > > > > > So should I take this patch or not? > > > > This clk patch, no. The sahara patch, yes, if it is fine for you. > > But will the second patch work fine without the first? It will work once a device is registered. The necessary clocks for it will be provided by the devicetree then. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] i.MX27: Add clock support for SAHARA2.
On Sun, Mar 10, 2013 at 04:34:01PM +0800, Herbert Xu wrote: > > > > > > https://patchwork.kernel.org/patch/1817741/ > > > > > > So the change above becomes unnecessary > > > > Very good. Then this patch can be safely dropped. > > So should I take this patch or not? This clk patch, no. The sahara patch, yes, if it is fine for you. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] i.MX27: Add clock support for SAHARA2.
On Fri, Mar 01, 2013 at 12:37:52PM +0100, Javier Martin wrote: > > Signed-off-by: Javier Martin > --- > arch/arm/mach-imx/clk-imx27.c |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c > index 4c1d1e4..0b9664a 100644 > --- a/arch/arm/mach-imx/clk-imx27.c > +++ b/arch/arm/mach-imx/clk-imx27.c > @@ -253,6 +253,8 @@ int __init mx27_clocks_init(unsigned long fref) > clk_register_clkdev(clk[nfc_baud_gate], NULL, "imx27-nand.0"); > clk_register_clkdev(clk[vpu_baud_gate], "per", "coda-imx27.0"); > clk_register_clkdev(clk[vpu_ahb_gate], "ahb", "coda-imx27.0"); > + clk_register_clkdev(clk[sahara_ahb_gate], "ahb", "sahara-imx27.0"); > + clk_register_clkdev(clk[sahara_ipg_gate], "ipg", "sahara-imx27.0"); One of the first patches I want to push upstream for the next merge window is this one: https://patchwork.kernel.org/patch/1817741/ So the change above becomes unnecessary Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] crypto: sahara: Add device tree binding for SAHARA2.
Hi Javier, On Wed, Feb 27, 2013 at 11:41:51AM +0100, Javier Martin wrote: > > Signed-off-by: Javier Martin > --- > .../devicetree/bindings/crypto/fsl-imx-sahara.txt | 14 ++ > 1 file changed, 14 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/crypto/fsl-imx-sahara.txt > > diff --git a/Documentation/devicetree/bindings/crypto/fsl-imx-sahara.txt > b/Documentation/devicetree/bindings/crypto/fsl-imx-sahara.txt > new file mode 100644 > index 000..44abf11 > --- /dev/null > +++ b/Documentation/devicetree/bindings/crypto/fsl-imx-sahara.txt > @@ -0,0 +1,14 @@ > +* Freescale i.MX SAHARA Cryptographic Accelerator > + > +Required properties: > +- compatible : Should be "fsl,-sahara" Please add explicitly which SoCs are supported. You can fold this patch into the driver patch. This way people reading the commit history have a direct cross link between the driver and the documentation. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] crypto: sahara: Add support for SAHARA in i.MX27.
On Thu, Feb 21, 2013 at 02:40:59PM +, Arnd Bergmann wrote: > On Thursday 21 February 2013, javier Martin wrote: > > We know about the existence of DT and the constant migration process > > that is taking place towards it. > > > > Moreover we are strongly interested in converting the Visstrim SM10 > > platform to DT. Unfortunately, there are several issues that need to > > be solved so that Visstrim M10 can be fully converted to DT: > > - full SoC camera DT support > > - DT support for ov7670 > > - pinctrl support for i.MX27 > > Ok, I see. Full device tree support is certainly asking too much then. > > > Furthermore, we have limited resources at the moment and we have to > > decide priorities. But having an hybrid platform-DT support is our > > next task for Visstrim M10. > > Ok, cool. > > > However, I understand your concern about new drivers having DT > > support. Maybe a good approach could be that I added compile-tested > > only DT support for this driver and remove platform support for > > mainline submission (although we maintain it internally in our > > repositories). > > > > What do you think? > > Sounds good. Since it sounds like the hardware is available in all > imx27 device, maybe someone else can test your driver on an imx27 > board that already has partial DT support when you add it to > arch/arm/boot/dts/imx27.dtsi? > > Is there any reason why an imx27 based board would not support this > driver? Are there any other chips that could use it, e.g. imx25 or > imx6? Neither i.MX25, i.MX31 nor i.MX21 have this unit. I haven't checked the others, but I assume this is i.MX27 only. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] crypto: sahara: Add support for SAHARA in i.MX27.
On Thu, Feb 21, 2013 at 03:18:36PM +0100, javier Martin wrote: > Hi Arnd, > > On 21 February 2013 13:59, Arnd Bergmann wrote: > > On Thursday 21 February 2013, Javier Martin wrote: > >> This series of patches provide AES-ECB and AES-CBC support > >> for the SAHARA2 cryptographic accelerator which is inside the i.MX27 SoC. > >> It is expected that more algorithms will be supported in the future. > >> > >> For testing, a Visstrim M10 board has been used and the code related > >> to this platform has been included too. > > > > As a new device driver, this needs to be supported on the DT based imx27 > > machines, while support for the individual board files seems unnecessary. > > > > How about converting the Visstrim M10 board file over to DT so you don't > > have to add any more infrastructure for imx that we will just have to > > remove again one day? > > We know about the existence of DT and the constant migration process > that is taking place towards it. > > Moreover we are strongly interested in converting the Visstrim SM10 > platform to DT. Unfortunately, there are several issues that need to > be solved so that Visstrim M10 can be fully converted to DT: > - full SoC camera DT support > - DT support for ov7670 > - pinctrl support for i.MX27 For the pinctrl stuff I suggest doing what we have done for the newer i.MX SoCs before we had pinctrl: Match by board compatible string and call into the board code for using the static pinctrl tables which are already there. I would like to have propert pinctrl support for the older i.MX, but probably the motivation for writing it is quite low for the older SoCs. We have a patch in the queue converting the clock lookups for i.MX27 to dt which should be ready for posting soon. Then the main road blockers should be out of the way to get rid of the bulk of the board code. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
