Re: [PATCH v5] crypto: s5p-sss: Add HASH support for Exynos

[Vladimir Zapolskiy]

Hello Kamil,

thank you for the change, please find below a number of minor
review comments.

On 10/09/2017 02:12 PM, Kamil Konieczny wrote:
> Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW.
> It uses the crypto framework asynchronous hash api.
> It is based on omap-sham.c driver.
> S5P has some HW differencies and is not implemented.
> 

[snip]

>  
>  /* Feed control registers */
>  #define SSS_REG_FCINTSTAT   0x
> +#define SSS_FCINTSTAT_HPARTINT   BIT(7)
> +#define SSS_FCINTSTAT_HDONEINT   BIT(5)

  ^
Please use the same style of whitespaces as it is found around.

Generally I do agree that the tabs are better here, please feel free
to send a preceding change, which changes the spacing to tabs, otherwise
use space symbols in this change.

>  #define SSS_FCINTSTAT_BRDMAINT  BIT(3)
>  #define SSS_FCINTSTAT_BTDMAINT  BIT(2)
>  #define SSS_FCINTSTAT_HRDMAINT  BIT(1)
>  #define SSS_FCINTSTAT_PKDMAINT  BIT(0)
>  
>  #define SSS_REG_FCINTENSET  0x0004
> +#define SSS_FCINTENSET_HPARTINTENSET BIT(7)
> +#define SSS_FCINTENSET_HDONEINTENSET BIT(5)

Same as above.

>  #define SSS_FCINTENSET_BRDMAINTENSETBIT(3)
>  #define SSS_FCINTENSET_BTDMAINTENSETBIT(2)
>  #define SSS_FCINTENSET_HRDMAINTENSETBIT(1)
>  #define SSS_FCINTENSET_PKDMAINTENSETBIT(0)
>  
>  #define SSS_REG_FCINTENCLR  0x0008
> +#define SSS_FCINTENCLR_HPARTINTENCLR BIT(7)
> +#define SSS_FCINTENCLR_HDONEINTENCLR BIT(5)

Same as above.

>  #define SSS_FCINTENCLR_BRDMAINTENCLRBIT(3)
>  #define SSS_FCINTENCLR_BTDMAINTENCLRBIT(2)
>  #define SSS_FCINTENCLR_HRDMAINTENCLRBIT(1)
>  #define SSS_FCINTENCLR_PKDMAINTENCLRBIT(0)
>  
>  #define SSS_REG_FCINTPEND   0x000C
> +#define SSS_FCINTPEND_HPARTINTP  BIT(7)
> +#define SSS_FCINTPEND_HDONEINTP  BIT(5)

Same as above.

>  #define SSS_FCINTPEND_BRDMAINTP BIT(3)
>  #define SSS_FCINTPEND_BTDMAINTP BIT(2)
>  #define SSS_FCINTPEND_HRDMAINTP BIT(1)
> @@ -72,6 +88,7 @@
>  #define SSS_HASHIN_INDEPENDENT  _SBF(0, 0x00)
>  #define SSS_HASHIN_CIPHER_INPUT _SBF(0, 0x01)
>  #define SSS_HASHIN_CIPHER_OUTPUT_SBF(0, 0x02)
> +#define SSS_HASHIN_MASK  _SBF(0, 0x03)

Same as above.

[snip]

>  struct s5p_aes_reqctx {
> @@ -195,6 +284,19 @@ struct s5p_aes_ctx {
>   *   protects against concurrent access to these fields.
>   * @lock:Lock for protecting both access to device hardware registers
>   *   and fields related to current request (including the busy 
> field).
> + * @res: Resources for hash.
> + * @io_hash_base: Per-variant offset for HASH block IO memory.
> + * @hash_lock:   Lock for protecting hash_req, hash_queue and hash_flags
> + *   variable.
> + * @hash_tasklet: New HASH request scheduling job.
> + * @xmit_buf:Buffer for current HASH request transfer into SSS block.
> + * @hash_flags:  Flags for current HASH op.
> + * @hash_queue:  Async hash queue.
> + * @hash_req:Current request sending to SSS HASH block.
> + * @hash_sg_iter: Scatterlist transferred through DMA into SSS HASH block.
> + * @hash_sg_cnt: Counter for hash_sg_iter.
> + *
> + * @use_hash:true if HASH algs enabled

You may want to reorder the lines to get the same order as in the struct.

>   */
>  struct s5p_aes_dev {
>   struct device   *dev;
> @@ -215,16 +317,88 @@ struct s5p_aes_dev {
>   struct crypto_queue queue;
>   boolbusy;
>   spinlock_t  lock;
> +
> + struct resource *res;
> + void __iomem*io_hash_base;
> +
> + spinlock_t  hash_lock; /* protect hash_ vars */
> + unsigned long   hash_flags;
> + struct crypto_queue hash_queue;
> + struct tasklet_struct   hash_tasklet;
> +
> + u8  xmit_buf[BUFLEN];
> + struct ahash_request*hash_req;
> + struct scatterlist  *hash_sg_iter;
> + int hash_sg_cnt;

Please change the type to 'unsigned int'.

> +
> + booluse_hash;
>  };
>  
> -static struct s5p_aes_dev *s5p_dev;
> +enum hash_op {
> + HASH_OP_UPDATE = 1,
> + HASH_OP_FINAL = 2
> +};

If this type is not going to be extended, then I'd rather suggest to
use a boolean correspondent field in the struct s5p_hash_reqctx instead
of a new introduced type.

> +
> +/**
> + * struct s5p_hash_reqctx - HASH request context
> + * @dev: Associated device
> + * @op:  Current request operation (OP_UPDATE or OP_FINAL)

See a comment above.

> + * @digcnt:  Number of bytes processed by HW (without buffer[] ones)
> + * @digest:  Digest message or IV for partial result
> + * 

Re: [PATCH v5] crypto: s5p-sss: Add HASH support for Exynos

[Krzysztof Kozlowski]

On Mon, Oct 09, 2017 at 01:12:30PM +0200, Kamil Konieczny wrote:
> Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW.
> It uses the crypto framework asynchronous hash api.
> It is based on omap-sham.c driver.
> S5P has some HW differencies and is not implemented.
> 
> Modifications in s5p-sss:
> 
> - Add hash supporting structures and functions.
> 
> - Modify irq handler to handle both aes and hash signals.
> 
> - Resize resource end in probe if EXYNOS_HASH is enabled in
>   Kconfig.
> 
> - Add new copyright line and new author.
> 
> - Tested on Odroid-U3 with Exynos 4412 CPU, kernel 4.13-rc6
>   with crypto run-time self test testmgr
>   and with tcrypt module with: modprobe tcrypt sec=1 mode=N
>   where N=402, 403, 404 (MD5, SHA1, SHA256).
> 
> Modifications in drivers/crypto/Kconfig:
> 
> - Add new CRYPTO_DEV_EXYNOS_HASH, depend on !EXYNOS_RNG
>   and CRYPTO_DEV_S5P
> 
> - Select sw algorithms MD5, SHA1 and SHA256 in EXYNOS_HASH
>   as they are nedded for fallback.
> 
> Signed-off-by: Kamil Konieczny 
> ---
> version 5:
> - fix suggested by Krzysztof Kozlowski: change defines HASH_OP into enum, fix
>   comments
> 
> version 4:
> - fixes suggested by Krzysztof Kozlowski: reformat comments, convert context
>   flags into two bool vars, drop SSS_ALIGNED, change name of SSS_DMA_ALIGN and
>   SSS_DMA_ALIGN_MASK, split assignments into separate lines, use IS_ENABLED in
>   place of ifdef, remove sss_hash_algs_info and simplify register and 
> deregister
>   HASH algs
> 
> version 3:
> - many fixes suggested by Krzysztof Kozlowski: comments, uppercases in const,
>   remove unused defines, remove unused variable bs, constify aes_variant,
>   remove global var use_hash, remove WARN_ON, improve hash_import(),
>   change goto label into 'out' in s5p_hash_handle_queue(), reorder variable
>   declarations, add spinlock to protect clearing HASH_FLAGS_BUSY
> - simplify code: replace one-line functions s5p_hash_update_req(),
>   s5p_hash_final_req() with call to s5p_hash_xmit_dma(), and delete them
> - replace call to s5p_hash_hw_init() into s5p_ahash_dma_init() and delete it
> - fix clearing shash flag CRYPTO_TFM_REQ_MAY_SLEEP
> - fix s5p_hash_set_flow()
> 
> version 2:
> - change patch format so number of lines drops
> - change in Kconfig as suggested by Krzysztof Kozlowski, add
>   EXYNOS_HASH subsection
> - change #ifndef EXYNOS_RNG into #ifdef CRYPTO_DEV_EXYNOS_HASH
> - remove style fixups in aes, as they should go in separate patch
> - remove FLOW_LOG, FLOW_DUMP macros and its uses
> - remove #if 0 ... endif
> - remove unused function hash_wait and its defines
> - fix compiler warning in dev_dbg
> - remove some comments
> - other minor fixes in comments
> 
>  drivers/crypto/Kconfig   |   14 +
>  drivers/crypto/s5p-sss.c | 1441 
> +-
>  2 files changed, 1445 insertions(+), 10 deletions(-)
> 

Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

[PATCH v5] crypto: s5p-sss: Add HASH support for Exynos

[Kamil Konieczny]

Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW.
It uses the crypto framework asynchronous hash api.
It is based on omap-sham.c driver.
S5P has some HW differencies and is not implemented.

Modifications in s5p-sss:

- Add hash supporting structures and functions.

- Modify irq handler to handle both aes and hash signals.

- Resize resource end in probe if EXYNOS_HASH is enabled in
  Kconfig.

- Add new copyright line and new author.

- Tested on Odroid-U3 with Exynos 4412 CPU, kernel 4.13-rc6
  with crypto run-time self test testmgr
  and with tcrypt module with: modprobe tcrypt sec=1 mode=N
  where N=402, 403, 404 (MD5, SHA1, SHA256).

Modifications in drivers/crypto/Kconfig:

- Add new CRYPTO_DEV_EXYNOS_HASH, depend on !EXYNOS_RNG
  and CRYPTO_DEV_S5P

- Select sw algorithms MD5, SHA1 and SHA256 in EXYNOS_HASH
  as they are nedded for fallback.

Signed-off-by: Kamil Konieczny 
---
version 5:
- fix suggested by Krzysztof Kozlowski: change defines HASH_OP into enum, fix
  comments

version 4:
- fixes suggested by Krzysztof Kozlowski: reformat comments, convert context
  flags into two bool vars, drop SSS_ALIGNED, change name of SSS_DMA_ALIGN and
  SSS_DMA_ALIGN_MASK, split assignments into separate lines, use IS_ENABLED in
  place of ifdef, remove sss_hash_algs_info and simplify register and deregister
  HASH algs

version 3:
- many fixes suggested by Krzysztof Kozlowski: comments, uppercases in const,
  remove unused defines, remove unused variable bs, constify aes_variant,
  remove global var use_hash, remove WARN_ON, improve hash_import(),
  change goto label into 'out' in s5p_hash_handle_queue(), reorder variable
  declarations, add spinlock to protect clearing HASH_FLAGS_BUSY
- simplify code: replace one-line functions s5p_hash_update_req(),
  s5p_hash_final_req() with call to s5p_hash_xmit_dma(), and delete them
- replace call to s5p_hash_hw_init() into s5p_ahash_dma_init() and delete it
- fix clearing shash flag CRYPTO_TFM_REQ_MAY_SLEEP
- fix s5p_hash_set_flow()

version 2:
- change patch format so number of lines drops
- change in Kconfig as suggested by Krzysztof Kozlowski, add
EXYNOS_HASH subsection
- change #ifndef EXYNOS_RNG into #ifdef CRYPTO_DEV_EXYNOS_HASH
- remove style fixups in aes, as they should go in separate patch
- remove FLOW_LOG, FLOW_DUMP macros and its uses
- remove #if 0 ... endif
- remove unused function hash_wait and its defines
- fix compiler warning in dev_dbg
- remove some comments
- other minor fixes in comments

 drivers/crypto/Kconfig   |   14 +
 drivers/crypto/s5p-sss.c | 1441 +-
 2 files changed, 1445 insertions(+), 10 deletions(-)

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index fe33c199fc1a..01cf07ce34c5 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -439,6 +439,20 @@ config CRYPTO_DEV_S5P
  Select this to offload Samsung S5PV210 or S5PC110, Exynos from AES
  algorithms execution.
 
+config CRYPTO_DEV_EXYNOS_HASH
+   bool "Support for Samsung Exynos HASH accelerator"
+   depends on CRYPTO_DEV_S5P
+   depends on !CRYPTO_DEV_EXYNOS_RNG && CRYPTO_DEV_EXYNOS_RNG!=m
+   select CRYPTO_SHA1
+   select CRYPTO_MD5
+   select CRYPTO_SHA256
+   help
+ Select this to offload Exynos from HASH MD5/SHA1/SHA256.
+ This will select software SHA1, MD5 and SHA256 as they are
+ needed for small and zero-size messages.
+ HASH algorithms will be disabled if EXYNOS_RNG
+ is enabled due to hw conflict.
+
 config CRYPTO_DEV_NX
bool "Support for IBM PowerPC Nest (NX) cryptographic acceleration"
depends on PPC64
diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 7ac657f46d15..29bec5a69fe8 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -1,18 +1,21 @@
 /*
  * Cryptographic API.
  *
- * Support for Samsung S5PV210 HW acceleration.
+ * Support for Samsung S5PV210 and Exynos HW acceleration.
  *
  * Copyright (C) 2011 NetUP Inc. All rights reserved.
+ * Copyright (c) 2017 Samsung Electronics Co., Ltd. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as published
  * by the Free Software Foundation.
  *
+ * Hash part based on omap-sham.c driver.
  */
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -30,28 +33,41 @@
 #include 
 #include 
 
+#include 
+#include 
+#include 
+#include 
+
 #define _SBF(s, v)  ((v) << (s))
 
 /* Feed control registers */
 #define SSS_REG_FCINTSTAT   0x
+#define SSS_FCINTSTAT_HPARTINT BIT(7)
+#define SSS_FCINTSTAT_HDONEINT BIT(5)
 #define SSS_FCINTSTAT_BRDMAINT  BIT(3)
 #define SSS_FCINTSTAT_BTDMAINT  BIT(2)
 #define SSS_FCINTSTAT_HRDMAINT  BIT(1)
 #define SSS_FCINTSTAT_PKDMAINT