[PATCH 00/18] crypto: talitos - fixes and performance improvement

[PATCH 00/18] crypto: talitos - fixes and performance improvement

[Christophe Leroy]

This serie fixes and improves the talitos crypto driver.

First 6 patchs are fixes of failures reported by the new tests in the
kernel crypto test manager.

The 8 following patches are cleanups and simplifications.

The last 4 ones are performance improvement. The main improvement is
in the one before the last, it divides by 2 the time needed for a md5
hash on the SEC1.

Christophe Leroy (18):
  crypto: talitos - fix AEAD test failures
  crypto: talitos - fix memory corruption on SEC2
  crypto: talitos - fix setkey to check key weakness
  crypto: talitos - fix AEAD for sha224 on non sha224 capable chips
  crypto: talitos - fix use of sg_link_tbl_len
  crypto: talitos - fix ctr-aes-talitos
  crypto: talitos - zeroize the descriptor with memset()
  crypto: talitos - declare local functions static
  crypto: talitos - use devm_kmalloc()
  crypto: talitos - use of_property_read_u32()
  crypto: talitos - use devm_ioremap()
  crypto: talitos - don't check the number of channels at each interrupt
  crypto: talitos - remove to_talitos_ptr_len()
  crypto: talitos - simplify tests in ipsec_esp()
  crypto: talitos - DMA map key in setkey()
  crypto: talitos - do hw_context DMA mapping outside the requests
  crypto: talitos - chain in buffered data for ahash on SEC1
  crypto: talitos - avoid useless copy

 drivers/crypto/talitos.c | 544 ++++++++++++++++++++++++++++++-----------------
 drivers/crypto/talitos.h |   7 +-
 2 files changed, 356 insertions(+), 195 deletions(-)

-- 
2.13.3

[PATCH 01/18] crypto: talitos - fix AEAD test failures

[Christophe Leroy]

AEAD tests fail when destination SG list has more than 1 element.

[    2.058752] alg: aead: Test 1 failed on encryption for authenc-hmac-sha1-cbc-aes-talitos
[    2.066965] 00000000: 53 69 6e 67 6c 65 20 62 6c 6f 63 6b 20 6d 73 67
00000010: c0 43 ff 74 c0 43 ff e0 de 83 d1 20 de 84 8e 54
00000020: de 83 d7 c4
[    2.082138] alg: aead: Test 1 failed on encryption for authenc-hmac-sha1-cbc-aes-talitos
[    2.090435] 00000000: 53 69 6e 67 6c 65 20 62 6c 6f 63 6b 20 6d 73 67
00000010: de 84 ea 58 c0 93 1a 24 de 84 e8 59 de 84 f1 20
00000020: 00 00 00 00
[    2.105721] alg: aead: Test 1 failed on encryption for authenc-hmac-sha1-cbc-3des-talitos
[    2.114259] 00000000: 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72 73 74
00000010: 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63 74 65
00000020: 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72
00000030: 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63
00000040: 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65
00000050: 65 72 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53
00000060: 72 63 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20
00000070: 63 65 65 72 73 74 54 20 6f 6f 4d 20 6e 61 0a 79
00000080: c0 50 f1 ac c0 50 f3 38 c0 50 f3 94 c0 50 f5 30
00000090: c0 99 74 3c
[    2.166410] alg: aead: Test 1 failed on encryption for authenc-hmac-sha1-cbc-3des-talitos
[    2.174794] 00000000: 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72 73 74
00000010: 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63 74 65
00000020: 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72
00000030: 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63
00000040: 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65
00000050: 65 72 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53
00000060: 72 63 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20
00000070: 63 65 65 72 73 74 54 20 6f 6f 4d 20 6e 61 0a 79
00000080: c0 50 f1 ac c0 50 f3 38 c0 50 f3 94 c0 50 f5 30
00000090: c0 99 74 3c
[    2.226486] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos)
[    2.236459] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos)
[    2.247196] alg: aead: Test 1 failed on encryption for authenc-hmac-sha224-cbc-3des-talitos
[    2.255555] 00000000: 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72 73 74
00000010: 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63 74 65
00000020: 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72
00000030: 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63
00000040: 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65
00000050: 65 72 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53
00000060: 72 63 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20
00000070: 63 65 65 72 73 74 54 20 6f 6f 4d 20 6e 61 0a 79
00000080: c0 50 f1 ac c0 50 f3 38 c0 50 f3 94 c0 50 f5 30
00000090: c0 99 74 3c c0 96 e5 b8
[    2.309004] alg: aead: Test 1 failed on encryption for authenc-hmac-sha224-cbc-3des-talitos
[    2.317562] 00000000: 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72 73 74
00000010: 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63 74 65
00000020: 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72
00000030: 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63
00000040: 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65
00000050: 65 72 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53
00000060: 72 63 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20
00000070: 63 65 65 72 73 74 54 20 6f 6f 4d 20 6e 61 0a 79
00000080: c0 50 f1 ac c0 50 f3 38 c0 50 f3 94 c0 50 f5 30
00000090: c0 99 74 3c c0 96 e5 b8
[    2.370710] alg: aead: Test 1 failed on encryption for authenc-hmac-sha256-cbc-aes-talitos
[    2.379177] 00000000: 53 69 6e 67 6c 65 20 62 6c 6f 63 6b 20 6d 73 67
00000010: 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63 74 65
00000020: 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72
[    2.397863] alg: aead: Test 1 failed on encryption for authenc-hmac-sha256-cbc-aes-talitos
[    2.406134] 00000000: 53 69 6e 67 6c 65 20 62 6c 6f 63 6b 20 6d 73 67
00000010: 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63 74 65
00000020: 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72
[    2.424789] alg: aead: Test 1 failed on encryption for authenc-hmac-sha256-cbc-3des-talitos
[    2.433491] 00000000: 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72 73 74
00000010: 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63 74 65
00000020: 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72
00000030: 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63
00000040: 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65
00000050: 65 72 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53
00000060: 72 63 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20
00000070: 63 65 65 72 73 74 54 20 6f 6f 4d 20 6e 61 0a 79
00000080: c0 50 f1 ac c0 50 f3 38 c0 50 f3 94 c0 50 f5 30
00000090: c0 99 74 3c c0 96 e5 b8 c0 96 e9 20 c0 00 3d dc
[    2.488832] alg: aead: Test 1 failed on encryption for authenc-hmac-sha256-cbc-3des-talitos
[    2.497387] 00000000: 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72 73 74
00000010: 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63 74 65
00000020: 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65 65 72
00000030: 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53 72 63
00000040: 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20 63 65
00000050: 65 72 73 74 54 20 6f 6f 4d 20 6e 61 20 79 65 53
00000060: 72 63 74 65 20 73 6f 54 20 6f 61 4d 79 6e 53 20
00000070: 63 65 65 72 73 74 54 20 6f 6f 4d 20 6e 61 0a 79
00000080: c0 50 f1 ac c0 50 f3 38 c0 50 f3 94 c0 50 f5 30
00000090: c0 99 74 3c c0 96 e5 b8 c0 96 e9 20 c0 00 3d dc

This patch fixes that.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index dff88838dce7..cd8a37e60259 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1232,12 +1232,11 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 			sg_link_tbl_len += authsize;
 	}
 
-	sg_count = talitos_sg_map(dev, areq->src, cryptlen, edesc,
-				  &desc->ptr[4], sg_count, areq->assoclen,
-				  tbl_off);
+	ret = talitos_sg_map(dev, areq->src, cryptlen, edesc, &desc->ptr[4],
+			     sg_count, areq->assoclen, tbl_off);
 
-	if (sg_count > 1) {
-		tbl_off += sg_count;
+	if (ret > 1) {
+		tbl_off += ret;
 		sync_needed = true;
 	}
 
-- 
2.13.3

[PATCH 02/18] crypto: talitos - fix memory corruption on SEC2

[Christophe Leroy]

On SEC2, when using the old descriptors type (hmac snoop no afeu)
for doing IPsec, the CICV out pointeur points out of the allocated
memory.

[    2.502554] =============================================================================
[    2.510740] BUG dma-kmalloc-256 (Not tainted): Redzone overwritten
[    2.516907] -----------------------------------------------------------------------------
[    2.516907]
[    2.526535] Disabling lock debugging due to kernel taint
[    2.531845] INFO: 0xde858108-0xde85810b. First byte 0xf8 instead of 0xcc
[    2.538549] INFO: Allocated in 0x806181a9 age=0 cpu=0 pid=58
[    2.544229] 	__kmalloc+0x374/0x564
[    2.547649] 	talitos_edesc_alloc+0x17c/0x48c
[    2.551929] 	aead_edesc_alloc+0x80/0x154
[    2.555863] 	aead_encrypt+0x30/0xe0
[    2.559368] 	__test_aead+0x5a0/0x1f3c
[    2.563042] 	test_aead+0x2c/0x110
[    2.566371] 	alg_test_aead+0x5c/0xf4
[    2.569958] 	alg_test+0x1dc/0x5a0
[    2.573305] 	cryptomgr_test+0x50/0x70
[    2.576984] 	kthread+0xd8/0x134
[    2.580155] 	ret_from_kernel_thread+0x5c/0x64
[    2.584534] INFO: Freed in ipsec_esp_encrypt_done+0x130/0x240 age=6 cpu=0 pid=0
[    2.591839] 	ipsec_esp_encrypt_done+0x130/0x240
[    2.596395] 	flush_channel+0x1dc/0x488
[    2.600161] 	talitos2_done_4ch+0x30/0x200
[    2.604185] 	tasklet_action+0xa0/0x13c
[    2.607948] 	__do_softirq+0x148/0x6cc
[    2.611623] 	irq_exit+0xc0/0x124
[    2.614869] 	call_do_irq+0x24/0x3c
[    2.618292] 	do_IRQ+0x78/0x108
[    2.621369] 	ret_from_except+0x0/0x14
[    2.625055] 	finish_task_switch+0x58/0x350
[    2.629165] 	schedule+0x80/0x134
[    2.632409] 	schedule_preempt_disabled+0x38/0xc8
[    2.637042] 	cpu_startup_entry+0xe4/0x190
[    2.641074] 	start_kernel+0x3f4/0x408
[    2.644741] 	0x3438
[    2.646857] INFO: Slab 0xdffbdb00 objects=9 used=1 fp=0xde8581c0 flags=0x0080
[    2.653978] INFO: Object 0xde858008 @offset=8 fp=0xca4395df
[    2.653978]
[    2.661032] Redzone de858000: cc cc cc cc cc cc cc cc                          ........
[    2.669029] Object de858008: 00 00 00 02 00 00 00 02 00 6b 6b 6b 1e 83 ea 28  .........kkk...(
[    2.677628] Object de858018: 00 00 00 70 1e 85 80 64 ff 73 1d 21 6b 6b 6b 6b  ...p...d.s.!kkkk
[    2.686228] Object de858028: 00 20 00 00 1e 84 17 24 00 10 00 00 1e 85 70 00  . .....$......p.
[    2.694829] Object de858038: 00 18 00 00 1e 84 17 44 00 08 00 00 1e 83 ea 28  .......D.......(
[    2.703430] Object de858048: 00 80 00 00 1e 84 f0 00 00 80 00 00 1e 85 70 10  ..............p.
[    2.712030] Object de858058: 00 20 6b 00 1e 85 80 f4 6b 6b 6b 6b 00 80 02 00  . k.....kkkk....
[    2.720629] Object de858068: 1e 84 f0 00 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  ....kkkkkkkkkkkk
[    2.729230] Object de858078: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[    2.737830] Object de858088: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[    2.746429] Object de858098: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[    2.755029] Object de8580a8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[    2.763628] Object de8580b8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[    2.772229] Object de8580c8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[    2.780829] Object de8580d8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[    2.789430] Object de8580e8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 73 b0 ea 9f  kkkkkkkkkkkks...
[    2.798030] Object de8580f8: e8 18 80 d6 56 38 44 c0 db e3 4f 71 f7 ce d1 d3  ....V8D...Oq....
[    2.806629] Redzone de858108: f8 bd 3e 4f                                      ..>O
[    2.814279] Padding de8581b0: 5a 5a 5a 5a 5a 5a 5a 5a                          ZZZZZZZZ
[    2.822283] CPU: 0 PID: 0 Comm: swapper Tainted: G    B           4.9.50-g995be12679 #179
[    2.831819] Call Trace:
[    2.834301] [dffefd20] [c01aa9a8] check_bytes_and_report+0x100/0x194 (unreliable)
[    2.841801] [dffefd50] [c01aac3c] check_object+0x200/0x530
[    2.847306] [dffefd80] [c01ae584] free_debug_processing+0x290/0x690
[    2.853585] [dffefde0] [c01aec8c] __slab_free+0x308/0x628
[    2.859000] [dffefe80] [c05057f4] ipsec_esp_encrypt_done+0x130/0x240
[    2.865378] [dffefeb0] [c05002c4] flush_channel+0x1dc/0x488
[    2.870968] [dffeff10] [c05007a8] talitos2_done_4ch+0x30/0x200
[    2.876814] [dffeff30] [c002fe38] tasklet_action+0xa0/0x13c
[    2.882399] [dffeff60] [c002f118] __do_softirq+0x148/0x6cc
[    2.887896] [dffeffd0] [c002f954] irq_exit+0xc0/0x124
[    2.892968] [dffefff0] [c0013adc] call_do_irq+0x24/0x3c
[    2.898213] [c0d4be00] [c000757c] do_IRQ+0x78/0x108
[    2.903113] [c0d4be30] [c0015c08] ret_from_except+0x0/0x14
[    2.908634] --- interrupt: 501 at finish_task_switch+0x70/0x350
[    2.908634]     LR = finish_task_switch+0x58/0x350
[    2.919327] [c0d4bf20] [c085e1d4] schedule+0x80/0x134
[    2.924398] [c0d4bf50] [c085e2c0] schedule_preempt_disabled+0x38/0xc8
[    2.930853] [c0d4bf60] [c007f064] cpu_startup_entry+0xe4/0x190
[    2.936707] [c0d4bfb0] [c096c434] start_kernel+0x3f4/0x408
[    2.942198] [c0d4bff0] [00003438] 0x3438
[    2.946137] FIX dma-kmalloc-256: Restoring 0xde858108-0xde85810b=0xcc
[    2.946137]
[    2.954158] FIX dma-kmalloc-256: Object at 0xde858008 not freed

This patch reworks the handling of the CICV out in order
to properly handle all cases.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index cd8a37e60259..1e799886c57d 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1247,14 +1247,15 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 			dma_map_sg(dev, areq->dst, sg_count, DMA_FROM_DEVICE);
 	}
 
-	sg_count = talitos_sg_map(dev, areq->dst, cryptlen, edesc,
-				  &desc->ptr[5], sg_count, areq->assoclen,
-				  tbl_off);
+	ret = talitos_sg_map(dev, areq->dst, cryptlen, edesc, &desc->ptr[5],
+			     sg_count, areq->assoclen, tbl_off);
 
 	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)
 		to_talitos_ptr_ext_or(&desc->ptr[5], authsize, is_sec1);
 
-	if (sg_count > 1) {
+	/* ICV data */
+	if (ret > 1) {
+		tbl_off += ret;
 		edesc->icv_ool = true;
 		sync_needed = true;
 
@@ -1264,9 +1265,7 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 				     sizeof(struct talitos_ptr) + authsize;
 
 			/* Add an entry to the link table for ICV data */
-			tbl_ptr += sg_count - 1;
-			to_talitos_ptr_ext_set(tbl_ptr, 0, is_sec1);
-			tbl_ptr++;
+			to_talitos_ptr_ext_set(tbl_ptr - 1, 0, is_sec1);
 			to_talitos_ptr_ext_set(tbl_ptr, DESC_PTR_LNKTBL_RETURN,
 					       is_sec1);
 			to_talitos_ptr_len(tbl_ptr, authsize, is_sec1);
@@ -1274,18 +1273,33 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 			/* icv data follows link tables */
 			to_talitos_ptr(tbl_ptr, edesc->dma_link_tbl + offset,
 				       is_sec1);
+		} else {
+			dma_addr_t addr = edesc->dma_link_tbl;
+
+			if (is_sec1)
+				addr += areq->assoclen + cryptlen;
+			else
+				addr += sizeof(struct talitos_ptr) * tbl_off;
+
+			to_talitos_ptr(&desc->ptr[6], addr, is_sec1);
+			to_talitos_ptr_len(&desc->ptr[6], authsize, is_sec1);
+		}
+	} else if (!(desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)) {
+		ret = talitos_sg_map(dev, areq->dst, authsize, edesc,
+				     &desc->ptr[6], sg_count, areq->assoclen +
+							      cryptlen,
+				     tbl_off);
+		if (ret > 1) {
+			tbl_off += ret;
+			edesc->icv_ool = true;
+			sync_needed = true;
+		} else {
+			edesc->icv_ool = false;
 		}
 	} else {
 		edesc->icv_ool = false;
 	}
 
-	/* ICV data */
-	if (!(desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)) {
-		to_talitos_ptr_len(&desc->ptr[6], authsize, is_sec1);
-		to_talitos_ptr(&desc->ptr[6], edesc->dma_link_tbl +
-			       areq->assoclen + cryptlen, is_sec1);
-	}
-
 	/* iv out */
 	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)
 		map_single_talitos_ptr(dev, &desc->ptr[6], ivsize, ctx->iv,
-- 
2.13.3

[PATCH 03/18] crypto: talitos - fix setkey to check key weakness

[Christophe Leroy]

Crypto manager test report the following failures:
[    3.061081] alg: skcipher: setkey failed on test 5 for ecb-des-talitos: flags=100
[    3.069342] alg: skcipher-ddst: setkey failed on test 5 for ecb-des-talitos: flags=100
[    3.077754] alg: skcipher-ddst: setkey failed on test 5 for ecb-des-talitos: flags=100

This is due to setkey being expected to detect weak keys.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 1e799886c57d..8aa1212086f4 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1507,12 +1507,20 @@ static int ablkcipher_setkey(struct crypto_ablkcipher *cipher,
 			     const u8 *key, unsigned int keylen)
 {
 	struct talitos_ctx *ctx = crypto_ablkcipher_ctx(cipher);
+	u32 tmp[DES_EXPKEY_WORDS];
 
 	if (keylen > TALITOS_MAX_KEY_SIZE) {
 		crypto_ablkcipher_set_flags(cipher, CRYPTO_TFM_RES_BAD_KEY_LEN);
 		return -EINVAL;
 	}
 
+	if (unlikely(crypto_ablkcipher_get_flags(cipher) &
+		     CRYPTO_TFM_REQ_WEAK_KEY) &&
+	    !des_ekey(tmp, key)) {
+		crypto_ablkcipher_set_flags(cipher, CRYPTO_TFM_RES_WEAK_KEY);
+		return -EINVAL;
+	}
+
 	memcpy(&ctx->key, key, keylen);
 	ctx->keylen = keylen;
 
-- 
2.13.3

[PATCH 04/18] crypto: talitos - fix AEAD for sha224 on non sha224 capable chips

[Christophe Leroy]

sha224 AEAD test fails with:

[    2.803125] talitos ff020000.crypto: DEUISR 0x00000000_00000000
[    2.808743] talitos ff020000.crypto: MDEUISR 0x80100000_00000000
[    2.814678] talitos ff020000.crypto: DESCBUF 0x20731f21_00000018
[    2.820616] talitos ff020000.crypto: DESCBUF 0x0628d64c_00000010
[    2.826554] talitos ff020000.crypto: DESCBUF 0x0631005c_00000018
[    2.832492] talitos ff020000.crypto: DESCBUF 0x0628d664_00000008
[    2.838430] talitos ff020000.crypto: DESCBUF 0x061b13a0_00000080
[    2.844369] talitos ff020000.crypto: DESCBUF 0x0631006c_00000080
[    2.850307] talitos ff020000.crypto: DESCBUF 0x0631006c_00000018
[    2.856245] talitos ff020000.crypto: DESCBUF 0x063100ec_00000000
[    2.884972] talitos ff020000.crypto: failed to reset channel 0
[    2.890503] talitos ff020000.crypto: done overflow, internal time out, or rngu error: ISR 0x20000000_00020000
[    2.900652] alg: aead: encryption failed on test 1 for authenc-hmac-sha224-cbc-3des-talitos: ret=22

This is due to SHA224 not being supported by the HW. Allthough for
hash we are able to init the hash context by SW, it is not
possible for AEAD. Therefore SHA224 AEAD has to be deactivated.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 8aa1212086f4..b7184f305867 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -3068,6 +3068,11 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
 		t_alg->algt.alg.aead.setkey = aead_setkey;
 		t_alg->algt.alg.aead.encrypt = aead_encrypt;
 		t_alg->algt.alg.aead.decrypt = aead_decrypt;
+		if (!(priv->features & TALITOS_FTR_SHA224_HWINIT) &&
+		    !strncmp(alg->cra_name, "authenc(hmac(sha224)", 20)) {
+			kfree(t_alg);
+			return ERR_PTR(-ENOTSUPP);
+		}
 		break;
 	case CRYPTO_ALG_TYPE_AHASH:
 		alg = &t_alg->algt.alg.hash.halg.base;
-- 
2.13.3

[PATCH 05/18] crypto: talitos - fix use of sg_link_tbl_len

[Christophe Leroy]

sg_link_tbl_len shall be used instead of cryptlen, otherwise
SECs which perform HW CICV verification will fail.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index b7184f305867..cf5c9701b898 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1232,8 +1232,8 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 			sg_link_tbl_len += authsize;
 	}
 
-	ret = talitos_sg_map(dev, areq->src, cryptlen, edesc, &desc->ptr[4],
-			     sg_count, areq->assoclen, tbl_off);
+	ret = talitos_sg_map(dev, areq->src, sg_link_tbl_len, edesc,
+			     &desc->ptr[4], sg_count, areq->assoclen, tbl_off);
 
 	if (ret > 1) {
 		tbl_off += ret;
-- 
2.13.3

[PATCH 06/18] crypto: talitos - fix ctr-aes-talitos

[Christophe Leroy]

ctr-aes-talitos test fails as follows on SEC2

[    0.837427] alg: skcipher: Test 1 failed (invalid result) on encryption for ctr-aes-talitos
[    0.845763] 00000000: 16 36 d5 ee 34 f8 06 25 d7 7f 8e 56 ca 88 43 45
[    0.852345] 00000010: f9 3f f7 17 2a b2 12 23 30 43 09 15 82 dd e1 97
[    0.858940] 00000020: a7 f7 32 b5 eb 25 06 13 9a ec f5 29 25 f8 4d 66
[    0.865366] 00000030: b0 03 5b 8e aa 9a 42 b6 19 33 8a e2 9d 65 96 95

This patch fixes the descriptor type which is special for CTR AES

Fixes: 5e75ae1b3cef6 ("crypto: talitos - add new crypto modes")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index cf5c9701b898..a19b5d0300a9 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -2635,7 +2635,7 @@ static struct talitos_alg_template driver_algs[] = {
 				.ivsize = AES_BLOCK_SIZE,
 			}
 		},
-		.desc_hdr_template = DESC_HDR_TYPE_COMMON_NONSNOOP_NO_AFEU |
+		.desc_hdr_template = DESC_HDR_TYPE_AESU_CTR_NONSNOOP |
 				     DESC_HDR_SEL0_AESU |
 				     DESC_HDR_MODE0_AESU_CTR,
 	},
-- 
2.13.3

[PATCH 07/18] crypto: talitos - zeroize the descriptor with memset()

[Christophe Leroy]

This patch zeroize the descriptor at allocation using memset().
This has two advantages:
- It reduces the number of places where data has to be set to 0
- It avoids reading memory and loading the cache with data that
will be entirely replaced.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 19 +------------------
 drivers/crypto/talitos.h |  2 --
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index a19b5d0300a9..266e7e626e12 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -75,7 +75,6 @@ static void to_talitos_ptr_len(struct talitos_ptr *ptr, unsigned int len,
 			       bool is_sec1)
 {
 	if (is_sec1) {
-		ptr->res = 0;
 		ptr->len1 = cpu_to_be16(len);
 	} else {
 		ptr->len = cpu_to_be16(len);
@@ -118,7 +117,6 @@ static void map_single_talitos_ptr(struct device *dev,
 
 	to_talitos_ptr_len(ptr, len, is_sec1);
 	to_talitos_ptr(ptr, dma_addr, is_sec1);
-	to_talitos_ptr_ext_set(ptr, 0, is_sec1);
 }
 
 /*
@@ -287,7 +285,6 @@ int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
 	/* map descriptor and save caller data */
 	if (is_sec1) {
 		desc->hdr1 = desc->hdr;
-		desc->next_desc = 0;
 		request->dma_desc = dma_map_single(dev, &desc->hdr1,
 						   TALITOS_DESC_SIZE,
 						   DMA_BIDIRECTIONAL);
@@ -1125,7 +1122,6 @@ int talitos_sg_map(struct device *dev, struct scatterlist *src,
 	bool is_sec1 = has_ftr_sec1(priv);
 
 	to_talitos_ptr_len(ptr, len, is_sec1);
-	to_talitos_ptr_ext_set(ptr, 0, is_sec1);
 
 	if (sg_count == 1) {
 		to_talitos_ptr(ptr, sg_dma_address(src) + offset, is_sec1);
@@ -1197,11 +1193,9 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP) {
 		to_talitos_ptr(&desc->ptr[2], edesc->iv_dma, is_sec1);
 		to_talitos_ptr_len(&desc->ptr[2], ivsize, is_sec1);
-		to_talitos_ptr_ext_set(&desc->ptr[2], 0, is_sec1);
 	} else {
 		to_talitos_ptr(&desc->ptr[3], edesc->iv_dma, is_sec1);
 		to_talitos_ptr_len(&desc->ptr[3], ivsize, is_sec1);
-		to_talitos_ptr_ext_set(&desc->ptr[3], 0, is_sec1);
 	}
 
 	/* cipher key */
@@ -1221,7 +1215,6 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	 * typically 12 for ipsec
 	 */
 	to_talitos_ptr_len(&desc->ptr[4], cryptlen, is_sec1);
-	to_talitos_ptr_ext_set(&desc->ptr[4], 0, is_sec1);
 
 	sg_link_tbl_len = cryptlen;
 
@@ -1406,6 +1399,7 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 		err = ERR_PTR(-ENOMEM);
 		goto error_sg;
 	}
+	memset(&edesc->desc, 0, sizeof(edesc->desc));
 
 	edesc->src_nents = src_nents;
 	edesc->dst_nents = dst_nents;
@@ -1481,7 +1475,6 @@ static int aead_decrypt(struct aead_request *req)
 				  DESC_HDR_MODE1_MDEU_CICV;
 
 		/* reset integrity check result bits */
-		edesc->desc.hdr_lo = 0;
 
 		return ipsec_esp(edesc, req, ipsec_esp_decrypt_hwauth_done);
 	}
@@ -1576,12 +1569,10 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 	bool is_sec1 = has_ftr_sec1(priv);
 
 	/* first DWORD empty */
-	desc->ptr[0] = zero_entry;
 
 	/* cipher iv */
 	to_talitos_ptr(&desc->ptr[1], edesc->iv_dma, is_sec1);
 	to_talitos_ptr_len(&desc->ptr[1], ivsize, is_sec1);
-	to_talitos_ptr_ext_set(&desc->ptr[1], 0, is_sec1);
 
 	/* cipher key */
 	map_single_talitos_ptr(dev, &desc->ptr[2], ctx->keylen,
@@ -1620,7 +1611,6 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 			       DMA_FROM_DEVICE);
 
 	/* last DWORD empty */
-	desc->ptr[6] = zero_entry;
 
 	if (sync_needed)
 		dma_sync_single_for_device(dev, edesc->dma_link_tbl,
@@ -1766,7 +1756,6 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 	int sg_count;
 
 	/* first DWORD empty */
-	desc->ptr[0] = zero_entry;
 
 	/* hash context in */
 	if (!req_ctx->first || req_ctx->swinit) {
@@ -1775,8 +1764,6 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				       (char *)req_ctx->hw_context,
 				       DMA_TO_DEVICE);
 		req_ctx->swinit = 0;
-	} else {
-		desc->ptr[1] = zero_entry;
 	}
 	/* Indicate next op is not the first. */
 	req_ctx->first = 0;
@@ -1785,8 +1772,6 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 	if (ctx->keylen)
 		map_single_talitos_ptr(dev, &desc->ptr[2], ctx->keylen,
 				       (char *)&ctx->key, DMA_TO_DEVICE);
-	else
-		desc->ptr[2] = zero_entry;
 
 	sg_count = edesc->src_nents ?: 1;
 	if (is_sec1 && sg_count > 1)
@@ -1803,7 +1788,6 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 		sync_needed = true;
 
 	/* fifth DWORD empty */
-	desc->ptr[4] = zero_entry;
 
 	/* hash/HMAC out -or- hash context out */
 	if (req_ctx->last)
@@ -1816,7 +1800,6 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				       req_ctx->hw_context, DMA_FROM_DEVICE);
 
 	/* last DWORD empty */
-	desc->ptr[6] = zero_entry;
 
 	if (is_sec1 && from_talitos_ptr_len(&desc->ptr[3], true) == 0)
 		talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 8dd8f40e2771..6112ff1fc334 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -52,8 +52,6 @@ struct talitos_ptr {
 	__be32 ptr;     /* address */
 };
 
-static const struct talitos_ptr zero_entry;
-
 /* descriptor */
 struct talitos_desc {
 	__be32 hdr;                     /* header high bits */
-- 
2.13.3

[PATCH 08/18] crypto: talitos - declare local functions static

[Christophe Leroy]

talitos_handle_buggy_hash() and talitos_sg_map() are only used
locally, make them static

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 266e7e626e12..dd6b1fc90020 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1113,7 +1113,7 @@ static int sg_to_link_tbl_offset(struct scatterlist *sg, int sg_count,
 	return count;
 }
 
-int talitos_sg_map(struct device *dev, struct scatterlist *src,
+static int talitos_sg_map(struct device *dev, struct scatterlist *src,
 		   unsigned int len, struct talitos_edesc *edesc,
 		   struct talitos_ptr *ptr,
 		   int sg_count, unsigned int offset, int tbl_off)
@@ -1721,7 +1721,7 @@ static void ahash_done(struct device *dev,
  * SEC1 doesn't like hashing of 0 sized message, so we do the padding
  * ourself and submit a padded block
  */
-void talitos_handle_buggy_hash(struct talitos_ctx *ctx,
+static void talitos_handle_buggy_hash(struct talitos_ctx *ctx,
 			       struct talitos_edesc *edesc,
 			       struct talitos_ptr *ptr)
 {
-- 
2.13.3

[PATCH 09/18] crypto: talitos - use devm_kmalloc()

[Christophe Leroy]

Replace kmalloc() by devm_kmalloc()

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index dd6b1fc90020..2a53d0f2a869 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -2993,17 +2993,11 @@ static int talitos_remove(struct platform_device *ofdev)
 			break;
 		}
 		list_del(&t_alg->entry);
-		kfree(t_alg);
 	}
 
 	if (hw_supports(dev, DESC_HDR_SEL0_RNG))
 		talitos_unregister_rng(dev);
 
-	for (i = 0; priv->chan && i < priv->num_channels; i++)
-		kfree(priv->chan[i].fifo);
-
-	kfree(priv->chan);
-
 	for (i = 0; i < 2; i++)
 		if (priv->irq[i]) {
 			free_irq(priv->irq[i], dev);
@@ -3016,8 +3010,6 @@ static int talitos_remove(struct platform_device *ofdev)
 
 	iounmap(priv->reg);
 
-	kfree(priv);
-
 	return 0;
 }
 
@@ -3029,7 +3021,8 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
 	struct talitos_crypto_alg *t_alg;
 	struct crypto_alg *alg;
 
-	t_alg = kzalloc(sizeof(struct talitos_crypto_alg), GFP_KERNEL);
+	t_alg = devm_kzalloc(dev, sizeof(struct talitos_crypto_alg),
+			     GFP_KERNEL);
 	if (!t_alg)
 		return ERR_PTR(-ENOMEM);
 
@@ -3053,7 +3046,7 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
 		t_alg->algt.alg.aead.decrypt = aead_decrypt;
 		if (!(priv->features & TALITOS_FTR_SHA224_HWINIT) &&
 		    !strncmp(alg->cra_name, "authenc(hmac(sha224)", 20)) {
-			kfree(t_alg);
+			devm_kfree(dev, t_alg);
 			return ERR_PTR(-ENOTSUPP);
 		}
 		break;
@@ -3073,7 +3066,7 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
 
 		if (!(priv->features & TALITOS_FTR_HMAC_OK) &&
 		    !strncmp(alg->cra_name, "hmac", 4)) {
-			kfree(t_alg);
+			devm_kfree(dev, t_alg);
 			return ERR_PTR(-ENOTSUPP);
 		}
 		if (!(priv->features & TALITOS_FTR_SHA224_HWINIT) &&
@@ -3088,7 +3081,7 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
 		break;
 	default:
 		dev_err(dev, "unknown algorithm type %d\n", t_alg->algt.type);
-		kfree(t_alg);
+		devm_kfree(dev, t_alg);
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -3169,7 +3162,7 @@ static int talitos_probe(struct platform_device *ofdev)
 	int i, err;
 	int stride;
 
-	priv = kzalloc(sizeof(struct talitos_private), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(struct talitos_private), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
@@ -3267,8 +3260,8 @@ static int talitos_probe(struct platform_device *ofdev)
 		}
 	}
 
-	priv->chan = kzalloc(sizeof(struct talitos_channel) *
-			     priv->num_channels, GFP_KERNEL);
+	priv->chan = devm_kzalloc(dev, sizeof(struct talitos_channel) *
+				       priv->num_channels, GFP_KERNEL);
 	if (!priv->chan) {
 		dev_err(dev, "failed to allocate channel management space\n");
 		err = -ENOMEM;
@@ -3285,8 +3278,9 @@ static int talitos_probe(struct platform_device *ofdev)
 		spin_lock_init(&priv->chan[i].head_lock);
 		spin_lock_init(&priv->chan[i].tail_lock);
 
-		priv->chan[i].fifo = kzalloc(sizeof(struct talitos_request) *
-					     priv->fifo_len, GFP_KERNEL);
+		priv->chan[i].fifo = devm_kzalloc(dev,
+						sizeof(struct talitos_request) *
+						priv->fifo_len, GFP_KERNEL);
 		if (!priv->chan[i].fifo) {
 			dev_err(dev, "failed to allocate request fifo %d\n", i);
 			err = -ENOMEM;
@@ -3352,7 +3346,7 @@ static int talitos_probe(struct platform_device *ofdev)
 			if (err) {
 				dev_err(dev, "%s alg registration failed\n",
 					alg->cra_driver_name);
-				kfree(t_alg);
+				devm_kfree(dev, t_alg);
 			} else
 				list_add_tail(&t_alg->entry, &priv->alg_list);
 		}
-- 
2.13.3

[PATCH 10/18] crypto: talitos - use of_property_read_u32()

[Christophe Leroy]

Use of_property_read_u32() to simplify DT read

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 2a53d0f2a869..f139a0cef2e2 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -3158,7 +3158,6 @@ static int talitos_probe(struct platform_device *ofdev)
 	struct device *dev = &ofdev->dev;
 	struct device_node *np = ofdev->dev.of_node;
 	struct talitos_private *priv;
-	const unsigned int *prop;
 	int i, err;
 	int stride;
 
@@ -3182,21 +3181,11 @@ static int talitos_probe(struct platform_device *ofdev)
 	}
 
 	/* get SEC version capabilities from device tree */
-	prop = of_get_property(np, "fsl,num-channels", NULL);
-	if (prop)
-		priv->num_channels = *prop;
-
-	prop = of_get_property(np, "fsl,channel-fifo-len", NULL);
-	if (prop)
-		priv->chfifo_len = *prop;
-
-	prop = of_get_property(np, "fsl,exec-units-mask", NULL);
-	if (prop)
-		priv->exec_units = *prop;
-
-	prop = of_get_property(np, "fsl,descriptor-types-mask", NULL);
-	if (prop)
-		priv->desc_types = *prop;
+	of_property_read_u32(np, "fsl,num-channels", &priv->num_channels);
+	of_property_read_u32(np, "fsl,channel-fifo-len", &priv->chfifo_len);
+	of_property_read_u32(np, "fsl,exec-units-mask", &priv->exec_units);
+	of_property_read_u32(np, "fsl,descriptor-types-mask",
+			     &priv->desc_types);
 
 	if (!is_power_of_2(priv->num_channels) || !priv->chfifo_len ||
 	    !priv->exec_units || !priv->desc_types) {
-- 
2.13.3

[PATCH 11/18] crypto: talitos - use devm_ioremap()

[Christophe Leroy]

Use devm_ioremap()

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index f139a0cef2e2..83b2a70a1ba7 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -3008,8 +3008,6 @@ static int talitos_remove(struct platform_device *ofdev)
 	if (priv->irq[1])
 		tasklet_kill(&priv->done_task[1]);
 
-	iounmap(priv->reg);
-
 	return 0;
 }
 
@@ -3160,6 +3158,7 @@ static int talitos_probe(struct platform_device *ofdev)
 	struct talitos_private *priv;
 	int i, err;
 	int stride;
+	struct resource *res;
 
 	priv = devm_kzalloc(dev, sizeof(struct talitos_private), GFP_KERNEL);
 	if (!priv)
@@ -3173,7 +3172,10 @@ static int talitos_probe(struct platform_device *ofdev)
 
 	spin_lock_init(&priv->reg_lock);
 
-	priv->reg = of_iomap(np, 0);
+	res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENXIO;
+	priv->reg = devm_ioremap(dev, res->start, resource_size(res));
 	if (!priv->reg) {
 		dev_err(dev, "failed to of_iomap\n");
 		err = -ENOMEM;
-- 
2.13.3

[PATCH 12/18] crypto: talitos - don't check the number of channels at each interrupt

[Christophe Leroy]

The number of channels is known from the beginning, no need to
test it everytime.
This patch defines two additional done functions handling only channel 0.
Then the probe registers the correct one based on the number of channels.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 27 +++++++++++++++------------
 drivers/crypto/talitos.h |  4 ++++
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 83b2a70a1ba7..e7e1bada03df 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -390,8 +390,6 @@ static void talitos1_done_##name(unsigned long data)			\
 									\
 	if (ch_done_mask & 0x10000000)					\
 		flush_channel(dev, 0, 0, 0);			\
-	if (priv->num_channels == 1)					\
-		goto out;						\
 	if (ch_done_mask & 0x40000000)					\
 		flush_channel(dev, 1, 0, 0);			\
 	if (ch_done_mask & 0x00010000)					\
@@ -399,7 +397,6 @@ static void talitos1_done_##name(unsigned long data)			\
 	if (ch_done_mask & 0x00040000)					\
 		flush_channel(dev, 3, 0, 0);			\
 									\
-out:									\
 	/* At this point, all completed channels have been processed */	\
 	/* Unmask done interrupts for channels completed later on. */	\
 	spin_lock_irqsave(&priv->reg_lock, flags);			\
@@ -409,6 +406,7 @@ out:									\
 }
 
 DEF_TALITOS1_DONE(4ch, TALITOS1_ISR_4CHDONE)
+DEF_TALITOS1_DONE(ch0, TALITOS1_ISR_CH_0_DONE)
 
 #define DEF_TALITOS2_DONE(name, ch_done_mask)				\
 static void talitos2_done_##name(unsigned long data)			\
@@ -419,8 +417,6 @@ static void talitos2_done_##name(unsigned long data)			\
 									\
 	if (ch_done_mask & 1)						\
 		flush_channel(dev, 0, 0, 0);				\
-	if (priv->num_channels == 1)					\
-		goto out;						\
 	if (ch_done_mask & (1 << 2))					\
 		flush_channel(dev, 1, 0, 0);				\
 	if (ch_done_mask & (1 << 4))					\
@@ -428,7 +424,6 @@ static void talitos2_done_##name(unsigned long data)			\
 	if (ch_done_mask & (1 << 6))					\
 		flush_channel(dev, 3, 0, 0);				\
 									\
-out:									\
 	/* At this point, all completed channels have been processed */	\
 	/* Unmask done interrupts for channels completed later on. */	\
 	spin_lock_irqsave(&priv->reg_lock, flags);			\
@@ -438,6 +433,7 @@ out:									\
 }
 
 DEF_TALITOS2_DONE(4ch, TALITOS2_ISR_4CHDONE)
+DEF_TALITOS2_DONE(ch0, TALITOS2_ISR_CH_0_DONE)
 DEF_TALITOS2_DONE(ch0_2, TALITOS2_ISR_CH_0_2_DONE)
 DEF_TALITOS2_DONE(ch1_3, TALITOS2_ISR_CH_1_3_DONE)
 
@@ -3237,17 +3233,24 @@ static int talitos_probe(struct platform_device *ofdev)
 		goto err_out;
 
 	if (of_device_is_compatible(np, "fsl,sec1.0")) {
-		tasklet_init(&priv->done_task[0], talitos1_done_4ch,
-			     (unsigned long)dev);
-	} else {
-		if (!priv->irq[1]) {
-			tasklet_init(&priv->done_task[0], talitos2_done_4ch,
+		if (priv->num_channels == 1)
+			tasklet_init(&priv->done_task[0], talitos1_done_ch0,
 				     (unsigned long)dev);
-		} else {
+		else
+			tasklet_init(&priv->done_task[0], talitos1_done_4ch,
+				     (unsigned long)dev);
+	} else {
+		if (priv->irq[1]) {
 			tasklet_init(&priv->done_task[0], talitos2_done_ch0_2,
 				     (unsigned long)dev);
 			tasklet_init(&priv->done_task[1], talitos2_done_ch1_3,
 				     (unsigned long)dev);
+		} else if (priv->num_channels == 1) {
+			tasklet_init(&priv->done_task[0], talitos2_done_ch0,
+				     (unsigned long)dev);
+		} else {
+			tasklet_init(&priv->done_task[0], talitos2_done_4ch,
+				     (unsigned long)dev);
 		}
 	}
 
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 6112ff1fc334..2f04d83c3062 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -208,9 +208,13 @@ static inline bool has_ftr_sec1(struct talitos_private *priv)
 #define TALITOS_ISR			0x1010  /* interrupt status register */
 #define   TALITOS1_ISR_4CHERR		ISR1_FORMAT(0xa) /* 4 ch errors mask */
 #define   TALITOS1_ISR_4CHDONE		ISR1_FORMAT(0x5) /* 4 ch done mask */
+#define   TALITOS1_ISR_CH_0_ERR		(2 << 28) /* ch 0 errors mask */
+#define   TALITOS1_ISR_CH_0_DONE	(1 << 28) /* ch 0 done mask */
 #define   TALITOS1_ISR_TEA_ERR		0x00000040
 #define   TALITOS2_ISR_4CHERR		ISR2_FORMAT(0xa) /* 4 ch errors mask */
 #define   TALITOS2_ISR_4CHDONE		ISR2_FORMAT(0x5) /* 4 ch done mask */
+#define   TALITOS2_ISR_CH_0_ERR		2 /* ch 0 errors mask */
+#define   TALITOS2_ISR_CH_0_DONE	1 /* ch 0 done mask */
 #define   TALITOS2_ISR_CH_0_2_ERR	ISR2_FORMAT(0x2) /* ch 0, 2 err mask */
 #define   TALITOS2_ISR_CH_0_2_DONE	ISR2_FORMAT(0x1) /* ch 0, 2 done mask */
 #define   TALITOS2_ISR_CH_1_3_ERR	ISR2_FORMAT(0x8) /* ch 1, 3 err mask */
-- 
2.13.3

[PATCH 13/18] crypto: talitos - remove to_talitos_ptr_len()

[Christophe Leroy]

to_talitos_ptr() and to_talitos_ptr_len() are always called together
in order to fully set a ptr, so lets merge them into a single
helper.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 56 ++++++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index e7e1bada03df..7e96db75347a 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -56,28 +56,26 @@
 #include "talitos.h"
 
 static void to_talitos_ptr(struct talitos_ptr *ptr, dma_addr_t dma_addr,
-			   bool is_sec1)
+			   unsigned int len, bool is_sec1)
 {
 	ptr->ptr = cpu_to_be32(lower_32_bits(dma_addr));
-	if (!is_sec1)
+	if (is_sec1) {
+		ptr->len1 = cpu_to_be16(len);
+	} else {
+		ptr->len = cpu_to_be16(len);
 		ptr->eptr = upper_32_bits(dma_addr);
+	}
 }
 
 static void copy_talitos_ptr(struct talitos_ptr *dst_ptr,
 			     struct talitos_ptr *src_ptr, bool is_sec1)
 {
 	dst_ptr->ptr = src_ptr->ptr;
-	if (!is_sec1)
-		dst_ptr->eptr = src_ptr->eptr;
-}
-
-static void to_talitos_ptr_len(struct talitos_ptr *ptr, unsigned int len,
-			       bool is_sec1)
-{
 	if (is_sec1) {
-		ptr->len1 = cpu_to_be16(len);
+		dst_ptr->len1 = src_ptr->len1;
 	} else {
-		ptr->len = cpu_to_be16(len);
+		dst_ptr->len = src_ptr->len;
+		dst_ptr->eptr = src_ptr->eptr;
 	}
 }
 
@@ -115,8 +113,7 @@ static void map_single_talitos_ptr(struct device *dev,
 	struct talitos_private *priv = dev_get_drvdata(dev);
 	bool is_sec1 = has_ftr_sec1(priv);
 
-	to_talitos_ptr_len(ptr, len, is_sec1);
-	to_talitos_ptr(ptr, dma_addr, is_sec1);
+	to_talitos_ptr(ptr, dma_addr, len, is_sec1);
 }
 
 /*
@@ -1090,8 +1087,7 @@ static int sg_to_link_tbl_offset(struct scatterlist *sg, int sg_count,
 			len = cryptlen;
 
 		to_talitos_ptr(link_tbl_ptr + count,
-			       sg_dma_address(sg) + offset, 0);
-		to_talitos_ptr_len(link_tbl_ptr + count, len, 0);
+			       sg_dma_address(sg) + offset, len, 0);
 		to_talitos_ptr_ext_set(link_tbl_ptr + count, 0, 0);
 		count++;
 		cryptlen -= len;
@@ -1117,14 +1113,12 @@ static int talitos_sg_map(struct device *dev, struct scatterlist *src,
 	struct talitos_private *priv = dev_get_drvdata(dev);
 	bool is_sec1 = has_ftr_sec1(priv);
 
-	to_talitos_ptr_len(ptr, len, is_sec1);
-
 	if (sg_count == 1) {
-		to_talitos_ptr(ptr, sg_dma_address(src) + offset, is_sec1);
+		to_talitos_ptr(ptr, sg_dma_address(src) + offset, len, is_sec1);
 		return sg_count;
 	}
 	if (is_sec1) {
-		to_talitos_ptr(ptr, edesc->dma_link_tbl + offset, is_sec1);
+		to_talitos_ptr(ptr, edesc->dma_link_tbl + offset, len, is_sec1);
 		return sg_count;
 	}
 	sg_count = sg_to_link_tbl_offset(src, sg_count, offset, len,
@@ -1135,7 +1129,7 @@ static int talitos_sg_map(struct device *dev, struct scatterlist *src,
 		return sg_count;
 	}
 	to_talitos_ptr(ptr, edesc->dma_link_tbl +
-			    tbl_off * sizeof(struct talitos_ptr), is_sec1);
+			    tbl_off * sizeof(struct talitos_ptr), len, is_sec1);
 	to_talitos_ptr_ext_or(ptr, DESC_PTR_LNKTBL_JUMP, is_sec1);
 
 	return sg_count;
@@ -1186,13 +1180,10 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	}
 
 	/* cipher iv */
-	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP) {
-		to_talitos_ptr(&desc->ptr[2], edesc->iv_dma, is_sec1);
-		to_talitos_ptr_len(&desc->ptr[2], ivsize, is_sec1);
-	} else {
-		to_talitos_ptr(&desc->ptr[3], edesc->iv_dma, is_sec1);
-		to_talitos_ptr_len(&desc->ptr[3], ivsize, is_sec1);
-	}
+	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)
+		to_talitos_ptr(&desc->ptr[2], edesc->iv_dma, ivsize, is_sec1);
+	else
+		to_talitos_ptr(&desc->ptr[3], edesc->iv_dma, ivsize, is_sec1);
 
 	/* cipher key */
 	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)
@@ -1210,8 +1201,6 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	 * extent is bytes of HMAC postpended to ciphertext,
 	 * typically 12 for ipsec
 	 */
-	to_talitos_ptr_len(&desc->ptr[4], cryptlen, is_sec1);
-
 	sg_link_tbl_len = cryptlen;
 
 	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP) {
@@ -1257,11 +1246,10 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 			to_talitos_ptr_ext_set(tbl_ptr - 1, 0, is_sec1);
 			to_talitos_ptr_ext_set(tbl_ptr, DESC_PTR_LNKTBL_RETURN,
 					       is_sec1);
-			to_talitos_ptr_len(tbl_ptr, authsize, is_sec1);
 
 			/* icv data follows link tables */
 			to_talitos_ptr(tbl_ptr, edesc->dma_link_tbl + offset,
-				       is_sec1);
+				       authsize, is_sec1);
 		} else {
 			dma_addr_t addr = edesc->dma_link_tbl;
 
@@ -1270,8 +1258,7 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 			else
 				addr += sizeof(struct talitos_ptr) * tbl_off;
 
-			to_talitos_ptr(&desc->ptr[6], addr, is_sec1);
-			to_talitos_ptr_len(&desc->ptr[6], authsize, is_sec1);
+			to_talitos_ptr(&desc->ptr[6], addr, authsize, is_sec1);
 		}
 	} else if (!(desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)) {
 		ret = talitos_sg_map(dev, areq->dst, authsize, edesc,
@@ -1567,8 +1554,7 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 	/* first DWORD empty */
 
 	/* cipher iv */
-	to_talitos_ptr(&desc->ptr[1], edesc->iv_dma, is_sec1);
-	to_talitos_ptr_len(&desc->ptr[1], ivsize, is_sec1);
+	to_talitos_ptr(&desc->ptr[1], edesc->iv_dma, ivsize, is_sec1);
 
 	/* cipher key */
 	map_single_talitos_ptr(dev, &desc->ptr[2], ctx->keylen,
-- 
2.13.3

[PATCH 14/18] crypto: talitos - simplify tests in ipsec_esp()

[Christophe Leroy]

Do (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP) only once.
Limit number of if/else paths

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 7e96db75347a..307d534a0f2f 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -938,12 +938,15 @@ static void ipsec_esp_unmap(struct device *dev,
 	struct crypto_aead *aead = crypto_aead_reqtfm(areq);
 	struct talitos_ctx *ctx = crypto_aead_ctx(aead);
 	unsigned int ivsize = crypto_aead_ivsize(aead);
+	bool is_ipsec_esp = edesc->desc.hdr & DESC_HDR_TYPE_IPSEC_ESP;
+	struct talitos_ptr *civ_ptr = &edesc->desc.ptr[is_ipsec_esp ? 2 : 3];
+	struct talitos_ptr *ckey_ptr = &edesc->desc.ptr[is_ipsec_esp ? 3 : 2];
 
-	if (edesc->desc.hdr & DESC_HDR_TYPE_IPSEC_ESP)
+	if (is_ipsec_esp)
 		unmap_single_talitos_ptr(dev, &edesc->desc.ptr[6],
 					 DMA_FROM_DEVICE);
-	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[3], DMA_TO_DEVICE);
-	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[2], DMA_TO_DEVICE);
+	unmap_single_talitos_ptr(dev, ckey_ptr, DMA_TO_DEVICE);
+	unmap_single_talitos_ptr(dev, civ_ptr, DMA_TO_DEVICE);
 	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[0], DMA_TO_DEVICE);
 
 	talitos_sg_unmap(dev, edesc, areq->src, areq->dst, areq->cryptlen,
@@ -953,7 +956,7 @@ static void ipsec_esp_unmap(struct device *dev,
 		dma_unmap_single(dev, edesc->dma_link_tbl, edesc->dma_len,
 				 DMA_BIDIRECTIONAL);
 
-	if (!(edesc->desc.hdr & DESC_HDR_TYPE_IPSEC_ESP)) {
+	if (!is_ipsec_esp) {
 		unsigned int dst_nents = edesc->dst_nents ? : 1;
 
 		sg_pcopy_to_buffer(areq->dst, dst_nents, ctx->iv, ivsize,
@@ -1156,6 +1159,9 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	bool sync_needed = false;
 	struct talitos_private *priv = dev_get_drvdata(dev);
 	bool is_sec1 = has_ftr_sec1(priv);
+	bool is_ipsec_esp = desc->hdr & DESC_HDR_TYPE_IPSEC_ESP;
+	struct talitos_ptr *civ_ptr = &desc->ptr[is_ipsec_esp ? 2 : 3];
+	struct talitos_ptr *ckey_ptr = &desc->ptr[is_ipsec_esp ? 3 : 2];
 
 	/* hmac key */
 	map_single_talitos_ptr(dev, &desc->ptr[0], ctx->authkeylen, &ctx->key,
@@ -1180,20 +1186,12 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	}
 
 	/* cipher iv */
-	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)
-		to_talitos_ptr(&desc->ptr[2], edesc->iv_dma, ivsize, is_sec1);
-	else
-		to_talitos_ptr(&desc->ptr[3], edesc->iv_dma, ivsize, is_sec1);
+	to_talitos_ptr(civ_ptr, edesc->iv_dma, ivsize, is_sec1);
 
 	/* cipher key */
-	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)
-		map_single_talitos_ptr(dev, &desc->ptr[3], ctx->enckeylen,
-				       (char *)&ctx->key + ctx->authkeylen,
-				       DMA_TO_DEVICE);
-	else
-		map_single_talitos_ptr(dev, &desc->ptr[2], ctx->enckeylen,
-				       (char *)&ctx->key + ctx->authkeylen,
-				       DMA_TO_DEVICE);
+	map_single_talitos_ptr(dev, ckey_ptr, ctx->enckeylen,
+			       (char *)&ctx->key + ctx->authkeylen,
+			       DMA_TO_DEVICE);
 
 	/*
 	 * cipher in
@@ -1203,10 +1201,10 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	 */
 	sg_link_tbl_len = cryptlen;
 
-	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP) {
+	if (is_ipsec_esp) {
 		to_talitos_ptr_ext_set(&desc->ptr[4], authsize, is_sec1);
 
-		if (edesc->desc.hdr & DESC_HDR_MODE1_MDEU_CICV)
+		if (desc->hdr & DESC_HDR_MODE1_MDEU_CICV)
 			sg_link_tbl_len += authsize;
 	}
 
@@ -1228,7 +1226,7 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	ret = talitos_sg_map(dev, areq->dst, cryptlen, edesc, &desc->ptr[5],
 			     sg_count, areq->assoclen, tbl_off);
 
-	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)
+	if (is_ipsec_esp)
 		to_talitos_ptr_ext_or(&desc->ptr[5], authsize, is_sec1);
 
 	/* ICV data */
@@ -1237,7 +1235,7 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 		edesc->icv_ool = true;
 		sync_needed = true;
 
-		if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP) {
+		if (is_ipsec_esp) {
 			struct talitos_ptr *tbl_ptr = &edesc->link_tbl[tbl_off];
 			int offset = (edesc->src_nents + edesc->dst_nents + 2) *
 				     sizeof(struct talitos_ptr) + authsize;
@@ -1260,7 +1258,7 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 
 			to_talitos_ptr(&desc->ptr[6], addr, authsize, is_sec1);
 		}
-	} else if (!(desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)) {
+	} else if (!is_ipsec_esp) {
 		ret = talitos_sg_map(dev, areq->dst, authsize, edesc,
 				     &desc->ptr[6], sg_count, areq->assoclen +
 							      cryptlen,
@@ -1277,7 +1275,7 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	}
 
 	/* iv out */
-	if (desc->hdr & DESC_HDR_TYPE_IPSEC_ESP)
+	if (is_ipsec_esp)
 		map_single_talitos_ptr(dev, &desc->ptr[6], ivsize, ctx->iv,
 				       DMA_FROM_DEVICE);
 
-- 
2.13.3

[PATCH 15/18] crypto: talitos - DMA map key in setkey()

[Christophe Leroy]

dma_map_single() is an heavy operation which doesn't need to
be done at each request as the key doesn't change.

Instead of DMA mapping the key at every request, this patch maps it
once in setkey()

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 56 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 307d534a0f2f..ebfd6d982ed6 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -815,6 +815,7 @@ struct talitos_ctx {
 	__be32 desc_hdr_template;
 	u8 key[TALITOS_MAX_KEY_SIZE];
 	u8 iv[TALITOS_MAX_IV_LENGTH];
+	dma_addr_t dma_key;
 	unsigned int keylen;
 	unsigned int enckeylen;
 	unsigned int authkeylen;
@@ -851,6 +852,7 @@ static int aead_setkey(struct crypto_aead *authenc,
 		       const u8 *key, unsigned int keylen)
 {
 	struct talitos_ctx *ctx = crypto_aead_ctx(authenc);
+	struct device *dev = ctx->dev;
 	struct crypto_authenc_keys keys;
 
 	if (crypto_authenc_extractkeys(&keys, key, keylen) != 0)
@@ -859,12 +861,17 @@ static int aead_setkey(struct crypto_aead *authenc,
 	if (keys.authkeylen + keys.enckeylen > TALITOS_MAX_KEY_SIZE)
 		goto badkey;
 
+	if (ctx->keylen)
+		dma_unmap_single(dev, ctx->dma_key, ctx->keylen, DMA_TO_DEVICE);
+
 	memcpy(ctx->key, keys.authkey, keys.authkeylen);
 	memcpy(&ctx->key[keys.authkeylen], keys.enckey, keys.enckeylen);
 
 	ctx->keylen = keys.authkeylen + keys.enckeylen;
 	ctx->enckeylen = keys.enckeylen;
 	ctx->authkeylen = keys.authkeylen;
+	ctx->dma_key = dma_map_single(dev, ctx->key, ctx->keylen,
+				      DMA_TO_DEVICE);
 
 	return 0;
 
@@ -940,14 +947,11 @@ static void ipsec_esp_unmap(struct device *dev,
 	unsigned int ivsize = crypto_aead_ivsize(aead);
 	bool is_ipsec_esp = edesc->desc.hdr & DESC_HDR_TYPE_IPSEC_ESP;
 	struct talitos_ptr *civ_ptr = &edesc->desc.ptr[is_ipsec_esp ? 2 : 3];
-	struct talitos_ptr *ckey_ptr = &edesc->desc.ptr[is_ipsec_esp ? 3 : 2];
 
 	if (is_ipsec_esp)
 		unmap_single_talitos_ptr(dev, &edesc->desc.ptr[6],
 					 DMA_FROM_DEVICE);
-	unmap_single_talitos_ptr(dev, ckey_ptr, DMA_TO_DEVICE);
 	unmap_single_talitos_ptr(dev, civ_ptr, DMA_TO_DEVICE);
-	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[0], DMA_TO_DEVICE);
 
 	talitos_sg_unmap(dev, edesc, areq->src, areq->dst, areq->cryptlen,
 			 areq->assoclen);
@@ -976,6 +980,7 @@ static void ipsec_esp_encrypt_done(struct device *dev,
 	struct aead_request *areq = context;
 	struct crypto_aead *authenc = crypto_aead_reqtfm(areq);
 	unsigned int authsize = crypto_aead_authsize(authenc);
+	unsigned int ivsize = crypto_aead_ivsize(authenc);
 	struct talitos_edesc *edesc;
 	struct scatterlist *sg;
 	void *icvdata;
@@ -996,6 +1001,8 @@ static void ipsec_esp_encrypt_done(struct device *dev,
 		       icvdata, authsize);
 	}
 
+	dma_unmap_single(dev, edesc->iv_dma, ivsize, DMA_TO_DEVICE);
+
 	kfree(edesc);
 
 	aead_request_complete(areq, err);
@@ -1164,8 +1171,7 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	struct talitos_ptr *ckey_ptr = &desc->ptr[is_ipsec_esp ? 3 : 2];
 
 	/* hmac key */
-	map_single_talitos_ptr(dev, &desc->ptr[0], ctx->authkeylen, &ctx->key,
-			       DMA_TO_DEVICE);
+	to_talitos_ptr(&desc->ptr[0], ctx->dma_key, ctx->authkeylen, is_sec1);
 
 	sg_count = edesc->src_nents ?: 1;
 	if (is_sec1 && sg_count > 1)
@@ -1189,9 +1195,8 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 	to_talitos_ptr(civ_ptr, edesc->iv_dma, ivsize, is_sec1);
 
 	/* cipher key */
-	map_single_talitos_ptr(dev, ckey_ptr, ctx->enckeylen,
-			       (char *)&ctx->key + ctx->authkeylen,
-			       DMA_TO_DEVICE);
+	to_talitos_ptr(ckey_ptr, ctx->dma_key  + ctx->authkeylen,
+		       ctx->enckeylen, is_sec1);
 
 	/*
 	 * cipher in
@@ -1481,6 +1486,7 @@ static int ablkcipher_setkey(struct crypto_ablkcipher *cipher,
 			     const u8 *key, unsigned int keylen)
 {
 	struct talitos_ctx *ctx = crypto_ablkcipher_ctx(cipher);
+	struct device *dev = ctx->dev;
 	u32 tmp[DES_EXPKEY_WORDS];
 
 	if (keylen > TALITOS_MAX_KEY_SIZE) {
@@ -1495,9 +1501,14 @@ static int ablkcipher_setkey(struct crypto_ablkcipher *cipher,
 		return -EINVAL;
 	}
 
+	if (ctx->keylen)
+		dma_unmap_single(dev, ctx->dma_key, ctx->keylen, DMA_TO_DEVICE);
+
 	memcpy(&ctx->key, key, keylen);
 	ctx->keylen = keylen;
 
+	ctx->dma_key = dma_map_single(dev, ctx->key, keylen, DMA_TO_DEVICE);
+
 	return 0;
 }
 
@@ -1508,7 +1519,6 @@ static void common_nonsnoop_unmap(struct device *dev,
 	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[5], DMA_FROM_DEVICE);
 
 	talitos_sg_unmap(dev, edesc, areq->src, areq->dst, areq->nbytes, 0);
-	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[2], DMA_TO_DEVICE);
 	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[1], DMA_TO_DEVICE);
 
 	if (edesc->dma_len)
@@ -1555,8 +1565,7 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 	to_talitos_ptr(&desc->ptr[1], edesc->iv_dma, ivsize, is_sec1);
 
 	/* cipher key */
-	map_single_talitos_ptr(dev, &desc->ptr[2], ctx->keylen,
-			       (char *)&ctx->key, DMA_TO_DEVICE);
+	to_talitos_ptr(&desc->ptr[2], ctx->dma_key, ctx->keylen, is_sec1);
 
 	sg_count = edesc->src_nents ?: 1;
 	if (is_sec1 && sg_count > 1)
@@ -1666,10 +1675,6 @@ static void common_nonsnoop_hash_unmap(struct device *dev,
 		unmap_single_talitos_ptr(dev, &edesc->desc.ptr[1],
 					 DMA_TO_DEVICE);
 
-	if (from_talitos_ptr_len(&edesc->desc.ptr[2], is_sec1))
-		unmap_single_talitos_ptr(dev, &edesc->desc.ptr[2],
-					 DMA_TO_DEVICE);
-
 	if (edesc->dma_len)
 		dma_unmap_single(dev, edesc->dma_link_tbl, edesc->dma_len,
 				 DMA_BIDIRECTIONAL);
@@ -1750,8 +1755,8 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 
 	/* HMAC key */
 	if (ctx->keylen)
-		map_single_talitos_ptr(dev, &desc->ptr[2], ctx->keylen,
-				       (char *)&ctx->key, DMA_TO_DEVICE);
+		to_talitos_ptr(&desc->ptr[2], ctx->dma_key, ctx->keylen,
+			       is_sec1);
 
 	sg_count = edesc->src_nents ?: 1;
 	if (is_sec1 && sg_count > 1)
@@ -2084,6 +2089,7 @@ static int ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
 			unsigned int keylen)
 {
 	struct talitos_ctx *ctx = crypto_tfm_ctx(crypto_ahash_tfm(tfm));
+	struct device *dev = ctx->dev;
 	unsigned int blocksize =
 			crypto_tfm_alg_blocksize(crypto_ahash_tfm(tfm));
 	unsigned int digestsize = crypto_ahash_digestsize(tfm);
@@ -2106,7 +2112,11 @@ static int ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
 		memcpy(ctx->key, hash, digestsize);
 	}
 
+	if (ctx->keylen)
+		dma_unmap_single(dev, ctx->dma_key, ctx->keylen, DMA_TO_DEVICE);
+
 	ctx->keylen = keysize;
+	ctx->dma_key = dma_map_single(dev, ctx->key, keysize, DMA_TO_DEVICE);
 
 	return 0;
 }
@@ -2935,6 +2945,15 @@ static int talitos_cra_init_ahash(struct crypto_tfm *tfm)
 	return 0;
 }
 
+static void talitos_cra_exit(struct crypto_tfm *tfm)
+{
+	struct talitos_ctx *ctx = crypto_tfm_ctx(tfm);
+	struct device *dev = ctx->dev;
+
+	if (ctx->keylen)
+		dma_unmap_single(dev, ctx->dma_key, ctx->keylen, DMA_TO_DEVICE);
+}
+
 /*
  * given the alg's descriptor header template, determine whether descriptor
  * type and primary/secondary execution units required match the hw
@@ -3010,6 +3029,7 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
 	case CRYPTO_ALG_TYPE_ABLKCIPHER:
 		alg = &t_alg->algt.alg.crypto;
 		alg->cra_init = talitos_cra_init;
+		alg->cra_exit = talitos_cra_exit;
 		alg->cra_type = &crypto_ablkcipher_type;
 		alg->cra_ablkcipher.setkey = ablkcipher_setkey;
 		alg->cra_ablkcipher.encrypt = ablkcipher_encrypt;
@@ -3018,6 +3038,7 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
 		break;
 	case CRYPTO_ALG_TYPE_AEAD:
 		alg = &t_alg->algt.alg.aead.base;
+		alg->cra_exit = talitos_cra_exit;
 		t_alg->algt.alg.aead.init = talitos_cra_init_aead;
 		t_alg->algt.alg.aead.setkey = aead_setkey;
 		t_alg->algt.alg.aead.encrypt = aead_encrypt;
@@ -3031,6 +3052,7 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
 	case CRYPTO_ALG_TYPE_AHASH:
 		alg = &t_alg->algt.alg.hash.halg.base;
 		alg->cra_init = talitos_cra_init_ahash;
+		alg->cra_exit = talitos_cra_exit;
 		alg->cra_type = &crypto_ahash_type;
 		t_alg->algt.alg.hash.init = ahash_init;
 		t_alg->algt.alg.hash.update = ahash_update;
-- 
2.13.3

[PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Christophe Leroy]

At every request, we map and unmap the same hash hw_context.

This patch moves the dma mapping/unmapping in functions ahash_init()
and ahash_import().

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 80 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 23 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index ebfd6d982ed6..d495649d5267 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -819,6 +819,7 @@ struct talitos_ctx {
 	unsigned int keylen;
 	unsigned int enckeylen;
 	unsigned int authkeylen;
+	dma_addr_t dma_hw_context;
 };
 
 #define HASH_MAX_BLOCK_SIZE		SHA512_BLOCK_SIZE
@@ -1663,18 +1664,9 @@ static void common_nonsnoop_hash_unmap(struct device *dev,
 				       struct ahash_request *areq)
 {
 	struct talitos_ahash_req_ctx *req_ctx = ahash_request_ctx(areq);
-	struct talitos_private *priv = dev_get_drvdata(dev);
-	bool is_sec1 = has_ftr_sec1(priv);
-
-	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[5], DMA_FROM_DEVICE);
 
 	talitos_sg_unmap(dev, edesc, req_ctx->psrc, NULL, 0, 0);
 
-	/* When using hashctx-in, must unmap it. */
-	if (from_talitos_ptr_len(&edesc->desc.ptr[1], is_sec1))
-		unmap_single_talitos_ptr(dev, &edesc->desc.ptr[1],
-					 DMA_TO_DEVICE);
-
 	if (edesc->dma_len)
 		dma_unmap_single(dev, edesc->dma_link_tbl, edesc->dma_len,
 				 DMA_BIDIRECTIONAL);
@@ -1744,10 +1736,8 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 
 	/* hash context in */
 	if (!req_ctx->first || req_ctx->swinit) {
-		map_single_talitos_ptr(dev, &desc->ptr[1],
-				       req_ctx->hw_context_size,
-				       (char *)req_ctx->hw_context,
-				       DMA_TO_DEVICE);
+		to_talitos_ptr(&desc->ptr[1], ctx->dma_hw_context,
+			       req_ctx->hw_context_size, is_sec1);
 		req_ctx->swinit = 0;
 	}
 	/* Indicate next op is not the first. */
@@ -1780,9 +1770,8 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				       crypto_ahash_digestsize(tfm),
 				       areq->result, DMA_FROM_DEVICE);
 	else
-		map_single_talitos_ptr(dev, &desc->ptr[5],
-				       req_ctx->hw_context_size,
-				       req_ctx->hw_context, DMA_FROM_DEVICE);
+		to_talitos_ptr(&desc->ptr[5], ctx->dma_hw_context,
+			       req_ctx->hw_context_size, is_sec1);
 
 	/* last DWORD empty */
 
@@ -1815,17 +1804,25 @@ static struct talitos_edesc *ahash_edesc_alloc(struct ahash_request *areq,
 static int ahash_init(struct ahash_request *areq)
 {
 	struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
+	struct talitos_ctx *ctx = crypto_ahash_ctx(tfm);
+	struct device *dev = ctx->dev;
 	struct talitos_ahash_req_ctx *req_ctx = ahash_request_ctx(areq);
+	unsigned int size;
 
 	/* Initialize the context */
 	req_ctx->nbuf = 0;
 	req_ctx->first = 1; /* first indicates h/w must init its context */
 	req_ctx->swinit = 0; /* assume h/w init of context */
-	req_ctx->hw_context_size =
-		(crypto_ahash_digestsize(tfm) <= SHA256_DIGEST_SIZE)
+	size =	(crypto_ahash_digestsize(tfm) <= SHA256_DIGEST_SIZE)
 			? TALITOS_MDEU_CONTEXT_SIZE_MD5_SHA1_SHA256
 			: TALITOS_MDEU_CONTEXT_SIZE_SHA384_SHA512;
+	req_ctx->hw_context_size = size;
 
+	if (ctx->dma_hw_context)
+		dma_unmap_single(dev, ctx->dma_hw_context, size,
+				 DMA_BIDIRECTIONAL);
+	ctx->dma_hw_context = dma_map_single(dev, req_ctx->hw_context, size,
+					     DMA_BIDIRECTIONAL);
 	return 0;
 }
 
@@ -1836,6 +1833,9 @@ static int ahash_init(struct ahash_request *areq)
 static int ahash_init_sha224_swinit(struct ahash_request *areq)
 {
 	struct talitos_ahash_req_ctx *req_ctx = ahash_request_ctx(areq);
+	struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
+	struct talitos_ctx *ctx = crypto_ahash_ctx(tfm);
+	struct device *dev = ctx->dev;
 
 	ahash_init(areq);
 	req_ctx->swinit = 1;/* prevent h/w initting context with sha256 values*/
@@ -1853,6 +1853,9 @@ static int ahash_init_sha224_swinit(struct ahash_request *areq)
 	req_ctx->hw_context[8] = 0;
 	req_ctx->hw_context[9] = 0;
 
+	dma_sync_single_for_device(dev, ctx->dma_hw_context,
+				   req_ctx->hw_context_size, DMA_TO_DEVICE);
+
 	return 0;
 }
 
@@ -1990,7 +1993,12 @@ static int ahash_export(struct ahash_request *areq, void *out)
 {
 	struct talitos_ahash_req_ctx *req_ctx = ahash_request_ctx(areq);
 	struct talitos_export_state *export = out;
+	struct crypto_ahash *ahash = crypto_ahash_reqtfm(areq);
+	struct talitos_ctx *ctx = crypto_ahash_ctx(ahash);
+	struct device *dev = ctx->dev;
 
+	dma_sync_single_for_cpu(dev, ctx->dma_hw_context,
+				req_ctx->hw_context_size, DMA_FROM_DEVICE);
 	memcpy(export->hw_context, req_ctx->hw_context,
 	       req_ctx->hw_context_size);
 	memcpy(export->buf, req_ctx->buf, req_ctx->nbuf);
@@ -2008,14 +2016,22 @@ static int ahash_import(struct ahash_request *areq, const void *in)
 	struct talitos_ahash_req_ctx *req_ctx = ahash_request_ctx(areq);
 	struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
 	const struct talitos_export_state *export = in;
+	unsigned int size;
+	struct talitos_ctx *ctx = crypto_ahash_ctx(tfm);
+	struct device *dev = ctx->dev;
 
 	memset(req_ctx, 0, sizeof(*req_ctx));
-	req_ctx->hw_context_size =
-		(crypto_ahash_digestsize(tfm) <= SHA256_DIGEST_SIZE)
+	size = (crypto_ahash_digestsize(tfm) <= SHA256_DIGEST_SIZE)
 			? TALITOS_MDEU_CONTEXT_SIZE_MD5_SHA1_SHA256
 			: TALITOS_MDEU_CONTEXT_SIZE_SHA384_SHA512;
-	memcpy(req_ctx->hw_context, export->hw_context,
-	       req_ctx->hw_context_size);
+	req_ctx->hw_context_size = size;
+	if (ctx->dma_hw_context)
+		dma_unmap_single(dev, ctx->dma_hw_context, size,
+				 DMA_BIDIRECTIONAL);
+
+	memcpy(req_ctx->hw_context, export->hw_context, size);
+	ctx->dma_hw_context = dma_map_single(dev, req_ctx->hw_context, size,
+					     DMA_BIDIRECTIONAL);
 	memcpy(req_ctx->buf, export->buf, export->nbuf);
 	req_ctx->swinit = export->swinit;
 	req_ctx->first = export->first;
@@ -2954,6 +2970,24 @@ static void talitos_cra_exit(struct crypto_tfm *tfm)
 		dma_unmap_single(dev, ctx->dma_key, ctx->keylen, DMA_TO_DEVICE);
 }
 
+static void talitos_cra_exit_ahash(struct crypto_tfm *tfm)
+{
+	struct talitos_ctx *ctx = crypto_tfm_ctx(tfm);
+	struct device *dev = ctx->dev;
+	unsigned int size;
+
+	talitos_cra_exit(tfm);
+
+	size = (crypto_ahash_digestsize(__crypto_ahash_cast(tfm)) <=
+		SHA256_DIGEST_SIZE)
+	       ? TALITOS_MDEU_CONTEXT_SIZE_MD5_SHA1_SHA256
+	       : TALITOS_MDEU_CONTEXT_SIZE_SHA384_SHA512;
+
+	if (ctx->dma_hw_context)
+		dma_unmap_single(dev, ctx->dma_hw_context, size,
+				 DMA_BIDIRECTIONAL);
+}
+
 /*
  * given the alg's descriptor header template, determine whether descriptor
  * type and primary/secondary execution units required match the hw
@@ -3052,7 +3086,7 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
 	case CRYPTO_ALG_TYPE_AHASH:
 		alg = &t_alg->algt.alg.hash.halg.base;
 		alg->cra_init = talitos_cra_init_ahash;
-		alg->cra_exit = talitos_cra_exit;
+		alg->cra_exit = talitos_cra_exit_ahash;
 		alg->cra_type = &crypto_ahash_type;
 		t_alg->algt.alg.hash.init = ahash_init;
 		t_alg->algt.alg.hash.update = ahash_update;
-- 
2.13.3

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 10/6/2017 4:06 PM, Christophe Leroy wrote:
> At every request, we map and unmap the same hash hw_context.
> 
> This patch moves the dma mapping/unmapping in functions ahash_init()
> and ahash_import().
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  drivers/crypto/talitos.c | 80 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
> index ebfd6d982ed6..d495649d5267 100644
> --- a/drivers/crypto/talitos.c
> +++ b/drivers/crypto/talitos.c
> @@ -819,6 +819,7 @@ struct talitos_ctx {
>  	unsigned int keylen;
>  	unsigned int enckeylen;
>  	unsigned int authkeylen;
> +	dma_addr_t dma_hw_context;
This doesn't look correct.

talitos_ctx structure is the tfm context.
dma_hw_context is the IOVA of hw_context, located in talitos_ahash_req_ctx
structure (request context).

If there are multiple requests in flight for the same tfm, dma_hw_context will
be overwritten.

dma_hw_context needs to be moved in request context (talitos_ahash_req_ctx struct).

Thanks,
Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 10/6/2017 4:06 PM, Christophe Leroy wrote:=0A=
> At every request, we map and unmap the same hash hw_context.=0A=
> =0A=
> This patch moves the dma mapping/unmapping in functions ahash_init()=0A=
> and ahash_import().=0A=
> =0A=
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>=0A=
> ---=0A=
>  drivers/crypto/talitos.c | 80 ++++++++++++++++++++++++++++++++++--------=
------=0A=
>  1 file changed, 57 insertions(+), 23 deletions(-)=0A=
> =0A=
> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c=0A=
> index ebfd6d982ed6..d495649d5267 100644=0A=
> --- a/drivers/crypto/talitos.c=0A=
> +++ b/drivers/crypto/talitos.c=0A=
> @@ -819,6 +819,7 @@ struct talitos_ctx {=0A=
>  	unsigned int keylen;=0A=
>  	unsigned int enckeylen;=0A=
>  	unsigned int authkeylen;=0A=
> +	dma_addr_t dma_hw_context;=0A=
This doesn't look correct.=0A=
=0A=
talitos_ctx structure is the tfm context.=0A=
dma_hw_context is the IOVA of hw_context, located in talitos_ahash_req_ctx=
=0A=
structure (request context).=0A=
=0A=
If there are multiple requests in flight for the same tfm, dma_hw_context w=
ill=0A=
be overwritten.=0A=
=0A=
dma_hw_context needs to be moved in request context (talitos_ahash_req_ctx =
struct).=0A=
=0A=
Thanks,=0A=
Horia=0A=

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Christophe LEROY]

Le 07/02/2018 à 15:39, Horia Geantă a écrit :
> On 10/6/2017 4:06 PM, Christophe Leroy wrote:
>> At every request, we map and unmap the same hash hw_context.
>>
>> This patch moves the dma mapping/unmapping in functions ahash_init()
>> and ahash_import().
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   drivers/crypto/talitos.c | 80 ++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 57 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
>> index ebfd6d982ed6..d495649d5267 100644
>> --- a/drivers/crypto/talitos.c
>> +++ b/drivers/crypto/talitos.c
>> @@ -819,6 +819,7 @@ struct talitos_ctx {
>>   	unsigned int keylen;
>>   	unsigned int enckeylen;
>>   	unsigned int authkeylen;
>> +	dma_addr_t dma_hw_context;
> This doesn't look correct.
> 
> talitos_ctx structure is the tfm context.
> dma_hw_context is the IOVA of hw_context, located in talitos_ahash_req_ctx
> structure (request context).

Yes but I have now found how I can know that the request context is 
being released in order to unmap() dma at that time.
It is tricky to use the tmf context I agree, but at least I know when 
tmf context get destroyed, ie in talitos_cra_exit_ahash()
The request context is created by ahash_request_alloc() and released by
ahash_request_free(). I have not found the way to call dma_unmap() 
before ahash_request_free() gets called.

> 
> If there are multiple requests in flight for the same tfm, dma_hw_context will
> be overwritten.

Before overwritting dma_hw_context, it is always released, see 
talitos_cra_exit_ahash(), ahash_init(), ahash_import()

> 
> dma_hw_context needs to be moved in request context (talitos_ahash_req_ctx struct).

Any suggestion then on how to handle the issue explained above ?

Thanks
Christophe

> 
> Thanks,
> Horia
> 

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/17/2018 6:32 PM, Christophe LEROY wrote:
> 
> 
> Le 07/02/2018 à 15:39, Horia Geantă a écrit :
>> On 10/6/2017 4:06 PM, Christophe Leroy wrote:
>>> At every request, we map and unmap the same hash hw_context.
>>>
>>> This patch moves the dma mapping/unmapping in functions ahash_init()
>>> and ahash_import().
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>> ---
>>>   drivers/crypto/talitos.c | 80 ++++++++++++++++++++++++++++++++++--------------
>>>   1 file changed, 57 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
>>> index ebfd6d982ed6..d495649d5267 100644
>>> --- a/drivers/crypto/talitos.c
>>> +++ b/drivers/crypto/talitos.c
>>> @@ -819,6 +819,7 @@ struct talitos_ctx {
>>>   	unsigned int keylen;
>>>   	unsigned int enckeylen;
>>>   	unsigned int authkeylen;
>>> +	dma_addr_t dma_hw_context;
>> This doesn't look correct.
>>
>> talitos_ctx structure is the tfm context.
>> dma_hw_context is the IOVA of hw_context, located in talitos_ahash_req_ctx
>> structure (request context).
> 
> Yes but I have now found how I can know that the request context is 
> being released in order to unmap() dma at that time.
> It is tricky to use the tmf context I agree, but at least I know when 
> tmf context get destroyed, ie in talitos_cra_exit_ahash()
> The request context is created by ahash_request_alloc() and released by
> ahash_request_free(). I have not found the way to call dma_unmap() 
> before ahash_request_free() gets called.
> 
>>
>> If there are multiple requests in flight for the same tfm, dma_hw_context will
>> be overwritten.
> 
> Before overwritting dma_hw_context, it is always released, see 
> talitos_cra_exit_ahash(), ahash_init(), ahash_import()
> 
The problem is not the unmapping.
If there are two requests for the same tfm, then given the following sequence
1. tfm->ahash_init(req1)
	tfm_ctx->dma_hw_context points to req1_ctx->hw_context
2. tfm->ahash_init(req2)
	tfm_ctx->dma_hw_context [unmapped, then] points to req2_ctx->hw_context
i.e. req1 will use the hw_context of req2.

>>
>> dma_hw_context needs to be moved in request context (talitos_ahash_req_ctx struct).
> 
> Any suggestion then on how to handle the issue explained above ?
> 
There is no ahash_exit() callback mirroring ahash_init().

The clean-up of request ctx should be done in the last states of the hash flows
described here:
https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-definition-with-struct-shash-alg-and-ahash-alg
for e.g. in the final() callback.

Hope this helps,
Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/17/2018 6:32 PM, Christophe LEROY wrote:=0A=
> =0A=
> =0A=
> Le 07/02/2018 =E0 15:39, Horia Geant=E3 a =E9crit=A0:=0A=
>> On 10/6/2017 4:06 PM, Christophe Leroy wrote:=0A=
>>> At every request, we map and unmap the same hash hw_context.=0A=
>>>=0A=
>>> This patch moves the dma mapping/unmapping in functions ahash_init()=0A=
>>> and ahash_import().=0A=
>>>=0A=
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>=0A=
>>> ---=0A=
>>>   drivers/crypto/talitos.c | 80 ++++++++++++++++++++++++++++++++++-----=
---------=0A=
>>>   1 file changed, 57 insertions(+), 23 deletions(-)=0A=
>>>=0A=
>>> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c=0A=
>>> index ebfd6d982ed6..d495649d5267 100644=0A=
>>> --- a/drivers/crypto/talitos.c=0A=
>>> +++ b/drivers/crypto/talitos.c=0A=
>>> @@ -819,6 +819,7 @@ struct talitos_ctx {=0A=
>>>   	unsigned int keylen;=0A=
>>>   	unsigned int enckeylen;=0A=
>>>   	unsigned int authkeylen;=0A=
>>> +	dma_addr_t dma_hw_context;=0A=
>> This doesn't look correct.=0A=
>>=0A=
>> talitos_ctx structure is the tfm context.=0A=
>> dma_hw_context is the IOVA of hw_context, located in talitos_ahash_req_c=
tx=0A=
>> structure (request context).=0A=
> =0A=
> Yes but I have now found how I can know that the request context is =0A=
> being released in order to unmap() dma at that time.=0A=
> It is tricky to use the tmf context I agree, but at least I know when =0A=
> tmf context get destroyed, ie in talitos_cra_exit_ahash()=0A=
> The request context is created by ahash_request_alloc() and released by=
=0A=
> ahash_request_free(). I have not found the way to call dma_unmap() =0A=
> before ahash_request_free() gets called.=0A=
> =0A=
>>=0A=
>> If there are multiple requests in flight for the same tfm, dma_hw_contex=
t will=0A=
>> be overwritten.=0A=
> =0A=
> Before overwritting dma_hw_context, it is always released, see =0A=
> talitos_cra_exit_ahash(), ahash_init(), ahash_import()=0A=
> =0A=
The problem is not the unmapping.=0A=
If there are two requests for the same tfm, then given the following sequen=
ce=0A=
1. tfm->ahash_init(req1)=0A=
	tfm_ctx->dma_hw_context points to req1_ctx->hw_context=0A=
2. tfm->ahash_init(req2)=0A=
	tfm_ctx->dma_hw_context [unmapped, then] points to req2_ctx->hw_context=0A=
i.e. req1 will use the hw_context of req2.=0A=
=0A=
>>=0A=
>> dma_hw_context needs to be moved in request context (talitos_ahash_req_c=
tx struct).=0A=
> =0A=
> Any suggestion then on how to handle the issue explained above ?=0A=
> =0A=
There is no ahash_exit() callback mirroring ahash_init().=0A=
=0A=
The clean-up of request ctx should be done in the last states of the hash f=
lows=0A=
described here:=0A=
https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-defin=
ition-with-struct-shash-alg-and-ahash-alg=0A=
for e.g. in the final() callback.=0A=
=0A=
Hope this helps,=0A=
Horia=0A=

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Christophe LEROY]

Le 18/02/2018 à 18:14, Horia Geantă a écrit :
> On 2/17/2018 6:32 PM, Christophe LEROY wrote:
>>
>>
>> Le 07/02/2018 à 15:39, Horia Geantă a écrit :
>>> On 10/6/2017 4:06 PM, Christophe Leroy wrote:
>>>> At every request, we map and unmap the same hash hw_context.
>>>>
>>>> This patch moves the dma mapping/unmapping in functions ahash_init()
>>>> and ahash_import().
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> ---
>>>>    drivers/crypto/talitos.c | 80 ++++++++++++++++++++++++++++++++++--------------
>>>>    1 file changed, 57 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
>>>> index ebfd6d982ed6..d495649d5267 100644
>>>> --- a/drivers/crypto/talitos.c
>>>> +++ b/drivers/crypto/talitos.c
>>>> @@ -819,6 +819,7 @@ struct talitos_ctx {
>>>>    	unsigned int keylen;
>>>>    	unsigned int enckeylen;
>>>>    	unsigned int authkeylen;
>>>> +	dma_addr_t dma_hw_context;
>>> This doesn't look correct.
>>>
>>> talitos_ctx structure is the tfm context.
>>> dma_hw_context is the IOVA of hw_context, located in talitos_ahash_req_ctx
>>> structure (request context).
>>
>> Yes but I have now found how I can know that the request context is
>> being released in order to unmap() dma at that time.
>> It is tricky to use the tmf context I agree, but at least I know when
>> tmf context get destroyed, ie in talitos_cra_exit_ahash()
>> The request context is created by ahash_request_alloc() and released by
>> ahash_request_free(). I have not found the way to call dma_unmap()
>> before ahash_request_free() gets called.
>>
>>>
>>> If there are multiple requests in flight for the same tfm, dma_hw_context will
>>> be overwritten.
>>
>> Before overwritting dma_hw_context, it is always released, see
>> talitos_cra_exit_ahash(), ahash_init(), ahash_import()
>>
> The problem is not the unmapping.
> If there are two requests for the same tfm, then given the following sequence
> 1. tfm->ahash_init(req1)
> 	tfm_ctx->dma_hw_context points to req1_ctx->hw_context
> 2. tfm->ahash_init(req2)
> 	tfm_ctx->dma_hw_context [unmapped, then] points to req2_ctx->hw_context
> i.e. req1 will use the hw_context of req2.
> 
>>>
>>> dma_hw_context needs to be moved in request context (talitos_ahash_req_ctx struct).
>>
>> Any suggestion then on how to handle the issue explained above ?
>>
> There is no ahash_exit() callback mirroring ahash_init().
> 
> The clean-up of request ctx should be done in the last states of the hash flows
> described here:
> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-definition-with-struct-shash-alg-and-ahash-alg
> for e.g. in the final() callback.

Unfortunatly it seems that we can't rely on those finalising functions 
being called all the time.
If you look into test_ahash_jiffies() for instance, in case of error the 
call of crypto_hash_final() is skipped.
So at the time being, I can't see any place to put the unmapping to be 
100% sure it will be done before the call of ahash_request_free()

Christophe

> 
> Hope this helps,
> Horia
> 

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Christophe LEROY]

Le 18/02/2018 à 18:14, Horia Geantă a écrit :
> On 2/17/2018 6:32 PM, Christophe LEROY wrote:
>>
>>
>> Le 07/02/2018 à 15:39, Horia Geantă a écrit :
>>> On 10/6/2017 4:06 PM, Christophe Leroy wrote:
>>>> At every request, we map and unmap the same hash hw_context.
>>>>
>>>> This patch moves the dma mapping/unmapping in functions ahash_init()
>>>> and ahash_import().
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> ---
>>>>    drivers/crypto/talitos.c | 80 ++++++++++++++++++++++++++++++++++--------------
>>>>    1 file changed, 57 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
>>>> index ebfd6d982ed6..d495649d5267 100644
>>>> --- a/drivers/crypto/talitos.c
>>>> +++ b/drivers/crypto/talitos.c
>>>> @@ -819,6 +819,7 @@ struct talitos_ctx {
>>>>    	unsigned int keylen;
>>>>    	unsigned int enckeylen;
>>>>    	unsigned int authkeylen;
>>>> +	dma_addr_t dma_hw_context;
>>> This doesn't look correct.
>>>
>>> talitos_ctx structure is the tfm context.
>>> dma_hw_context is the IOVA of hw_context, located in talitos_ahash_req_ctx
>>> structure (request context).
>>
>> Yes but I have now found how I can know that the request context is
>> being released in order to unmap() dma at that time.
>> It is tricky to use the tmf context I agree, but at least I know when
>> tmf context get destroyed, ie in talitos_cra_exit_ahash()
>> The request context is created by ahash_request_alloc() and released by
>> ahash_request_free(). I have not found the way to call dma_unmap()
>> before ahash_request_free() gets called.
>>
>>>
>>> If there are multiple requests in flight for the same tfm, dma_hw_context will
>>> be overwritten.
>>
>> Before overwritting dma_hw_context, it is always released, see
>> talitos_cra_exit_ahash(), ahash_init(), ahash_import()
>>
> The problem is not the unmapping.
> If there are two requests for the same tfm, then given the following sequence
> 1. tfm->ahash_init(req1)
> 	tfm_ctx->dma_hw_context points to req1_ctx->hw_context
> 2. tfm->ahash_init(req2)
> 	tfm_ctx->dma_hw_context [unmapped, then] points to req2_ctx->hw_context
> i.e. req1 will use the hw_context of req2.
> 
>>>
>>> dma_hw_context needs to be moved in request context (talitos_ahash_req_ctx struct).
>>
>> Any suggestion then on how to handle the issue explained above ?
>>
> There is no ahash_exit() callback mirroring ahash_init().
> 
> The clean-up of request ctx should be done in the last states of the hash flows
> described here:
> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-definition-with-struct-shash-alg-and-ahash-alg
> for e.g. in the final() callback.

Unfortunatly it seems that we can't rely on those finalising functions 
being called all the time.
If you look into test_ahash_jiffies() for instance, in case of error the 
call of crypto_hash_final() is skipped.
So at the time being, I can't see any place to put the unmapping to be 
100% sure it will be done before the call of ahash_request_free()

Christophe

> 
> Hope this helps,
> Horia
> 

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/19/2018 9:58 AM, Christophe LEROY wrote:
> Le 18/02/2018 à 18:14, Horia Geantă a écrit :
>> There is no ahash_exit() callback mirroring ahash_init().
>>
>> The clean-up of request ctx should be done in the last states of the hash flows
>> described here:
>> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-definition-with-struct-shash-alg-and-ahash-alg
>> for e.g. in the final() callback.
> 
> Unfortunatly it seems that we can't rely on those finalising functions 
> being called all the time.
> If you look into test_ahash_jiffies() for instance, in case of error the 
> call of crypto_hash_final() is skipped.

If test_ahash_jiffies() errors before calling crypto_ahash_final(req), this
means a previous callback failed.
Accordingly, DMA unmapping should be performed also on the corresponding errors
paths in the driver.

Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/19/2018 9:58 AM, Christophe LEROY wrote:=0A=
> Le 18/02/2018 =E0 18:14, Horia Geant=E3 a =E9crit=A0:=0A=
>> There is no ahash_exit() callback mirroring ahash_init().=0A=
>>=0A=
>> The clean-up of request ctx should be done in the last states of the has=
h flows=0A=
>> described here:=0A=
>> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-de=
finition-with-struct-shash-alg-and-ahash-alg=0A=
>> for e.g. in the final() callback.=0A=
> =0A=
> Unfortunatly it seems that we can't rely on those finalising functions =
=0A=
> being called all the time.=0A=
> If you look into test_ahash_jiffies() for instance, in case of error the =
=0A=
> call of crypto_hash_final() is skipped.=0A=
=0A=
If test_ahash_jiffies() errors before calling crypto_ahash_final(req), this=
=0A=
means a previous callback failed.=0A=
Accordingly, DMA unmapping should be performed also on the corresponding er=
rors=0A=
paths in the driver.=0A=
=0A=
Horia=0A=

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/19/2018 9:58 AM, Christophe LEROY wrote:
> Le 18/02/2018 à 18:14, Horia Geantă a écrit :
>> There is no ahash_exit() callback mirroring ahash_init().
>>
>> The clean-up of request ctx should be done in the last states of the hash flows
>> described here:
>> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-definition-with-struct-shash-alg-and-ahash-alg
>> for e.g. in the final() callback.
> 
> Unfortunatly it seems that we can't rely on those finalising functions 
> being called all the time.
> If you look into test_ahash_jiffies() for instance, in case of error the 
> call of crypto_hash_final() is skipped.

If test_ahash_jiffies() errors before calling crypto_ahash_final(req), this
means a previous callback failed.
Accordingly, DMA unmapping should be performed also on the corresponding errors
paths in the driver.

Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Christophe LEROY]

Le 19/02/2018 à 09:30, Horia Geantă a écrit :
> On 2/19/2018 9:58 AM, Christophe LEROY wrote:
>> Le 18/02/2018 à 18:14, Horia Geantă a écrit :
>>> There is no ahash_exit() callback mirroring ahash_init().
>>>
>>> The clean-up of request ctx should be done in the last states of the hash flows
>>> described here:
>>> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-definition-with-struct-shash-alg-and-ahash-alg
>>> for e.g. in the final() callback.
>>
>> Unfortunatly it seems that we can't rely on those finalising functions
>> being called all the time.
>> If you look into test_ahash_jiffies() for instance, in case of error the
>> call of crypto_hash_final() is skipped.
> 
> If test_ahash_jiffies() errors before calling crypto_ahash_final(req), this
> means a previous callback failed.
> Accordingly, DMA unmapping should be performed also on the corresponding errors
> paths in the driver.
> 

And what about ALGIF path from user space ?
What if the user never calls the last sendmsg() which will call 
hash_finup() ?

Christophe

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Christophe LEROY]

Le 19/02/2018 à 09:30, Horia Geantă a écrit :
> On 2/19/2018 9:58 AM, Christophe LEROY wrote:
>> Le 18/02/2018 à 18:14, Horia Geantă a écrit :
>>> There is no ahash_exit() callback mirroring ahash_init().
>>>
>>> The clean-up of request ctx should be done in the last states of the hash flows
>>> described here:
>>> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-definition-with-struct-shash-alg-and-ahash-alg
>>> for e.g. in the final() callback.
>>
>> Unfortunatly it seems that we can't rely on those finalising functions
>> being called all the time.
>> If you look into test_ahash_jiffies() for instance, in case of error the
>> call of crypto_hash_final() is skipped.
> 
> If test_ahash_jiffies() errors before calling crypto_ahash_final(req), this
> means a previous callback failed.
> Accordingly, DMA unmapping should be performed also on the corresponding errors
> paths in the driver.
> 

And what about ALGIF path from user space ?
What if the user never calls the last sendmsg() which will call 
hash_finup() ?

Christophe

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/19/2018 11:14 AM, Christophe LEROY wrote:
> Le 19/02/2018 à 09:30, Horia Geantă a écrit :
>> On 2/19/2018 9:58 AM, Christophe LEROY wrote:
>>> Le 18/02/2018 à 18:14, Horia Geantă a écrit :
>>>> There is no ahash_exit() callback mirroring ahash_init().
>>>>
>>>> The clean-up of request ctx should be done in the last states of the hash flows
>>>> described here:
>>>> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-definition-with-struct-shash-alg-and-ahash-alg
>>>> for e.g. in the final() callback.
>>>
>>> Unfortunatly it seems that we can't rely on those finalising functions
>>> being called all the time.
>>> If you look into test_ahash_jiffies() for instance, in case of error the
>>> call of crypto_hash_final() is skipped.
>>
>> If test_ahash_jiffies() errors before calling crypto_ahash_final(req), this
>> means a previous callback failed.
>> Accordingly, DMA unmapping should be performed also on the corresponding errors
>> paths in the driver.
>>
> 
> And what about ALGIF path from user space ?
> What if the user never calls the last sendmsg() which will call 
> hash_finup() ?
> 
User is expected to follow the rules of the crypto API.
Of course, kernel won't (or at least shouldn't) crash in case of misuse.
However, in these cases some resources might not be freed - it's unavoidable.

Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/19/2018 11:14 AM, Christophe LEROY wrote:=0A=
> Le 19/02/2018 =E0 09:30, Horia Geant=E3 a =E9crit=A0:=0A=
>> On 2/19/2018 9:58 AM, Christophe LEROY wrote:=0A=
>>> Le 18/02/2018 =E0 18:14, Horia Geant=E3 a =E9crit=A0:=0A=
>>>> There is no ahash_exit() callback mirroring ahash_init().=0A=
>>>>=0A=
>>>> The clean-up of request ctx should be done in the last states of the h=
ash flows=0A=
>>>> described here:=0A=
>>>> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-=
definition-with-struct-shash-alg-and-ahash-alg=0A=
>>>> for e.g. in the final() callback.=0A=
>>>=0A=
>>> Unfortunatly it seems that we can't rely on those finalising functions=
=0A=
>>> being called all the time.=0A=
>>> If you look into test_ahash_jiffies() for instance, in case of error th=
e=0A=
>>> call of crypto_hash_final() is skipped.=0A=
>>=0A=
>> If test_ahash_jiffies() errors before calling crypto_ahash_final(req), t=
his=0A=
>> means a previous callback failed.=0A=
>> Accordingly, DMA unmapping should be performed also on the corresponding=
 errors=0A=
>> paths in the driver.=0A=
>>=0A=
> =0A=
> And what about ALGIF path from user space ?=0A=
> What if the user never calls the last sendmsg() which will call =0A=
> hash_finup() ?=0A=
> =0A=
User is expected to follow the rules of the crypto API.=0A=
Of course, kernel won't (or at least shouldn't) crash in case of misuse.=0A=
However, in these cases some resources might not be freed - it's unavoidabl=
e.=0A=
=0A=
Horia=0A=

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/19/2018 11:14 AM, Christophe LEROY wrote:
> Le 19/02/2018 à 09:30, Horia Geantă a écrit :
>> On 2/19/2018 9:58 AM, Christophe LEROY wrote:
>>> Le 18/02/2018 à 18:14, Horia Geantă a écrit :
>>>> There is no ahash_exit() callback mirroring ahash_init().
>>>>
>>>> The clean-up of request ctx should be done in the last states of the hash flows
>>>> described here:
>>>> https://www.kernel.org/doc/html/latest/crypto/devel-algos.html#cipher-definition-with-struct-shash-alg-and-ahash-alg
>>>> for e.g. in the final() callback.
>>>
>>> Unfortunatly it seems that we can't rely on those finalising functions
>>> being called all the time.
>>> If you look into test_ahash_jiffies() for instance, in case of error the
>>> call of crypto_hash_final() is skipped.
>>
>> If test_ahash_jiffies() errors before calling crypto_ahash_final(req), this
>> means a previous callback failed.
>> Accordingly, DMA unmapping should be performed also on the corresponding errors
>> paths in the driver.
>>
> 
> And what about ALGIF path from user space ?
> What if the user never calls the last sendmsg() which will call 
> hash_finup() ?
> 
User is expected to follow the rules of the crypto API.
Of course, kernel won't (or at least shouldn't) crash in case of misuse.
However, in these cases some resources might not be freed - it's unavoidable.

Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Herbert Xu]

On Mon, Feb 19, 2018 at 01:16:30PM +0000, Horia Geantă wrote:
>
> > And what about ALGIF path from user space ?
> > What if the user never calls the last sendmsg() which will call 
> > hash_finup() ?
> > 
> User is expected to follow the rules of the crypto API.
> Of course, kernel won't (or at least shouldn't) crash in case of misuse.
> However, in these cases some resources might not be freed - it's unavoidable.

the crypto API does not require the presence of a finalisation.
It is entirely optional.  So leaving resources pinned down until
final/finup occurs is unacceptable, both from user-space and the
kernel.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Herbert Xu]

On Mon, Feb 19, 2018 at 01:16:30PM +0000, Horia Geantă wrote:
>
> > And what about ALGIF path from user space ?
> > What if the user never calls the last sendmsg() which will call 
> > hash_finup() ?
> > 
> User is expected to follow the rules of the crypto API.
> Of course, kernel won't (or at least shouldn't) crash in case of misuse.
> However, in these cases some resources might not be freed - it's unavoidable.

the crypto API does not require the presence of a finalisation.
It is entirely optional.  So leaving resources pinned down until
final/finup occurs is unacceptable, both from user-space and the
kernel.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/20/2018 12:34 PM, Herbert Xu wrote:
> On Mon, Feb 19, 2018 at 01:16:30PM +0000, Horia Geantă wrote:
>>
>>> And what about ALGIF path from user space ?
>>> What if the user never calls the last sendmsg() which will call 
>>> hash_finup() ?
>>>
>> User is expected to follow the rules of the crypto API.
>> Of course, kernel won't (or at least shouldn't) crash in case of misuse.
>> However, in these cases some resources might not be freed - it's unavoidable.
> 
> the crypto API does not require the presence of a finalisation.
> It is entirely optional.  So leaving resources pinned down until
> final/finup occurs is unacceptable, both from user-space and the
> kernel.
> 
If final/finup is optional, how is the final hash supposed to be retrieved?

According to documentation, these are the accepted flows (with the option to
export/import a partial hash b/w update and final/finup):

.init() -> .update() -> .final()
            ^    |         |
            '----'         '---> HASH

.init() -> .update() -> .finup()
            ^    |         |
            '----'         '---> HASH

           .digest()
               |
               '---------------> HASH

Note that digest() is not an issue in the case we are discussing, since resource
allocation happens only in init().

Thanks,
Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/20/2018 12:34 PM, Herbert Xu wrote:=0A=
> On Mon, Feb 19, 2018 at 01:16:30PM +0000, Horia Geant=E3 wrote:=0A=
>>=0A=
>>> And what about ALGIF path from user space ?=0A=
>>> What if the user never calls the last sendmsg() which will call =0A=
>>> hash_finup() ?=0A=
>>>=0A=
>> User is expected to follow the rules of the crypto API.=0A=
>> Of course, kernel won't (or at least shouldn't) crash in case of misuse.=
=0A=
>> However, in these cases some resources might not be freed - it's unavoid=
able.=0A=
> =0A=
> the crypto API does not require the presence of a finalisation.=0A=
> It is entirely optional.  So leaving resources pinned down until=0A=
> final/finup occurs is unacceptable, both from user-space and the=0A=
> kernel.=0A=
> =0A=
If final/finup is optional, how is the final hash supposed to be retrieved?=
=0A=
=0A=
According to documentation, these are the accepted flows (with the option t=
o=0A=
export/import a partial hash b/w update and final/finup):=0A=
=0A=
.init() -> .update() -> .final()=0A=
            ^    |         |=0A=
            '----'         '---> HASH=0A=
=0A=
.init() -> .update() -> .finup()=0A=
            ^    |         |=0A=
            '----'         '---> HASH=0A=
=0A=
           .digest()=0A=
               |=0A=
               '---------------> HASH=0A=
=0A=
Note that digest() is not an issue in the case we are discussing, since res=
ource=0A=
allocation happens only in init().=0A=
=0A=
Thanks,=0A=
Horia=0A=

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/20/2018 12:34 PM, Herbert Xu wrote:
> On Mon, Feb 19, 2018 at 01:16:30PM +0000, Horia Geantă wrote:
>>
>>> And what about ALGIF path from user space ?
>>> What if the user never calls the last sendmsg() which will call 
>>> hash_finup() ?
>>>
>> User is expected to follow the rules of the crypto API.
>> Of course, kernel won't (or at least shouldn't) crash in case of misuse.
>> However, in these cases some resources might not be freed - it's unavoidable.
> 
> the crypto API does not require the presence of a finalisation.
> It is entirely optional.  So leaving resources pinned down until
> final/finup occurs is unacceptable, both from user-space and the
> kernel.
> 
If final/finup is optional, how is the final hash supposed to be retrieved?

According to documentation, these are the accepted flows (with the option to
export/import a partial hash b/w update and final/finup):

.init() -> .update() -> .final()
            ^    |         |
            '----'         '---> HASH

.init() -> .update() -> .finup()
            ^    |         |
            '----'         '---> HASH

           .digest()
               |
               '---------------> HASH

Note that digest() is not an issue in the case we are discussing, since resource
allocation happens only in init().

Thanks,
Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Herbert Xu]

On Tue, Feb 20, 2018 at 11:32:25AM +0000, Horia Geantă wrote:
>
> If final/finup is optional, how is the final hash supposed to be retrieved?

Sometimes the computation ends with a partial hash, that's what
export is for.  Also it is completely legal to abandon the hash
state entirely.

> According to documentation, these are the accepted flows (with the option to
> export/import a partial hash b/w update and final/finup):
> 
> .init() -> .update() -> .final()
>             ^    |         |
>             '----'         '---> HASH
> 
> .init() -> .update() -> .finup()
>             ^    |         |
>             '----'         '---> HASH
> 
>            .digest()
>                |
>                '---------------> HASH

The documentation is simply incomplete in this regard.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Herbert Xu]

On Tue, Feb 20, 2018 at 11:32:25AM +0000, Horia Geantă wrote:
>
> If final/finup is optional, how is the final hash supposed to be retrieved?

Sometimes the computation ends with a partial hash, that's what
export is for.  Also it is completely legal to abandon the hash
state entirely.

> According to documentation, these are the accepted flows (with the option to
> export/import a partial hash b/w update and final/finup):
> 
> .init() -> .update() -> .final()
>             ^    |         |
>             '----'         '---> HASH
> 
> .init() -> .update() -> .finup()
>             ^    |         |
>             '----'         '---> HASH
> 
>            .digest()
>                |
>                '---------------> HASH

The documentation is simply incomplete in this regard.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/22/2018 1:47 PM, Herbert Xu wrote:
> On Tue, Feb 20, 2018 at 11:32:25AM +0000, Horia Geantă wrote:
>>
>> If final/finup is optional, how is the final hash supposed to be retrieved?
> 
> Sometimes the computation ends with a partial hash, that's what
> export is for.  Also it is completely legal to abandon the hash
> state entirely.
> 
Thanks for the explanation.
It's unintuitive to call .init() -> .update() and then not to call any of
.final(), .finup(), .export().

Christophe,

IIUC this means that there is no room for improvement.
This patch needs to be reverted, to restore previous behaviour when the
hw_context was mapped / unmapped for every request.

Thanks,
Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/22/2018 1:47 PM, Herbert Xu wrote:=0A=
> On Tue, Feb 20, 2018 at 11:32:25AM +0000, Horia Geant=E3 wrote:=0A=
>>=0A=
>> If final/finup is optional, how is the final hash supposed to be retriev=
ed?=0A=
> =0A=
> Sometimes the computation ends with a partial hash, that's what=0A=
> export is for.  Also it is completely legal to abandon the hash=0A=
> state entirely.=0A=
> =0A=
Thanks for the explanation.=0A=
It's unintuitive to call .init() -> .update() and then not to call any of=
=0A=
.final(), .finup(), .export().=0A=
=0A=
Christophe,=0A=
=0A=
IIUC this means that there is no room for improvement.=0A=
This patch needs to be reverted, to restore previous behaviour when the=0A=
hw_context was mapped / unmapped for every request.=0A=
=0A=
Thanks,=0A=
Horia=0A=

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Horia Geantă]

On 2/22/2018 1:47 PM, Herbert Xu wrote:
> On Tue, Feb 20, 2018 at 11:32:25AM +0000, Horia Geantă wrote:
>>
>> If final/finup is optional, how is the final hash supposed to be retrieved?
> 
> Sometimes the computation ends with a partial hash, that's what
> export is for.  Also it is completely legal to abandon the hash
> state entirely.
> 
Thanks for the explanation.
It's unintuitive to call .init() -> .update() and then not to call any of
.final(), .finup(), .export().

Christophe,

IIUC this means that there is no room for improvement.
This patch needs to be reverted, to restore previous behaviour when the
hw_context was mapped / unmapped for every request.

Thanks,
Horia

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Herbert Xu]

On Thu, Feb 22, 2018 at 12:29:28PM +0000, Horia Geantă wrote:
>
> IIUC this means that there is no room for improvement.
> This patch needs to be reverted, to restore previous behaviour when the
> hw_context was mapped / unmapped for every request.

In general we should avoid trying to do batching in drivers.  Such
optimisations should instead be done at a higher level.  For example,
for disk encryption we want to do the aggregation at the block layer
rather than the crypto API because that has innate knowledge of the
data layout which we can only guess.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 16/18] crypto: talitos - do hw_context DMA mapping outside the requests

[Herbert Xu]

On Thu, Feb 22, 2018 at 12:29:28PM +0000, Horia Geantă wrote:
>
> IIUC this means that there is no room for improvement.
> This patch needs to be reverted, to restore previous behaviour when the
> hw_context was mapped / unmapped for every request.

In general we should avoid trying to do batching in drivers.  Such
optimisations should instead be done at a higher level.  For example,
for disk encryption we want to do the aggregation at the block layer
rather than the crypto API because that has innate knowledge of the
data layout which we can only guess.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 17/18] crypto: talitos - chain in buffered data for ahash on SEC1

[Christophe Leroy]

SEC1 doesn't support S/G in descriptors so for hash operations,
the CPU has to build a buffer containing the buffered block and
the incoming data. This generates a lot of memory copies which
represents more than 50% of CPU time of a md5sum operation as
shown below with a 'perf record'.

|--86.24%-- kcapi_md_digest
|          |
|          |--86.18%-- _kcapi_common_vmsplice_chunk_fd
|          |          |
|          |          |--83.68%-- splice
|          |          |          |
|          |          |          |--83.59%-- ret_from_syscall
|          |          |          |          |
|          |          |          |          |--83.52%-- sys_splice
|          |          |          |          |          |
|          |          |          |          |          |--83.49%-- splice_from_pipe
|          |          |          |          |          |          |
|          |          |          |          |          |          |--83.04%-- __splice_from_pipe
|          |          |          |          |          |          |          |
|          |          |          |          |          |          |          |--80.67%-- pipe_to_sendpage
|          |          |          |          |          |          |          |          |
|          |          |          |          |          |          |          |          |--78.25%-- hash_sendpage
|          |          |          |          |          |          |          |          |          |
|          |          |          |          |          |          |          |          |          |--60.08%-- ahash_process_req
|          |          |          |          |          |          |          |          |          |          |
|          |          |          |          |          |          |          |          |          |          |--56.36%-- sg_copy_buffer
|          |          |          |          |          |          |          |          |          |          |          |
|          |          |          |          |          |          |          |          |          |          |          |--55.29%-- memcpy
|          |          |          |          |          |          |          |          |          |          |          |

However, unlike SEC2+, SEC1 offers the possibility to chain
descriptors. It is therefore possible to build a first descriptor
pointing to the buffered data and a second descriptor pointing to
the incoming data, hence avoiding the memory copy to a single
buffer.

With this patch, the time necessary for a md5sum on a 90Mbytes file
is approximately 3 seconds. Without the patch it takes 6 seconds.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 139 ++++++++++++++++++++++++++++++++++++++++++-----
 drivers/crypto/talitos.h |   1 +
 2 files changed, 127 insertions(+), 13 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index d495649d5267..5c4499a85611 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -160,6 +160,10 @@ static int reset_channel(struct device *dev, int ch)
 	/* set 36-bit addressing, done writeback enable and done IRQ enable */
 	setbits32(priv->chan[ch].reg + TALITOS_CCCR_LO, TALITOS_CCCR_LO_EAE |
 		  TALITOS_CCCR_LO_CDWE | TALITOS_CCCR_LO_CDIE);
+	/* enable chaining descriptors */
+	if (is_sec1)
+		setbits32(priv->chan[ch].reg + TALITOS_CCCR_LO,
+			  TALITOS_CCCR_LO_NE);
 
 	/* and ICCR writeback, if available */
 	if (priv->features & TALITOS_FTR_HW_AUTH_CHECK)
@@ -333,7 +337,12 @@ static void flush_channel(struct device *dev, int ch, int error, int reset_ch)
 
 		/* descriptors with their done bits set don't get the error */
 		rmb();
-		hdr = is_sec1 ? request->desc->hdr1 : request->desc->hdr;
+		if (!is_sec1)
+			hdr = request->desc->hdr;
+		else if (request->desc->next_desc)
+			hdr = (request->desc + 1)->hdr1;
+		else
+			hdr = request->desc->hdr1;
 
 		if ((hdr & DESC_HDR_DONE) == DESC_HDR_DONE)
 			status = 0;
@@ -454,7 +463,8 @@ static u32 current_desc_hdr(struct device *dev, int ch)
 	tail = priv->chan[ch].tail;
 
 	iter = tail;
-	while (priv->chan[ch].fifo[iter].dma_desc != cur_desc) {
+	while (priv->chan[ch].fifo[iter].dma_desc != cur_desc &&
+	       priv->chan[ch].fifo[iter].desc->next_desc != cur_desc) {
 		iter = (iter + 1) & (priv->fifo_len - 1);
 		if (iter == tail) {
 			dev_err(dev, "couldn't locate current descriptor\n");
@@ -462,6 +472,9 @@ static u32 current_desc_hdr(struct device *dev, int ch)
 		}
 	}
 
+	if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc)
+		return (priv->chan[ch].fifo[iter].desc + 1)->hdr;
+
 	return priv->chan[ch].fifo[iter].desc->hdr;
 }
 
@@ -819,6 +832,7 @@ struct talitos_ctx {
 	unsigned int keylen;
 	unsigned int enckeylen;
 	unsigned int authkeylen;
+	dma_addr_t dma_buf;
 	dma_addr_t dma_hw_context;
 };
 
@@ -1380,6 +1394,10 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 		alloc_len += icv_stashing ? authsize : 0;
 	}
 
+	/* if its a ahash, add space for a second desc next to the first one */
+	if (is_sec1 && !dst)
+		alloc_len += sizeof(struct talitos_desc);
+
 	edesc = kmalloc(alloc_len, GFP_DMA | flags);
 	if (!edesc) {
 		dev_err(dev, "could not allocate edescriptor\n");
@@ -1392,11 +1410,15 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 	edesc->dst_nents = dst_nents;
 	edesc->iv_dma = iv_dma;
 	edesc->dma_len = dma_len;
-	if (dma_len)
-		edesc->dma_link_tbl = dma_map_single(dev, &edesc->link_tbl[0],
+	if (dma_len) {
+		void *addr = &edesc->link_tbl[0];
+
+		if (is_sec1 && !dst)
+			addr += sizeof(struct talitos_desc);
+		edesc->dma_link_tbl = dma_map_single(dev, addr,
 						     edesc->dma_len,
 						     DMA_BIDIRECTIONAL);
-
+	}
 	return edesc;
 error_sg:
 	if (iv_dma)
@@ -1671,6 +1693,9 @@ static void common_nonsnoop_hash_unmap(struct device *dev,
 		dma_unmap_single(dev, edesc->dma_link_tbl, edesc->dma_len,
 				 DMA_BIDIRECTIONAL);
 
+	if (edesc->desc.next_desc)
+		dma_unmap_single(dev, be32_to_cpu(edesc->desc.next_desc),
+				 TALITOS_DESC_SIZE, DMA_BIDIRECTIONAL);
 }
 
 static void ahash_done(struct device *dev,
@@ -1717,6 +1742,7 @@ static void talitos_handle_buggy_hash(struct talitos_ctx *ctx,
 
 static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				struct ahash_request *areq, unsigned int length,
+				unsigned int offset,
 				void (*callback) (struct device *dev,
 						  struct talitos_desc *desc,
 						  void *context, int error))
@@ -1748,19 +1774,29 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 		to_talitos_ptr(&desc->ptr[2], ctx->dma_key, ctx->keylen,
 			       is_sec1);
 
+	if (is_sec1 && req_ctx->nbuf)
+		length -= req_ctx->nbuf;
+
 	sg_count = edesc->src_nents ?: 1;
 	if (is_sec1 && sg_count > 1)
-		sg_copy_to_buffer(req_ctx->psrc, sg_count, edesc->buf, length);
-	else
+		sg_pcopy_to_buffer(req_ctx->psrc, sg_count,
+				   edesc->buf + sizeof(struct talitos_desc),
+				   length, req_ctx->nbuf);
+	else if (length)
 		sg_count = dma_map_sg(dev, req_ctx->psrc, sg_count,
 				      DMA_TO_DEVICE);
 	/*
 	 * data in
 	 */
-	sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
-				  &desc->ptr[3], sg_count, 0, 0);
-	if (sg_count > 1)
-		sync_needed = true;
+	if (is_sec1 && req_ctx->nbuf) {
+		to_talitos_ptr(&desc->ptr[3], ctx->dma_buf, req_ctx->nbuf,
+			       is_sec1);
+	} else {
+		sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
+					  &desc->ptr[3], sg_count, offset, 0);
+		if (sg_count > 1)
+			sync_needed = true;
+	}
 
 	/* fifth DWORD empty */
 
@@ -1778,6 +1814,36 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 	if (is_sec1 && from_talitos_ptr_len(&desc->ptr[3], true) == 0)
 		talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
 
+	if (is_sec1 && req_ctx->nbuf && length) {
+		struct talitos_desc *desc2 = desc + 1;
+		dma_addr_t next_desc;
+
+		memset(desc2, 0, sizeof(*desc2));
+		desc2->hdr = desc->hdr;
+		desc2->hdr &= ~DESC_HDR_MODE0_MDEU_INIT;
+		desc2->hdr1 = desc2->hdr;
+		desc->hdr &= ~DESC_HDR_MODE0_MDEU_PAD;
+		desc->hdr |= DESC_HDR_MODE0_MDEU_CONT;
+		desc->hdr &= ~DESC_HDR_DONE_NOTIFY;
+
+		to_talitos_ptr(&desc2->ptr[1], ctx->dma_hw_context,
+			       req_ctx->hw_context_size, is_sec1);
+
+		copy_talitos_ptr(&desc2->ptr[2], &desc->ptr[2], is_sec1);
+		sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
+					  &desc2->ptr[3], sg_count, offset, 0);
+		if (sg_count > 1)
+			sync_needed = true;
+		copy_talitos_ptr(&desc2->ptr[5], &desc->ptr[5], is_sec1);
+		if (req_ctx->last)
+			to_talitos_ptr(&desc->ptr[5], ctx->dma_hw_context,
+				       req_ctx->hw_context_size, is_sec1);
+
+		next_desc = dma_map_single(dev, &desc2->hdr1, TALITOS_DESC_SIZE,
+					   DMA_BIDIRECTIONAL);
+		desc->next_desc = cpu_to_be32(next_desc);
+	}
+
 	if (sync_needed)
 		dma_sync_single_for_device(dev, edesc->dma_link_tbl,
 					   edesc->dma_len, DMA_BIDIRECTIONAL);
@@ -1796,6 +1862,11 @@ static struct talitos_edesc *ahash_edesc_alloc(struct ahash_request *areq,
 	struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
 	struct talitos_ctx *ctx = crypto_ahash_ctx(tfm);
 	struct talitos_ahash_req_ctx *req_ctx = ahash_request_ctx(areq);
+	struct talitos_private *priv = dev_get_drvdata(ctx->dev);
+	bool is_sec1 = has_ftr_sec1(priv);
+
+	if (is_sec1)
+		nbytes -= req_ctx->nbuf;
 
 	return talitos_edesc_alloc(ctx->dev, req_ctx->psrc, NULL, NULL, 0,
 				   nbytes, 0, 0, 0, areq->base.flags, false);
@@ -1808,6 +1879,8 @@ static int ahash_init(struct ahash_request *areq)
 	struct device *dev = ctx->dev;
 	struct talitos_ahash_req_ctx *req_ctx = ahash_request_ctx(areq);
 	unsigned int size;
+	struct talitos_private *priv = dev_get_drvdata(dev);
+	bool is_sec1 = has_ftr_sec1(priv);
 
 	/* Initialize the context */
 	req_ctx->nbuf = 0;
@@ -1823,6 +1896,13 @@ static int ahash_init(struct ahash_request *areq)
 				 DMA_BIDIRECTIONAL);
 	ctx->dma_hw_context = dma_map_single(dev, req_ctx->hw_context, size,
 					     DMA_BIDIRECTIONAL);
+	if (ctx->dma_buf)
+		dma_unmap_single(dev, ctx->dma_buf, sizeof(req_ctx->buf),
+				 DMA_TO_DEVICE);
+	if (is_sec1)
+		ctx->dma_buf = dma_map_single(dev, req_ctx->buf,
+					      sizeof(req_ctx->buf),
+					      DMA_TO_DEVICE);
 	return 0;
 }
 
@@ -1871,6 +1951,10 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	unsigned int to_hash_later;
 	unsigned int nsg;
 	int nents;
+	struct device *dev = ctx->dev;
+	struct talitos_private *priv = dev_get_drvdata(dev);
+	bool is_sec1 = has_ftr_sec1(priv);
+	int offset = 0;
 
 	if (!req_ctx->last && (nbytes + req_ctx->nbuf <= blocksize)) {
 		/* Buffer up to one whole block */
@@ -1901,13 +1985,27 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	}
 
 	/* Chain in any previously buffered data */
-	if (req_ctx->nbuf) {
+	if (!is_sec1 && req_ctx->nbuf) {
 		nsg = (req_ctx->nbuf < nbytes_to_hash) ? 2 : 1;
 		sg_init_table(req_ctx->bufsl, nsg);
 		sg_set_buf(req_ctx->bufsl, req_ctx->buf, req_ctx->nbuf);
 		if (nsg > 1)
 			sg_chain(req_ctx->bufsl, 2, areq->src);
 		req_ctx->psrc = req_ctx->bufsl;
+	} else if (is_sec1 && req_ctx->nbuf && req_ctx->nbuf < blocksize) {
+		if (nbytes_to_hash > blocksize)
+			offset = blocksize - req_ctx->nbuf;
+		else
+			offset = nbytes_to_hash - req_ctx->nbuf;
+		nents = sg_nents_for_len(areq->src, offset);
+		if (nents < 0) {
+			dev_err(ctx->dev, "Invalid number of src SG.\n");
+			return nents;
+		}
+		sg_copy_to_buffer(areq->src, nents,
+				  req_ctx->buf + req_ctx->nbuf, offset);
+		req_ctx->nbuf += offset;
+		req_ctx->psrc = areq->src;
 	} else
 		req_ctx->psrc = areq->src;
 
@@ -1940,6 +2038,9 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	/* request SEC to INIT hash. */
 	if (req_ctx->first && !req_ctx->swinit)
 		edesc->desc.hdr |= DESC_HDR_MODE0_MDEU_INIT;
+	if (is_sec1)
+		dma_sync_single_for_device(dev, ctx->dma_buf,
+					   req_ctx->nbuf, DMA_TO_DEVICE);
 
 	/* When the tfm context has a keylen, it's an HMAC.
 	 * A first or last (ie. not middle) descriptor must request HMAC.
@@ -1947,7 +2048,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	if (ctx->keylen && (req_ctx->first || req_ctx->last))
 		edesc->desc.hdr |= DESC_HDR_MODE0_MDEU_HMAC;
 
-	return common_nonsnoop_hash(edesc, areq, nbytes_to_hash,
+	return common_nonsnoop_hash(edesc, areq, nbytes_to_hash, offset,
 				    ahash_done);
 }
 
@@ -2019,6 +2120,8 @@ static int ahash_import(struct ahash_request *areq, const void *in)
 	unsigned int size;
 	struct talitos_ctx *ctx = crypto_ahash_ctx(tfm);
 	struct device *dev = ctx->dev;
+	struct talitos_private *priv = dev_get_drvdata(dev);
+	bool is_sec1 = has_ftr_sec1(priv);
 
 	memset(req_ctx, 0, sizeof(*req_ctx));
 	size = (crypto_ahash_digestsize(tfm) <= SHA256_DIGEST_SIZE)
@@ -2032,7 +2135,14 @@ static int ahash_import(struct ahash_request *areq, const void *in)
 	memcpy(req_ctx->hw_context, export->hw_context, size);
 	ctx->dma_hw_context = dma_map_single(dev, req_ctx->hw_context, size,
 					     DMA_BIDIRECTIONAL);
+	if (ctx->dma_buf)
+		dma_unmap_single(dev, ctx->dma_buf, sizeof(req_ctx->buf),
+				 DMA_TO_DEVICE);
 	memcpy(req_ctx->buf, export->buf, export->nbuf);
+	if (is_sec1)
+		ctx->dma_buf = dma_map_single(dev, req_ctx->buf,
+					      sizeof(req_ctx->buf),
+					      DMA_TO_DEVICE);
 	req_ctx->swinit = export->swinit;
 	req_ctx->first = export->first;
 	req_ctx->last = export->last;
@@ -2986,6 +3096,9 @@ static void talitos_cra_exit_ahash(struct crypto_tfm *tfm)
 	if (ctx->dma_hw_context)
 		dma_unmap_single(dev, ctx->dma_hw_context, size,
 				 DMA_BIDIRECTIONAL);
+	if (ctx->dma_buf)
+		dma_unmap_single(dev, ctx->dma_buf, HASH_MAX_BLOCK_SIZE,
+				 DMA_TO_DEVICE);
 }
 
 /*
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 2f04d83c3062..a65a63e0d6c1 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -236,6 +236,7 @@ static inline bool has_ftr_sec1(struct talitos_private *priv)
 #define   TALITOS_CCCR_LO_IWSE		0x80   /* chan. ICCR writeback enab. */
 #define   TALITOS_CCCR_LO_EAE		0x20   /* extended address enable */
 #define   TALITOS_CCCR_LO_CDWE		0x10   /* chan. done writeback enab. */
+#define   TALITOS_CCCR_LO_NE		0x8    /* fetch next descriptor enab. */
 #define   TALITOS_CCCR_LO_NT		0x4    /* notification type */
 #define   TALITOS_CCCR_LO_CDIE		0x2    /* channel done IRQ enable */
 #define   TALITOS1_CCCR_LO_RESET	0x1    /* channel reset on SEC1 */
-- 
2.13.3

Re: [PATCH 17/18] crypto: talitos - chain in buffered data for ahash on SEC1

[Horia Geantă]

On 10/6/2017 4:05 PM, Christophe Leroy wrote:
[...]
> @@ -1778,6 +1814,36 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
>  	if (is_sec1 && from_talitos_ptr_len(&desc->ptr[3], true) == 0)
>  		talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
>  
> +	if (is_sec1 && req_ctx->nbuf && length) {
> +		struct talitos_desc *desc2 = desc + 1;
> +		dma_addr_t next_desc;
[...]
> +		next_desc = dma_map_single(dev, &desc2->hdr1, TALITOS_DESC_SIZE,
> +					   DMA_BIDIRECTIONAL);
> +		desc->next_desc = cpu_to_be32(next_desc);
Where is desc->next_desc initialized for the !is_sec1 case?
Memory allocation is done using kmalloc(), and since desc->next_desc is checked
in some cases also for SEC 2.x+, it should be initialized to 0.

Thanks,
Horia

Re: [PATCH 17/18] crypto: talitos - chain in buffered data for ahash on SEC1

[Horia Geantă]

On 10/6/2017 4:05 PM, Christophe Leroy wrote:=0A=
[...]=0A=
> @@ -1778,6 +1814,36 @@ static int common_nonsnoop_hash(struct talitos_ede=
sc *edesc,=0A=
>  	if (is_sec1 && from_talitos_ptr_len(&desc->ptr[3], true) =3D=3D 0)=0A=
>  		talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);=0A=
>  =0A=
> +	if (is_sec1 && req_ctx->nbuf && length) {=0A=
> +		struct talitos_desc *desc2 =3D desc + 1;=0A=
> +		dma_addr_t next_desc;=0A=
[...]=0A=
> +		next_desc =3D dma_map_single(dev, &desc2->hdr1, TALITOS_DESC_SIZE,=0A=
> +					   DMA_BIDIRECTIONAL);=0A=
> +		desc->next_desc =3D cpu_to_be32(next_desc);=0A=
Where is desc->next_desc initialized for the !is_sec1 case?=0A=
Memory allocation is done using kmalloc(), and since desc->next_desc is che=
cked=0A=
in some cases also for SEC 2.x+, it should be initialized to 0.=0A=
=0A=
Thanks,=0A=
Horia=0A=
=0A=

Re: [PATCH 17/18] crypto: talitos - chain in buffered data for ahash on SEC1

[Christophe LEROY]

Le 02/03/2018 à 18:27, Horia Geantă a écrit :
> On 10/6/2017 4:05 PM, Christophe Leroy wrote:
> [...]
>> @@ -1778,6 +1814,36 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
>>   	if (is_sec1 && from_talitos_ptr_len(&desc->ptr[3], true) == 0)
>>   		talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
>>   
>> +	if (is_sec1 && req_ctx->nbuf && length) {
>> +		struct talitos_desc *desc2 = desc + 1;
>> +		dma_addr_t next_desc;
> [...]
>> +		next_desc = dma_map_single(dev, &desc2->hdr1, TALITOS_DESC_SIZE,
>> +					   DMA_BIDIRECTIONAL);
>> +		desc->next_desc = cpu_to_be32(next_desc);
> Where is desc->next_desc initialized for the !is_sec1 case?
> Memory allocation is done using kmalloc(), and since desc->next_desc is checked
> in some cases also for SEC 2.x+, it should be initialized to 0.

See 
https://elixir.bootlin.com/linux/v4.16-rc3/source/drivers/crypto/talitos.c#L1411

	edesc = kmalloc(alloc_len, GFP_DMA | flags);
	if (!edesc) {
		dev_err(dev, "could not allocate edescriptor\n");
		err = ERR_PTR(-ENOMEM);
		goto error_sg;
	}
	memset(&edesc->desc, 0, sizeof(edesc->desc));


Christophe

[PATCH 18/18] crypto: talitos - avoid useless copy

[Christophe Leroy]

This patch avoids copy of buffered data to hash from bufnext to buf

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 5c4499a85611..5bd8191405d8 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -842,8 +842,8 @@ struct talitos_ctx {
 struct talitos_ahash_req_ctx {
 	u32 hw_context[TALITOS_MDEU_MAX_CONTEXT_SIZE / sizeof(u32)];
 	unsigned int hw_context_size;
-	u8 buf[HASH_MAX_BLOCK_SIZE];
-	u8 bufnext[HASH_MAX_BLOCK_SIZE];
+	u8 buf[2][HASH_MAX_BLOCK_SIZE];
+	int buf_idx;
 	unsigned int swinit;
 	unsigned int first;
 	unsigned int last;
@@ -1709,7 +1709,7 @@ static void ahash_done(struct device *dev,
 
 	if (!req_ctx->last && req_ctx->to_hash_later) {
 		/* Position any partial block for next update/final/finup */
-		memcpy(req_ctx->buf, req_ctx->bufnext, req_ctx->to_hash_later);
+		req_ctx->buf_idx = (req_ctx->buf_idx + 1) & 1;
 		req_ctx->nbuf = req_ctx->to_hash_later;
 	}
 	common_nonsnoop_hash_unmap(dev, edesc, areq);
@@ -1789,8 +1789,10 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 	 * data in
 	 */
 	if (is_sec1 && req_ctx->nbuf) {
-		to_talitos_ptr(&desc->ptr[3], ctx->dma_buf, req_ctx->nbuf,
-			       is_sec1);
+		dma_addr_t dma_buf = ctx->dma_buf + req_ctx->buf_idx *
+						    HASH_MAX_BLOCK_SIZE;
+
+		to_talitos_ptr(&desc->ptr[3], dma_buf, req_ctx->nbuf, is_sec1);
 	} else {
 		sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
 					  &desc->ptr[3], sg_count, offset, 0);
@@ -1883,6 +1885,7 @@ static int ahash_init(struct ahash_request *areq)
 	bool is_sec1 = has_ftr_sec1(priv);
 
 	/* Initialize the context */
+	req_ctx->buf_idx = 0;
 	req_ctx->nbuf = 0;
 	req_ctx->first = 1; /* first indicates h/w must init its context */
 	req_ctx->swinit = 0; /* assume h/w init of context */
@@ -1955,6 +1958,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	struct talitos_private *priv = dev_get_drvdata(dev);
 	bool is_sec1 = has_ftr_sec1(priv);
 	int offset = 0;
+	u8 *ctx_buf = req_ctx->buf[req_ctx->buf_idx];
 
 	if (!req_ctx->last && (nbytes + req_ctx->nbuf <= blocksize)) {
 		/* Buffer up to one whole block */
@@ -1964,7 +1968,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 			return nents;
 		}
 		sg_copy_to_buffer(areq->src, nents,
-				  req_ctx->buf + req_ctx->nbuf, nbytes);
+				  ctx_buf + req_ctx->nbuf, nbytes);
 		req_ctx->nbuf += nbytes;
 		return 0;
 	}
@@ -1988,7 +1992,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	if (!is_sec1 && req_ctx->nbuf) {
 		nsg = (req_ctx->nbuf < nbytes_to_hash) ? 2 : 1;
 		sg_init_table(req_ctx->bufsl, nsg);
-		sg_set_buf(req_ctx->bufsl, req_ctx->buf, req_ctx->nbuf);
+		sg_set_buf(req_ctx->bufsl, ctx_buf, req_ctx->nbuf);
 		if (nsg > 1)
 			sg_chain(req_ctx->bufsl, 2, areq->src);
 		req_ctx->psrc = req_ctx->bufsl;
@@ -2003,7 +2007,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 			return nents;
 		}
 		sg_copy_to_buffer(areq->src, nents,
-				  req_ctx->buf + req_ctx->nbuf, offset);
+				  ctx_buf + req_ctx->nbuf, offset);
 		req_ctx->nbuf += offset;
 		req_ctx->psrc = areq->src;
 	} else
@@ -2016,7 +2020,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 			return nents;
 		}
 		sg_pcopy_to_buffer(areq->src, nents,
-				      req_ctx->bufnext,
+				   req_ctx->buf[(req_ctx->buf_idx + 1) & 1],
 				      to_hash_later,
 				      nbytes - to_hash_later);
 	}
@@ -2038,9 +2042,13 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	/* request SEC to INIT hash. */
 	if (req_ctx->first && !req_ctx->swinit)
 		edesc->desc.hdr |= DESC_HDR_MODE0_MDEU_INIT;
-	if (is_sec1)
-		dma_sync_single_for_device(dev, ctx->dma_buf,
+	if (is_sec1) {
+		dma_addr_t dma_buf = ctx->dma_buf + req_ctx->buf_idx *
+						    HASH_MAX_BLOCK_SIZE;
+
+		dma_sync_single_for_device(dev, dma_buf,
 					   req_ctx->nbuf, DMA_TO_DEVICE);
+	}
 
 	/* When the tfm context has a keylen, it's an HMAC.
 	 * A first or last (ie. not middle) descriptor must request HMAC.
@@ -2102,7 +2110,7 @@ static int ahash_export(struct ahash_request *areq, void *out)
 				req_ctx->hw_context_size, DMA_FROM_DEVICE);
 	memcpy(export->hw_context, req_ctx->hw_context,
 	       req_ctx->hw_context_size);
-	memcpy(export->buf, req_ctx->buf, req_ctx->nbuf);
+	memcpy(export->buf, req_ctx->buf[req_ctx->buf_idx], req_ctx->nbuf);
 	export->swinit = req_ctx->swinit;
 	export->first = req_ctx->first;
 	export->last = req_ctx->last;
@@ -2138,7 +2146,7 @@ static int ahash_import(struct ahash_request *areq, const void *in)
 	if (ctx->dma_buf)
 		dma_unmap_single(dev, ctx->dma_buf, sizeof(req_ctx->buf),
 				 DMA_TO_DEVICE);
-	memcpy(req_ctx->buf, export->buf, export->nbuf);
+	memcpy(req_ctx->buf[0], export->buf, export->nbuf);
 	if (is_sec1)
 		ctx->dma_buf = dma_map_single(dev, req_ctx->buf,
 					      sizeof(req_ctx->buf),
@@ -3097,7 +3105,7 @@ static void talitos_cra_exit_ahash(struct crypto_tfm *tfm)
 		dma_unmap_single(dev, ctx->dma_hw_context, size,
 				 DMA_BIDIRECTIONAL);
 	if (ctx->dma_buf)
-		dma_unmap_single(dev, ctx->dma_buf, HASH_MAX_BLOCK_SIZE,
+		dma_unmap_single(dev, ctx->dma_buf, HASH_MAX_BLOCK_SIZE * 2,
 				 DMA_TO_DEVICE);
 }
 
-- 
2.13.3

Re: [PATCH 00/18] crypto: talitos - fixes and performance improvement

[Herbert Xu]

On Fri, Oct 06, 2017 at 03:04:31PM +0200, Christophe Leroy wrote:
> This serie fixes and improves the talitos crypto driver.
> 
> First 6 patchs are fixes of failures reported by the new tests in the
> kernel crypto test manager.
> 
> The 8 following patches are cleanups and simplifications.
> 
> The last 4 ones are performance improvement. The main improvement is
> in the one before the last, it divides by 2 the time needed for a md5
> hash on the SEC1.
> 
> Christophe Leroy (18):
>   crypto: talitos - fix AEAD test failures
>   crypto: talitos - fix memory corruption on SEC2
>   crypto: talitos - fix setkey to check key weakness
>   crypto: talitos - fix AEAD for sha224 on non sha224 capable chips
>   crypto: talitos - fix use of sg_link_tbl_len
>   crypto: talitos - fix ctr-aes-talitos
>   crypto: talitos - zeroize the descriptor with memset()
>   crypto: talitos - declare local functions static
>   crypto: talitos - use devm_kmalloc()
>   crypto: talitos - use of_property_read_u32()
>   crypto: talitos - use devm_ioremap()
>   crypto: talitos - don't check the number of channels at each interrupt
>   crypto: talitos - remove to_talitos_ptr_len()
>   crypto: talitos - simplify tests in ipsec_esp()
>   crypto: talitos - DMA map key in setkey()
>   crypto: talitos - do hw_context DMA mapping outside the requests
>   crypto: talitos - chain in buffered data for ahash on SEC1
>   crypto: talitos - avoid useless copy

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/18] crypto: talitos - fixes and performance improvement

[Horia Geantă]

On 10/12/2017 6:20 PM, Herbert Xu wrote:
> On Fri, Oct 06, 2017 at 03:04:31PM +0200, Christophe Leroy wrote:
>> This serie fixes and improves the talitos crypto driver.
>>
>> First 6 patchs are fixes of failures reported by the new tests in the
>> kernel crypto test manager.
>>
Looks like these fixes are required also on older 4.9+ -stable kernels.
(I haven't seen them on latest 4.9.68-stable mail from Greg, even though
they are in main tree.)

In case you agree, what would be the recommended way to add the patches
to -stable?

Thanks,
Horia

>> The 8 following patches are cleanups and simplifications.
>>
>> The last 4 ones are performance improvement. The main improvement is
>> in the one before the last, it divides by 2 the time needed for a md5
>> hash on the SEC1.
>>
>> Christophe Leroy (18):
>>   crypto: talitos - fix AEAD test failures
>>   crypto: talitos - fix memory corruption on SEC2
>>   crypto: talitos - fix setkey to check key weakness
>>   crypto: talitos - fix AEAD for sha224 on non sha224 capable chips
>>   crypto: talitos - fix use of sg_link_tbl_len
>>   crypto: talitos - fix ctr-aes-talitos
>>   crypto: talitos - zeroize the descriptor with memset()
>>   crypto: talitos - declare local functions static
>>   crypto: talitos - use devm_kmalloc()
>>   crypto: talitos - use of_property_read_u32()
>>   crypto: talitos - use devm_ioremap()
>>   crypto: talitos - don't check the number of channels at each interrupt
>>   crypto: talitos - remove to_talitos_ptr_len()
>>   crypto: talitos - simplify tests in ipsec_esp()
>>   crypto: talitos - DMA map key in setkey()
>>   crypto: talitos - do hw_context DMA mapping outside the requests
>>   crypto: talitos - chain in buffered data for ahash on SEC1
>>   crypto: talitos - avoid useless copy
> 
> All applied.  Thanks.
> 

Re: [PATCH 00/18] crypto: talitos - fixes and performance improvement

[Horia Geantă]

On 10/12/2017 6:20 PM, Herbert Xu wrote:=0A=
> On Fri, Oct 06, 2017 at 03:04:31PM +0200, Christophe Leroy wrote:=0A=
>> This serie fixes and improves the talitos crypto driver.=0A=
>>=0A=
>> First 6 patchs are fixes of failures reported by the new tests in the=0A=
>> kernel crypto test manager.=0A=
>>=0A=
Looks like these fixes are required also on older 4.9+ -stable kernels.=0A=
(I haven't seen them on latest 4.9.68-stable mail from Greg, even though=0A=
they are in main tree.)=0A=
=0A=
In case you agree, what would be the recommended way to add the patches=0A=
to -stable?=0A=
=0A=
Thanks,=0A=
Horia=0A=
=0A=
>> The 8 following patches are cleanups and simplifications.=0A=
>>=0A=
>> The last 4 ones are performance improvement. The main improvement is=0A=
>> in the one before the last, it divides by 2 the time needed for a md5=0A=
>> hash on the SEC1.=0A=
>>=0A=
>> Christophe Leroy (18):=0A=
>>   crypto: talitos - fix AEAD test failures=0A=
>>   crypto: talitos - fix memory corruption on SEC2=0A=
>>   crypto: talitos - fix setkey to check key weakness=0A=
>>   crypto: talitos - fix AEAD for sha224 on non sha224 capable chips=0A=
>>   crypto: talitos - fix use of sg_link_tbl_len=0A=
>>   crypto: talitos - fix ctr-aes-talitos=0A=
>>   crypto: talitos - zeroize the descriptor with memset()=0A=
>>   crypto: talitos - declare local functions static=0A=
>>   crypto: talitos - use devm_kmalloc()=0A=
>>   crypto: talitos - use of_property_read_u32()=0A=
>>   crypto: talitos - use devm_ioremap()=0A=
>>   crypto: talitos - don't check the number of channels at each interrupt=
=0A=
>>   crypto: talitos - remove to_talitos_ptr_len()=0A=
>>   crypto: talitos - simplify tests in ipsec_esp()=0A=
>>   crypto: talitos - DMA map key in setkey()=0A=
>>   crypto: talitos - do hw_context DMA mapping outside the requests=0A=
>>   crypto: talitos - chain in buffered data for ahash on SEC1=0A=
>>   crypto: talitos - avoid useless copy=0A=
> =0A=
> All applied.  Thanks.=0A=
> =0A=

Re: [PATCH 00/18] crypto: talitos - fixes and performance improvement

[Herbert Xu]

On Fri, Dec 08, 2017 at 03:20:40PM +0000, Horia Geantă wrote:
> On 10/12/2017 6:20 PM, Herbert Xu wrote:
> > On Fri, Oct 06, 2017 at 03:04:31PM +0200, Christophe Leroy wrote:
> >> This serie fixes and improves the talitos crypto driver.
> >>
> >> First 6 patchs are fixes of failures reported by the new tests in the
> >> kernel crypto test manager.
> >>
> Looks like these fixes are required also on older 4.9+ -stable kernels.
> (I haven't seen them on latest 4.9.68-stable mail from Greg, even though
> they are in main tree.)
> 
> In case you agree, what would be the recommended way to add the patches
> to -stable?

I'll forward them to stable.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt