Re: [PATCH 06/61] crypto: simplify getting .drvdata

[Krzysztof Kozlowski]

On Thu, Apr 19, 2018 at 4:05 PM, Wolfram Sang
<wsa+rene...@sang-engineering.com> wrote:
> We should get drvdata from struct device directly. Going via
> platform_device is an unneeded step back and forth.
>
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> ---
>
> Build tested only. buildbot is happy. Please apply individually.
>
>  drivers/crypto/exynos-rng.c   | 6 ++
>  drivers/crypto/picoxcell_crypto.c | 6 ++
>  2 files changed, 4 insertions(+), 8 deletions(-)

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

[PATCH 2/4] crypto: omap-sham - Fix misleading indentation

[Krzysztof Kozlowski]

Commit 8043bb1ae03c ("crypto: omap-sham - convert driver logic to use
sgs for data xmit") removed the if() clause leaving the statement as is.
The intention was in that case to finish the request always so the goto
instruction seems sensible.

Remove the indentation to fix Smatch warning:
drivers/crypto/omap-sham.c:1761 omap_sham_done_task() warn: inconsistent 
indenting

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/omap-sham.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index 7650b1b449bb..6cb6ab6f52c0 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -1758,7 +1758,7 @@ static void omap_sham_done_task(unsigned long data)
if (test_and_clear_bit(FLAGS_OUTPUT_READY, >flags)) {
/* hash or semi-hash ready */
clear_bit(FLAGS_DMA_READY, >flags);
-   goto finish;
+   goto finish;
}
}
 
-- 
2.7.4

[PATCH 4/4] crypto: s5p-sss - Constify pointed data (arguments and local variables)

[Krzysztof Kozlowski]

Improve the code (safety and readability) by indicating that data passed
through pointer is not modified.  This adds const keyword in many places,
most notably:
 - the driver data (pointer to struct samsung_aes_variant),
 - scatterlist addresses written as value to device registers,
 - key and IV arrays.

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/s5p-sss.c | 31 +--
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index d7c8163e5068..bf7163042569 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -404,29 +404,31 @@ static const struct of_device_id s5p_sss_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, s5p_sss_dt_match);
 
-static inline struct samsung_aes_variant *find_s5p_sss_version
-  (struct platform_device *pdev)
+static inline const struct samsung_aes_variant *find_s5p_sss_version
+  (const struct platform_device *pdev)
 {
if (IS_ENABLED(CONFIG_OF) && (pdev->dev.of_node)) {
const struct of_device_id *match;
 
match = of_match_node(s5p_sss_dt_match,
pdev->dev.of_node);
-   return (struct samsung_aes_variant *)match->data;
+   return (const struct samsung_aes_variant *)match->data;
}
-   return (struct samsung_aes_variant *)
+   return (const struct samsung_aes_variant *)
platform_get_device_id(pdev)->driver_data;
 }
 
 static struct s5p_aes_dev *s5p_dev;
 
-static void s5p_set_dma_indata(struct s5p_aes_dev *dev, struct scatterlist *sg)
+static void s5p_set_dma_indata(struct s5p_aes_dev *dev,
+  const struct scatterlist *sg)
 {
SSS_WRITE(dev, FCBRDMAS, sg_dma_address(sg));
SSS_WRITE(dev, FCBRDMAL, sg_dma_len(sg));
 }
 
-static void s5p_set_dma_outdata(struct s5p_aes_dev *dev, struct scatterlist 
*sg)
+static void s5p_set_dma_outdata(struct s5p_aes_dev *dev,
+   const struct scatterlist *sg)
 {
SSS_WRITE(dev, FCBTDMAS, sg_dma_address(sg));
SSS_WRITE(dev, FCBTDMAL, sg_dma_len(sg));
@@ -619,7 +621,7 @@ static inline void s5p_hash_write(struct s5p_aes_dev *dd,
  * @sg:scatterlist ready to DMA transmit
  */
 static void s5p_set_dma_hashdata(struct s5p_aes_dev *dev,
-struct scatterlist *sg)
+const struct scatterlist *sg)
 {
dev->hash_sg_cnt--;
SSS_WRITE(dev, FCHRDMAS, sg_dma_address(sg));
@@ -792,9 +794,9 @@ static void s5p_hash_read_msg(struct ahash_request *req)
  * @ctx:   request context
  */
 static void s5p_hash_write_ctx_iv(struct s5p_aes_dev *dd,
- struct s5p_hash_reqctx *ctx)
+ const struct s5p_hash_reqctx *ctx)
 {
-   u32 *hash = (u32 *)ctx->digest;
+   const u32 *hash = (const u32 *)ctx->digest;
unsigned int i;
 
for (i = 0; i < ctx->nregs; i++)
@@ -818,7 +820,7 @@ static void s5p_hash_write_iv(struct ahash_request *req)
  */
 static void s5p_hash_copy_result(struct ahash_request *req)
 {
-   struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
+   const struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
 
if (!req->result)
return;
@@ -1290,7 +1292,7 @@ static int s5p_hash_prepare_request(struct ahash_request 
*req, bool update)
  */
 static void s5p_hash_update_dma_stop(struct s5p_aes_dev *dd)
 {
-   struct s5p_hash_reqctx *ctx = ahash_request_ctx(dd->hash_req);
+   const struct s5p_hash_reqctx *ctx = ahash_request_ctx(dd->hash_req);
 
dma_unmap_sg(dd->dev, ctx->sg, ctx->sg_len, DMA_TO_DEVICE);
clear_bit(HASH_FLAGS_DMA_ACTIVE, >hash_flags);
@@ -1717,7 +1719,7 @@ static void s5p_hash_cra_exit(struct crypto_tfm *tfm)
  */
 static int s5p_hash_export(struct ahash_request *req, void *out)
 {
-   struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
+   const struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
 
memcpy(out, ctx, sizeof(*ctx) + ctx->bufcnt);
 
@@ -1831,7 +1833,8 @@ static struct ahash_alg algs_sha1_md5_sha256[] = {
 };
 
 static void s5p_set_aes(struct s5p_aes_dev *dev,
-   uint8_t *key, uint8_t *iv, unsigned int keylen)
+   const uint8_t *key, const uint8_t *iv,
+   unsigned int keylen)
 {
void __iomem *keystart;
 
@@ -2150,7 +2153,7 @@ static int s5p_aes_probe(struct platform_device *pdev)
 {
struct device *dev = >dev;
int i, j, err = -ENODEV;
-   struct samsung_aes_variant *variant;
+   const struct samsung_aes_variant *variant;
struct s5p_aes_dev *pdata;
struct resource *res;
unsigned int hash_i;
-- 
2.7.4

[PATCH 3/4] crypto: s5p-sss: Remove useless check for non-null request

[Krzysztof Kozlowski]

ahash_request 'req' argument passed by the caller
s5p_hash_handle_queue() cannot be NULL here because it is obtained from
non-NULL pointer via container_of().

This fixes smatch warning:
drivers/crypto/s5p-sss.c:1213 s5p_hash_prepare_request() warn: variable 
dereferenced before check 'req' (see line 1208)

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/s5p-sss.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 5d64c08b7f47..d7c8163e5068 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -1210,9 +1210,6 @@ static int s5p_hash_prepare_request(struct ahash_request 
*req, bool update)
int xmit_len, hash_later, nbytes;
int ret;
 
-   if (!req)
-   return 0;
-
if (update)
nbytes = req->nbytes;
else
-- 
2.7.4

[PATCH 1/4] crypto: omap-sham: Remove useless check for non-null request

[Krzysztof Kozlowski]

ahash_request 'req' argument passed by the caller
omap_sham_handle_queue() cannot be NULL here because it is obtained from
non-NULL pointer via container_of().

This fixes smatch warning:
drivers/crypto/omap-sham.c:812 omap_sham_prepare_request() warn: variable 
dereferenced before check 'req' (see line 805)

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/omap-sham.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index 86b89ace836f..7650b1b449bb 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -809,9 +809,6 @@ static int omap_sham_prepare_request(struct ahash_request 
*req, bool update)
bool final = rctx->flags & BIT(FLAGS_FINUP);
int xmit_len, hash_later;
 
-   if (!req)
-   return 0;
-
bs = get_block_size(rctx);
 
if (update)
-- 
2.7.4

Re: [PATCH] crypto: s5p-sss.c: Fix kernel Oops in AES-ECB mode

[Krzysztof Kozlowski]

On Mon, Feb 5, 2018 at 6:40 PM, Kamil Konieczny
<k.koniec...@partner.samsung.com> wrote:
>
> In AES-ECB mode crypt is done with key only, so any use of IV
> can cause kernel Oops, as reported by Anand Moon.
> Fixed it by using IV only in AES-CBC and AES-CTR.
>
> Signed-off-by: Kamil Konieczny <k.koniec...@partner.samsung.com>
> Reported-by: Anand Moon <linux.am...@gmail.com>
> ---
> Tested on Odroid XU4/HC1, kernel 4.15 with following command:
>
> fallocate -l 128MiB /tmp/test.bin
> dd if=/dev/urandom of=/tmp/testkey.key bs=128 count=1
> sync
> cryptsetup luksFormat --debug -q -d /tmp/testkey.key \
>   --cipher aes-cbc-essiv:sha256 -h sha256 -s 128 /tmp/test.bin
>
> The original report by Anand Moon:
> https://www.spinics.net/lists/linux-crypto/msg31180.html
>
> Oops reproduced with cryptsetup 2.0.0, kernel 4.15,
> in .config in crypto API ECB support was turned off, and s5p-sss AES driver 
> on.
>
> cryptsetup is using aes-ecb and has req->info in aes_ctx set to 0x10,
> which caused Oops:
>
> [ 2078.683779] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
> [ 2078.689148] Modules linked in: algif_skcipher af_alg sd_mod sg
> evdev uas usb_storage scsi_mod gpio_keys fbtft(C) spidev spi_s3c64xx
> ipv6
> [ 2078.701377] CPU: 1 PID: 15 Comm: ksoftirqd/1 Tainted: G C
> 4.15.0-rc9-xu4krck #1
> [ 2078.709861] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [ 2078.715932] PC is at memcpy+0x80/0x330
> [ 2078.719652] LR is at s5p_tasklet_cb+0x19c/0x328
>
>  drivers/crypto/s5p-sss.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)

Fixes and cc-stable?

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: Odroid HC1 cryptsetup:encrypt sata driver

[Krzysztof Kozlowski]

On Wed, Jan 24, 2018 at 2:04 PM, Anand Moon  wrote:
> Hi Kamil Konieczny,
>
> I am looking in setup of encrypted sata hard-disk on Odroid XU4/HC1 device.
> using following encryption method.
>
> aes-cbc-essiv:sha256 128
> aes-cbc-essiv:sha256 256
>
> Here is my defconfig I am using. https://pastebin.com/gF5T2stp
>
> Following crypt benchmark we use to test : https://pastebin.com/WiexsJA2

No problems on my side with a 128 MB file (not a device):
# cryptsetup -v luksFormat /tmp/testcrypt /dev/urandom
--keyfile-size=32 --cipher aes-cbc-essiv:sha256
# Command successful.
# cryptsetup -v luksFormat ~/testcrypt /dev/urandom --keyfile-size=32
--cipher aes-cbc-essiv:sha256
# Command successful.

Linux 4.15.0-rc9-00023-g1f07476ec143.

Some time ago you were building from not usual source code and your
kernel version from WARN is not unambiguous.

What is necessary to reproduce it?

Best regards,
Krzysztof


>
> When I am trying to format the the hard drive I am getting kernel panic.
> I have tired different option like below.
>
> *Please guide me in how to fix this bug*
>
> # cryptsetup luksFormat /dev/sda1 --cipher aes-cbc-essiv:sha256
> # cryptsetup --cipher aes-cbc-essiv --hash sha256 --use-urandom
> --key-file=/dev/urandom --master-key-file=/dev/urandom
> --keyfile-size=256 --key-size=256 luksFormat /dev/sda1
>
> [kernel panic]-
> root@odroid:~# cryptsetup luksFormat /dev/sda1 --cipher aes-cbc-essiv:sha256
>
> WARNING!
> 
> This will overwrite data on /dev/sda1 irrevocably.
>
> Are you sure? (Type uppercase yes): YES
> Enter passphrase:
> Verify passphrase:
>
> [ 2078.670930] Unable to handle kernel NULL pointer dereference at
> virtual address 0010
> [ 2078.677550] pgd = b0cb4e51
> [ 2078.680220] [0010] *pgd=
> [ 2078.683779] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
> [ 2078.689148] Modules linked in: algif_skcipher af_alg sd_mod sg
> evdev uas usb_storage scsi_mod gpio_keys fbtft(C) spidev spi_s3c64xx
> ipv6
> [ 2078.701377] CPU: 1 PID: 15 Comm: ksoftirqd/1 Tainted: G C
> 4.15.0-rc9-xu4krck #1
> [ 2078.709861] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [ 2078.715932] PC is at memcpy+0x80/0x330
> [ 2078.719652] LR is at s5p_tasklet_cb+0x19c/0x328
> [ 2078.724155] pc : []lr : []psr: 00070093
> [ 2078.730396] sp : ee917ebc  ip : 0010  fp : ee8cbe30
> [ 2078.735594] r10: 0007  r9 :   r8 : 60070013
> [ 2078.740794] r7 : e7812fc4  r6 : ee23a268  r5 : 0020  r4 : ee23a210
> [ 2078.747294] r3 : e7812fc0  r2 : fff0  r1 : 0010  r0 : f1a6b230
> [ 2078.753794] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
> Segment none
> [ 2078.760986] Control: 10c5387d  Table: 6d6b006a  DAC: 0051
> [ 2078.766705] Process ksoftirqd/1 (pid: 15, stack limit = 0x2572141a)
> [ 2078.772944] Stack: (0xee917ebc to 0xee918000)
> [ 2078.777276] 7ea0:
>  0020
> [ 2078.785426] 7ec0: ee23a268 e7812fc4 60070013 f1a6b230 ee23a210
> c06f48b0 ee23a23c ee23a240
> [ 2078.793578] 7ee0:  c0c7d29c  0100 0007
> c012ad88 c0d03098 0006
> [ 2078.801723] 7f00: ee916000 c0d93948 0040 c010159c 
> c09030a8 ee917f64 ee917f10
> [ 2078.809866] 7f20: c0d03080 c0dab3c0 000a 0006c904 c0d03d00
> 04208040 ee8afa40 e000
> [ 2078.818012] 7f40: ee8afa40 c0d0c718 e000  
> ee8afb1c ee8cbe30 c012a550
> [ 2078.826157] 7f60: ee916000 c0146eac ee8afb00 ee8afac0 
> ee916000 ee8afa40 c0146d5c
> [ 2078.834303] 7f80: ee8afb1c c0142ff4  ee8afac0 c0142ed0
>   
> [ 2078.842448] 7fa0:    c01088e8 
>   
> [ 2078.850593] 7fc0:     
>   
> [ 2078.858739] 7fe0:     0013
>  0040 00080008
> [ 2078.866894] [] (memcpy) from []
> (s5p_tasklet_cb+0x19c/0x328)
> [ 2078.874255] [] (s5p_tasklet_cb) from []
> (tasklet_action+0x58/0xe4)
> [ 2078.882140] [] (tasklet_action) from []
> (__do_softirq+0x114/0x3b0)
> [ 2078.890023] [] (__do_softirq) from []
> (run_ksoftirqd+0x3c/0x64)
> [ 2078.897650] [] (run_ksoftirqd) from []
> (smpboot_thread_fn+0x150/0x268)
> [ 2078.905884] [] (smpboot_thread_fn) from []
> (kthread+0x124/0x154)
> [ 2078.913596] [] (kthread) from []
> (ret_from_fork+0x14/0x2c)
> [ 2078.920784] Code: e320f000 e4913004 e4914004 e4915004 (e4916004)
> [ 2078.926846] ---[ end trace 025fbaef2835f80b ]---
> [ 2078.931435] Kernel panic - not syncing: Fatal exception in interrupt
> [ 2078.937781] CPU2: stopping
> [ 2078.940450] CPU: 2 PID: 0 Comm: swapper/2 Tainted: G  D  C
>  4.15.0-rc9-xu4krck #1
> [ 2078.948684] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [ 2078.954757] [] (unwind_backtrace) from []
> (show_stack+0x10/0x14)
> [ 2078.962467] [] (show_stack) from []
> (dump_stack+0x84/0x98)
> [ 2078.969659] [] (dump_stack) from []
> 

Re: [PATCH][next] hwrng: exynos: check for -ve error return from readl_poll_timeout

[Krzysztof Kozlowski]

On Fri, Jan 12, 2018 at 5:30 PM, Colin King  wrote:
> From: Colin Ian King 
>
> Currently, the return from readl_poll_timeout is being assigned to
> a u32 and this is being checked for a -ve return which is always
> false since a u32 cannot be less than zero.  Fix this by changing
> val to an int so that error returns can be correctly detected.
>
> Detected by CoverityScan, CID#1463776 ("Logically dead code")
>
> Fixes: 6cd225cc5d8a ("hwrng: exynos - add Samsung Exynos True RNG driver")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/char/hw_random/exynos-trng.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Thanks for the patch! Dan already fixed this few days before:
https://www.spinics.net/lists/linux-samsung-soc/msg61724.html

Best regards,
Krzysztof

Re: [PATCH -next] hwrng: exynos - remove redundant dev_err call in exynos_trng_probe()

[Krzysztof Kozlowski]

On Wed, Jan 10, 2018 at 2:30 PM, Wei Yongjun <weiyongj...@huawei.com> wrote:
> There is a error message within devm_ioremap_resource
> already, so remove the dev_err call to avoid redundant
> error message.
>
> Signed-off-by: Wei Yongjun <weiyongj...@huawei.com>
> ---
>  drivers/char/hw_random/exynos-trng.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH] hwrng: exynos - Signedness bug in exynos_trng_do_read()

[Krzysztof Kozlowski]

On Wed, Jan 10, 2018 at 10:36 AM, Dan Carpenter
<dan.carpen...@oracle.com> wrote:
> "val" needs to be signed for the error handling to work.
>
> Fixes: 6cd225cc5d8a ("hwrng: exynos - add Samsung Exynos True RNG driver")
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
>
> diff --git a/drivers/char/hw_random/exynos-trng.c 
> b/drivers/char/hw_random/exynos-trng.c
> index 34d6f51ecbee..f4643e3ec346 100644
> --- a/drivers/char/hw_random/exynos-trng.c
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -55,7 +55,7 @@ static int exynos_trng_do_read(struct hwrng *rng, void 
> *data, size_t max,
>bool wait)
>  {
> struct exynos_trng_dev *trng;
> -   u32 val;
> +   int val;

It seems that one forgot to run Smatch on the driver :).

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

[PATCH 1/2] crypto: exynos-rng - Add SPDX license identifier and correct module license

[Krzysztof Kozlowski]

Replace GPL license statement with SPDX GPL-2.0 license identifier and
correct the module license to GPLv2.

The license itself was a generic GPL because of copy-and-paste from old
drivers/char/hw_random/exynos-rng.c driver (on which this was based on).
However the module license indicated GPL-2.0 or later.  GPL-2.0 was
intended by author so fix up this mess.

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/exynos-rng.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
index 451620b475a0..a2e91f94096f 100644
--- a/drivers/crypto/exynos-rng.c
+++ b/drivers/crypto/exynos-rng.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * exynos-rng.c - Random Number Generator driver for the Exynos
  *
@@ -6,15 +7,6 @@
  * Loosely based on old driver from drivers/char/hw_random/exynos-rng.c:
  * Copyright (C) 2012 Samsung Electronics
  * Jonghwa Lee <jonghwa3@samsung.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation;
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
  */
 
 #include 
@@ -386,4 +378,4 @@ module_platform_driver(exynos_rng_driver);
 
 MODULE_DESCRIPTION("Exynos H/W Random Number Generator driver");
 MODULE_AUTHOR("Krzysztof Kozlowski <k...@kernel.org>");
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

[PATCH 2/2] crypto: s5p-sss - Add SPDX license identifier

[Krzysztof Kozlowski]

Replace GPL license statement with SPDX GPL-2.0 license identifier.

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/s5p-sss.c | 24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 62830a43d959..188f44b7eb27 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -1,17 +1,13 @@
-/*
- * Cryptographic API.
- *
- * 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.
- */
+// SPDX-License-Identifier: GPL-2.0
+//
+// Cryptographic API.
+//
+// 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.
+//
+// Hash part based on omap-sham.c driver.
 
 #include 
 #include 
-- 
2.11.0

Re: [PATCH v3 3/4] crypto: exynos - Reseed PRNG after generating 2^16 random bytes

[Krzysztof Kozlowski]

On Tue, Dec 12, 2017 at 5:36 PM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> Reseed PRNG after reading 65 kB of randomness. Although this may reduce
> performance, in most cases the loss is not noticeable. Also the time
> based threshold for reseeding is changed to one second. Reseeding is
> performed whenever either limit is exceeded.
>
> Reseeding of a PRNG does not increase entropy, but it helps preventing
> backtracking the internal state of the device from its output sequence,
> and hence, prevents potential attacker from predicting numbers to be
> generated.
>
> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
> Reviewed-by: Stephan Mueller <smuel...@chronox.de>
> ---
>  drivers/crypto/exynos-rng.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
>

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v3 2/4] crypto: exynos - Improve performance of PRNG

[Krzysztof Kozlowski]

On Tue, Dec 12, 2017 at 5:36 PM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> Use memcpy_fromio() instead of custom exynos_rng_copy_random() function
> to retrieve generated numbers from the registers of PRNG.

If I recall correctly, you mentioned before that it improves the
performance. If so it would be nice to mention some numbers here.

Anyway it is fine with me:
Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

>
> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
> ---
>  drivers/crypto/exynos-rng.c | 31 ++-
>  1 file changed, 2 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> index 825c09619eb8..dcdd444d0b3b 100644
> --- a/drivers/crypto/exynos-rng.c
> +++ b/drivers/crypto/exynos-rng.c
> @@ -131,34 +131,6 @@ static int exynos_rng_set_seed(struct exynos_rng_dev 
> *rng,
>  }
>
>  /*
> - * Read from output registers and put the data under 'dst' array,
> - * up to dlen bytes.
> - *
> - * Returns number of bytes actually stored in 'dst' (dlen
> - * or EXYNOS_RNG_SEED_SIZE).
> - */
> -static unsigned int exynos_rng_copy_random(struct exynos_rng_dev *rng,
> -  u8 *dst, unsigned int dlen)
> -{
> -   unsigned int cnt = 0;
> -   int i, j;
> -   u32 val;
> -
> -   for (j = 0; j < EXYNOS_RNG_SEED_REGS; j++) {
> -   val = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> -
> -   for (i = 0; i < 4; i++) {
> -   dst[cnt] = val & 0xff;
> -   val >>= 8;
> -   if (++cnt >= dlen)
> -   return cnt;
> -   }
> -   }
> -
> -   return cnt;
> -}
> -
> -/*
>   * Start the engine and poll for finish.  Then read from output registers
>   * filling the 'dst' buffer up to 'dlen' bytes or up to size of generated
>   * random data (EXYNOS_RNG_SEED_SIZE).
> @@ -190,7 +162,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev 
> *rng,
> /* Clear status bit */
> exynos_rng_writel(rng, EXYNOS_RNG_STATUS_RNG_DONE,
>   EXYNOS_RNG_STATUS);
> -   *read = exynos_rng_copy_random(rng, dst, dlen);
> +   *read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
> +   memcpy_fromio(dst, rng->mem + EXYNOS_RNG_OUT_BASE, *read);
>
> return 0;
>  }
> --
> 2.11.0
>

Re: [PATCH v3 1/4] crypto: exynos - Support Exynos5250+ SoCs

[Krzysztof Kozlowski]

On Tue, Dec 12, 2017 at 5:36 PM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> Add support for PRNG in Exynos5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
> ---
>  .../bindings/crypto/samsung,exynos-rng4.txt|  4 +++-
>  drivers/crypto/exynos-rng.c| 27 
> --
>  2 files changed, 28 insertions(+), 3 deletions(-)

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v4] ARM: dts: exynos: Add nodes for True Random Number Generator

[Krzysztof Kozlowski]

On Tue, Dec 12, 2017 at 01:09:02PM +0100, Łukasz Stelmach wrote:
> Add nodes for the True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
> 
> Signed-off-by: Łukasz Stelmach 
> ---
> Changes since v3:
> 
> - Rebased accroding to Krzysztof Kozłowski's request
> 
>  arch/arm/boot/dts/exynos5.dtsi| 5 +
>  arch/arm/boot/dts/exynos5250.dtsi | 5 +
>  arch/arm/boot/dts/exynos5410.dtsi | 5 +
>  arch/arm/boot/dts/exynos5420.dtsi | 5 +
>  4 files changed, 20 insertions(+)
> 

Thanks, applied.

Best regards,
Krzysztof

Re: [PATCH v2 4/4] crypto: exynos - Introduce mutex to prevent concurrent access to hardware

[Krzysztof Kozlowski]

On Tue, Dec 12, 2017 at 11:30 AM, Łukasz Stelmach
<l.stelm...@samsung.com> wrote:
> It was <2017-12-11 pon 16:03>, when Krzysztof Kozlowski wrote:
>> On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach <l.stelm...@samsung.com> 
>> wrote:
>>> Cc: Marek Szyprowski <m.szyprow...@samsung.com>, Bartlomiej Zolnierkiewicz 
>>> <b.zolnier...@samsung.com>
>>>
>>> Hardware operations like reading random numbers and setting a seed need
>>> to be conducted in a single thread. Therefore a mutex is required to
>>> prevent multiple threads (processes) from accessing the hardware at the
>>> same time.
>>>
>>> The sequence of mutex_lock() and mutex_unlock() in the exynos_rng_reseed()
>>> function enables switching between different threads waiting for the
>>> driver to generate random numbers for them.
>>>
>>> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>>> ---
>>>  drivers/crypto/exynos-rng.c | 21 +
>>>  1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
>>> index c72a838f1932..6209035ca659 100644
>>> --- a/drivers/crypto/exynos-rng.c
>>> +++ b/drivers/crypto/exynos-rng.c
>>> @@ -22,6 +22,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>>
>>> @@ -79,6 +80,7 @@ struct exynos_rng_dev {
>>> enum exynos_prng_type   type;
>>> void __iomem*mem;
>>> struct clk  *clk;
>>> +   struct mutexlock;
>>> /* Generated numbers stored for seeding during resume */
>>> u8  seed_save[EXYNOS_RNG_SEED_SIZE];
>>> unsigned intseed_save_len;
>>> @@ -192,6 +194,10 @@ static void exynos_rng_reseed(struct exynos_rng_dev 
>>> *rng)
>>> return;
>>>
>>> exynos_rng_set_seed(rng, seed, read);
>>> +
>>> +   /* Let others do some of their job. */
>>> +   mutex_unlock(>lock);
>>> +   mutex_lock(>lock);
>>>  }
>>>
>>>  static int exynos_rng_generate(struct crypto_rng *tfm,
>>> @@ -207,6 +213,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>>> if (ret)
>>> return ret;
>>>
>>> +   mutex_lock(>lock);
>>> do {
>>> ret = exynos_rng_get_random(rng, dst, dlen, );
>>> if (ret)
>>> @@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>>>
>>> exynos_rng_reseed(rng);
>>> } while (dlen > 0);
>>> +   mutex_unlock(>lock);
>>>
>>> clk_disable_unprepare(rng->clk);
>>>
>>> @@ -234,7 +242,9 @@ static int exynos_rng_seed(struct crypto_rng *tfm, 
>>> const u8 *seed,
>>> if (ret)
>>> return ret;
>>>
>>> +   mutex_lock(>lock);
>>> ret = exynos_rng_set_seed(ctx->rng, seed, slen);
>>> +   mutex_unlock(>lock);
>>
>> I think the number of mutex locks/unlock statements can be reduced
>> (including the mutex unlock+lock pattern) after moving the mutex to
>> exynos_rng_set_seed() and exynos_rng_get_random() because actually you
>> want to protect them. This would remove the new code from suspend and
>> resume path and gave you the fairness.
>>
>> On the other hand the mutex would be unlocked+locked many times for
>> large generate() calls...
>
> Moving locks/unlocks to exynos_rng_get_random() means taking a lock to
> retrieve 20 bytes. It doesn't scale at all. I really wanted to avoid it,
> because the performance loss is quite noticable in such case. That is
> why I put the lock around the loop in exynos_rng_generatr(). As a
> consequence I had to move locks out of exynos_rng_set_seed() too.

I understand. With the fix for first line (cc):
Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v3 3/3] ARM: dts: exynos: Add nodes for True Random Number Generator

[Krzysztof Kozlowski]

On Tue, Dec 12, 2017 at 11:35 AM, Łukasz Stelmach
<l.stelm...@samsung.com> wrote:
> It was <2017-12-11 pon 19:49>, when Krzysztof Kozlowski wrote:
>> On Mon, Dec 04, 2017 at 01:53:51PM +0100, Łukasz Stelmach wrote:
>>> Add nodes for the True Random Number Generator found in Samsung Exynos
>>> 5250+ SoCs.
>>>
>>> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>>> ---
>>>  arch/arm/boot/dts/exynos5.dtsi| 5 +
>>>  arch/arm/boot/dts/exynos5250.dtsi | 5 +
>>>  arch/arm/boot/dts/exynos5410.dtsi | 5 +
>>>  arch/arm/boot/dts/exynos5420.dtsi | 5 +
>>>  4 files changed, 20 insertions(+)
>>>
>>
>> Unfortunately the same story as with your PRNG patch - does not apply on
>> top of my next/dt (after taking PRNG). Also did not apply on v4.15-rc1 +
>> PRNG.
>>
>> Could you rebase on my next/dt?
>
> Sure. Should I send it as along with the other two patches, if there were
> no changes in them since?

It is fine to resend just this one as it will go through different tree anyway.

Best regards,
Krzysztof

Re: [PATCH v3 3/3] ARM: dts: exynos: Add nodes for True Random Number Generator

[Krzysztof Kozlowski]

On Mon, Dec 04, 2017 at 01:53:51PM +0100, Łukasz Stelmach wrote:
> Add nodes for the True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
> 
> Signed-off-by: Łukasz Stelmach 
> ---
>  arch/arm/boot/dts/exynos5.dtsi| 5 +
>  arch/arm/boot/dts/exynos5250.dtsi | 5 +
>  arch/arm/boot/dts/exynos5410.dtsi | 5 +
>  arch/arm/boot/dts/exynos5420.dtsi | 5 +
>  4 files changed, 20 insertions(+)
>

Unfortunately the same story as with your PRNG patch - does not apply on
top of my next/dt (after taking PRNG). Also did not apply on v4.15-rc1 +
PRNG.

Could you rebase on my next/dt?

Best regards,
Krzysztof

Re: [PATCH v2 4/4] crypto: exynos - Introduce mutex to prevent concurrent access to hardware

[Krzysztof Kozlowski]

On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach  wrote:
> Cc: Marek Szyprowski , Bartlomiej Zolnierkiewicz 
> 
>
> Hardware operations like reading random numbers and setting a seed need
> to be conducted in a single thread. Therefore a mutex is required to
> prevent multiple threads (processes) from accessing the hardware at the
> same time.
>
> The sequence of mutex_lock() and mutex_unlock() in the exynos_rng_reseed()
> function enables switching between different threads waiting for the
> driver to generate random numbers for them.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  drivers/crypto/exynos-rng.c | 21 +
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> index c72a838f1932..6209035ca659 100644
> --- a/drivers/crypto/exynos-rng.c
> +++ b/drivers/crypto/exynos-rng.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -79,6 +80,7 @@ struct exynos_rng_dev {
> enum exynos_prng_type   type;
> void __iomem*mem;
> struct clk  *clk;
> +   struct mutexlock;
> /* Generated numbers stored for seeding during resume */
> u8  seed_save[EXYNOS_RNG_SEED_SIZE];
> unsigned intseed_save_len;
> @@ -192,6 +194,10 @@ static void exynos_rng_reseed(struct exynos_rng_dev *rng)
> return;
>
> exynos_rng_set_seed(rng, seed, read);
> +
> +   /* Let others do some of their job. */
> +   mutex_unlock(>lock);
> +   mutex_lock(>lock);
>  }
>
>  static int exynos_rng_generate(struct crypto_rng *tfm,
> @@ -207,6 +213,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
> if (ret)
> return ret;
>
> +   mutex_lock(>lock);
> do {
> ret = exynos_rng_get_random(rng, dst, dlen, );
> if (ret)
> @@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>
> exynos_rng_reseed(rng);
> } while (dlen > 0);
> +   mutex_unlock(>lock);
>
> clk_disable_unprepare(rng->clk);
>
> @@ -234,7 +242,9 @@ static int exynos_rng_seed(struct crypto_rng *tfm, const 
> u8 *seed,
> if (ret)
> return ret;
>
> +   mutex_lock(>lock);
> ret = exynos_rng_set_seed(ctx->rng, seed, slen);
> +   mutex_unlock(>lock);

I think the number of mutex locks/unlock statements can be reduced
(including the mutex unlock+lock pattern) after moving the mutex to
exynos_rng_set_seed() and exynos_rng_get_random() because actually you
want to protect them. This would remove the new code from suspend and
resume path and gave you the fairness.

On the other hand the mutex would be unlocked+locked many times for
large generate() calls...

Best regards,
Krzysztof

> clk_disable_unprepare(rng->clk);
>
> @@ -284,6 +294,8 @@ static int exynos_rng_probe(struct platform_device *pdev)
> return -ENOTSUPP;
> }
>
> +   mutex_init(>lock);
> +
> rng->dev = >dev;
> rng->clk = devm_clk_get(>dev, "secss");
> if (IS_ERR(rng->clk)) {
> @@ -334,9 +346,14 @@ static int __maybe_unused exynos_rng_suspend(struct 
> device *dev)
> if (ret)
> return ret;
>
> +   mutex_lock(>lock);
> +
> /* Get new random numbers and store them for seeding on resume. */
> exynos_rng_get_random(rng, rng->seed_save, sizeof(rng->seed_save),
>   &(rng->seed_save_len));
> +
> +   mutex_unlock(>lock);
> +
> dev_dbg(rng->dev, "Stored %u bytes for seeding on system resume\n",
> rng->seed_save_len);
>
> @@ -359,8 +376,12 @@ static int __maybe_unused exynos_rng_resume(struct 
> device *dev)
> if (ret)
> return ret;
>
> +   mutex_lock(>lock);
> +
> ret = exynos_rng_set_seed(rng, rng->seed_save, rng->seed_save_len);
>
> +   mutex_unlock(>lock);
> +
> clk_disable_unprepare(rng->clk);
>
> return ret;
> --
> 2.11.0
>

Re: [PATCH v2 3/4] crypto: exynos - Reseed PRNG after generating 2^16 random bytes

[Krzysztof Kozlowski]

On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach  wrote:
> Cc: Marek Szyprowski , Bartlomiej Zolnierkiewicz 
> 

Same as in 1/4 and 2/4.

>
> Reseed PRNG after reading 65 kB of randomness. Although this may reduce
> performance, in most cases the loss is not noticeable.

You missed the comment about mentioning the change in time. Both from
me and Stephan.

Best regards,
Krzysztof

Re: [PATCH v2 2/4] crypto: exynos - Improve performance of PRNG

[Krzysztof Kozlowski]

On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach  wrote:
> Cc: Marek Szyprowski , Bartlomiej Zolnierkiewicz 
> 

This should not appear here.

>
> Use memcpy_fromio() instead of custom exynos_rng_copy_random() function
> to retrieve generated numbers from the registers of PRNG.
>
> Rearrange the loop around cpu_relax(). In a loop with while() at the
> beginning and the cpu_relax() removed the retry variable is decremented
> twice (down to 98).

I had troubles with understanding this sentence... and then I figured
out that you are referring to some case without cpu_relax(). I do not
see how it is relevant to this case. Compare the new code with old,
not with some imaginary case without barriers (thus maybe reordered?).

Your solution is strictly performance oriented so it would be nice to
see here the exact difference in numbers justifying the change. But
only the change for while() -> do-while(), not mixed with
memcpy_fromio.

> This means, the status register is read three
> times before the hardware is ready (which is very quick). Apparently,
> cpu_relax() requires noticeable amount of time to execute, so it seems
> better to call it for the first time before checking the status of
> PRNG. The do {} while () loop decrements the retry variable only once,
> which means, the time required for cpu_relax() is long enough to for
> the PRNG to provide results.

So basically you want to say that you removed one call to exynos_rng_read()?

> On the other hand, performance in this
> arrangement isn't much worse than in a loop without cpu_relax().

I think it is not relevant as cpu_relax() removal is not part of
current nor the new code.

Best regards,
Krzysztof

>
> Signed-off-by: Łukasz Stelmach 
> ---
>  drivers/crypto/exynos-rng.c | 36 +---
>  1 file changed, 5 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> index 2f554b82f49f..7d8f658480d3 100644
> --- a/drivers/crypto/exynos-rng.c
> +++ b/drivers/crypto/exynos-rng.c
> @@ -131,34 +131,6 @@ static int exynos_rng_set_seed(struct exynos_rng_dev 
> *rng,
>  }
>
>  /*
> - * Read from output registers and put the data under 'dst' array,
> - * up to dlen bytes.
> - *
> - * Returns number of bytes actually stored in 'dst' (dlen
> - * or EXYNOS_RNG_SEED_SIZE).
> - */
> -static unsigned int exynos_rng_copy_random(struct exynos_rng_dev *rng,
> -  u8 *dst, unsigned int dlen)
> -{
> -   unsigned int cnt = 0;
> -   int i, j;
> -   u32 val;
> -
> -   for (j = 0; j < EXYNOS_RNG_SEED_REGS; j++) {
> -   val = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> -
> -   for (i = 0; i < 4; i++) {
> -   dst[cnt] = val & 0xff;
> -   val >>= 8;
> -   if (++cnt >= dlen)
> -   return cnt;
> -   }
> -   }
> -
> -   return cnt;
> -}
> -
> -/*
>   * Start the engine and poll for finish.  Then read from output registers
>   * filling the 'dst' buffer up to 'dlen' bytes or up to size of generated
>   * random data (EXYNOS_RNG_SEED_SIZE).
> @@ -180,9 +152,10 @@ static int exynos_rng_get_random(struct exynos_rng_dev 
> *rng,
>   EXYNOS_RNG_SEED_CONF);
> }
>
> -   while (!(exynos_rng_readl(rng,
> -   EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && 
> --retry)
> +   do {
> cpu_relax();
> +   } while (!(exynos_rng_readl(rng, EXYNOS_RNG_STATUS) &
> +  EXYNOS_RNG_STATUS_RNG_DONE) && --retry);
>
> if (!retry)
> return -ETIMEDOUT;
> @@ -190,7 +163,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev 
> *rng,
> /* Clear status bit */
> exynos_rng_writel(rng, EXYNOS_RNG_STATUS_RNG_DONE,
>   EXYNOS_RNG_STATUS);
> -   *read = exynos_rng_copy_random(rng, dst, dlen);
> +   *read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
> +   memcpy_fromio(dst, rng->mem + EXYNOS_RNG_OUT_BASE, *read);
>
> return 0;
>  }
> --
> 2.11.0
>

Re: [PATCH v2 1/4] crypto: exynos - Support Exynos5250+ SoCs

[Krzysztof Kozlowski]

On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach  wrote:
> Cc: Marek Szyprowski , Bartlomiej Zolnierkiewicz 
> 
>
> Add support for PRNG in Exynos5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  .../bindings/crypto/samsung,exynos-rng4.txt|  4 ++-
>  drivers/crypto/exynos-rng.c| 32 
> --
>  2 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt 
> b/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
> index 4ca8dd4d7e66..a13fbdb4bd88 100644
> --- a/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
> +++ b/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
> @@ -2,7 +2,9 @@ Exynos Pseudo Random Number Generator
>
>  Required properties:
>
> -- compatible  : Should be "samsung,exynos4-rng".
> +- compatible  : One of:
> +- "samsung,exynos4-rng" for Exynos4210 and Exynos4412
> +- "samsung,exynos5250-prng" for Exynos5250+
>  - reg : Specifies base physical address and size of the registers 
> map.
>  - clocks  : Phandle to clock-controller plus clock-specifier pair.
>  - clock-names : "secss" as a clock name.
> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> index ed6ba796ad71..2f554b82f49f 100644
> --- a/drivers/crypto/exynos-rng.c
> +++ b/drivers/crypto/exynos-rng.c
> @@ -22,12 +22,17 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include 
>
>  #define EXYNOS_RNG_CONTROL 0x0
>  #define EXYNOS_RNG_STATUS  0x10
> +
> +#define EXYNOS_RNG_SEED_CONF   0x14
> +#define EXYNOS_RNG_GEN_PRNGBIT(1)
> +
>  #define EXYNOS_RNG_SEED_BASE   0x140
>  #define EXYNOS_RNG_SEED(n) (EXYNOS_RNG_SEED_BASE + (n * 0x4))
>  #define EXYNOS_RNG_OUT_BASE0x160
> @@ -43,6 +48,12 @@
>  #define EXYNOS_RNG_SEED_REGS   5
>  #define EXYNOS_RNG_SEED_SIZE   (EXYNOS_RNG_SEED_REGS * 4)
>
> +enum exynos_prng_type {
> +   EXYNOS_PRNG_UNKNOWN = 0,
> +   EXYNOS_PRNG_EXYNOS4,
> +   EXYNOS_PRNG_EXYNOS5,
> +};
> +
>  /*
>   * Driver re-seeds itself with generated random numbers to increase
>   * the randomness.
> @@ -63,6 +74,7 @@ struct exynos_rng_ctx {
>  /* Device associated memory */
>  struct exynos_rng_dev {
> struct device   *dev;
> +   enum exynos_prng_type   type;
> void __iomem*mem;
> struct clk  *clk;
> /* Generated numbers stored for seeding during resume */
> @@ -160,8 +172,13 @@ static int exynos_rng_get_random(struct exynos_rng_dev 
> *rng,
>  {
> int retry = EXYNOS_RNG_WAIT_RETRIES;
>
> -   exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
> - EXYNOS_RNG_CONTROL);
> +   if (rng->type == EXYNOS_PRNG_EXYNOS4) {
> +   exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
> + EXYNOS_RNG_CONTROL);
> +   } else if (rng->type == EXYNOS_PRNG_EXYNOS5) {
> +   exynos_rng_writel(rng, EXYNOS_RNG_GEN_PRNG,
> + EXYNOS_RNG_SEED_CONF);
> +   }
>
> while (!(exynos_rng_readl(rng,
> EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && 
> --retry)
> @@ -279,6 +296,13 @@ static int exynos_rng_probe(struct platform_device *pdev)
> if (!rng)
> return -ENOMEM;
>
> +   rng->type = (enum 
> exynos_prng_type)of_device_get_match_data(>dev);
> +   if (rng->type != EXYNOS_PRNG_EXYNOS4 &&
> +   rng->type != EXYNOS_PRNG_EXYNOS5) {
> +   dev_err(>dev, "Unsupported PRNG type: %d", rng->type);
> +   return -ENOTSUPP;
> +   }
> +

This cannot happen. Device will be matched only on matching compatible
thus rng->type will be always correct.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: exynos - Support Exynos5250+ SoCs

[Krzysztof Kozlowski]

On Wed, Dec 6, 2017 at 3:53 PM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> It was <2017-12-06 śro 15:05>, when Krzysztof Kozlowski wrote:
>> On Wed, Dec 6, 2017 at 2:42 PM, Łukasz Stelmach <l.stelm...@samsung.com> 
>> wrote:
>>> It was <2017-12-05 wto 14:34>, when Krzysztof Kozlowski wrote:
>>>> On Tue, Dec 5, 2017 at 1:35 PM, Łukasz Stelmach <l.stelm...@samsung.com> 
>>>> wrote:
>>>>> Add support for PRNG in Exynos5250+ SoCs.
>>>>>
>>>>> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>>>>> ---
>>>>>  .../bindings/crypto/samsung,exynos-rng4.txt|  4 ++-
>>>>>  drivers/crypto/exynos-rng.c| 36 
>>>>> --
>>>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
>>>>> b/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
>>>>> index 4ca8dd4d7e66..a13fbdb4bd88 100644
>>>>> --- a/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
>>>>> +++ b/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
>>>>> @@ -2,7 +2,9 @@ Exynos Pseudo Random Number Generator
>>>>>
>>>>>  Required properties:
>>>>>
>>>>> -- compatible  : Should be "samsung,exynos4-rng".
>>>>> +- compatible  : One of:
>>>>> +- "samsung,exynos4-rng" for Exynos4210 and Exynos4412
>>>>> +- "samsung,exynos5250-prng" for Exynos5250+
>>>>>  - reg : Specifies base physical address and size of the 
>>>>> registers map.
>>>>>  - clocks  : Phandle to clock-controller plus clock-specifier pair.
>>>>>  - clock-names : "secss" as a clock name.
>>>>> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
>>>>> index 451620b475a0..894ef93ef5ec 100644
>>>>> --- a/drivers/crypto/exynos-rng.c
>>>>> +++ b/drivers/crypto/exynos-rng.c
>>>>> @@ -22,12 +22,17 @@
>>>>>  #include 
>>>>>  #include 
>>>>>  #include 
>>>>> +#include 
>>>>>  #include 
>>>>>
>>>>>  #include 
>>>>>
>>>>>  #define EXYNOS_RNG_CONTROL 0x0
>>>>>  #define EXYNOS_RNG_STATUS  0x10
>>>>> +
>>>>> +#define EXYNOS_RNG_SEED_CONF   0x14
>>>>> +#define EXYNOS_RNG_GEN_PRNG0x02
>>>>
>>>> Use BIT(1) instead.
>
> Done.
>
>>>>> +
>>>>>  #define EXYNOS_RNG_SEED_BASE   0x140
>>>>>  #define EXYNOS_RNG_SEED(n) (EXYNOS_RNG_SEED_BASE + (n * 0x4))
>>>>>  #define EXYNOS_RNG_OUT_BASE0x160
>>>>> @@ -43,6 +48,11 @@
>>>>>  #define EXYNOS_RNG_SEED_REGS   5
>>>>>  #define EXYNOS_RNG_SEED_SIZE   (EXYNOS_RNG_SEED_REGS * 4)
>>>>>
>>>>> +enum exynos_prng_type {
>>>>> +   EXYNOS_PRNG_TYPE4 = 4,
>>>>> +   EXYNOS_PRNG_TYPE5 = 5,
>>>>
>>>> That's unusual numbering and naming, so just:
>>>> enum exynos_prng_type {
>>>>   EXYNOS_PRNG_EXYNOS4,
>>>>   EXYNOS_PRNG_EXYNOS5,
>>>> };
>>>>
>>>> Especially that TYPE4 and TYPE5 suggest so kind of sub-type (like
>>>> versions of some IP blocks, e.g. MFC) but it is just the family of
>>>> Exynos.
>>>
>>> Half done. I've changed TYPE to EXYNOS.
>>>
>>> I used explicit numbering in the enum because I want both values to act
>>> same true-false-wise. If one is 0 this condition is not met.
>>
>> First of all - that condition cannot happen. It is not possible from
>> the device-matching code.
>
> Let me explain what I didn't want. With the enum:
>
> enum exynos_prng_type {
> EXYNOS_PRNG_EXYNOS4,
> EXYNOS_PRNG_EXYNOS5,
> };
>
> and a code like this
>
> if (rng->type) {
>
> }
>
> EXYNOS_PRNG_EXYNOS4 is identical to false while EXYNOS_PRNG_EXYNPOS5
> evaluates as true. I think this is a bad idea. I don't want it ever to
> happen.
> Because chips have their own numbers I thought using tho

Re: [PATCH 1/3] crypto: exynos - Support Exynos5250+ SoCs

[Krzysztof Kozlowski]

On Wed, Dec 6, 2017 at 2:42 PM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> It was <2017-12-05 wto 14:34>, when Krzysztof Kozlowski wrote:
>> On Tue, Dec 5, 2017 at 1:35 PM, Łukasz Stelmach <l.stelm...@samsung.com> 
>> wrote:
>>> Add support for PRNG in Exynos5250+ SoCs.
>>>
>>> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>>> ---
>>>  .../bindings/crypto/samsung,exynos-rng4.txt|  4 ++-
>>>  drivers/crypto/exynos-rng.c| 36 
>>> --
>>>  2 files changed, 36 insertions(+), 4 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
>>> b/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
>>> index 4ca8dd4d7e66..a13fbdb4bd88 100644
>>> --- a/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
>>> +++ b/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
>>> @@ -2,7 +2,9 @@ Exynos Pseudo Random Number Generator
>>>
>>>  Required properties:
>>>
>>> -- compatible  : Should be "samsung,exynos4-rng".
>>> +- compatible  : One of:
>>> +- "samsung,exynos4-rng" for Exynos4210 and Exynos4412
>>> +- "samsung,exynos5250-prng" for Exynos5250+
>>>  - reg : Specifies base physical address and size of the registers 
>>> map.
>>>  - clocks  : Phandle to clock-controller plus clock-specifier pair.
>>>  - clock-names : "secss" as a clock name.
>>> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
>>> index 451620b475a0..894ef93ef5ec 100644
>>> --- a/drivers/crypto/exynos-rng.c
>>> +++ b/drivers/crypto/exynos-rng.c
>>> @@ -22,12 +22,17 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>
>>>  #include 
>>>
>>>  #define EXYNOS_RNG_CONTROL 0x0
>>>  #define EXYNOS_RNG_STATUS  0x10
>>> +
>>> +#define EXYNOS_RNG_SEED_CONF   0x14
>>> +#define EXYNOS_RNG_GEN_PRNG0x02
>>
>> Use BIT(1) instead.
>>
>>> +
>>>  #define EXYNOS_RNG_SEED_BASE   0x140
>>>  #define EXYNOS_RNG_SEED(n) (EXYNOS_RNG_SEED_BASE + (n * 0x4))
>>>  #define EXYNOS_RNG_OUT_BASE0x160
>>> @@ -43,6 +48,11 @@
>>>  #define EXYNOS_RNG_SEED_REGS   5
>>>  #define EXYNOS_RNG_SEED_SIZE   (EXYNOS_RNG_SEED_REGS * 4)
>>>
>>> +enum exynos_prng_type {
>>> +   EXYNOS_PRNG_TYPE4 = 4,
>>> +   EXYNOS_PRNG_TYPE5 = 5,
>>
>> That's unusual numbering and naming, so just:
>> enum exynos_prng_type {
>>   EXYNOS_PRNG_EXYNOS4,
>>   EXYNOS_PRNG_EXYNOS5,
>> };
>>
>> Especially that TYPE4 and TYPE5 suggest so kind of sub-type (like
>> versions of some IP blocks, e.g. MFC) but it is just the family of
>> Exynos.
>
> Half done. I've changed TYPE to EXYNOS.
>
> I used explicit numbering in the enum because I want both values to act
> same true-false-wise. If one is 0 this condition is not met.

First of all - that condition cannot happen. It is not possible from
the device-matching code. But if you want to indicate it explicitly
(for code reviewing?) then how about:
enum exynos_prng_type {
   EXYNOS_PRNG_UNKNOWN = 0,
   EXYNOS_PRNG_EXYNOS4,
   EXYNOS_PRNG_EXYNOS5,
};

In such case you have the same effect but your intentions are clear
(you expect possibility of =0... which is not possible :) ).

>>> +};
>>> +
>>>  /*
>>>   * Driver re-seeds itself with generated random numbers to increase
>>>   * the randomness.
>>> @@ -63,6 +73,7 @@ struct exynos_rng_ctx {
>>>  /* Device associated memory */
>>>  struct exynos_rng_dev {
>>> struct device   *dev;
>>> +   enum exynos_prng_type   type;
>>> void __iomem*mem;
>>> struct clk  *clk;
>>> /* Generated numbers stored for seeding during resume */
>>> @@ -160,8 +171,13 @@ static int exynos_rng_get_random(struct exynos_rng_dev 
>>> *rng,
>>>  {
>>> int retry = EXYNOS_RNG_WAIT_RETRIES;
>>>
>>> -   exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
>>> - EXYNOS_RNG_CONTROL);
>>> +   if (rng->type == EXYNOS_PRNG_TYPE4)

Re: [PATCH 2/3] crypto: exynos - Improve performance of PRNG

[Krzysztof Kozlowski]

On Wed, Dec 6, 2017 at 12:32 PM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> It was <2017-12-05 wto 19:06>, when Krzysztof Kozlowski wrote:
>> On Tue, Dec 5, 2017 at 6:53 PM, Krzysztof Kozlowski <k...@kernel.org> wrote:
>>> On Tue, Dec 05, 2017 at 05:43:10PM +0100, Łukasz Stelmach wrote:
>>>> It was <2017-12-05 wto 14:54>, when Stephan Mueller wrote:
>>>>> Am Dienstag, 5. Dezember 2017, 13:35:57 CET schrieb Łukasz Stelmach:
>>>>>> Use memcpy_fromio() instead of custom exynos_rng_copy_random() function
>>>>>> to retrieve generated numbers from the registers of PRNG.
>>>>>>
>>>>>> Remove unnecessary invocation of cpu_relax().
>>>>>>
>>>>>> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>>>>>> ---
>>>>>>  drivers/crypto/exynos-rng.c | 36 +---
>>>>>>  1 file changed, 5 insertions(+), 31 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
>>>>>> index 894ef93ef5ec..002e9d2a83cc 100644
>>>>>> --- a/drivers/crypto/exynos-rng.c
>>>>>> +++ b/drivers/crypto/exynos-rng.c
>>>>
>>>> [...]
>>>>
>>>>>> @@ -171,6 +143,8 @@ static int exynos_rng_get_random(struct 
>>>>>> exynos_rng_dev
>>>>>> *rng, {
>>>>>>int retry = EXYNOS_RNG_WAIT_RETRIES;
>>>>>>
>>>>>> +  *read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
>>>>>> +
>>>>>>if (rng->type == EXYNOS_PRNG_TYPE4) {
>>>>>>exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
>>>>>>  EXYNOS_RNG_CONTROL);
>>>>>> @@ -180,8 +154,8 @@ static int exynos_rng_get_random(struct 
>>>>>> exynos_rng_dev
>>>>>> *rng, }
>>>>>>
>>>>>>while (!(exynos_rng_readl(rng,
>>>>>> -  EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && 
>>>>>> --retry)
>>>>>> -  cpu_relax();
>>>>>> +  EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) &&
>>>>>> + --retry);
>
> [...]
>
>>>> The busy loop is not very busy. Every time I checked the loop (w/o
>>>> cpu_relax()) was executed twice (retry was 98) and the operation was
>>>> reliable. I don't see why do we need a memory barrier here. On the other
>>>> hand, I am not sure the whole exynos_rng_get_random() shouldn't be ran
>>>> under a mutex or a spinlock (I don't see anything like this in the upper
>>>> layers of the crypto framework).
>>>>
>>>> The *_relaxed() I/O operations do not enforce memory
>>>
>>> The cpu_relax() is a common pattern for busy-loop. If you want to break
>>> this pattern - please explain why only this part of kernel should not
>>> follow it (and rest of kernel should).
>>>
>>> The other part - this code is already using relaxed versions which might
>>> get you into difficult to debug issues. You mentioned that loop works
>>> reliable after removing the cpu_relax... yeah, it might for 99.999% but
>>> that's not the argument. I remember few emails from Arnd Bergmann
>>> mentioning explicitly to avoid using relaxed versions "just because",
>>> unless it is necessary or really understood.
>>>
>>> The code first writes to control register, then checks for status so you
>>> should have these operations strictly ordered. Therefore I think
>>> cpu_relax() should not be removed.
>>
>> ... or just convert it to readl_poll_timeout() because it makes code
>> more readable, takes care of timeout and you do not have care about
>> specific implementation (whether there should or should not be
>> cpu_relax).
>
> OK. This appears to perform reasonably.
>
> do {
> cpu_relax();
> } while (!(exynos_rng_readl(rng, EXYNOS_RNG_STATUS) &
>EXYNOS_RNG_STATUS_RNG_DONE) && --retry);

You mean that:
  while (readl_relaxed()) cpu_relax();
is slower than
  do cpu_relax() while (readl_relaxed())
?

Hmm... Interesting. I would be happy to learn more about it why it
behaves so differently. Maybe the execution of cpu_relax() before
readl_relaxed() reduces the amount of loops to actually one read?

Indeed some parts of kernel code for ARM prefers this approach,
although still the most popular pattern is existing one (while()
cpu_relax).

Best regards,
Krzysztof

Re: [PATCH 2/3] crypto: exynos - Improve performance of PRNG

[Krzysztof Kozlowski]

On Tue, Dec 5, 2017 at 6:53 PM, Krzysztof Kozlowski <k...@kernel.org> wrote:
> On Tue, Dec 05, 2017 at 05:43:10PM +0100, Łukasz Stelmach wrote:
>> It was <2017-12-05 wto 14:54>, when Stephan Mueller wrote:
>> > Am Dienstag, 5. Dezember 2017, 13:35:57 CET schrieb Łukasz Stelmach:
>> >
>> > Hi Łukasz,
>> >
>> >> Use memcpy_fromio() instead of custom exynos_rng_copy_random() function
>> >> to retrieve generated numbers from the registers of PRNG.
>> >>
>> >> Remove unnecessary invocation of cpu_relax().
>> >>
>> >> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>> >> ---
>> >>  drivers/crypto/exynos-rng.c | 36 +---
>> >>  1 file changed, 5 insertions(+), 31 deletions(-)
>> >>
>> >> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
>> >> index 894ef93ef5ec..002e9d2a83cc 100644
>> >> --- a/drivers/crypto/exynos-rng.c
>> >> +++ b/drivers/crypto/exynos-rng.c
>>
>> [...]
>>
>> >> @@ -171,6 +143,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev
>> >> *rng, {
>> >>int retry = EXYNOS_RNG_WAIT_RETRIES;
>> >>
>> >> +  *read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
>> >> +
>> >>if (rng->type == EXYNOS_PRNG_TYPE4) {
>> >>exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
>> >>  EXYNOS_RNG_CONTROL);
>> >> @@ -180,8 +154,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev
>> >> *rng, }
>> >>
>> >>while (!(exynos_rng_readl(rng,
>> >> -  EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && 
>> >> --retry)
>> >> -  cpu_relax();
>> >> +  EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) &&
>> >> + --retry);
>> SM>
>> SM> Is this related to the patch?
>>
>> KK> It looks like unrelated change so split it into separate commit with
>> KK> explanation why you are changing the common busy-loop pattern.
>> KK> exynos_rng_readl() uses relaxed versions of readl() so I would expect
>> KK> here cpu_relax().
>>
>> Yes. As far as I can tell this gives the major part of the performance
>> improvement brought by this patch.
>
> In that case definitely split and explain... what and why you want to
> achieve here.
>
>>
>> The busy loop is not very busy. Every time I checked the loop (w/o
>> cpu_relax()) was executed twice (retry was 98) and the operation was
>> reliable. I don't see why do we need a memory barrier here. On the other
>> hand, I am not sure the whole exynos_rng_get_random() shouldn't be ran
>> under a mutex or a spinlock (I don't see anything like this in the upper
>> layers of the crypto framework).
>>
>> The *_relaxed() I/O operations do not enforce memory
>
> The cpu_relax() is a common pattern for busy-loop. If you want to break
> this pattern - please explain why only this part of kernel should not
> follow it (and rest of kernel should).
>
> The other part - this code is already using relaxed versions which might
> get you into difficult to debug issues. You mentioned that loop works
> reliable after removing the cpu_relax... yeah, it might for 99.999% but
> that's not the argument. I remember few emails from Arnd Bergmann
> mentioning explicitly to avoid using relaxed versions "just because",
> unless it is necessary or really understood.
>
> The code first writes to control register, then checks for status so you
> should have these operations strictly ordered. Therefore I think
> cpu_relax() should not be removed.

... or just convert it to readl_poll_timeout() because it makes code
more readable, takes care of timeout and you do not have care about
specific implementation (whether there should or should not be
cpu_relax).

Best regards,
Krzysztof

Re: [PATCH] crypto: exynos - Icrease the priority of the driver

[Krzysztof Kozlowski]

On Tue, Dec 05, 2017 at 05:20:46PM +0100, Łukasz Stelmach wrote:
> exynos-rng is one of many implementations of stdrng. With priority as
> low as 100 it isn't selected, if software implementations (DRBG) are
> available. The value 300 was selected to give the PRNG priority before
> software implementations, but allow them to be selected in FIPS-mode
> (fips=1 in the kernel command line).

Typo in subject ("Increase").

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 2/3] crypto: exynos - Improve performance of PRNG

[Krzysztof Kozlowski]

On Tue, Dec 05, 2017 at 05:43:10PM +0100, Łukasz Stelmach wrote:
> It was <2017-12-05 wto 14:54>, when Stephan Mueller wrote:
> > Am Dienstag, 5. Dezember 2017, 13:35:57 CET schrieb Łukasz Stelmach:
> >
> > Hi Łukasz,
> >
> >> Use memcpy_fromio() instead of custom exynos_rng_copy_random() function
> >> to retrieve generated numbers from the registers of PRNG.
> >> 
> >> Remove unnecessary invocation of cpu_relax().
> >> 
> >> Signed-off-by: Łukasz Stelmach 
> >> ---
> >>  drivers/crypto/exynos-rng.c | 36 +---
> >>  1 file changed, 5 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> >> index 894ef93ef5ec..002e9d2a83cc 100644
> >> --- a/drivers/crypto/exynos-rng.c
> >> +++ b/drivers/crypto/exynos-rng.c
> 
> [...]
> 
> >> @@ -171,6 +143,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev
> >> *rng, {
> >>int retry = EXYNOS_RNG_WAIT_RETRIES;
> >> 
> >> +  *read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
> >> +
> >>if (rng->type == EXYNOS_PRNG_TYPE4) {
> >>exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
> >>  EXYNOS_RNG_CONTROL);
> >> @@ -180,8 +154,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev
> >> *rng, }
> >> 
> >>while (!(exynos_rng_readl(rng,
> >> -  EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && 
> >> --retry)
> >> -  cpu_relax();
> >> +  EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) &&
> >> + --retry);
> SM>
> SM> Is this related to the patch?
> 
> KK> It looks like unrelated change so split it into separate commit with
> KK> explanation why you are changing the common busy-loop pattern.
> KK> exynos_rng_readl() uses relaxed versions of readl() so I would expect
> KK> here cpu_relax().
> 
> Yes. As far as I can tell this gives the major part of the performance
> improvement brought by this patch.

In that case definitely split and explain... what and why you want to
achieve here.

> 
> The busy loop is not very busy. Every time I checked the loop (w/o
> cpu_relax()) was executed twice (retry was 98) and the operation was
> reliable. I don't see why do we need a memory barrier here. On the other
> hand, I am not sure the whole exynos_rng_get_random() shouldn't be ran
> under a mutex or a spinlock (I don't see anything like this in the upper
> layers of the crypto framework).
> 
> The *_relaxed() I/O operations do not enforce memory

The cpu_relax() is a common pattern for busy-loop. If you want to break
this pattern - please explain why only this part of kernel should not
follow it (and rest of kernel should).

The other part - this code is already using relaxed versions which might
get you into difficult to debug issues. You mentioned that loop works
reliable after removing the cpu_relax... yeah, it might for 99.999% but
that's not the argument. I remember few emails from Arnd Bergmann
mentioning explicitly to avoid using relaxed versions "just because",
unless it is necessary or really understood.

The code first writes to control register, then checks for status so you
should have these operations strictly ordered. Therefore I think
cpu_relax() should not be removed.

Best regards,
Krzysztof

Re: [PATCH 3/3] crypto: exynos - Reseed PRNG after generating 2^16 random bytes

[Krzysztof Kozlowski]

On Tue, Dec 5, 2017 at 1:35 PM, Łukasz Stelmach  wrote:
> Reseed PRNG after reading 65 kB of randomness. Although this may reduce
> performance, in most casese the loss is not noticable.
s/casese/cases/
s/noticable/noticeable/

Please explain why you want to reseed after 65 kB (as opposite to
current implementation). Mention also why you are changing the time of
reseed.

>
> Signed-off-by: Łukasz Stelmach 
> ---
>  drivers/crypto/exynos-rng.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> index 002e9d2a83cc..0bf07a655813 100644
> --- a/drivers/crypto/exynos-rng.c
> +++ b/drivers/crypto/exynos-rng.c
> @@ -54,12 +54,15 @@ enum exynos_prng_type {
>  };
>
>  /*
> - * Driver re-seeds itself with generated random numbers to increase
> - * the randomness.
> + * Driver re-seeds itself with generated random numbers to hinder
> + * backtracking of the original seed.
>   *
>   * Time for next re-seed in ms.
>   */
> -#define EXYNOS_RNG_RESEED_TIME 100
> +#define EXYNOS_RNG_RESEED_TIME 1000
> +#define EXYNOS_RNG_RESEED_BYTES65536
> +
> +

Just one empty line.

>  /*
>   * In polling mode, do not wait infinitely for the engine to finish the work.
>   */
> @@ -81,6 +84,8 @@ struct exynos_rng_dev {
> unsigned intseed_save_len;
> /* Time of last seeding in jiffies */
> unsigned long   last_seeding;
> +   /* Bytes generated since last seeding */
> +   unsigned long   bytes_seeding;
>  };
>
>  static struct exynos_rng_dev *exynos_rng_dev;
> @@ -125,6 +130,7 @@ static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
> }
>
> rng->last_seeding = jiffies;
> +   rng->bytes_seeding = 0;
>
> return 0;
>  }
> @@ -166,6 +172,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev 
> *rng,
> memcpy_fromio(dst, rng->mem + EXYNOS_RNG_OUT_BASE, *read);
>
> return 0;
> +
> +

No need for these lines.

Best regards,
Krzysztof

Re: [PATCH 2/3] crypto: exynos - Improve performance of PRNG

[Krzysztof Kozlowski]

On Tue, Dec 5, 2017 at 1:35 PM, Łukasz Stelmach  wrote:
> Use memcpy_fromio() instead of custom exynos_rng_copy_random() function
> to retrieve generated numbers from the registers of PRNG.
>
> Remove unnecessary invocation of cpu_relax().
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  drivers/crypto/exynos-rng.c | 36 +---
>  1 file changed, 5 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> index 894ef93ef5ec..002e9d2a83cc 100644
> --- a/drivers/crypto/exynos-rng.c
> +++ b/drivers/crypto/exynos-rng.c
> @@ -130,34 +130,6 @@ static int exynos_rng_set_seed(struct exynos_rng_dev 
> *rng,
>  }
>
>  /*
> - * Read from output registers and put the data under 'dst' array,
> - * up to dlen bytes.
> - *
> - * Returns number of bytes actually stored in 'dst' (dlen
> - * or EXYNOS_RNG_SEED_SIZE).
> - */
> -static unsigned int exynos_rng_copy_random(struct exynos_rng_dev *rng,
> -  u8 *dst, unsigned int dlen)
> -{
> -   unsigned int cnt = 0;
> -   int i, j;
> -   u32 val;
> -
> -   for (j = 0; j < EXYNOS_RNG_SEED_REGS; j++) {
> -   val = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> -
> -   for (i = 0; i < 4; i++) {
> -   dst[cnt] = val & 0xff;
> -   val >>= 8;
> -   if (++cnt >= dlen)
> -   return cnt;
> -   }
> -   }
> -
> -   return cnt;
> -}
> -
> -/*
>   * Start the engine and poll for finish.  Then read from output registers
>   * filling the 'dst' buffer up to 'dlen' bytes or up to size of generated
>   * random data (EXYNOS_RNG_SEED_SIZE).
> @@ -171,6 +143,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev 
> *rng,
>  {
> int retry = EXYNOS_RNG_WAIT_RETRIES;
>
> +   *read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
> +

Do not set *read on error path. Only on success. Although now it will
not matter but that is the expected behavior - if possible, do not
affect state outside of a block in case of error.

> if (rng->type == EXYNOS_PRNG_TYPE4) {
> exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
>   EXYNOS_RNG_CONTROL);
> @@ -180,8 +154,8 @@ static int exynos_rng_get_random(struct exynos_rng_dev 
> *rng,
> }
>
> while (!(exynos_rng_readl(rng,
> -   EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && 
> --retry)
> -   cpu_relax();
> +   EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) &&
> +  --retry);

It looks like unrelated change so split it into separate commit with
explanation why you are changing the common busy-loop pattern.
exynos_rng_readl() uses relaxed versions of readl() so I would expect
here cpu_relax().

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: exynos - Support Exynos5250+ SoCs

[Krzysztof Kozlowski]

On Tue, Dec 5, 2017 at 1:35 PM, Łukasz Stelmach  wrote:
> Add support for PRNG in Exynos5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  .../bindings/crypto/samsung,exynos-rng4.txt|  4 ++-
>  drivers/crypto/exynos-rng.c| 36 
> --
>  2 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt 
> b/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
> index 4ca8dd4d7e66..a13fbdb4bd88 100644
> --- a/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
> +++ b/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
> @@ -2,7 +2,9 @@ Exynos Pseudo Random Number Generator
>
>  Required properties:
>
> -- compatible  : Should be "samsung,exynos4-rng".
> +- compatible  : One of:
> +- "samsung,exynos4-rng" for Exynos4210 and Exynos4412
> +- "samsung,exynos5250-prng" for Exynos5250+
>  - reg : Specifies base physical address and size of the registers 
> map.
>  - clocks  : Phandle to clock-controller plus clock-specifier pair.
>  - clock-names : "secss" as a clock name.
> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> index 451620b475a0..894ef93ef5ec 100644
> --- a/drivers/crypto/exynos-rng.c
> +++ b/drivers/crypto/exynos-rng.c
> @@ -22,12 +22,17 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include 
>
>  #define EXYNOS_RNG_CONTROL 0x0
>  #define EXYNOS_RNG_STATUS  0x10
> +
> +#define EXYNOS_RNG_SEED_CONF   0x14
> +#define EXYNOS_RNG_GEN_PRNG0x02

Use BIT(1) instead.

> +
>  #define EXYNOS_RNG_SEED_BASE   0x140
>  #define EXYNOS_RNG_SEED(n) (EXYNOS_RNG_SEED_BASE + (n * 0x4))
>  #define EXYNOS_RNG_OUT_BASE0x160
> @@ -43,6 +48,11 @@
>  #define EXYNOS_RNG_SEED_REGS   5
>  #define EXYNOS_RNG_SEED_SIZE   (EXYNOS_RNG_SEED_REGS * 4)
>
> +enum exynos_prng_type {
> +   EXYNOS_PRNG_TYPE4 = 4,
> +   EXYNOS_PRNG_TYPE5 = 5,

That's unusual numbering and naming, so just:
enum exynos_prng_type {
  EXYNOS_PRNG_EXYNOS4,
  EXYNOS_PRNG_EXYNOS5,
};

Especially that TYPE4 and TYPE5 suggest so kind of sub-type (like
versions of some IP blocks, e.g. MFC) but it is just the family of
Exynos.

> +};
> +
>  /*
>   * Driver re-seeds itself with generated random numbers to increase
>   * the randomness.
> @@ -63,6 +73,7 @@ struct exynos_rng_ctx {
>  /* Device associated memory */
>  struct exynos_rng_dev {
> struct device   *dev;
> +   enum exynos_prng_type   type;
> void __iomem*mem;
> struct clk  *clk;
> /* Generated numbers stored for seeding during resume */
> @@ -160,8 +171,13 @@ static int exynos_rng_get_random(struct exynos_rng_dev 
> *rng,
>  {
> int retry = EXYNOS_RNG_WAIT_RETRIES;
>
> -   exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
> - EXYNOS_RNG_CONTROL);
> +   if (rng->type == EXYNOS_PRNG_TYPE4) {
> +   exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START,
> + EXYNOS_RNG_CONTROL);
> +   } else if (rng->type == EXYNOS_PRNG_TYPE5) {
> +   exynos_rng_writel(rng, EXYNOS_RNG_GEN_PRNG,
> + EXYNOS_RNG_SEED_CONF);
> +   }
>
> while (!(exynos_rng_readl(rng,
> EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && 
> --retry)
> @@ -279,6 +295,13 @@ static int exynos_rng_probe(struct platform_device *pdev)
> if (!rng)
> return -ENOMEM;
>
> +   rng->type = (enum 
> exynos_prng_type)of_device_get_match_data(>dev);
> +   if (rng->type != EXYNOS_PRNG_TYPE4 &&
> +   rng->type != EXYNOS_PRNG_TYPE5) {
> +   dev_err(>dev, "Unsupported PRNG type: %d", rng->type);
> +   return -ENOTSUPP;
> +   }
> +
> rng->dev = >dev;
> rng->clk = devm_clk_get(>dev, "secss");
> if (IS_ERR(rng->clk)) {
> @@ -300,7 +323,10 @@ static int exynos_rng_probe(struct platform_device *pdev)
> dev_err(>dev,
> "Couldn't register rng crypto alg: %d\n", ret);
> exynos_rng_dev = NULL;
> -   }
> +   } else

Missing {} around else clause. Probably checkpatch should point it.

> +   dev_info(>dev,
> +"Exynos Pseudo Random Number Generator (type:%d)\n",

dev_dbg, this is not that important information to affect the boot time.

Best regards,
Krzysztof

> +rng->type);
>
> return ret;
>  }
> @@ -367,6 +393,10 @@ static SIMPLE_DEV_PM_OPS(exynos_rng_pm_ops, 
> exynos_rng_suspend,
>  static const struct of_device_id exynos_rng_dt_match[] = {
> {
> .compatible = 

Re: [PATCH v3 1/3] dt-bindings: hwrng: Add Samsung Exynos 5250+ True RNG bindings

[Krzysztof Kozlowski]

On Tue, Dec 5, 2017 at 10:30 AM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> It was <2017-12-04 pon 14:13>, when Krzysztof Kozlowski wrote:
>> On Mon, Dec 4, 2017 at 1:53 PM, Łukasz Stelmach <l.stelm...@samsung.com> 
>> wrote:
>>> Add binding documentation for the True Random Number Generator
>>> found on Samsung Exynos 5250+ SoCs.
>>>
>>> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>>> ---
>>>  .../devicetree/bindings/rng/samsung,exynos5250-trng.txt | 17 
>>> +
>>>  1 file changed, 17 insertions(+)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>>> b/Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>>> new file mode 100644
>>> index ..5a613a4ec780
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>>> @@ -0,0 +1,17 @@
>>> +Exynos True Random Number Generator
>>> +
>>> +Required properties:
>>> +
>>> +- compatible  : Should be "samsung,exynos5250-trng".
>>> +- reg : Specifies base physical address and size of the registers 
>>> map.
>>> +- clocks  : Phandle to clock-controller plus clock-specifier pair.
>>> +- clock-names : "secss" as a clock name.
>>> +
>>> +Example:
>>> +
>>> +   rng@10830600 {
>>> +   compatible = "samsung,exynos5250-trng";
>>> +   reg = <0x10830600 0x100>;
>>> +   clocks = < CLK_SSS>;
>>> +   clock-names = "secss";
>>> +   };
>>> --
>>> 2.11.0
>>
>> Mine and Rob's tags disappeared and I think you did not introduce any
>> major changes here, right?
>
> A very experienced kernel developer adviced me to remove them.

In that case:
Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

BR,
Krzysztof

Re: [PATCH v3 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[Krzysztof Kozlowski]

On Mon, Dec 4, 2017 at 1:53 PM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
> ---
>  MAINTAINERS  |   7 +
>  drivers/char/hw_random/Kconfig   |  12 ++
>  drivers/char/hw_random/Makefile  |   1 +
>  drivers/char/hw_random/exynos-trng.c | 245 
> +++
>  4 files changed, 265 insertions(+)
>  create mode 100644 drivers/char/hw_random/exynos-trng.c

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: hwrng: Add Samsung Exynos 5250+ True RNG bindings

[Krzysztof Kozlowski]

On Mon, Dec 4, 2017 at 1:53 PM, Łukasz Stelmach  wrote:
> Add binding documentation for the True Random Number Generator
> found on Samsung Exynos 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  .../devicetree/bindings/rng/samsung,exynos5250-trng.txt | 17 
> +
>  1 file changed, 17 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>
> diff --git 
> a/Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt 
> b/Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> new file mode 100644
> index ..5a613a4ec780
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> @@ -0,0 +1,17 @@
> +Exynos True Random Number Generator
> +
> +Required properties:
> +
> +- compatible  : Should be "samsung,exynos5250-trng".
> +- reg : Specifies base physical address and size of the registers 
> map.
> +- clocks  : Phandle to clock-controller plus clock-specifier pair.
> +- clock-names : "secss" as a clock name.
> +
> +Example:
> +
> +   rng@10830600 {
> +   compatible = "samsung,exynos5250-trng";
> +   reg = <0x10830600 0x100>;
> +   clocks = < CLK_SSS>;
> +   clock-names = "secss";
> +   };
> --
> 2.11.0

Mine and Rob's tags disappeared and I think you did not introduce any
major changes here, right?

Best regards,
Krzysztof

Re: [PATCH v2 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[Krzysztof Kozlowski]

On Mon, Nov 27, 2017 at 10:58 AM, Łukasz Stelmach
 wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  MAINTAINERS  |   7 +
>  drivers/char/hw_random/Kconfig   |  12 ++
>  drivers/char/hw_random/Makefile  |   1 +
>  drivers/char/hw_random/exynos-trng.c | 245 
> +++
>  4 files changed, 265 insertions(+)
>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2811a211632c..992074cca612 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11780,6 +11780,13 @@ S: Maintained
>  F: drivers/crypto/exynos-rng.c
>  F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>
> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
> +M: Łukasz Stelmach 
> +L: linux-samsung-...@vger.kernel.org
> +S: Maintained
> +F: drivers/char/hw_random/exynos-trng.c
> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> +
>  SAMSUNG FRAMEBUFFER DRIVER
>  M: Jingoo Han 
>  L: linux-fb...@vger.kernel.org
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced..292e6b36d493 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>
>   If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS
> +   tristate "Samsung Exynos True Random Number Generator support"
> +   depends on ARCH_EXYNOS || COMPILE_TEST
> +   default HW_RANDOM
> +   ---help---
> + This driver provides support for the True Random Number
> + Generator available in Exynos SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called exynos-trng.
> +
> + If unsure, say Y.
>  endif # HW_RANDOM
>
>  config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index f3728d008fff..5595df97da7a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>  obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>  n2-rng-y := n2-drv.o n2-asm.o
>  obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>  obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-trng.c 
> b/drivers/char/hw_random/exynos-trng.c
> new file mode 100644
> index ..91b2ddb249fa
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,245 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach 
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation;
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define EXYNOS_TRNG_CLKDIV (0x0)
> +#define EXYNOS_TRNG_CTRL   (0x20)
> +#define EXYNOS_TRNG_POST_CTRL  (0x30)
> +#define EXYNOS_TRNG_ONLINE_CTRL(0x40)
> +#define EXYNOS_TRNG_ONLINE_STAT(0x44)
> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
> +#define EXYNOS_TRNG_FIFO_CTRL  (0x50)
> +#define EXYNOS_TRNG_FIFO_0 (0x80)
> +#define EXYNOS_TRNG_FIFO_1 (0x84)
> +#define EXYNOS_TRNG_FIFO_2 (0x88)
> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
> +#define EXYNOS_TRNG_FIFO_4 (0x90)
> +#define EXYNOS_TRNG_FIFO_5 (0x94)
> +#define EXYNOS_TRNG_FIFO_6 (0x98)
> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
> +#define EXYNOS_TRNG_FIFO_LEN   (8)
> +#define EXYNOS_TRNG_CLOCK_RATE (50)
> +
> +#define TRNG_CTRL_RGNENBIT(31)

Add a EXYNOS prefix and please put it after the register where it is
used (CTRL).

> +
> +struct exynos_trng_dev {
> +   struct device*dev;
> +   void __iomem *mem;
> +   struct clk   *clk;
> +   struct hwrng rng;
> +};
> +
> +static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max,
> +  bool wait)
> +{
> +   struct exynos_trng_dev 

Re: [PATCH v2 1/3] dt-bindings: hwrng: Add Samsung Exynos 5250+ True RNG bindings

[Krzysztof Kozlowski]

On Mon, Nov 27, 2017 at 10:58 AM, Łukasz Stelmach
<l.stelm...@samsung.com> wrote:
> Add binding documentation for the True Random Number Generator
> found on Samsung Exynos 5250+ SoCs.
>
> Acked-by: Rob Herring <r...@kernel.org>
> Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>
> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>

A minor nit: add new tags after your Signed-off-by, in same order you
received them.

Best regards,
Krzysztof

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[Krzysztof Kozlowski]

On Fri, Nov 24, 2017 at 2:05 PM, Stephan Müller <smuel...@chronox.de> wrote:
> Am Freitag, 24. November 2017, 13:09:06 CET schrieb Krzysztof Kozlowski:
>
> Hi Krzysztof,
>> >>
>> >> 1. I was rather thinking about extending existing exynos-rng.c [1] so
>> >> it would be using TRNG as seed for PRNG as this gives you much more
>> >> random data. Instead you developed totally separate driver which has
>> >> its own benefits - one can choose which interface he wants. Although
>> >> it is a little bit duplication.
>> >
>> > As far as I can tell, these are two different devices. However, PRNG
>> > shares hardware with the hash engine. Indeed there is a hardware to
>> > connect TRNG and PRNG, but, IMHO, it might be hard to model that
>> > dependency in kernel.
>>
>> It should be as simple as setting few more registers in SSS module
>> (actually maybe just enabling TRNG_SEED_START in PRNG). You do not
>> have to model it in a kernel like connecting some hw_rng entity to
>> cryptoai's rng_alg. See the jitterentropy-kcapi.c. I understand that
>> in that case existing exynos-rng.c could expose two different RNG
>> devices - one PRNG based on user's seed and second TRNG (actually
>> TRNG+PRNG).
>>
>> It does not seem difficult to model but the question is whether that
>> makes sense.
>
> The usage strategy for the PRNGs registered at the kernel crypto API is as
> follows:
>
> 1. crypto_rng_alloc
>
> 2. crypto_rng_reset
>
> 3. crypto_rng_generate
>
> If in step 2 you provide NULL as input, the kernel takes get_random_bytes as
> seed source. Step 2 is the mandatory.
>
> The Linux-RNG can be fed internally from the hw_random framework by the
> function hwrng_fillfn. This function is only used if the current_quality or
> default_quality values in the hw_random framework is set.
>
> For the TRNG, it seems to be not set per default, but could be set as either a
> boot argument or at runtime via /sys.
>
> If that variable is set and the TRNG is registered, it feeds random data into
> the Linux-RNG which in turn is used per default to seed a PRNG. In this case,
> no detour via user space is needed to push data from TRNG to the PRNG. Using
> that mechanism allows you to benefit from additional entropy the Linux-RNG
> collects elsewhere.
>>
>> > To me it seems easier to read TRNG (or
>> > /dev/random) and and write the result to PRNG manually (in software).
>>
>> Indeed this gives more flexibility to the user (choice of engine) but
>> first, it is slower, and second it reduces the quality of random
>> numbers (PRNG reseeds itself... but in this model cannot reseed from
>> TRNG).
>
> Given the reasons above, I would think that keeping the PRMG and TRNG separate
> as offered by the current patch seems reasonable. If configured correctly, the
> TRNG can seed the PRNG at any time (including boot time) without the need of
> user space.

Hi Stephan,

Thanks for explaining the details. This convinces me so I do not have
any objections against current approach of this second RNG driver for
Exynos.

Best regards,
Krzysztof

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[Krzysztof Kozlowski]

On Thu, Nov 23, 2017 at 7:46 PM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> It was <2017-11-23 czw 17:31>, when Krzysztof Kozlowski wrote:
>> On Thu, Nov 23, 2017 at 4:09 PM, Łukasz Stelmach <l.stelm...@samsung.com> 
>> wrote:
>>> Add support for True Random Number Generator found in Samsung Exynos
>>> 5250+ SoCs.
>>>
>>> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>>> ---
>>>  MAINTAINERS  |   7 +
>>>  drivers/char/hw_random/Kconfig   |  12 ++
>>>  drivers/char/hw_random/Makefile  |   1 +
>>>  drivers/char/hw_random/exynos-trng.c | 256 
>>> +++
>>>  4 files changed, 276 insertions(+)
>>>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>>>
>

+Cc Stephan Müller, who reviewed intensively exynos-rng.c.

> [...]
>
>>>  endif # HW_RANDOM
>>
>> Thanks for working on TRNG.
>>
>> 1. I was rather thinking about extending existing exynos-rng.c [1] so
>> it would be using TRNG as seed for PRNG as this gives you much more
>> random data. Instead you developed totally separate driver which has
>> its own benefits - one can choose which interface he wants. Although
>> it is a little bit duplication.
>
> As far as I can tell, these are two different devices. However, PRNG
> shares hardware with the hash engine. Indeed there is a hardware to
> connect TRNG and PRNG, but, IMHO, it might be hard to model that
> dependency in kernel.

It should be as simple as setting few more registers in SSS module
(actually maybe just enabling TRNG_SEED_START in PRNG). You do not
have to model it in a kernel like connecting some hw_rng entity to
cryptoai's rng_alg. See the jitterentropy-kcapi.c. I understand that
in that case existing exynos-rng.c could expose two different RNG
devices - one PRNG based on user's seed and second TRNG (actually
TRNG+PRNG).

It does not seem difficult to model but the question is whether that
makes sense.


> To me it seems easier to read TRNG (or
> /dev/random) and and write the result to PRNG manually (in software).

Indeed this gives more flexibility to the user (choice of engine) but
first, it is slower, and second it reduces the quality of random
numbers (PRNG reseeds itself... but in this model cannot reseed from
TRNG).

Best regards,
Krzysztof

Re: [PATCH 3/3] ARM: dts: exynos: Add nodes for True Random Number Generator

[Krzysztof Kozlowski]

On Thu, Nov 23, 2017 at 4:09 PM, Łukasz Stelmach  wrote:
> Add nodes for the True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  arch/arm/boot/dts/exynos5.dtsi| 5 +
>  arch/arm/boot/dts/exynos5250.dtsi | 5 +
>  arch/arm/boot/dts/exynos5410.dtsi | 5 +
>  arch/arm/boot/dts/exynos5420.dtsi | 5 +
>  4 files changed, 20 insertions(+)
>

Look okay, I'll take it once bindings and driver get accepted as wel.

Best regards,
Krzysztof

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[Krzysztof Kozlowski]

On Thu, Nov 23, 2017 at 4:09 PM, Łukasz Stelmach  wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  MAINTAINERS  |   7 +
>  drivers/char/hw_random/Kconfig   |  12 ++
>  drivers/char/hw_random/Makefile  |   1 +
>  drivers/char/hw_random/exynos-trng.c | 256 
> +++
>  4 files changed, 276 insertions(+)
>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2811a211632c..992074cca612 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11780,6 +11780,13 @@ S: Maintained
>  F: drivers/crypto/exynos-rng.c
>  F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>
> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
> +M: Łukasz Stelmach 
> +L: linux-samsung-...@vger.kernel.org
> +S: Maintained
> +F: drivers/char/hw_random/exynos-trng.c
> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> +
>  SAMSUNG FRAMEBUFFER DRIVER
>  M: Jingoo Han 
>  L: linux-fb...@vger.kernel.org
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced..a788ac07081b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>
>   If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS
> +   tristate "Samsung Exynos True Random Number Generator support"
> +   depends on ARCH_EXYNOS || COMPILE_TEST
> +   default HW_RANDOM
> +   ---help---
> + This driver provides support for the True Random Number
> + Generator available in Exynos SoCs.
> +
> +To compile this driver as a module, choose M here: the module
> +will be called exynos-trng.
> +
> +If unsure, say Y.
>  endif # HW_RANDOM

Thanks for working on TRNG.

1. I was rather thinking about extending existing exynos-rng.c [1] so
it would be using TRNG as seed for PRNG as this gives you much more
random data. Instead you developed totally separate driver which has
its own benefits - one can choose which interface he wants. Although
it is a little bit duplication.

2. I understand that in your current version of TRNG driver there is
no conflict with PRNG and they can work both at the same time?

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/exynos-rng.c

>
>  config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index f3728d008fff..5595df97da7a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>  obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>  n2-rng-y := n2-drv.o n2-asm.o
>  obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>  obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-trng.c 
> b/drivers/char/hw_random/exynos-trng.c
> new file mode 100644
> index ..340e106cae24
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,256 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach 
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation;
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define EXYNOS_TRNG_CLKDIV (0x0)
> +#define EXYNOS_TRNG_CTRL   (0x20)
> +#define EXYNOS_TRNG_POST_CTRL  (0x30)
> +#define EXYNOS_TRNG_ONLINE_CTRL(0x40)
> +#define EXYNOS_TRNG_ONLINE_STAT(0x44)
> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
> +#define EXYNOS_TRNG_FIFO_CTRL  (0x50)
> +#define EXYNOS_TRNG_FIFO_0 (0x80)
> +#define EXYNOS_TRNG_FIFO_1 (0x84)
> +#define EXYNOS_TRNG_FIFO_2 (0x88)
> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
> +#define EXYNOS_TRNG_FIFO_4 (0x90)
> +#define EXYNOS_TRNG_FIFO_5 (0x94)
> +#define EXYNOS_TRNG_FIFO_6 (0x98)
> +#define 

Re: [PATCH 1/3] dt-bindings: hwrng: Add Samsung Exynos 5250+ True RNG bindings

[Krzysztof Kozlowski]

On Thu, Nov 23, 2017 at 4:09 PM, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
> Add binding documentation for the True Random Number Generator
> found on Samsung Exynos 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
> ---
>  .../devicetree/bindings/rng/samsung,exynos5250-trng.txt | 17 
> +
>  1 file changed, 17 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH] crypto: s5p-sss - Remove a stray tab

[Krzysztof Kozlowski]

On Thu, Nov 9, 2017 at 10:26 PM, Dan Carpenter <dan.carpen...@oracle.com> wrote:
> This code seems correct, but the goto was indented too far.
>
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
>
> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH] crypto/s5p-sss: Use common error handling code in s5p_aes_probe()

[Krzysztof Kozlowski]

On Sun, Oct 22, 2017 at 03:05:05PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Sun, 22 Oct 2017 15:00:27 +0200
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/crypto/s5p-sss.c | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> index 7ac657f46d15..ea59e184c199 100644
> --- a/drivers/crypto/s5p-sss.c
> +++ b/drivers/crypto/s5p-sss.c
> @@ -863,16 +863,13 @@ static int s5p_aes_probe(struct platform_device *pdev)
>   pdata->irq_fc = platform_get_irq(pdev, 0);
>   if (pdata->irq_fc < 0) {
>   err = pdata->irq_fc;
> - dev_warn(dev, "feed control interrupt is not available.\n");
> - goto err_irq;
> + goto report_failure;
>   }
>   err = devm_request_threaded_irq(dev, pdata->irq_fc, NULL,
>   s5p_aes_interrupt, IRQF_ONESHOT,
>   pdev->name, pdev);
> - if (err < 0) {
> - dev_warn(dev, "feed control interrupt is not available.\n");
> - goto err_irq;
> - }
> + if (err < 0)
> + goto report_failure;

No, one exit path just to report error does not seem to be more
readable. Instead, printing error after the code causing it looks to me
as common pattern, easy to maintain.

This patch does not bring improvement, in my opinion.

Best regards,
Krzysztof


>  
>   pdata->busy = false;
>   pdata->dev = dev;
> @@ -906,6 +903,10 @@ static int s5p_aes_probe(struct platform_device *pdev)
>   s5p_dev = NULL;
>  
>   return err;
> +
> +report_failure:
> + dev_warn(dev, "feed control interrupt is not available.\n");
> + goto err_irq;
>  }
>  
>  static int s5p_aes_remove(struct platform_device *pdev)
> -- 
> 2.14.2
> 

Re: [PATCH v7 2/2] crypto: s5p-sss: Add HASH support for Exynos

[Krzysztof Kozlowski]

On Tue, Oct 17, 2017 at 1:28 PM, Kamil Konieczny
<k.koniec...@partner.samsung.com> 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 <k.koniec...@partner.samsung.com>
> ---
>  drivers/crypto/Kconfig   |   14 +
>  drivers/crypto/s5p-sss.c | 1407 
> +++++-
>  2 files changed, 1411 insertions(+), 10 deletions(-)


Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v7 1/2] crypto: s5p-sss: change spaces into tabs in defines

[Krzysztof Kozlowski]

On Tue, Oct 17, 2017 at 1:28 PM, Kamil Konieczny
 wrote:
> change spaces into tabs in defines
>
> Signed-off-by: Kamil Konieczny 
> ---
>  drivers/crypto/s5p-sss.c | 190 
> +++
>  1 file changed, 95 insertions(+), 95 deletions(-)
>

I reviewed this already...

Best regards,
Krzysztof

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 <k.koniec...@partner.samsung.com>
> ---
> 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 <k...@kernel.org>

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Wed, Oct 04, 2017 at 06:38:11PM +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 <k.koniec...@partner.samsung.com>
> ---
> version 4:
> - fixes suggested by Krzysztof Kozlowski: reformat comments, convert context
>   flag 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..2864efeaf8f8 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.
>   */
>  
>  #incl

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

[Krzysztof Kozlowski]

On Tue, Oct 03, 2017 at 04:57:43PM +0200, Kamil Konieczny wrote:
 
> >> [...]
> >> +static struct ahash_alg algs_sha256[] = {
> >> +{
> >> +  .init   = s5p_hash_init,
> >> +  .update = s5p_hash_update,
> >> +  .final  = s5p_hash_final,
> >> +  .finup  = s5p_hash_finup,
> >> +  .digest = s5p_hash_digest,
> >> +  .halg.digestsize= SHA256_DIGEST_SIZE,
> >> +  .halg.base  = {
> >> +  .cra_name   = "sha256",
> >> +  .cra_driver_name= "exynos-sha256",
> >> +  .cra_priority   = 100,
> >> +  .cra_flags  = CRYPTO_ALG_TYPE_AHASH |
> >> +CRYPTO_ALG_KERN_DRIVER_ONLY |
> >> +CRYPTO_ALG_ASYNC |
> >> +CRYPTO_ALG_NEED_FALLBACK,
> >> +  .cra_blocksize  = HASH_BLOCK_SIZE,
> >> +  .cra_ctxsize= sizeof(struct s5p_hash_ctx),
> >> +  .cra_alignmask  = SSS_DMA_ALIGN_MASK,
> >> +  .cra_module = THIS_MODULE,
> >> +  .cra_init   = s5p_hash_cra_init,
> >> +  .cra_exit   = s5p_hash_cra_exit,
> >> +  }
> >> +}
> >> +
> >> +};
> >> +
> >> +static struct sss_hash_algs_info exynos_hash_algs_info[] = {
> > 
> > You have warnings in your code. Please be sure that all compiler,
> > Smatch, Sparse, checkpatch and coccicheck warnings are fixed.
> > 
> > ../drivers/crypto/s5p-sss.c:1896:34: warning: ‘exynos_hash_algs_info’ 
> > defined but not used [-Wunused-variable]
> >  static struct sss_hash_algs_info exynos_hash_algs_info[] = {
> > 
> > Probably this should be __maybe_unused.
> 
> You are right, I did not check this with EXYNOS_HASH disabled, I will
> rewrite it.
> 
> > Also this should be const. I do not understand why you have to add one
> > more static variable (which sticks the driver to only one instance...)
> > and then modify it during runtime. Everything should be stored in device
> > state container (s5p_aes_dev) - directly or through some other pointers.
> 
> There is .registered field which is incremented with each algo register.
> I can move assignes to fields .import, .export and .statesize into struct.
> When I tried add const, I got compiler warn:
> drivers/crypto/s5p-sss.c: In function ‘s5p_aes_remove’:
> drivers/crypto/s5p-sss.c:2397:6: warning: passing argument 1 of 
> ‘crypto_unregister_ahash’ discards ‘const’ qualifier from pointer target type 
> [-Wdiscarded-qualifiers]
>   _algs_i[i].algs_list[j]);
> so it was not designed to be const (in crypto framework).
> In AES code the alg struct is also static:
> static struct crypto_alg algs[] = {

The crypto_alg and ahash_alg must indeed stay non-const but
sss_hash_algs_info is different. You do not pass it to crypto-core.

> What you mean by 'stick the driver to only one instance' ? In Exynos 4412 
> there
> is only one SecSS block, in some other Exynos there is SlimSS, but it is not
> the same (it has lower capabilities and other io addresses), so there should 
> not
> be two s5p_aes_dev drivers loaded at the same time. 

Current driver matches hardware in one-to-one so indeed there cannot be
two s5p_aes_dev devices. However this might change thus almost every
driver tries to follow the pattern of state-container passed to device
(e.g. platform_set_drvdata()). With this approach the code is nicely
encapsulated and usually much easier to review. Globals (or file-scope
variables) usually makes code more difficult to maintain.

In this driver this is not entirely possible as some crypto-functions do
not allow passing driver-supplied opaque pointer. But except this case,
everywhere else the driver should follow common convention - do not use
static variables.


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Wed, Sep 27, 2017 at 02:25:50PM +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 <k.koniec...@partner.samsung.com>
> ---
> 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()

Thanks for the changes, looks better.

> 
> 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 | 1507 
> +-
>  2 files changed, 1509 insertions(+), 12 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..e801ec4bfd8e 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 */
>  

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

[Krzysztof Kozlowski]

On Tue, Sep 26, 2017 at 05:54:42PM +0200, Kamil Konieczny wrote:
> On 25.09.2017 20:27, Krzysztof Kozlowski wrote:
> [...]
> >>>> +struct tasklet_struct   hash_tasklet;
> >>>> +u8  xmit_buf[BUFLEN] SSS_ALIGNED;
> >>>> +
> >>>> +unsigned long   hash_flags;
> >>>
> >>> This should be probably DECLARE_BITMAP.
> >>
> >> I do not get it, it is used for bits HASH_FLAGS_... and is not HW related.
> >> This will grow this patch even longer.
> > 
> > Why longer? Only declaration changes, rest should be the same.
> > Just instead of playing with bits over unsigned long explicitly use a
> > type for that purpose - DECLARE_BITMAP.
> 
> It will make code ugly, using >hash_flags[0] instead of >hash_flags.

Wait, all the set_bit and clear_bit should be even simpler:
-set_bit(HASH_FLAGS_DMA_READY, >hash_flags);
+set_bit(HASH_FLAGS_DMA_READY, dev->hash_flags);
This looks actually better..

> I have 9 bits used, and I will not anticipate it grow above 64.

You mean 32.

> If you insist, I can add DECLARE_BITMAP and rewrite all calls to _bits()
> functions.

Actually not, I do not insist. Just for me it makes the code simpler
(lack of "&" operand) and more robust as you explicitly declare number
of used bits (instead of assumption that your data type on given
compiler matches the amount of flags). Both arguments are not that
important.


> [...]
> >>>> +struct samsung_aes_variant  *pdata;
> >>>
> >>> This should be const as pdata should not be modified.
> >>
> >> You are right, and I do not modify _offsets parts, but only hash
> >> functions pointers and hash array size. It is made non-const due
> >> to keeping it in the place, and not moving struct samsung_aes_variant
> >> below hash functions definitions.
> >>
> >> If I need to keep them const, then I will move whole part below, just
> >> before _probe, so I can properly set hash_algs_info and
> >> hash_algs_size (and again, small change, but bigger patch).
> >>
> >> I can do this, if you think it is safer to have them const.
> > 
> > I see, you are modifiying the variant->hash_algs_info and
> > variant->hash_algs_size. However all of them should be static const as
> > this is not a dynamic data. It does not depend on any runtime
> > conditions, except of course choosing the variant itself.
> 
> You are right, I will make samsung_aes_variant const again.
> 
> [...]
> >>>> +/**
> >>>> + * s5p_hash_finish_req - finish request
> >>>> + * @req:AHASH request
> >>>> + * @err:error
> >>>> + *
> >>>> + * Clear flags, free memory,
> >>>> + * if FINAL then read output into ctx->digest,
> >>>> + * call completetion
> >>>> + */
> >>>> +static void s5p_hash_finish_req(struct ahash_request *req, int err)
> >>>> +{
> >>>> +struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
> >>>> +struct s5p_aes_dev *dd = ctx->dd;
> >>>> +
> >>>> +if (test_bit(HASH_FLAGS_SGS_COPIED, >hash_flags))
> >>>> +free_pages((unsigned long)sg_virt(ctx->sg),
> >>>> +   get_order(ctx->sg->length));
> >>>> +
> >>>> +if (test_bit(HASH_FLAGS_SGS_ALLOCED, >hash_flags))
> >>>> +kfree(ctx->sg);
> >>>> +
> >>>> +ctx->sg = NULL;
> >>>> +
> >>>> +dd->hash_flags &= ~(BIT(HASH_FLAGS_SGS_ALLOCED) |
> >>>> +BIT(HASH_FLAGS_SGS_COPIED));
> >>>> +
> >>>> +if (!err && !test_bit(HASH_FLAGS_ERROR, >flags)) {
> >>>> +s5p_hash_read_msg(req);
> >>>> +if (test_bit(HASH_FLAGS_FINAL, >hash_flags))
> >>>> +err = s5p_hash_finish(req);
> >>>> +} else {
> >>>> +ctx->flags |= BIT(HASH_FLAGS_ERROR);
> >>>> +}
> >>>> +
> >>>> +/* atomic operation is not needed here */
> >>>
> >>> Ok, but why?
> >>>
> >>
> >> Good question, I frankly copy this. The hash vars in s5p_aes_dev
> >> are no longer used as al

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

[Krzysztof Kozlowski]

On Fri, Sep 22, 2017 at 08:00:17PM +0200, Kamil Konieczny wrote:
> On 19.09.2017 21:03, Krzysztof Kozlowski wrote:
> > On Fri, Sep 15, 2017 at 07:50:06PM +0200, Kamil Konieczny wrote:

(...)

> >> +
> >> +/* HASH flags */
> >
> > All defines below have "HASH_FLAGS" prefix... so actually useful would
> > be to explain also what is a hash flag.
> 
> These are bit numbers used by device driver, setting in dev->hash_flags,
> used with set_bit(), clear_bit() or test_bit(), and with macro BIT(),
> to keep device state BUSY or FREE, or to signal state from irq_handler
> to hash_tasklet.

Then add something similar as comment to the code.

>  
> >> +#define HASH_FLAGS_BUSY   0
> >> +#define HASH_FLAGS_FINAL  1
> >> +#define HASH_FLAGS_DMA_ACTIVE 2
> >> +#define HASH_FLAGS_OUTPUT_READY   3
> >> +#define HASH_FLAGS_INIT   4
> >> +#define HASH_FLAGS_DMA_READY  6
> >> +
> >> +#define HASH_FLAGS_SGS_COPIED 9
> >> +#define HASH_FLAGS_SGS_ALLOCED10
> >> +/* HASH context flags */
> >
> > Same here.
> 
> These are bit numbers internal for request context, reqctx->flags
> 
> >
> >> +#define HASH_FLAGS_FINUP  16
> >> +#define HASH_FLAGS_ERROR  17
> >> +
> >> +#define HASH_FLAGS_MODE_MD5   18
> >> +#define HASH_FLAGS_MODE_SHA1  19
> >> +#define HASH_FLAGS_MODE_SHA25620
> >
> > These are set and not read?
> 
> It is leftover from omap driver, it was used in hmac alg.
> I will remove it.
> 
> >
> >> +
> >> +#define HASH_FLAGS_MODE_MASK  (BIT(18) | BIT(19) | BIT(20))
> >
> > Not used.
> >
> >> +/* HASH op codes */
> >> +#define HASH_OP_UPDATE1
> >> +#define HASH_OP_FINAL 2
> >> +
> >> +/* HASH HW constants */
> >> +#define HASH_ALIGN_MASK   (HASH_BLOCK_SIZE-1)
> >
> > Not used.
> >
> >> +
> >> +#define BUFLENHASH_BLOCK_SIZE
> >> +
> >> +#define SSS_DMA_ALIGN 16
> >> +#define SSS_ALIGNED   __attribute__((aligned(SSS_DMA_ALIGN)))
> >> +#define SSS_DMA_ALIGN_MASK(SSS_DMA_ALIGN-1)
> >
> > Please make it consistent with existing code, e.g.: by replacing
> > AES .cra_alignmask with same macro. In separate patch of course.
> 
> Thank you, I will do it in next patches.
> 
> DMA align for AES is 16, and for HASH is 8 (both aligned up),
> but I think it is simpler to have it both 16. This align is for length,
> e.g when len is not divisible by 8 (HASH case), DMA FC will round it up
> by 8, but HASH IP consumes only bytes set by len registers (so HASH will
> be correctly computed).
> 
> >
> >> +
> >> +/* HASH queue constant */
> >
> > Pretty useless comment. Do not use comments which copy the code. You
> > could explain here why you have chosen 10 (existing AES code uses 1).
> >
> >> +#define SSS_HASH_QUEUE_LENGTH 10
> 
> Number 10 was copied from omap-sham.c
> 
> The question why to use queue in device driver is good topic for RFC,
> I do not see any advantages for it except for corner cases,
> when driver receives many digest() request,
> or many update() from different contexts.
> 
> Basically it will not queue more than one update() from one context
> because the state for HASH is kept in request context, e.g. once 
> crypto user calls update() it must wait for its end before sending next.
> 
> It does have some effect on design, as it allows copy bytes for small
> update into request context (instead of putting it in queue).
> 
> >> +
> >> +/**
> >> + * struct sss_hash_algs_info - platform specific SSS HASH algorithms
> >> + * @algs_list:array of transformations (algorithms)
> >> + * @size: size
> >> + * @registered:   counter used at probe/remove
> >> + *
> >> + * Specifies platform specific information about hash algorithms
> >> + * of SSS module.
> >> + */
> >> +struct sss_hash_algs_info {
> >> +  struct ahash_alg*algs_list;
> >> +  unsigned intsize;
> >> +  unsigned intregistered;
> >> +};
> >> +
> >>  /**
> >>   * struct samsung_aes_variant - platform specific SSS driver data
> >>   * @aes_offset: AES register offset from SSS module's base.
> >> + * @hash_offset: HASH register offset from SSS module's base.
> >> + *
> >> + * @hash_algs_info: HASH transformations provided by SS module

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

[Krzysztof Kozlowski]

On Fri, Sep 15, 2017 at 07:50:06PM +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 <k.koniec...@partner.samsung.com>
> ---
> 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

Thanks for the changes. I must admit that I did not finish the review...
I found a lot of minor stuff but actually I did not look at overall
concept, architecture and how it really works. Sorry, will do it
later... :)

>  drivers/crypto/Kconfig   |   12 +
>  drivers/crypto/s5p-sss.c | 1683 
> +-
>  2 files changed, 1674 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index fe33c199fc1a..2f094c433346 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -439,6 +439,18 @@ 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.
> +   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..e951f0ffe49b 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)

Your indentation is correct but existing uses spaces. Could you send a
follow up fixing existing indents to be consistent?

>  #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)
>  #define SSS_FCINTENSET_BR

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

[Krzysztof Kozlowski]

On Fri, Sep 15, 2017 at 8:10 AM, Krzysztof Kozlowski <k...@kernel.org> wrote:
> On Wed, Sep 13, 2017 at 4:24 PM, Kamil Konieczny
> <k.koniec...@partner.samsung.com> wrote:
>> Hi Krzysztof,
>>
>> On 13.09.2017 15:18, Krzysztof Kozlowski wrote:
>>> On Wed, Sep 13, 2017 at 2:44 PM, Kamil Konieczny
>>> <k.koniec...@partner.samsung.com> 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.
>>>>
>>>> - Disable HASH in probe if Exynos PRNG is enabled.
>>>>
>>>> - 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:
>>>>
>>>> - Select sw algorithms MD5, SHA1 and SHA256 in S5P
>>>>   as they are nedded for fallback.
>>>>
>>>> Signed-off-by: Kamil Konieczny <k.koniec...@partner.samsung.com>
>>>> ---
>>>>  drivers/crypto/Kconfig   |6 +
>>>>  drivers/crypto/s5p-sss.c | 2062 
>>>> +++---
>>>>  2 files changed, 1939 insertions(+), 129 deletions(-)
>>>>
>>>
>>> Nice work, thanks!
>>>
>>> You need to split the patch, it is just too huge making it very
>>> difficult to review. Please split it per logically sensible
>>> improvements.
>>
>> Can you suggest how to break it up ?>
>> It is now big update, added working functionallity in one piece,
>> but I agree it can be hard to read
>> as git did some strange things,
>> like delete few aes functions (and mixing this delete with '+' lines)
>> and then adding the same aes without any change.
>
> You know the changes you want to do, you know the new architecture so
> usually it is easier for you to figure out the split.
> But few ideas:
> 1. For the problem of functions being deleted-moved without any
> changes, you can try to reorder new code so diff will not show these
> hunks. Indeed a lot diff here looks like weird matching of diff/patch.
> This definitely has to be fixed because I cannot figure out the real
> changes around existing functions (e.g. s5p_set_indata_start(),
> s5p_aes_crypt_start()).
> 2. You add debugging code - FLOW_LOG - this is one candidate to split 
> entirely.
> 3. Cleanups go to separate patch.
>
> Probably working on (1) above will bring the most benefit.

BTW, using some stable git release (not latest master) might help as well...

BR,
Krzysztof

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

[Krzysztof Kozlowski]

On Wed, Sep 13, 2017 at 2:44 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.
>
> Modifications in s5p-sss:
>
> - Add hash supporting structures and functions.
>
> - Modify irq handler to handle both aes and hash signals.
>
> - Disable HASH in probe if Exynos PRNG is enabled.
>
> - 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:
>
> - Select sw algorithms MD5, SHA1 and SHA256 in S5P
>   as they are nedded for fallback.
>
> Signed-off-by: Kamil Konieczny 
> ---
>  drivers/crypto/Kconfig   |6 +
>  drivers/crypto/s5p-sss.c | 2062 
> +++---
>  2 files changed, 1939 insertions(+), 129 deletions(-)
>

Nice work, thanks!

You need to split the patch, it is just too huge making it very
difficult to review. Please split it per logically sensible
improvements. I see some cleanups mixed with new features - this
definitely must be split out.

This looks more or less like an early work or RFC... because I see
things like "#if 0", "HACK" or "CONFIG_". If so, ask the question
about your problems directly. Do not force readers to find them out...

Best regards,
Krzysztof

Re: [PATCH v2] dt/bindings: exynos-rng: Move dt binding documentation to bindings/crypto

[Krzysztof Kozlowski]

On Wed, Aug 23, 2017 at 10:14:29PM +0530, PrasannaKumar Muralidharan wrote:
> Hi Krzysztof,
> 
> On 23 August 2017 at 21:42, Krzysztof Kozlowski <k...@kernel.org> wrote:
> > On Wed, Aug 23, 2017 at 08:34:43PM +0530, PrasannaKumar Muralidharan wrote:
> >> Samsung exynos PRNG driver is using crypto framework instead of
> >> hw_random framework. So move the devicetree binding to crypto folder.
> >>
> >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
> >> ---
> >> Changes in v2:
> >> * Modify MAINTAINERS file to reflect file rename
> >>
> >>  .../devicetree/bindings/{rng => crypto}/samsung,exynos-rng4.txt | > >> 0
> >>  MAINTAINERS | 
> >> 2 +-
> >>  2 files changed, 1 insertion(+), 1 deletion(-)
> >>  rename Documentation/devicetree/bindings/{rng => 
> >> crypto}/samsung,exynos-rng4.txt (100%)
> >>
> >
> > Patch is okay but CC list is still incomplete. I do not know how you
> > could get Mauro for this patch... just use get_maintainers.pl.
> 
> I used get_maintainer.pl. Rechecked again, it says Mauro and couple of
> others who may not be interested in this. It did not give your or
> crypto mailing list. Its strange.

Hm, you're right. Aparently get_maintainer.pl does handle moved files
thus he printed only entries for MAINTAINERS.

You will get full entry with:
scripts/get_maintainer.pl -f 
Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt

Krzysztof Kozlowski <k...@kernel.org> (maintainer:SAMSUNG EXYNOS PSEUDO RANDOM 
NUMBER GENERATOR (...)
Herbert Xu <herb...@gondor.apana.org.au> (maintainer:CRYPTO API)
"David S. Miller" <da...@davemloft.net> (maintainer:CRYPTO API)
Rob Herring <robh...@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE 
TREE BINDINGS)
Mark Rutland <mark.rutl...@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED 
DEVICE TREE BINDINGS)
Kukjin Kim <kg...@kernel.org> (maintainer:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES)
linux-crypto@vger.kernel.org (open list:SAMSUNG EXYNOS PSEUDO RANDOM NUMBER 
GENERATOR (...)
linux-samsung-...@vger.kernel.org (open list:SAMSUNG EXYNOS PSEUDO RANDOM 
NUMBER GENERATOR (...)
devicet...@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE 
BINDINGS)
linux-arm-ker...@lists.infradead.org (moderated list:ARM/SAMSUNG EXYNOS ARM 
ARCHITECTURES)
linux-ker...@vger.kernel.org (open list)

Best regards,
Krzysztof

Re: [PATCH v2] dt/bindings: exynos-rng: Move dt binding documentation to bindings/crypto

[Krzysztof Kozlowski]

On Wed, Aug 23, 2017 at 08:34:43PM +0530, PrasannaKumar Muralidharan wrote:
> Samsung exynos PRNG driver is using crypto framework instead of
> hw_random framework. So move the devicetree binding to crypto folder.
> 
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
> ---
> Changes in v2:
> * Modify MAINTAINERS file to reflect file rename
> 
>  .../devicetree/bindings/{rng => crypto}/samsung,exynos-rng4.txt | 0
>  MAINTAINERS | 2 
> +-
>  2 files changed, 1 insertion(+), 1 deletion(-)
>  rename Documentation/devicetree/bindings/{rng => 
> crypto}/samsung,exynos-rng4.txt (100%)
> 

Patch is okay but CC list is still incomplete. I do not know how you
could get Mauro for this patch... just use get_maintainers.pl.

Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Best regards,
Krzysztof

Re: [PATCH] crypto: exynoes-rng: Set cra_ctxsize to 0

[Krzysztof Kozlowski]

On Mon, May 22, 2017 at 5:44 AM, PrasannaKumar Muralidharan
 wrote:
>>> I do not have access to the hardware, did not test the change. Sorry I
>>> forgot to mention that.
>>
>> That is quite important... By default everything must be tested so if
>> you are skipping this step then please mark the patch respectively so
>> others will provide testing.
>
> Sure. Will keep that in mind. Instead of marking it as a [PATCH]
> should I use something else for this? Will it make things easier?

If sending untested patch is really necessary, then mark it either as
RFT instead of PATCH or add a comment about it after --- separator.

Best regards,
Krzysztof

Re: [PATCH] crypto: exynoes-rng: Set cra_ctxsize to 0

[Krzysztof Kozlowski]

On Sun, May 21, 2017 at 9:11 AM, PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com> wrote:
> Hi Krzysztof
>
> On 21 May 2017 at 11:56, Krzysztof Kozlowski <k...@kernel.org> wrote:
>> On Sun, May 21, 2017 at 8:09 AM, PrasannaKumar Muralidharan
>> <prasannatsmku...@gmail.com> wrote:
>>> As cra_ctxsize is set but the allocated space is not used, set it 0.
>>
>> Why do you think it is not used? Did you test our change on hardware?
>
> Had a look at the crypto rng code. I think the additional size is used
> to store driver private data. But this driver does not store any
> private data in the crypto_tfm structure so I think the 'cra_ctxsize'
> can be safely set to 0.

Then from where does crypto_tfm_ctx() get its memory?

> I do not have access to the hardware, did not test the change. Sorry I
> forgot to mention that.

That is quite important... By default everything must be tested so if
you are skipping this step then please mark the patch respectively so
others will provide testing.

Best regards,
Krzysztof

Re: [PATCH] crypto: exynoes-rng: Set cra_ctxsize to 0

[Krzysztof Kozlowski]

On Sun, May 21, 2017 at 8:09 AM, PrasannaKumar Muralidharan
 wrote:
> As cra_ctxsize is set but the allocated space is not used, set it 0.

Why do you think it is not used? Did you test our change on hardware?

Best regards,
Krzysztof

>
> Signed-off-by: PrasannaKumar Muralidharan 
> ---
>  drivers/crypto/exynos-rng.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> index 451620b..d009df6 100644
> --- a/drivers/crypto/exynos-rng.c
> +++ b/drivers/crypto/exynos-rng.c
> @@ -260,7 +260,7 @@ static int exynos_rng_kcapi_init(struct crypto_tfm *tfm)
> .cra_name   = "stdrng",
> .cra_driver_name= "exynos_rng",
> .cra_priority   = 100,
> -   .cra_ctxsize= sizeof(struct exynos_rng_ctx),
> +   .cra_ctxsize= 0,
> .cra_module = THIS_MODULE,
> .cra_init   = exynos_rng_kcapi_init,
> }
> --
> 1.8.5.6
>

[PATCH v5 1/2] linux/kernel.h: Add ALIGN_DOWN macro

[Krzysztof Kozlowski]

Few parts of kernel define their own macro for aligning down so provide
a common define for this, with the same usage and assumptions as existing
ALIGN.

Convert also three existing implementations to this one.

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>

---

The metag change was not even compiled due to lack of cross compiler.
---
 arch/metag/kernel/stacktrace.c | 2 --
 drivers/gpu/drm/udl/udl_fb.c   | 2 +-
 include/linux/kernel.h | 1 +
 include/video/udlfb.h  | 2 +-
 4 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/metag/kernel/stacktrace.c b/arch/metag/kernel/stacktrace.c
index 91ffc4b75c33..09d67b7f51ca 100644
--- a/arch/metag/kernel/stacktrace.c
+++ b/arch/metag/kernel/stacktrace.c
@@ -31,8 +31,6 @@ static void tbi_boing_init(void)
 }
 #endif
 
-#define ALIGN_DOWN(addr, size)  ((addr)&(~((size)-1)))
-
 /*
  * Unwind the current stack frame and store the new register values in the
  * structure passed as argument. Unwinding is equivalent to a function return,
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index d05abc69e305..4a6500362564 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -37,7 +37,7 @@ struct udl_fbdev {
 };
 
 #define DL_ALIGN_UP(x, a) ALIGN(x, a)
-#define DL_ALIGN_DOWN(x, a) ALIGN(x-(a-1), a)
+#define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
 
 /** Read the red component (0..255) of a 32 bpp colour. */
 #define DLO_RGB_GETRED(col) (uint8_t)((col) & 0xFF)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4c26dc3a8295..3d9f8420f973 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -47,6 +47,7 @@
 
 /* @a is a power of 2 value */
 #define ALIGN(x, a)__ALIGN_KERNEL((x), (a))
+#define ALIGN_DOWN(x, a)   __ALIGN_KERNEL((x) - ((a) - 1), (a))
 #define __ALIGN_MASK(x, mask)  __ALIGN_KERNEL_MASK((x), (mask))
 #define PTR_ALIGN(p, a)((typeof(p))ALIGN((unsigned long)(p), 
(a)))
 #define IS_ALIGNED(x, a)   (((x) & ((typeof(x))(a) - 1)) == 0)
diff --git a/include/video/udlfb.h b/include/video/udlfb.h
index f9466fa54ba4..3ea90aea5617 100644
--- a/include/video/udlfb.h
+++ b/include/video/udlfb.h
@@ -92,6 +92,6 @@ struct dlfb_data {
 
 /* remove these once align.h patch is taken into kernel */
 #define DL_ALIGN_UP(x, a) ALIGN(x, a)
-#define DL_ALIGN_DOWN(x, a) ALIGN(x-(a-1), a)
+#define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
 
 #endif
-- 
2.9.3

[PATCH v5 0/2] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Hi,

This is a follow up of my questions around exynos-rng [1].

Changes since v4:
=
1. Patch 2/2: Use "stdrng" name, as suggested by Herbert.
2. Patch 2/2: Add Bartlomiej's reviewed-by.

Changes since v3:
=
1. New patch: 1/2 for ALIGN_DOWN macro. The change in metag architecture
   was not compiled. Please test it.
2. Dropped patches touching ARM defconfig as they are not changing and
   they pollute this submission.
3. Utilize all seed provided by kcapi (suggested by Stephan Müller).
4. Drop dev->ctx (suggested by PrasannaKumar Muralidharan).
5. Remove any printks from set_seed callback as this might be a way
   for unprivileged user to pollute the log (suggested by Stephan).
6. Minor cleanups: initialize 'read' variable in exynos_rng_reseed()
   for readability (it is not strictly required).
7. Add review tags from Stephen and PrasannaKumar.


Changes since v2:
=
1. Do not re-use random numbers for re-seed (neither for system resume
   nor for periodic re-seed).  Instead the driver will just generate new
   random numbers (suggested by Stephan Müller).

   Suspend path tested with suspend-to-freeze, not real suspend. Testing
   on Trats2 would be welcomed.

Changes since v1:
=
1. Re-work the code for seeding after system resume, following suggestions
   and review by Stephan Müller.

2. Re-seed itself from time to time (every 100 ms), suggested by Stephan
   Müller.

3. Use a define for retries (Bartlomiej Zolnierkiewicz).
4. Add some docs.


Description:

The existing exynos-rng has many issues.  The most important one is that
it is a pseudo RNG device but uses hw_random interface which does not allow
proper seeding.

The RNG module on Exynos4 requires seeding.  On newer SoCs (like Exynos5420)
it can seed itself from a true RNG.  Converting the existing driver
to use TRNG would effectively drop support for Exynos4 and break
compatibility with existing users.

Instead I decided to convert it to crypto API.  In the future I hope
to add support for seeding from TRNG module.

Tested with app [2].

Patches are independent. I will take the defconfig changes (2/3 and 3/3)
through samsung-soc tree.

Best regards,
Krzysztof

[1] https://www.spinics.net/lists/arm-kernel/msg569641.html
[2] https://www.spinics.net/lists/arm-kernel/msg571184.html

Krzysztof Kozlowski (2):
  linux/kernel.h: Add ALIGN_DOWN macro
  crypto: hw_random - Add new Exynos RNG driver

 MAINTAINERS |   8 +
 arch/metag/kernel/stacktrace.c  |   2 -
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 389 
 drivers/gpu/drm/udl/udl_fb.c|   2 +-
 include/linux/kernel.h  |   1 +
 include/video/udlfb.h   |   2 +-
 11 files changed, 416 insertions(+), 250 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

-- 
2.9.3

[PATCH v5 2/2] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
This is a driver for pseudo random number generator block which on
Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
chipsets it might seed itself from true random number generator block
but this is not implemented yet.

New driver is a complete rework to use the crypto ALGAPI instead of
hw_random API.  Rationale for the change:
1. hw_random interface is for true RNG devices.
2. The old driver was seeding itself with jiffies which is not a
   reliable source for randomness.
3. Device generates five random 32-bit numbers in each pass but old
   driver was returning only one 32-bit number thus its performance was
   reduced.

Compatibility with DeviceTree bindings is preserved.

New driver does not use runtime power management but manually enables
and disables the clock when needed.  This is preferred approach because
using runtime PM just to toggle clock is huge overhead.

Another difference is reseeding itself with generated random data
periodically and during resuming from system suspend (previously driver
was re-seeding itself again with jiffies).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
Reviewed-by: Stephan Müller <smuel...@chronox.de>
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnier...@samsung.com>
---
 MAINTAINERS |   8 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 389 
 7 files changed, 413 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index affecc6d59f4..371fda859d43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10977,6 +10977,14 @@ L: alsa-de...@alsa-project.org (moderated for 
non-subscribers)
 S: Supported
 F: sound/soc/samsung/
 
+SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
+M: Krzysztof Kozlowski <k...@kernel.org>
+L: linux-crypto@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org
+S: Maintained
+F: drivers/crypto/exynos-rng.c
+F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
+
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han <jingooh...@gmail.com>
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0cafe08919c9..bdae802e7154 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -294,20 +294,6 @@ config HW_RANDOM_POWERNV
 
  If unsure, say Y.
 
-config HW_RANDOM_EXYNOS
-   tristate "EXYNOS HW random number generator support"
-   depends on ARCH_EXYNOS || COMPILE_TEST
-   depends on HAS_IOMEM
-   default HW_RANDOM
-   ---help---
- This driver provides kernel-side support for the Random Number
- Generator hardware found on EXYNOS SOCs.
-
- To compile this driver as a module, choose M here: the
- module will be called exynos-rng.
-
- If unsure, say Y.
-
 config HW_RANDOM_TPM
tristate "TPM HW Random Number Generator support"
depends on TCG_TPM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e4e7be..6f1eecc2045c 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -24,7 +24,6 @@ obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
-obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
 obj-$(CONFIG_HW_RANDOM_HISI)   += hisi-rng.o
 obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
diff --git a/drivers/char/hw_random/exynos-rng.c 
b/drivers/char/hw_random/exynos-rng.c
deleted file mode 100644
index 23d358553b21..
--- a/drivers/char/hw_random/exynos-rng.c
+++ /dev/null
@@ -1,231 +0,0 @@
-/*
- * exynos-rng.c - Random Number Generator driver for the exynos
- *
- * Copyright (C) 2012 Samsung Electronics
- * Jonghwa Lee <jonghwa3@samsung.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation;
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public 

Re: [PATCH v4 2/2] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Mon, Apr 10, 2017 at 12:55 PM, Herbert Xu
<herb...@gondor.apana.org.au> wrote:
> On Sat, Apr 08, 2017 at 03:32:45PM +0200, Krzysztof Kozlowski wrote:
>>
>> +static struct rng_alg exynos_rng_alg = {
>> + .generate   = exynos_rng_generate,
>> + .seed   = exynos_rng_seed,
>> + .seedsize   = EXYNOS_RNG_SEED_SIZE,
>> + .base   = {
>> + .cra_name   = "exynos_rng",
>
> Please use stdrng.  Or is there a reason why this can't be used
> by the crypto layer itself?

I think there is no reason against, I'll fix it.

Best regards,
Krzysztof

[PATCH v4 2/2] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
This is a driver for pseudo random number generator block which on
Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
chipsets it might seed itself from true random number generator block
but this is not implemented yet.

New driver is a complete rework to use the crypto ALGAPI instead of
hw_random API.  Rationale for the change:
1. hw_random interface is for true RNG devices.
2. The old driver was seeding itself with jiffies which is not a
   reliable source for randomness.
3. Device generates five random 32-bit numbers in each pass but old
   driver was returning only one 32-bit number thus its performance was
   reduced.

Compatibility with DeviceTree bindings is preserved.

New driver does not use runtime power management but manually enables
and disables the clock when needed.  This is preferred approach because
using runtime PM just to toggle clock is huge overhead.

Another difference is reseeding itself with generated random data
periodically and during resuming from system suspend (previously driver
was re-seeding itself again with jiffies).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
Reviewed-by: Stephan Müller <smuel...@chronox.de>
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 MAINTAINERS |   8 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 389 
 7 files changed, 413 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index affecc6d59f4..371fda859d43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10977,6 +10977,14 @@ L: alsa-de...@alsa-project.org (moderated for 
non-subscribers)
 S: Supported
 F: sound/soc/samsung/
 
+SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
+M: Krzysztof Kozlowski <k...@kernel.org>
+L: linux-crypto@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org
+S: Maintained
+F: drivers/crypto/exynos-rng.c
+F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
+
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han <jingooh...@gmail.com>
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0cafe08919c9..bdae802e7154 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -294,20 +294,6 @@ config HW_RANDOM_POWERNV
 
  If unsure, say Y.
 
-config HW_RANDOM_EXYNOS
-   tristate "EXYNOS HW random number generator support"
-   depends on ARCH_EXYNOS || COMPILE_TEST
-   depends on HAS_IOMEM
-   default HW_RANDOM
-   ---help---
- This driver provides kernel-side support for the Random Number
- Generator hardware found on EXYNOS SOCs.
-
- To compile this driver as a module, choose M here: the
- module will be called exynos-rng.
-
- If unsure, say Y.
-
 config HW_RANDOM_TPM
tristate "TPM HW Random Number Generator support"
depends on TCG_TPM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e4e7be..6f1eecc2045c 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -24,7 +24,6 @@ obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
-obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
 obj-$(CONFIG_HW_RANDOM_HISI)   += hisi-rng.o
 obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
diff --git a/drivers/char/hw_random/exynos-rng.c 
b/drivers/char/hw_random/exynos-rng.c
deleted file mode 100644
index 23d358553b21..
--- a/drivers/char/hw_random/exynos-rng.c
+++ /dev/null
@@ -1,231 +0,0 @@
-/*
- * exynos-rng.c - Random Number Generator driver for the exynos
- *
- * Copyright (C) 2012 Samsung Electronics
- * Jonghwa Lee <jonghwa3@samsung.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation;
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software

[PATCH v4 0/2] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Hi,

This is a follow up of my questions around exynos-rng [1].

Changes since v3:
=
1. New patch: 1/2 for ALIGN_DOWN macro. The change in metag architecture
   was not compiled. Please test it.
2. Dropped patches touching ARM defconfig as they are not changing and
   they pollute this submission.
3. Utilize all seed provided by kcapi (suggested by Stephan Müller).
4. Drop dev->ctx (suggested by PrasannaKumar Muralidharan).
5. Remove any printks from set_seed callback as this might be a way
   for unprivileged user to pollute the log (suggested by Stephan).
6. Minor cleanups: initialize 'read' variable in exynos_rng_reseed()
   for readability (it is not strictly required).
7. Add review tags from Stephen and PrasannaKumar.


Changes since v2:
=
1. Do not re-use random numbers for re-seed (neither for system resume
   nor for periodic re-seed).  Instead the driver will just generate new
   random numbers (suggested by Stephan Müller).

   Suspend path tested with suspend-to-freeze, not real suspend. Testing
   on Trats2 would be welcomed.

Changes since v1:
=
1. Re-work the code for seeding after system resume, following suggestions
   and review by Stephan Müller.

2. Re-seed itself from time to time (every 100 ms), suggested by Stephan
   Müller.

3. Use a define for retries (Bartlomiej Zolnierkiewicz).
4. Add some docs.


Description:

The existing exynos-rng has many issues.  The most important one is that
it is a pseudo RNG device but uses hw_random interface which does not allow
proper seeding.

The RNG module on Exynos4 requires seeding.  On newer SoCs (like Exynos5420)
it can seed itself from a true RNG.  Converting the existing driver
to use TRNG would effectively drop support for Exynos4 and break
compatibility with existing users.

Instead I decided to convert it to crypto API.  In the future I hope
to add support for seeding from TRNG module.

Tested with app [2].

Patches are independent. I will take the defconfig changes (2/3 and 3/3)
through samsung-soc tree.

Best regards,
Krzysztof

[1] https://www.spinics.net/lists/arm-kernel/msg569641.html
[2] https://www.spinics.net/lists/arm-kernel/msg571184.html


Krzysztof Kozlowski (2):
  linux/kernel.h: Add ALIGN_DOWN macro
  crypto: hw_random - Add new Exynos RNG driver

 MAINTAINERS |   8 +
 arch/metag/kernel/stacktrace.c  |   2 -
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 389 
 drivers/gpu/drm/udl/udl_fb.c|   2 +-
 include/linux/kernel.h  |   1 +
 include/video/udlfb.h   |   2 +-
 11 files changed, 416 insertions(+), 250 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

-- 
2.9.3

[PATCH v4 1/2] linux/kernel.h: Add ALIGN_DOWN macro

[Krzysztof Kozlowski]

Few parts of kernel define their own macro for aligning down so provide
a common define for this, with the same usage and assumptions as existing
ALIGN.

Convert also three existing implementations to this one.

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>

---

The metag change was not even compiled due to lack of cross compiler.
---
 arch/metag/kernel/stacktrace.c | 2 --
 drivers/gpu/drm/udl/udl_fb.c   | 2 +-
 include/linux/kernel.h | 1 +
 include/video/udlfb.h  | 2 +-
 4 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/metag/kernel/stacktrace.c b/arch/metag/kernel/stacktrace.c
index 91ffc4b75c33..09d67b7f51ca 100644
--- a/arch/metag/kernel/stacktrace.c
+++ b/arch/metag/kernel/stacktrace.c
@@ -31,8 +31,6 @@ static void tbi_boing_init(void)
 }
 #endif
 
-#define ALIGN_DOWN(addr, size)  ((addr)&(~((size)-1)))
-
 /*
  * Unwind the current stack frame and store the new register values in the
  * structure passed as argument. Unwinding is equivalent to a function return,
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index d05abc69e305..4a6500362564 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -37,7 +37,7 @@ struct udl_fbdev {
 };
 
 #define DL_ALIGN_UP(x, a) ALIGN(x, a)
-#define DL_ALIGN_DOWN(x, a) ALIGN(x-(a-1), a)
+#define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
 
 /** Read the red component (0..255) of a 32 bpp colour. */
 #define DLO_RGB_GETRED(col) (uint8_t)((col) & 0xFF)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4c26dc3a8295..3d9f8420f973 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -47,6 +47,7 @@
 
 /* @a is a power of 2 value */
 #define ALIGN(x, a)__ALIGN_KERNEL((x), (a))
+#define ALIGN_DOWN(x, a)   __ALIGN_KERNEL((x) - ((a) - 1), (a))
 #define __ALIGN_MASK(x, mask)  __ALIGN_KERNEL_MASK((x), (mask))
 #define PTR_ALIGN(p, a)((typeof(p))ALIGN((unsigned long)(p), 
(a)))
 #define IS_ALIGNED(x, a)   (((x) & ((typeof(x))(a) - 1)) == 0)
diff --git a/include/video/udlfb.h b/include/video/udlfb.h
index f9466fa54ba4..3ea90aea5617 100644
--- a/include/video/udlfb.h
+++ b/include/video/udlfb.h
@@ -92,6 +92,6 @@ struct dlfb_data {
 
 /* remove these once align.h patch is taken into kernel */
 #define DL_ALIGN_UP(x, a) ALIGN(x, a)
-#define DL_ALIGN_DOWN(x, a) ALIGN(x-(a-1), a)
+#define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
 
 #endif
-- 
2.9.3

Re: [PATCH v3 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Tue, Mar 28, 2017 at 07:41:47PM +0200, Stephan Müller wrote:
> Am Dienstag, 28. März 2017, 18:48:24 CEST schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> 
> > I tested a little bit and:
> > 1. Seeding with some value
> > 2. generating random,
> > 3. kcapi_rng_destroy+kcrng_init, (I cannot do a hardware reset except
> >reboot of entire system)
> > 4. seeding with the same value as in (1) - different random numbers.
> > 
> > Doing a system reboot and repeating above - different random numbers
> > (all are different: step (2) and in (4)).
> > 
> > Your test case also produces different random values every time.
> 
> Then I would assume that simply adding an outer loop to your for() loop to 
> inject seed larger than the minimum required seed size should be fine.

Yes, makes sense. I'll send an updated version of patch.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Mon, Mar 27, 2017 at 03:53:03PM +0200, Stephan Müller wrote:
> Am Montag, 27. März 2017, 06:23:11 CEST schrieb PrasannaKumar Muralidharan:
> 
> Hi PrasannaKumar,
> 
> > > Oh my, if you are right with your first guess, this is a bad DRNG design.
> > > 
> > > Just out of curiousity: what happens if a caller invokes the seed function
> > > twice or more times (each time with the sufficient amount of bits)? What
> > > is
> > > your guess here?
> > 
> > Should the second seed use the random data generated by the device?
> 
> A DRNG should be capable of processing an arbitrary amount of seed data. It 
> may be the case that the seed data must be processed in chunks though.
> 

As I said, I do not know the implementation details about hardware. They
are just not disclossed.

> That said, it may be the case that after injecting one chunk of seed the 
> currently discussed RNG simply needs to generate a random number to process 
> the input data before another seed can be added. But that is pure speculation.
> 
> But I guess that can be easily tested: inject a known seed into the DRNG, 
> generate a random number, inject the same seed again and again generate a 
> random number. If both are identical (which I do not hope), then the internal 
> state is simply overwritten (strange DRNG design).
> 
> A similar test can be made to see whether a larger set of seed simply 
> overwrites the state or is really processed.
> 
> 1. seed
> 2. generate random data
> 3. reset
> 4. seed with anther seed
> 5. generate random data
> 6. reset
> 7. seed with same data from 1
> 8. seed with same data from 2
> 9. generate random data
> 
> If data from 9 is identical to 2, then additional seed data is discarded -> 
> bad design. If data from 9 is identical to 5, then the additional data 
> overwrites the initial data -> bad DRNG design. If data from 9 neither 
> matches 
> 2 or 5, then all seed is taken -> good design.

I tested a little bit and:
1. Seeding with some value
2. generating random,
3. kcapi_rng_destroy+kcrng_init, (I cannot do a hardware reset except
   reboot of entire system)
4. seeding with the same value as in (1) - different random numbers.

Doing a system reboot and repeating above - different random numbers
(all are different: step (2) and in (4)).

Your test case also produces different random values every time.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Sun, Mar 26, 2017 at 07:05:48PM +0200, Stephan Müller wrote:
> Am Sonntag, 26. März 2017, 18:46:02 CEST schrieb PrasannaKumar Muralidharan:
> 
> HiKrzysztof,
> 
> > >> > +   if (slen < EXYNOS_RNG_SEED_SIZE) {
> > >> > +   dev_warn(rng->dev, "Seed too short (only %u bytes)\n",
> > >> > slen); +   return -EINVAL;
> > >> > +   }
> > >> 
> > >> Will it be helpful to print the required seed size?
> > > 
> > > It is in /proc/crypto... It is not a problem to print it but isn't that
> > > redundant?
> > 
> > Not necessary if it is already available.
> 
> Maybe the dev_warn should be removed. Note, unprivileged user space can 
> trigger this warning by simply invoking the seeding operation over and over 
> again with an insufficient seed size. This would clutter the log.

Makes sense. The generic dev_dbg() before would bring enough
information.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Sun, Mar 26, 2017 at 07:11:28PM +0200, Stephan Müller wrote:
> Am Samstag, 25. März 2017, 17:26:52 CEST schrieb Krzysztof Kozlowski:
> > +static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
> > +  const u8 *seed, unsigned int slen)
> > +{
> > +   u32 val;
> > +   int i;
> > +
> > +   dev_dbg(rng->dev, "Seeding with %u bytes\n", slen);
> > +
> > +   if (slen < EXYNOS_RNG_SEED_SIZE) {
> > +   dev_warn(rng->dev, "Seed too short (only %u bytes)\n", slen);
> > +   return -EINVAL;
> > +   }
> > +
> > +   for (i = 0 ; i < EXYNOS_RNG_SEED_REGS ; i++) {
> > +   val = seed[i * 4] << 24;
> > +   val |= seed[i * 4 + 1] << 16;
> > +   val |= seed[i * 4 + 2] << 8;
> > +   val |= seed[i * 4 + 3] << 0;
> > +
> > +   exynos_rng_writel(rng, val, EXYNOS_RNG_SEED(i));
> > +   }
> 
> Would it make sense to add another outer loop here to allow all of slen to be 
> injected into the DRNG? Note, in some cases, a user wants to add more seed 
> into the DRNG than the actual seed size. In this case, the DRNG acts as a 
> compression operation of entropy. This is used when the entropy-to-data ratio 
> is not 1:1. In a lot of cases, users have a seed which has less entropy in 
> bits per data bit.

Hi,

I do not know whether this would have any benefit on hardware. The
datasheet is not describing too much here. It is actually saying only:
1. Write SEED to each register (five in total).
2. Confirm that STATUS register says seeding done.
3. Start RNG engine.
4. Wait for engine finish (another bit in STATUS - clear it then).
5. Read the randoms.

I would guess that the hardware will ignore all previously written seeds
and use the last one. Maybe the hardware will use all of the seeds
written as you imply. It is just a guessing.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Sun, Mar 26, 2017 at 08:50:42PM +0530, PrasannaKumar Muralidharan wrote:
> .Have some minor comments. Please feel free to correct if I am wrong.
> 
> On 25 March 2017 at 21:56, Krzysztof Kozlowski <k...@kernel.org> wrote:
> > Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
> > This is a driver for pseudo random number generator block which on
> > Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
> > chipsets it might seed itself from true random number generator block
> > but this is not implemented yet.
> >
> > New driver is a complete rework to use the crypto ALGAPI instead of
> > hw_random API.  Rationale for the change:
> > 1. hw_random interface is for true RNG devices.
> > 2. The old driver was seeding itself with jiffies which is not a
> >reliable source for randomness.
> > 3. Device generates five random numbers in each pass but old driver was
> >returning only one thus its performance was reduced.
> >
> > Compatibility with DeviceTree bindings is preserved.
> >
> > New driver does not use runtime power management but manually enables
> > and disables the clock when needed.  This is preferred approach because
> > using runtime PM just to toggle clock is huge overhead.
> >
> > Another difference is reseeding itself with generated random data
> > periodically and during resuming from system suspend (previously driver
> > was re-seeding itself again with jiffies).
> >
> > Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
> >
> > ---
> >  MAINTAINERS |   8 +
> >  drivers/char/hw_random/Kconfig  |  14 --
> >  drivers/char/hw_random/Makefile |   1 -
> >  drivers/char/hw_random/exynos-rng.c | 231 -
> >  drivers/crypto/Kconfig  |  15 ++
> >  drivers/crypto/Makefile |   1 +
> >  drivers/crypto/exynos-rng.c | 390 
> > 
> >  7 files changed, 414 insertions(+), 246 deletions(-)
> >  delete mode 100644 drivers/char/hw_random/exynos-rng.c
> >  create mode 100644 drivers/crypto/exynos-rng.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index affecc6d59f4..371fda859d43 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10977,6 +10977,14 @@ L: alsa-de...@alsa-project.org (moderated for 
> > non-subscribers)
> >  S: Supported
> >  F: sound/soc/samsung/
> >
> > +SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
> > +M: Krzysztof Kozlowski <k...@kernel.org>
> > +L: linux-crypto@vger.kernel.org
> > +L: linux-samsung-...@vger.kernel.org
> > +S: Maintained
> > +F: drivers/crypto/exynos-rng.c
> > +F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
> > +
> >  SAMSUNG FRAMEBUFFER DRIVER
> >  M: Jingoo Han <jingooh...@gmail.com>
> >  L: linux-fb...@vger.kernel.org
> > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> > index 0cafe08919c9..bdae802e7154 100644
> > --- a/drivers/char/hw_random/Kconfig
> > +++ b/drivers/char/hw_random/Kconfig
> > @@ -294,20 +294,6 @@ config HW_RANDOM_POWERNV
> >
> >   If unsure, say Y.
> >
> > -config HW_RANDOM_EXYNOS
> > -   tristate "EXYNOS HW random number generator support"
> > -   depends on ARCH_EXYNOS || COMPILE_TEST
> > -   depends on HAS_IOMEM
> > -   default HW_RANDOM
> > -   ---help---
> > - This driver provides kernel-side support for the Random Number
> > - Generator hardware found on EXYNOS SOCs.
> > -
> > - To compile this driver as a module, choose M here: the
> > - module will be called exynos-rng.
> > -
> > - If unsure, say Y.
> > -
> >  config HW_RANDOM_TPM
> > tristate "TPM HW Random Number Generator support"
> > depends on TCG_TPM
> > diff --git a/drivers/char/hw_random/Makefile 
> > b/drivers/char/hw_random/Makefile
> > index 5f52b1e4e7be..6f1eecc2045c 100644
> > --- a/drivers/char/hw_random/Makefile
> > +++ b/drivers/char/hw_random/Makefile
> > @@ -24,7 +24,6 @@ obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
> >  obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
> >  obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
> >  obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
> > -obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
> >  obj-$(CONFIG_HW_RANDOM_HISI)   += hisi-rng.o
> >  obj-$(CONFIG_HW

[PATCH v3 2/3] ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 arch/arm/configs/exynos_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/exynos_defconfig 
b/arch/arm/configs/exynos_defconfig
index 6dc661c4a2c1..960d55445e05 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -265,6 +265,12 @@ CONFIG_DEBUG_RT_MUTEXES=y
 CONFIG_DEBUG_SPINLOCK=y
 CONFIG_DEBUG_MUTEXES=y
 CONFIG_DEBUG_USER=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=y
 CONFIG_CRYPTO_DEV_S5P=y
 CONFIG_ARM_CRYPTO=y
 CONFIG_CRYPTO_SHA1_ARM_NEON=m
-- 
2.9.3

[PATCH v3 0/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Hi,


This is a follow up of my questions around exynos-rng [1].

Changes since v2:
=
1. Do not re-use random numbers for re-seed (neither for system resume
   nor for periodic re-seed).  Instead the driver will just generate new
   random numbers (suggested by Stephan Müller).

   Suspend path tested with suspend-to-freeze, not real suspend. Testing
   on Trats2 would be welcomed.

Changes since v1:
=
1. Re-work the code for seeding after system resume, following suggestions
   and review by Stephan Müller.

2. Re-seed itself from time to time (every 100 ms), suggested by Stephan
   Müller.

3. Use a define for retries (Bartlomiej Zolnierkiewicz).
4. Add some docs.


Description:

The existing exynos-rng has many issues.  The most important one is that
it is a pseudo RNG device but uses hw_random interface which does not allow
proper seeding.

The RNG module on Exynos4 requires seeding.  On newer SoCs (like Exynos5420)
it can seed itself from a true RNG.  Converting the existing driver
to use TRNG would effectively drop support for Exynos4 and break
compatibility with existing users.

Instead I decided to convert it to crypto API.  In the future I hope
to add support for seeding from TRNG module.

Tested with app [2].

Patches are independent. I will take the defconfig changes (2/3 and 3/3)
through samsung-soc tree.

Best regards,
Krzysztof

[1] https://www.spinics.net/lists/arm-kernel/msg569641.html
[2] https://www.spinics.net/lists/arm-kernel/msg571184.html


Krzysztof Kozlowski (3):
  crypto: hw_random - Add new Exynos RNG driver
  ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API
  ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

 MAINTAINERS |   8 +
 arch/arm/configs/exynos_defconfig   |   6 +
 arch/arm/configs/multi_v7_defconfig |   6 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 390 
 9 files changed, 426 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

-- 
2.9.3

[PATCH v3 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
This is a driver for pseudo random number generator block which on
Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
chipsets it might seed itself from true random number generator block
but this is not implemented yet.

New driver is a complete rework to use the crypto ALGAPI instead of
hw_random API.  Rationale for the change:
1. hw_random interface is for true RNG devices.
2. The old driver was seeding itself with jiffies which is not a
   reliable source for randomness.
3. Device generates five random numbers in each pass but old driver was
   returning only one thus its performance was reduced.

Compatibility with DeviceTree bindings is preserved.

New driver does not use runtime power management but manually enables
and disables the clock when needed.  This is preferred approach because
using runtime PM just to toggle clock is huge overhead.

Another difference is reseeding itself with generated random data
periodically and during resuming from system suspend (previously driver
was re-seeding itself again with jiffies).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>

---
 MAINTAINERS |   8 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 390 
 7 files changed, 414 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index affecc6d59f4..371fda859d43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10977,6 +10977,14 @@ L: alsa-de...@alsa-project.org (moderated for 
non-subscribers)
 S: Supported
 F: sound/soc/samsung/
 
+SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
+M: Krzysztof Kozlowski <k...@kernel.org>
+L: linux-crypto@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org
+S: Maintained
+F: drivers/crypto/exynos-rng.c
+F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
+
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han <jingooh...@gmail.com>
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0cafe08919c9..bdae802e7154 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -294,20 +294,6 @@ config HW_RANDOM_POWERNV
 
  If unsure, say Y.
 
-config HW_RANDOM_EXYNOS
-   tristate "EXYNOS HW random number generator support"
-   depends on ARCH_EXYNOS || COMPILE_TEST
-   depends on HAS_IOMEM
-   default HW_RANDOM
-   ---help---
- This driver provides kernel-side support for the Random Number
- Generator hardware found on EXYNOS SOCs.
-
- To compile this driver as a module, choose M here: the
- module will be called exynos-rng.
-
- If unsure, say Y.
-
 config HW_RANDOM_TPM
tristate "TPM HW Random Number Generator support"
depends on TCG_TPM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e4e7be..6f1eecc2045c 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -24,7 +24,6 @@ obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
-obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
 obj-$(CONFIG_HW_RANDOM_HISI)   += hisi-rng.o
 obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
diff --git a/drivers/char/hw_random/exynos-rng.c 
b/drivers/char/hw_random/exynos-rng.c
deleted file mode 100644
index 23d358553b21..
--- a/drivers/char/hw_random/exynos-rng.c
+++ /dev/null
@@ -1,231 +0,0 @@
-/*
- * exynos-rng.c - Random Number Generator driver for the exynos
- *
- * Copyright (C) 2012 Samsung Electronics
- * Jonghwa Lee <jonghwa3@samsung.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation;
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- *
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 

[PATCH v3 3/3] ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 arch/arm/configs/multi_v7_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index 36c1b39cfb54..9bc1bd7f2afa 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -935,7 +935,13 @@ CONFIG_CPUFREQ_DT=y
 CONFIG_KEYSTONE_IRQ=y
 CONFIG_HW_RANDOM=y
 CONFIG_HW_RANDOM_ST=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
 CONFIG_CRYPTO_DEV_MARVELL_CESA=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=m
 CONFIG_CRYPTO_DEV_S5P=m
 CONFIG_CRYPTO_DEV_SUN4I_SS=m
 CONFIG_CRYPTO_DEV_ROCKCHIP=m
-- 
2.9.3

Re: [PATCH v2 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 09:41:59PM +0100, Stephan Müller wrote:
> Am Freitag, 24. März 2017, 19:26:04 CET schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> 
> > +static unsigned int exynos_rng_copy_random(struct exynos_rng_dev *rng,
> > +  u8 *dst, unsigned int dlen)
> > +{
> > +   unsigned int cnt = 0;
> > +   int i, j;
> > +   u32 val;
> > +
> > +   for (j = 0; j < EXYNOS_RNG_SEED_REGS; j++) {
> > +   val = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> > +
> > +   for (i = 0; i < 4; i++) {
> > +   dst[cnt] = val & 0xff;
> > +   val >>= 8;
> > +   if (++cnt >= dlen)
> > +   return cnt;
> > +   }
> > +   rng->seed_save[j] = val;
> 
> Just to clarify: is this call right? Shouldn't that be removed? Any RNG that 
> is given to a caller is tainted and should not serve as seed.

In that case I could either re-use RNGs not passed to the caller (like
in the block quoted below) or generate another round of them just for
purpose of next seeding.

With the first approach the problem is that I might wait for such unused
numbers pretty long. If user is requesting large amount of data, then I
will always give him all five output numbers. I will not have unused
numbers.

The second approach seems safe, but requires additional engine run which
will slow down some of the generate() calls.

> > +   }
> > +
> > +   /*
> > +* Engine filled all output registers, so read the remaining registers
> > +* for storing data as future seed.
> > +*/
> > +   for (; j < EXYNOS_RNG_SEED_REGS; j++)
> > +   rng->seed_save[j] = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> 
> With this call, I guess the questioned line above could go away, right?

This is used in combination with the previous line so I will get five
seeds (for five registers).

Best regards,
Krzysztof

[PATCH v2 3/3] ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 arch/arm/configs/multi_v7_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index 36c1b39cfb54..9bc1bd7f2afa 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -935,7 +935,13 @@ CONFIG_CPUFREQ_DT=y
 CONFIG_KEYSTONE_IRQ=y
 CONFIG_HW_RANDOM=y
 CONFIG_HW_RANDOM_ST=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
 CONFIG_CRYPTO_DEV_MARVELL_CESA=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=m
 CONFIG_CRYPTO_DEV_S5P=m
 CONFIG_CRYPTO_DEV_SUN4I_SS=m
 CONFIG_CRYPTO_DEV_ROCKCHIP=m
-- 
2.9.3

[PATCH v2 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
This is a driver for pseudo random number generator block which on
Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
chipsets it might seed itself from true random number generator block
but this is not implemented yet.

New driver is a complete rework to use the crypto ALGAPI instead of
hw_random API.  Rationale for the change:
1. hw_random interface is for true RNG devices.
2. The old driver was seeding itself with jiffies which is not a
   reliable source for randomness.
3. Device generates five random numbers in each pass but old driver was
   returning only one thus its performance was reduced.

Compatibility with DeviceTree bindings is preserved.

New driver does not use runtime power management but manually enables
and disables the clock when needed.  This is preferred approach because
using runtime PM just to toggle clock is huge overhead.

Another difference is reseeding itself with generated random data
periodically and during resuming from system suspend (previously driver
was re-seeding itself again with jiffies).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>

---

The resume path is untested as my Odroid U3 fails to go online. Testing
on Trats2 or other devices would be appreciated.
---
 MAINTAINERS |   8 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 392 
 7 files changed, 416 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index affecc6d59f4..371fda859d43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10977,6 +10977,14 @@ L: alsa-de...@alsa-project.org (moderated for 
non-subscribers)
 S: Supported
 F: sound/soc/samsung/
 
+SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
+M: Krzysztof Kozlowski <k...@kernel.org>
+L: linux-crypto@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org
+S: Maintained
+F: drivers/crypto/exynos-rng.c
+F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
+
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han <jingooh...@gmail.com>
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0cafe08919c9..bdae802e7154 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -294,20 +294,6 @@ config HW_RANDOM_POWERNV
 
  If unsure, say Y.
 
-config HW_RANDOM_EXYNOS
-   tristate "EXYNOS HW random number generator support"
-   depends on ARCH_EXYNOS || COMPILE_TEST
-   depends on HAS_IOMEM
-   default HW_RANDOM
-   ---help---
- This driver provides kernel-side support for the Random Number
- Generator hardware found on EXYNOS SOCs.
-
- To compile this driver as a module, choose M here: the
- module will be called exynos-rng.
-
- If unsure, say Y.
-
 config HW_RANDOM_TPM
tristate "TPM HW Random Number Generator support"
depends on TCG_TPM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e4e7be..6f1eecc2045c 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -24,7 +24,6 @@ obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
-obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
 obj-$(CONFIG_HW_RANDOM_HISI)   += hisi-rng.o
 obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
diff --git a/drivers/char/hw_random/exynos-rng.c 
b/drivers/char/hw_random/exynos-rng.c
deleted file mode 100644
index 23d358553b21..
--- a/drivers/char/hw_random/exynos-rng.c
+++ /dev/null
@@ -1,231 +0,0 @@
-/*
- * exynos-rng.c - Random Number Generator driver for the exynos
- *
- * Copyright (C) 2012 Samsung Electronics
- * Jonghwa Lee <jonghwa3@samsung.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation;
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Tem

[PATCH v2 2/3] ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 arch/arm/configs/exynos_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/exynos_defconfig 
b/arch/arm/configs/exynos_defconfig
index 6dc661c4a2c1..960d55445e05 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -265,6 +265,12 @@ CONFIG_DEBUG_RT_MUTEXES=y
 CONFIG_DEBUG_SPINLOCK=y
 CONFIG_DEBUG_MUTEXES=y
 CONFIG_DEBUG_USER=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=y
 CONFIG_CRYPTO_DEV_S5P=y
 CONFIG_ARM_CRYPTO=y
 CONFIG_CRYPTO_SHA1_ARM_NEON=m
-- 
2.9.3

[PATCH v2 0/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Hi,


This is a follow up of my questions around exynos-rng [1].

Changes since v1:
=
1. Re-work the code for seeding after system resume, following suggestions
   and review by Stephan Müller.
   The resume path was not tested.

2. Re-seed itself from time to time (every 100 ms), suggested by Stephan
   Müller.

3. Use a define for retries (Bartlomiej Zolnierkiewicz).
4. Add some docs.


Description:

The existing exynos-rng has many issues.  The most important one is that
it is a pseudo RNG device but uses hw_random interface which does not allow
proper seeding.

The RNG module on Exynos4 requires seeding.  On newer SoCs (like Exynos5420)
it can seed itself from a true RNG.  Converting the existing driver
to use TRNG would effectively drop support for Exynos4 and break
compatibility with existing users.

Instead I decided to convert it to crypto API.  In the future I hope
to add support for seeding from TRNG module.

Tested with app [2].

Patches are independent. I will take the defconfig changes (2/3 and 3/3)
through samsung-soc tree.

Best regards,
Krzysztof

[1] https://www.spinics.net/lists/arm-kernel/msg569641.html
[2] https://www.spinics.net/lists/arm-kernel/msg571184.html


Krzysztof Kozlowski (3):
  crypto: hw_random - Add new Exynos RNG driver
  ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API
  ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

 MAINTAINERS |   8 +
 arch/arm/configs/exynos_defconfig   |   6 +
 arch/arm/configs/multi_v7_defconfig |   6 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 392 
 9 files changed, 428 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

-- 
2.9.3

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 05:45:41PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > > > And I think the probe might be called twice, for example in case of
> > > > mistake in DTB.
> > > 
> > > Even if this is possible resource allocation code in the driver will
> > > take take care of handling it just fine,
> > 
> > Indeed, the devm_ioremap_resource() solves the case. I can drop the
> > check then.
> 
> Looking on this a bit more it seems that devm_ioremap_resource() will
> not cover all mistakes (using compatible by mistake in some other DTB
> node).
> 
> Leave the check, I take my objection back.

Great! Thanks for feedback.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 05:11:25PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Friday, March 24, 2017 06:46:00 PM Krzysztof Kozlowski wrote:
> > I really do not like global or file-scope variables. I do not like
> > drivers using them. Actually I hate them.
> > 
> > From time to time I encounter a driver which was designed with that
> > approach - static fields and hidden assumption that there will be only
> > one instance. Usually that assumption is really hidden...
> > 
> > ... and then it happens that I want to use two instances which of course
> > fails.
> > 
> > This code serves as a clear documentation for this assumption - only one
> > instance is allowed. You can look at it as a self-documenting
> > requirement.
> 
> For me it looks as needless case of defensive programming and when
> I see the code like this it always raises questions about the real
> intentions of the code. I find it puzzling and not helpful.

I do not understand what might be puzzling about check for static
file-scope value. It is of course subjective, but for me that looks
pretty self-explanatory.

> 
> > And I think the probe might be called twice, for example in case of
> > mistake in DTB.
> 
> Even if this is possible resource allocation code in the driver will
> take take care of handling it just fine,

Indeed, the devm_ioremap_resource() solves the case. I can drop the
check then.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 04:26:47PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> Firstly, thanks for working on this.
> 
> The patch looks fine overall for me, some review comments below.
> 
> On Friday, March 24, 2017 05:24:44 PM Krzysztof Kozlowski wrote:
> > Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
> > This is a driver for pseudo random number generator block which on
> > Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
> > chipsets it might seed itself from true random number generator block
> > but this is not implemented yet.
> > 
> > New driver is a complete rework to use the crypto ALGAPI instead of
> > hw_random API.  Rationale for the change:
> > 1. hw_random interface is for true RNG devices.
> > 2. The old driver was seeding itself with jiffies which is not a
> >reliable source for randomness.
> > 3. Device generates five random numbers in each pass but old driver was
> >returning only one thus its performance was reduced.
> > 
> > Compatibility with DeviceTree bindings is preserved.
> > 
> > New driver does not use runtime power management but manually enables
> > and disables the clock when needed.  This is preferred approach because
> > using runtime PM just to toggle clock is huge overhead.  Another
> 
> I'm not entirely convinced that the new approach is better.
> 
> With the old approach exynos_rng_generate() can be called more
> than once before PM autosuspend kicks in and thus clk_prepare_enable()/
> clk_disable()_unprepare() operations will be done only once. This
> would give better performance on the "burst" operations.
> 
> [ The above assumes that clock operations are more costly than
>   going through PM core to check the current device state. ]

I agree that we loose the "burst" mode but:
1. At least on Exynso4 SSS is part of TOP power domain so it will not
   help to reduce any more power consumption (on Exynos5422 it is
   mentioned in G2D... which seems incorrect).
2. I think the overhead of clk operations is much smaller. These are only
   two locks (prepare mutex + enable spin), simple tree traversal and
   play with few SFRs.

   The power domain code in comparison to that is huge and complicated
   with inter-device links and dependencies. Also the actual runtime PM
   suspend would anyway fall back at then end to clk prepare/enable
   locks and paths.

   We've been talking about this a lot with Marek Szyprowski (cc'ed) and
   he was always (AFAIR) against attempts of runtime power
   management of a single clock...


> 
> > +static int exynos_rng_get_random(struct exynos_rng_dev *rng,
> > +u8 *dst, unsigned int dlen,
> > +unsigned int *read)
> > +{
> > +   int retry = 100;
> 
> I know that this is copied verbatim from the old driver but please
> use define for the maximum number of retries.

No problem. Number is chosen arbitrarily so there will not be any
additional information coming from the define.

> > +static int exynos_rng_probe(struct platform_device *pdev)
> > +{
> > +   struct exynos_rng_dev *rng;
> > +   struct resource *res;
> > +   int ret;
> > +
> > +   if (exynos_rng_dev)
> > +   return -EEXIST;
> 
> How this condition could ever happen? 
> 
> The probe function will never be called twice.

I really do not like global or file-scope variables. I do not like
drivers using them. Actually I hate them.

>From time to time I encounter a driver which was designed with that
approach - static fields and hidden assumption that there will be only
one instance. Usually that assumption is really hidden...

... and then it happens that I want to use two instances which of course
fails.

This code serves as a clear documentation for this assumption - only one
instance is allowed. You can look at it as a self-documenting
requirement.

And I think the probe might be called twice, for example in case of
mistake in DTB.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 03:55:22PM +0100, Stephan Müller wrote:
> Am Freitag, 24. März 2017, 15:51:56 CET schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> > 
> > I'll use the generated random numbers.
> 
> If you do that, may I propse that you update this seed field periodically? 
> E.g. with every (or every other) user request for random data, you may update 
> that field with new data.

Sounds reasonable. Thanks for feedback, it is much appreciated.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 03:46:44PM +0100, Stephan Müller wrote:
> Am Freitag, 24. März 2017, 15:43:48 CET schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> 
> > On Fri, Mar 24, 2017 at 03:37:59PM +0100, Stephan Müller wrote:
> > > Am Freitag, 24. März 2017, 15:24:44 CET schrieb Krzysztof Kozlowski:
> > > 
> > > Hi Krzysztof,
> > > 
> > > > +
> > > > +static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
> > > > +  const u8 *seed, unsigned int slen)
> > > > +{
> > > > +   int ret, i;
> > > > +   u32 val;
> > > > +
> > > > +   dev_dbg(rng->dev, "Seeding with %u bytes\n", slen);
> > > > +
> > > > +   ret = clk_prepare_enable(rng->clk);
> > > > +   if (ret)
> > > > +   return ret;
> > > > +
> > > > +   if (slen < EXYNOS_RNG_SEED_SIZE) {
> > > > +   dev_warn(rng->dev, "Seed too short (only %u bytes)\n", 
> > > > slen);
> > > > +   ret = -EINVAL;
> > > > +   goto out;
> > > > +   }
> > > > +
> > > > +   for (i = 0 ; i < EXYNOS_RNG_SEED_REGS ; i++) {
> > > > +   val = seed[i * 4] << 24;
> > > > +   val |= seed[i * 4 + 1] << 16;
> > > > +   val |= seed[i * 4 + 2] << 8;
> > > > +   val |= seed[i * 4 + 3] << 0;
> > > > +
> > > > +   exynos_rng_writel(rng, val, EXYNOS_RNG_SEED(i));
> > > > +   }
> > > > +
> > > > +   val = exynos_rng_readl(rng, EXYNOS_RNG_STATUS);
> > > > +   if (!(val & EXYNOS_RNG_STATUS_SEED_SETTING_DONE)) {
> > > > +   dev_warn(rng->dev, "Seed setting not finished\n");
> > > > +   ret = -EIO;
> > > > +   goto out;
> > > > +   }
> > > > +
> > > > +   ret = 0;
> > > > +   /* Save seed for suspend */
> > > > +   memcpy(rng->seed_save, seed, slen);
> > > 
> > > Is this really necessary? If you need to save some seed, shouldn't that be
> > > an output of the DRNG and not the real seed?
> > 
> > When suspended to RAM device will loose the contents of registers
> > (including SEED registers) so it has to be initialized with something on
> > resume.
> > 
> > The seed registers are write-only so I cannot read them and store
> > contents just before suspend.
> > 
> > I understand that real seed should not be stored... but then if I am not
> > able to re-seed it with same values, I will loose the continuous and
> > reproducible pseudo-random generation after suspend. Aren't this
> > expected out of PRNG?
> 
> An RNG has to be stateless from the perspective of the caller -- this is the 
> core implication of entropy.
> 
> Then, if you add the initial seed after the RNG lost its state implies that 
> the same sequence of random numbers starts again. I.e. where is the 
> randomness?

Ahhh, yes, you are right. The device would not continue on its random
generation because all state is lost anyway.

I'll use the generated random numbers.

> > > Besides, how do you know that slen is not larger than
> > > EXYNOS_RNG_SEED_SIZE?
> > 
> > Right, there is a overflow here. It should be sizeof(rng->seed_save);
> 
> shouldn't it be min(rng->seed_save, slen)?

Now it is irrelevant but anyway I am not allowing the seed to be less
then EXYNOS_RNG_SEED_SIZE (== sizeof(rng->seed_save)). The device
expects all five channels (registers) to be seeded.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 03:37:59PM +0100, Stephan Müller wrote:
> Am Freitag, 24. März 2017, 15:24:44 CET schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> 
> > +
> > +static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
> > +  const u8 *seed, unsigned int slen)
> > +{
> > +   int ret, i;
> > +   u32 val;
> > +
> > +   dev_dbg(rng->dev, "Seeding with %u bytes\n", slen);
> > +
> > +   ret = clk_prepare_enable(rng->clk);
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (slen < EXYNOS_RNG_SEED_SIZE) {
> > +   dev_warn(rng->dev, "Seed too short (only %u bytes)\n", slen);
> > +   ret = -EINVAL;
> > +   goto out;
> > +   }
> > +
> > +   for (i = 0 ; i < EXYNOS_RNG_SEED_REGS ; i++) {
> > +   val = seed[i * 4] << 24;
> > +   val |= seed[i * 4 + 1] << 16;
> > +   val |= seed[i * 4 + 2] << 8;
> > +   val |= seed[i * 4 + 3] << 0;
> > +
> > +   exynos_rng_writel(rng, val, EXYNOS_RNG_SEED(i));
> > +   }
> > +
> > +   val = exynos_rng_readl(rng, EXYNOS_RNG_STATUS);
> > +   if (!(val & EXYNOS_RNG_STATUS_SEED_SETTING_DONE)) {
> > +   dev_warn(rng->dev, "Seed setting not finished\n");
> > +   ret = -EIO;
> > +   goto out;
> > +   }
> > +
> > +   ret = 0;
> > +   /* Save seed for suspend */
> > +   memcpy(rng->seed_save, seed, slen);
> 
> Is this really necessary? If you need to save some seed, shouldn't that be an 
> output of the DRNG and not the real seed?

When suspended to RAM device will loose the contents of registers
(including SEED registers) so it has to be initialized with something on
resume.

The seed registers are write-only so I cannot read them and store
contents just before suspend.

I understand that real seed should not be stored... but then if I am not
able to re-seed it with same values, I will loose the continuous and
reproducible pseudo-random generation after suspend. Aren't this
expected out of PRNG?

> Besides, how do you know that slen is not larger than EXYNOS_RNG_SEED_SIZE?

Right, there is a overflow here. It should be sizeof(rng->seed_save);

Thanks for review!

Best regards,
Krzysztof

[PATCH 3/3] ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 arch/arm/configs/multi_v7_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index 36c1b39cfb54..9bc1bd7f2afa 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -935,7 +935,13 @@ CONFIG_CPUFREQ_DT=y
 CONFIG_KEYSTONE_IRQ=y
 CONFIG_HW_RANDOM=y
 CONFIG_HW_RANDOM_ST=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
 CONFIG_CRYPTO_DEV_MARVELL_CESA=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=m
 CONFIG_CRYPTO_DEV_S5P=m
 CONFIG_CRYPTO_DEV_SUN4I_SS=m
 CONFIG_CRYPTO_DEV_ROCKCHIP=m
-- 
2.9.3

[PATCH 2/3] ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 arch/arm/configs/exynos_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/exynos_defconfig 
b/arch/arm/configs/exynos_defconfig
index 6dc661c4a2c1..960d55445e05 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -265,6 +265,12 @@ CONFIG_DEBUG_RT_MUTEXES=y
 CONFIG_DEBUG_SPINLOCK=y
 CONFIG_DEBUG_MUTEXES=y
 CONFIG_DEBUG_USER=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=y
 CONFIG_CRYPTO_DEV_S5P=y
 CONFIG_ARM_CRYPTO=y
 CONFIG_CRYPTO_SHA1_ARM_NEON=m
-- 
2.9.3

[PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
This is a driver for pseudo random number generator block which on
Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
chipsets it might seed itself from true random number generator block
but this is not implemented yet.

New driver is a complete rework to use the crypto ALGAPI instead of
hw_random API.  Rationale for the change:
1. hw_random interface is for true RNG devices.
2. The old driver was seeding itself with jiffies which is not a
   reliable source for randomness.
3. Device generates five random numbers in each pass but old driver was
   returning only one thus its performance was reduced.

Compatibility with DeviceTree bindings is preserved.

New driver does not use runtime power management but manually enables
and disables the clock when needed.  This is preferred approach because
using runtime PM just to toggle clock is huge overhead.  Another
difference is using the same seed after resuming from system suspend
(previously driver was re-seeding itself again with jiffies).

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 MAINTAINERS |   8 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 ---
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 306 
 7 files changed, 330 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index affecc6d59f4..371fda859d43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10977,6 +10977,14 @@ L: alsa-de...@alsa-project.org (moderated for 
non-subscribers)
 S: Supported
 F: sound/soc/samsung/
 
+SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
+M: Krzysztof Kozlowski <k...@kernel.org>
+L: linux-crypto@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org
+S: Maintained
+F: drivers/crypto/exynos-rng.c
+F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
+
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han <jingooh...@gmail.com>
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0cafe08919c9..bdae802e7154 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -294,20 +294,6 @@ config HW_RANDOM_POWERNV
 
  If unsure, say Y.
 
-config HW_RANDOM_EXYNOS
-   tristate "EXYNOS HW random number generator support"
-   depends on ARCH_EXYNOS || COMPILE_TEST
-   depends on HAS_IOMEM
-   default HW_RANDOM
-   ---help---
- This driver provides kernel-side support for the Random Number
- Generator hardware found on EXYNOS SOCs.
-
- To compile this driver as a module, choose M here: the
- module will be called exynos-rng.
-
- If unsure, say Y.
-
 config HW_RANDOM_TPM
tristate "TPM HW Random Number Generator support"
depends on TCG_TPM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e4e7be..6f1eecc2045c 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -24,7 +24,6 @@ obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
-obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
 obj-$(CONFIG_HW_RANDOM_HISI)   += hisi-rng.o
 obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
diff --git a/drivers/char/hw_random/exynos-rng.c 
b/drivers/char/hw_random/exynos-rng.c
deleted file mode 100644
index 23d358553b21..
--- a/drivers/char/hw_random/exynos-rng.c
+++ /dev/null
@@ -1,231 +0,0 @@
-/*
- * exynos-rng.c - Random Number Generator driver for the exynos
- *
- * Copyright (C) 2012 Samsung Electronics
- * Jonghwa Lee <jonghwa3@samsung.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation;
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- *
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#define 

[PATCH 0/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Hi,

This is a follow up of my questions around exynos-rng [1].

The existing exynos-rng has many issues.  The most important one is that
it is a pseudo RNG device but uses hw_random interface which does not allow
proper seeding.

The RNG module on Exynos4 requires seeding.  On newer SoCs (like Exynos5420)
it can seed itself from a true RNG.  Converting the existing driver
to use TRNG would effectively drop support for Exynos4 and break
compatibility with existing users.

Instead I decided to convert it to crypto API.  In the future I hope
to add support for seeding from TRNG module.

Tested with app [2].

Patches are independent. I will take the defconfig changes (2/3 and 3/3)
through samsung-soc tree.

Best regards,
Krzysztof

[1] https://www.spinics.net/lists/arm-kernel/msg569641.html
[2] https://www.spinics.net/lists/arm-kernel/msg571184.html

Krzysztof Kozlowski (3):
  crypto: hw_random - Add new Exynos RNG driver
  ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API
  ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

 MAINTAINERS |   8 +
 arch/arm/configs/exynos_defconfig   |   6 +
 arch/arm/configs/multi_v7_defconfig |   6 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 ---
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 306 
 9 files changed, 342 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

-- 
2.9.3

Re: Question - seeding the hw pseudo random number generator

[Krzysztof Kozlowski]

On Mon, Mar 20, 2017 at 09:28:58PM +0800, Herbert Xu wrote:
> On Mon, Mar 20, 2017 at 12:19:32PM +0530, PrasannaKumar Muralidharan wrote:
> >
> > AF_ALG interface for rng does have seeding support. I think hw_random
> > does not provide seeding support intentionally as I understand that
> > True RNG need not require seeding (please correct me if I am wrong).
> 
> Yes.  We should be converting PRNGs in hwrng over to algif_rng.

The actual hardware block can be seeded from true RNG (taking data
from thermal noise) so the solutions (if I understand correctly) for
exynos-rng might be:
1. Seed from internal TRNG making it a proper hwrandom device,
2. Convert to AF_ALG and seed with data from user-space through that
   interface.

Thanks for explanation, I'll queue it to my tasks list.

Best regards,
Krzysztof

Question - seeding the hw pseudo random number generator

[Krzysztof Kozlowski]

Hi,

I looked at Exynos Pseudo Random Nubmer Generator driver
(drivers/char/hw_random/exynos-rng.c) and noticed that it always seeds
the device with jiffies.  Then I looked at few other drivers and found
that they do not seed themself (or at least I couldn't find this).

I think the hw_random API does not provide generic infrastructure for
seeding.

What is the preferred approach for seeding a PRNG device? Use jiffies or
a fixed value?

Or maybe the interface should be abandoned in favor of crypto API?

Best regards,
Krzysztof

Re: [PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock

[Krzysztof Kozlowski]

On Fri, Mar 17, 2017 at 06:28:29PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Friday, March 17, 2017 04:49:22 PM Krzysztof Kozlowski wrote:
> > Driver uses threaded interrupt handler so there is no real need for
> > using spinlocks for synchronization.  Mutexes would do fine and are
> > friendlier for overall system preemptivness and real-time behavior.
> 
> Are you sure that this conversion is safe?  This driver also uses
> a tasklet and tasklets run in the interrupt context.
>

Yes, you're right. This is not safe and patch should be dropped. Thanks
for spotting this.

Best regards,
Krzysztof

[PATCH 2/4] crypto: s5p-sss - Remove unused variant field from state container

[Krzysztof Kozlowski]

The driver uses type of device (variant) only during probe so there is
no need to store it for later.

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/s5p-sss.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 6c620487e9c2..35ea84b7d775 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -190,8 +190,6 @@ struct s5p_aes_dev {
struct crypto_queue queue;
boolbusy;
spinlock_t  lock;
-
-   struct samsung_aes_variant  *variant;
 };
 
 static struct s5p_aes_dev *s5p_dev;
@@ -852,7 +850,6 @@ static int s5p_aes_probe(struct platform_device *pdev)
}
 
pdata->busy = false;
-   pdata->variant = variant;
pdata->dev = dev;
platform_set_drvdata(pdev, pdata);
s5p_dev = pdata;
-- 
2.9.3

[PATCH 1/4] crypto: s5p-sss - Close possible race for completed requests

[Krzysztof Kozlowski]

Driver is capable of handling only one request at a time and it stores
it in its state container struct s5p_aes_dev.  This stored request must be
protected between concurrent invocations (e.g. completing current
request and scheduling new one).  Combination of lock and "busy" field
is used for that purpose.

When "busy" field is true, the driver will not accept new request thus
it will not overwrite currently handled data.

However commit 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion
on LRW(AES)") moved some of the write to "busy" field out of a lock
protected critical section.  This might lead to potential race between
completing current request and scheduling a new one.  Effectively the
request completion might try to operate on new crypto request.

Cc: <sta...@vger.kernel.org> # v4.10.x
Fixes: 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)")
Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/s5p-sss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 1b9da3dc799b..6c620487e9c2 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -287,7 +287,6 @@ static void s5p_sg_done(struct s5p_aes_dev *dev)
 static void s5p_aes_complete(struct s5p_aes_dev *dev, int err)
 {
dev->req->base.complete(>req->base, err);
-   dev->busy = false;
 }
 
 static void s5p_unset_outdata(struct s5p_aes_dev *dev)
@@ -462,7 +461,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
spin_unlock_irqrestore(>lock, flags);
 
s5p_aes_complete(dev, 0);
-   dev->busy = true;
+   /* Device is still busy */
tasklet_schedule(>tasklet);
} else {
/*
@@ -483,6 +482,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
 
 error:
s5p_sg_done(dev);
+   dev->busy = false;
spin_unlock_irqrestore(>lock, flags);
s5p_aes_complete(dev, err);
 
@@ -634,6 +634,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, 
unsigned long mode)
 
 indata_error:
s5p_sg_done(dev);
+   dev->busy = false;
spin_unlock_irqrestore(>lock, flags);
s5p_aes_complete(dev, err);
 }
-- 
2.9.3

[PATCH 0/4] crypto: s5p-sss - Fix and minor improvements

[Krzysztof Kozlowski]

Hi,

I still did not fix the NULL pointer dereference reported by
Nathan Royce [1], but I got some other improvements.

Testing done on Odroid U3 (Exynos4412) with tcrypt and cryptsetup.

Best regards,
Krzysztof


[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1351149.html

Krzysztof Kozlowski (4):
  crypto: s5p-sss - Close possible race for completed requests
  crypto: s5p-sss - Remove unused variant field from state container
  crypto: s5p-sss - Document the struct s5p_aes_dev
  crypto: s5p-sss - Use mutex instead of spinlock

 drivers/crypto/s5p-sss.c | 70 +++-
 1 file changed, 45 insertions(+), 25 deletions(-)

-- 
2.9.3

[PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock

[Krzysztof Kozlowski]

Driver uses threaded interrupt handler so there is no real need for
using spinlocks for synchronization.  Mutexes would do fine and are
friendlier for overall system preemptivness and real-time behavior.

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/s5p-sss.c | 35 ---
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 7ac657f46d15..1893cf5dedc0 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -214,7 +215,7 @@ struct s5p_aes_dev {
struct tasklet_struct   tasklet;
struct crypto_queue queue;
boolbusy;
-   spinlock_t  lock;
+   struct mutexlock;
 };
 
 static struct s5p_aes_dev *s5p_dev;
@@ -443,11 +444,10 @@ static irqreturn_t s5p_aes_interrupt(int irq, void 
*dev_id)
int err_dma_tx = 0;
int err_dma_rx = 0;
bool tx_end = false;
-   unsigned long flags;
uint32_t status;
int err;
 
-   spin_lock_irqsave(>lock, flags);
+   mutex_lock(>lock);
 
/*
 * Handle rx or tx interrupt. If there is still data (scatterlist did 
not
@@ -481,7 +481,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
if (tx_end) {
s5p_sg_done(dev);
 
-   spin_unlock_irqrestore(>lock, flags);
+   mutex_unlock(>lock);
 
s5p_aes_complete(dev, 0);
/* Device is still busy */
@@ -498,7 +498,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
if (err_dma_rx == 1)
s5p_set_dma_indata(dev, dev->sg_src);
 
-   spin_unlock_irqrestore(>lock, flags);
+   mutex_unlock(>lock);
}
 
return IRQ_HANDLED;
@@ -506,7 +506,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
 error:
s5p_sg_done(dev);
dev->busy = false;
-   spin_unlock_irqrestore(>lock, flags);
+   mutex_unlock(>lock);
s5p_aes_complete(dev, err);
 
return IRQ_HANDLED;
@@ -599,7 +599,6 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, 
unsigned long mode)
 {
struct ablkcipher_request *req = dev->req;
uint32_t aes_control;
-   unsigned long flags;
int err;
 
aes_control = SSS_AES_KEY_CHANGE_MODE;
@@ -625,7 +624,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, 
unsigned long mode)
|  SSS_AES_BYTESWAP_KEY
|  SSS_AES_BYTESWAP_CNT;
 
-   spin_lock_irqsave(>lock, flags);
+   mutex_lock(>lock);
 
SSS_WRITE(dev, FCINTENCLR,
  SSS_FCINTENCLR_BTDMAINTENCLR | SSS_FCINTENCLR_BRDMAINTENCLR);
@@ -648,7 +647,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, 
unsigned long mode)
SSS_WRITE(dev, FCINTENSET,
  SSS_FCINTENSET_BTDMAINTENSET | SSS_FCINTENSET_BRDMAINTENSET);
 
-   spin_unlock_irqrestore(>lock, flags);
+   mutex_unlock(>lock);
 
return;
 
@@ -658,7 +657,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, 
unsigned long mode)
 indata_error:
s5p_sg_done(dev);
dev->busy = false;
-   spin_unlock_irqrestore(>lock, flags);
+   mutex_unlock(>lock);
s5p_aes_complete(dev, err);
 }
 
@@ -667,18 +666,17 @@ static void s5p_tasklet_cb(unsigned long data)
struct s5p_aes_dev *dev = (struct s5p_aes_dev *)data;
struct crypto_async_request *async_req, *backlog;
struct s5p_aes_reqctx *reqctx;
-   unsigned long flags;
 
-   spin_lock_irqsave(>lock, flags);
+   mutex_lock(>lock);
backlog   = crypto_get_backlog(>queue);
async_req = crypto_dequeue_request(>queue);
 
if (!async_req) {
dev->busy = false;
-   spin_unlock_irqrestore(>lock, flags);
+   mutex_unlock(>lock);
return;
}
-   spin_unlock_irqrestore(>lock, flags);
+   mutex_unlock(>lock);
 
if (backlog)
backlog->complete(backlog, -EINPROGRESS);
@@ -693,18 +691,17 @@ static void s5p_tasklet_cb(unsigned long data)
 static int s5p_aes_handle_req(struct s5p_aes_dev *dev,
  struct ablkcipher_request *req)
 {
-   unsigned long flags;
int err;
 
-   spin_lock_irqsave(>lock, flags);
+   mutex_lock(>lock);
err = ablkcipher_enqueue_request(>queue, req);
if (dev->busy) {
-   spin_unlock_irqrestore(>lock, flags);
+   mutex_unlock(>lock);
goto exit;
}
dev->busy = true;
 
-   spin_unlock_irqrestore(>lock, flags);
+ 

[PATCH 3/4] crypto: s5p-sss - Document the struct s5p_aes_dev

[Krzysztof Kozlowski]

Add kernel-doc to s5p_aes_dev structure.

Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
---
 drivers/crypto/s5p-sss.c | 27 ++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 35ea84b7d775..7ac657f46d15 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -170,6 +170,32 @@ struct s5p_aes_ctx {
int keylen;
 };
 
+/**
+ * struct s5p_aes_dev - Crypto device state container
+ * @dev:   Associated device
+ * @clk:   Clock for accessing hardware
+ * @ioaddr:Mapped IO memory region
+ * @aes_ioaddr:Per-varian offset for AES block IO memory
+ * @irq_fc:Feed control interrupt line
+ * @req:   Crypto request currently handled by the device
+ * @ctx:   Configuration for currently handled crypto request
+ * @sg_src:Scatter list with source data for currently handled block
+ * in device.  This is DMA-mapped into device.
+ * @sg_dst:Scatter list with destination data for currently handled block
+ * in device. This is DMA-mapped into device.
+ * @sg_src_cpy:In case of unaligned access, copied scatter list
+ * with source data.
+ * @sg_dst_cpy:In case of unaligned access, copied scatter list
+ * with destination data.
+ * @tasklet:   New request scheduling jib
+ * @queue: Crypto queue
+ * @busy:  Indicates whether the device is currently handling some request
+ * thus it uses some of the fields from this state, like:
+ * req, ctx, sg_src/dst (and copies).  This essentially
+ * 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).
+ */
 struct s5p_aes_dev {
struct device   *dev;
struct clk  *clk;
@@ -182,7 +208,6 @@ struct s5p_aes_dev {
struct scatterlist  *sg_src;
struct scatterlist  *sg_dst;
 
-   /* In case of unaligned access: */
struct scatterlist  *sg_src_cpy;
struct scatterlist  *sg_dst_cpy;
 
-- 
2.9.3

Re: XTS Crypto Not Found In /proc/crypto Even After Compiled for 4.10.1.

[Krzysztof Kozlowski]

On Sun, Mar 12, 2017 at 09:13:22PM +0200, Krzysztof Kozlowski wrote:
> On Fri, Mar 10, 2017 at 03:44:45PM -0600, Nathan Royce wrote:
> > Sure, I went ahead and rebuilt it just using the bare exynos_defconfig
> > and adding XTS and ECB and no other changes.
> > 
> > No flags were used. No patches were used other than the 2 you
> > provided. Just the barest of bears, the barest of bones, the barest of
> > deserts, the barest of hairless cats.
> >
> 
> Okay, I reproduced it. Beside enabling crypto tests, ECB and XTS, the
> important step is to disable the "ARM Accelerated Cryptographic
> Algorithms" so S5P-SSS will be used with XTS. The xts(ecb-aes-s5p))
> itself passes TCRYPT tests but oopses on cryptswap.

Hi Herbert,

I bisected this to commit f1c131b45410 ("crypto: xts - Convert to
skcipher"). The s5p-sss driver stays the same... but the xts changes and
as a result we have a NULL pointer dereference (actually of value
0004):
[   18.930195] Unable to handle kernel NULL pointer dereference at virtual 
address 0004
...
[   18.972325] [] (post_crypt) from [] 
(decrypt_done+0x4c/0x54)
[   18.972343] [] (decrypt_done) from [] 
(s5p_aes_interrupt+0x1bc/0x208)
[   18.972360] [] (s5p_aes_interrupt) from [] 
(irq_thread_fn+0x1c/0x54)

Any hints?

Best regards,
Krzysztof