diff mbox series

[v5,3/7] crypto: stm32 - Simplify finup

Message ID E1pYOKH-000GYy-Rv@formenos.hmeau.com
State Superseded
Headers show
Series crypto: stm32 - Save and restore between each request | expand

Commit Message

Herbert Xu March 4, 2023, 9:37 a.m. UTC 
The current finup code is unnecessarily convoluted.  There is no
need to call update and final separately as update already does
all the necessary work on its own.

Simplify this by utilising the HASH_FLAGS_FINUP bit in rctx to
indicate only finup and use the HASH_FLAGS_FINAL bit instead to
signify processing common to both final and finup.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/stm32/stm32-hash.c |   34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

Comments

Linus Walleij March 5, 2023, 10:08 p.m. UTC | #1 
On Sat, Mar 4, 2023 at 10:37 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The current finup code is unnecessarily convoluted.  There is no
> need to call update and final separately as update already does
> all the necessary work on its own.
>
> Simplify this by utilising the HASH_FLAGS_FINUP bit in rctx to
> indicate only finup and use the HASH_FLAGS_FINAL bit instead to
> signify processing common to both final and finup.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

All tests sadly fail after this patch:

[    4.699857] stm32-hash a03c2000.hash: allocated hmac(sha256) fallback
[    4.708231] alg: ahash: stm32-hmac-sha256 test failed (wrong
result) on test vector 0, cfg="init+finup aligned buffer"
[    4.719048] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-22)
[    4.719070] ------------[ cut here ]------------
[    4.731602] WARNING: CPU: 0 PID: 88 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[    4.739832] alg: self-tests for hmac(sha256) using
stm32-hmac-sha256 failed (rc=-22)
[    4.739853] Modules linked in:
[    4.750718] CPU: 0 PID: 88 Comm: cryptomgr_test Not tainted
6.2.0-13567-g410ada7489b7 #76
[    4.758915] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[    4.765897]  unwind_backtrace from show_stack+0x10/0x14
[    4.771169]  show_stack from dump_stack_lvl+0x40/0x4c
[    4.776270]  dump_stack_lvl from __warn+0x94/0xc0
[    4.781022]  __warn from warn_slowpath_fmt+0x118/0x164
[    4.786199]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[    4.792167]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[    4.797696]  cryptomgr_test from kthread+0xc0/0xc4
[    4.802527]  kthread from ret_from_fork+0x14/0x2c
[    4.807257] Exception stack(0xf10b9fb0 to 0xf10b9ff8)
[    4.812320] 9fa0:                                     00000000
00000000 00000000 00000000
[    4.820510] 9fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    4.828698] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    4.835475] ---[ end trace 0000000000000000 ]---
[    4.840713] stm32-hash a03c2000.hash: allocated sha256 fallback
[    4.854071] alg: ahash: stm32-sha256 test failed (wrong result) on
test vector 1, cfg="init+finup aligned buffer"
[    4.864487] alg: self-tests for sha256 using stm32-sha256 failed (rc=-22)
[    4.864500] ------------[ cut here ]------------
[    4.875924] WARNING: CPU: 0 PID: 98 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[    4.884108] alg: self-tests for sha256 using stm32-sha256 failed (rc=-22)
[    4.884119] Modules linked in:
[    4.894022] CPU: 0 PID: 98 Comm: cryptomgr_test Tainted: G        W
         6.2.0-13567-g410ada7489b7 #76
[    4.903679] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[    4.910642]  unwind_backtrace from show_stack+0x10/0x14
[    4.915884]  show_stack from dump_stack_lvl+0x40/0x4c
[    4.920954]  dump_stack_lvl from __warn+0x94/0xc0
[    4.925675]  __warn from warn_slowpath_fmt+0x118/0x164
[    4.930826]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[    4.936764]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[    4.942265]  cryptomgr_test from kthread+0xc0/0xc4
[    4.947070]  kthread from ret_from_fork+0x14/0x2c
[    4.951780] Exception stack(0xf10e1fb0 to 0xf10e1ff8)
[    4.956830] 1fa0:                                     00000000
00000000 00000000 00000000
[    4.965005] 1fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    4.973179] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    4.979850] ---[ end trace 0000000000000000 ]---
[    5.449257] stm32-hash a03c2000.hash: allocated hmac(sha1) fallback
[    5.458347] alg: ahash: stm32-hmac-sha1 test failed (wrong result)
on test vector 0, cfg="init+finup aligned buffer"
[    5.469070] alg: self-tests for hmac(sha1) using stm32-hmac-sha1
failed (rc=-22)
[    5.469112] ------------[ cut here ]------------
[    5.481280] WARNING: CPU: 1 PID: 101 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[    5.489663] alg: self-tests for hmac(sha1) using stm32-hmac-sha1
failed (rc=-22)
[    5.489699] Modules linked in:
[    5.500277] CPU: 1 PID: 101 Comm: cryptomgr_test Tainted: G
W          6.2.0-13567-g410ada7489b7 #76
[    5.510038] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[    5.517014]  unwind_backtrace from show_stack+0x10/0x14
[    5.522287]  show_stack from dump_stack_lvl+0x40/0x4c
[    5.527390]  dump_stack_lvl from __warn+0x94/0xc0
[    5.532140]  __warn from warn_slowpath_fmt+0x118/0x164
[    5.537318]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[    5.543284]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[    5.548815]  cryptomgr_test from kthread+0xc0/0xc4
[    5.553648]  kthread from ret_from_fork+0x14/0x2c
[    5.558378] Exception stack(0xf10edfb0 to 0xf10edff8)
[    5.563443] dfa0:                                     00000000
00000000 00000000 00000000
[    5.571633] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    5.579822] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    5.586511] ---[ end trace 0000000000000000 ]---
[    5.591474] stm32-hash a03c2000.hash: allocated sha1 fallback
[    5.610829] alg: ahash: stm32-sha1 test failed (wrong result) on
test vector 1, cfg="init+finup aligned buffer"
[    5.621005] alg: self-tests for sha1 using stm32-sha1 failed (rc=-22)
[    5.621014] ------------[ cut here ]------------
[    5.632136] WARNING: CPU: 0 PID: 113 at crypto/testmgr.c:5858
alg_test.part.0+0x4d0/0x4dc
[    5.640368] alg: self-tests for sha1 using stm32-sha1 failed (rc=-22)
[    5.640377] Modules linked in:
[    5.649882] CPU: 0 PID: 113 Comm: cryptomgr_test Tainted: G
W          6.2.0-13567-g410ada7489b7 #76
[    5.659620] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[    5.666579]  unwind_backtrace from show_stack+0x10/0x14
[    5.671817]  show_stack from dump_stack_lvl+0x40/0x4c
[    5.676881]  dump_stack_lvl from __warn+0x94/0xc0
[    5.681594]  __warn from warn_slowpath_fmt+0x118/0x164
[    5.686739]  warn_slowpath_fmt from alg_test.part.0+0x4d0/0x4dc
[    5.692669]  alg_test.part.0 from cryptomgr_test+0x18/0x38
[    5.698163]  cryptomgr_test from kthread+0xc0/0xc4
[    5.702962]  kthread from ret_from_fork+0x14/0x2c
[    5.707668] Exception stack(0xf10edfb0 to 0xf10edff8)
[    5.712716] dfa0:                                     00000000
00000000 00000000 00000000
[    5.720887] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    5.729057] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    5.735717] ---[ end trace 0000000000000000 ]---

As usual the fallback works. I tried to look into it but I don't
understand this part:

@ -768,17 +773,14 @@ static int stm32_hash_final_req(struct
stm32_hash_dev *hdev)

> +       if (rctx->flags & HASH_FLAGS_FINUP)
> +               return stm32_hash_update_req(hdev);

stm32_hash_update_req() will do

if (!(rctx->flags & HASH_FLAGS_CPU))
    return stm32_hash_dma_send(hdev);
(...)
return stm32_hash_update_cpu(hdev);

So it replaces:

> -       if (!(rctx->flags & HASH_FLAGS_CPU))
> -               err = stm32_hash_dma_send(hdev);
> -       else
> -               err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);

But now we are calling stm32_hash_update_cpu() instead of
stm32_hash_xmit_cpu(), because that is what stm32_hash_update_cpu()
does.

Yours,
Linus Walleij
Herbert Xu March 6, 2023, 4:37 a.m. UTC | #2 
On Sun, Mar 05, 2023 at 11:08:24PM +0100, Linus Walleij wrote:
>
> All tests sadly fail after this patch:
> 
> [    4.699857] stm32-hash a03c2000.hash: allocated hmac(sha256) fallback
> [    4.708231] alg: ahash: stm32-hmac-sha256 test failed (wrong
> result) on test vector 0, cfg="init+finup aligned buffer"

We're making progress.  This worked on init+update+final, but then
failed on init+finup.  So it is indeed the finup code that was
broken by my patch as I forgot to set the rctx fields to include
the new data.

I'll repost the series.

Thanks,
diff mbox series

Patch

diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 298cabd29e36..473809b26566 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -417,7 +417,7 @@  static int stm32_hash_update_cpu(struct stm32_hash_dev *hdev)
 
 	dev_dbg(hdev->dev, "%s flags %lx\n", __func__, rctx->flags);
 
-	final = (rctx->flags & HASH_FLAGS_FINUP);
+	final = rctx->flags & HASH_FLAGS_FINAL;
 
 	while ((rctx->total >= rctx->buflen) ||
 	       (rctx->bufcnt + rctx->total >= rctx->buflen)) {
@@ -761,6 +761,11 @@  static int stm32_hash_init(struct ahash_request *req)
 
 static int stm32_hash_update_req(struct stm32_hash_dev *hdev)
 {
+	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
+
+	if (!(rctx->flags & HASH_FLAGS_CPU))
+		return stm32_hash_dma_send(hdev);
+
 	return stm32_hash_update_cpu(hdev);
 }
 
@@ -768,17 +773,14 @@  static int stm32_hash_final_req(struct stm32_hash_dev *hdev)
 {
 	struct ahash_request *req = hdev->req;
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
-	int err;
 	int buflen = rctx->bufcnt;
 
-	rctx->bufcnt = 0;
+	if (rctx->flags & HASH_FLAGS_FINUP)
+		return stm32_hash_update_req(hdev);
 
-	if (!(rctx->flags & HASH_FLAGS_CPU))
-		err = stm32_hash_dma_send(hdev);
-	else
-		err = stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
+	rctx->bufcnt = 0;
 
-	return err;
+	return stm32_hash_xmit_cpu(hdev, rctx->buffer, buflen, 1);
 }
 
 static void stm32_hash_emptymsg_fallback(struct ahash_request *req)
@@ -1000,7 +1002,7 @@  static int stm32_hash_final(struct ahash_request *req)
 {
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 
-	rctx->flags |= HASH_FLAGS_FINUP;
+	rctx->flags |= HASH_FLAGS_FINAL;
 
 	return stm32_hash_enqueue(req, HASH_OP_FINAL);
 }
@@ -1010,25 +1012,13 @@  static int stm32_hash_finup(struct ahash_request *req)
 	struct stm32_hash_request_ctx *rctx = ahash_request_ctx(req);
 	struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
 	struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
-	int err1, err2;
 
 	rctx->flags |= HASH_FLAGS_FINUP;
 
 	if (hdev->dma_lch && stm32_hash_dma_aligned_data(req))
 		rctx->flags &= ~HASH_FLAGS_CPU;
 
-	err1 = stm32_hash_update(req);
-
-	if (err1 == -EINPROGRESS || err1 == -EBUSY)
-		return err1;
-
-	/*
-	 * final() has to be always called to cleanup resources
-	 * even if update() failed, except EINPROGRESS
-	 */
-	err2 = stm32_hash_final(req);
-
-	return err1 ?: err2;
+	return stm32_hash_final(req);
 }
 
 static int stm32_hash_digest(struct ahash_request *req)