[PATCH 0/3] Assorted changes for Exynos PRNG driver

[PATCH 0/3] Assorted changes for Exynos PRNG driver

[Łukasz Stelmach]

Hello,

This is a series of patches for exynos-rng driver I've decided to
create after adding support for Exynos5250+ chips. They do not
strictly depend on each other, but I think it is better to send them
as a single patch-set.

Patch #1 Add support for PRNG in Exynos5250+ SoCs

Patch #2 Improve output performance by using memcpy() rather than a
custom function to retriev random bytes from registers.

Patch #3 Reseed the PRNG after reading 2^16 bytes. Simmilar approach
is implemented in DRBG. (Thanks Stephan Mueller)

Łukasz Stelmach (3):
  crypto: exynos - Support Exynos5250+ SoCs
  crypto: exynos - Improve performance of PRNG
  crypto: exynos - Reseed PRNG after generating 2^16 random bytes

 .../bindings/crypto/samsung,exynos-rng4.txt        |  4 +-
 drivers/crypto/exynos-rng.c                        | 90 +++++++++++++---------
 2 files changed, 55 insertions(+), 39 deletions(-)

-- 
2.11.0

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

[Łukasz Stelmach]

Add support for PRNG in Exynos5250+ SoCs.

Signed-off-by: Łukasz Stelmach <l.stelmach@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 <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 
 #include <crypto/internal/rng.h>
 
 #define EXYNOS_RNG_CONTROL		0x0
 #define EXYNOS_RNG_STATUS		0x10
+
+#define EXYNOS_RNG_SEED_CONF		0x14
+#define EXYNOS_RNG_GEN_PRNG		0x02
+
 #define EXYNOS_RNG_SEED_BASE		0x140
 #define EXYNOS_RNG_SEED(n)		(EXYNOS_RNG_SEED_BASE + (n * 0x4))
 #define EXYNOS_RNG_OUT_BASE		0x160
@@ -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,
+};
+
 /*
  * 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(&pdev->dev);
+	if (rng->type != EXYNOS_PRNG_TYPE4 &&
+	    rng->type != EXYNOS_PRNG_TYPE5) {
+		dev_err(&pdev->dev, "Unsupported PRNG type: %d", rng->type);
+		return -ENOTSUPP;
+	}
+
 	rng->dev = &pdev->dev;
 	rng->clk = devm_clk_get(&pdev->dev, "secss");
 	if (IS_ERR(rng->clk)) {
@@ -300,7 +323,10 @@ static int exynos_rng_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev,
 			"Couldn't register rng crypto alg: %d\n", ret);
 		exynos_rng_dev = NULL;
-	}
+	} else
+		dev_info(&pdev->dev,
+			 "Exynos Pseudo Random Number Generator (type:%d)\n",
+			 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 = "samsung,exynos4-rng",
+		.data = (const void *)EXYNOS_PRNG_TYPE4,
+	}, {
+		.compatible = "samsung,exynos5250-prng",
+		.data = (const void *)EXYNOS_PRNG_TYPE5,
 	},
 	{ },
 };
-- 
2.11.0

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

[Ł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.stelmach@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
@@ -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);
+
 	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);
 
 	if (!retry)
 		return -ETIMEDOUT;
@@ -189,7 +163,7 @@ 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);
+	memcpy_fromio(dst, rng->mem + EXYNOS_RNG_OUT_BASE, *read);
 
 	return 0;
 }
-- 
2.11.0

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

[Łukasz Stelmach]

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

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 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_BYTES		65536
+
+
 /*
  * In polling mode, do not wait infinitely for the engine to finish the work.
  */
@@ -81,6 +84,8 @@ struct exynos_rng_dev {
 	unsigned int			seed_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;
+
+
 }
 
 /* Re-seed itself from time to time */
@@ -177,7 +185,8 @@ static void exynos_rng_reseed(struct exynos_rng_dev *rng)
 	unsigned int read = 0;
 	u8 seed[EXYNOS_RNG_SEED_SIZE];
 
-	if (time_before(now, next_seeding))
+	if (time_before(now, next_seeding) &&
+	    rng->bytes_seeding < EXYNOS_RNG_RESEED_BYTES)
 		return;
 
 	if (exynos_rng_get_random(rng, seed, sizeof(seed), &read))
@@ -206,6 +215,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
 
 		dlen -= read;
 		dst += read;
+		rng->bytes_seeding += read;
 
 		exynos_rng_reseed(rng);
 	} while (dlen > 0);
-- 
2.11.0

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

[Krzysztof Kozlowski]

On Tue, Dec 5, 2017 at 1:35 PM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
> Add support for PRNG in Exynos5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelmach@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 <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>
>  #include <crypto/internal/rng.h>
>
>  #define EXYNOS_RNG_CONTROL             0x0
>  #define EXYNOS_RNG_STATUS              0x10
> +
> +#define EXYNOS_RNG_SEED_CONF           0x14
> +#define EXYNOS_RNG_GEN_PRNG            0x02

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_BASE            0x160
> @@ -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(&pdev->dev);
> +       if (rng->type != EXYNOS_PRNG_TYPE4 &&
> +           rng->type != EXYNOS_PRNG_TYPE5) {
> +               dev_err(&pdev->dev, "Unsupported PRNG type: %d", rng->type);
> +               return -ENOTSUPP;
> +       }
> +
>         rng->dev = &pdev->dev;
>         rng->clk = devm_clk_get(&pdev->dev, "secss");
>         if (IS_ERR(rng->clk)) {
> @@ -300,7 +323,10 @@ static int exynos_rng_probe(struct platform_device *pdev)
>                 dev_err(&pdev->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(&pdev->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 = "samsung,exynos4-rng",
> +               .data = (const void *)EXYNOS_PRNG_TYPE4,
> +       }, {
> +               .compatible = "samsung,exynos5250-prng",
> +               .data = (const void *)EXYNOS_PRNG_TYPE5,
>         },
>         { },
>  };
> --
> 2.11.0
>

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

[Krzysztof Kozlowski]

On Tue, Dec 5, 2017 at 1:35 PM, Łukasz Stelmach <l.stelmach@samsung.com> 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 <l.stelmach@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
> @@ -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 3/3] crypto: exynos - Reseed PRNG after generating 2^16 random bytes

[Stephan Mueller]

Am Dienstag, 5. Dezember 2017, 13:35:58 CET schrieb Łukasz Stelmach:

Hi Łukasz,

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

Please add to the log that you also increase the timer-based reseed to 1 
second?!

Another suggestion: maybe you want to add a comment to the reseed function to 
indicate it is for enhanced backtracking resistance. Otherwise a lot of folks 
would scratch their head why such code exists in the first place. :-)

Other than that:

Reviewed-by: Stephan Mueller <smueller@chronox.de>

Ciao
Stephan

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

[Stephan Mueller]

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.stelmach@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
> @@ -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);
> +
>  	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);

Is this related to the patch?
> 
>  	if (!retry)
>  		return -ETIMEDOUT;
> @@ -189,7 +163,7 @@ 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);
> +	memcpy_fromio(dst, rng->mem + EXYNOS_RNG_OUT_BASE, *read);
> 
>  	return 0;
>  }



Ciao
Stephan

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 <l.stelmach@samsung.com> 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 <l.stelmach@samsung.com>
> ---
>  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_BYTES                65536
> +
> +

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 int                    seed_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

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 2828 bytes --]

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.stelmach@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.

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 

Thank you for asking the questions. I will put the above explanations in
the commit message.

>> 
>>  	if (!retry)
>>  		return -ETIMEDOUT;
>> @@ -189,7 +163,7 @@ 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);
>> +	memcpy_fromio(dst, rng->mem + EXYNOS_RNG_OUT_BASE, *read);
>> 
>>  	return 0;
>>  }

Kind regards,
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

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 <l.stelmach@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.

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 <krzk@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.stelmach@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 2/3] crypto: exynos - Improve performance of PRNG

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 3545 bytes --]

It was <2017-12-05 wto 19:06>, when Krzysztof Kozlowski wrote:
> On Tue, Dec 5, 2017 at 6:53 PM, Krzysztof Kozlowski <krzk@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.stelmach@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);

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

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

[Krzysztof Kozlowski]

On Wed, Dec 6, 2017 at 12:32 PM, Łukasz Stelmach <l.stelmach@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 <krzk@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.stelmach@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

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 4614 bytes --]

It was <2017-12-06 śro 12:37>, when Krzysztof Kozlowski wrote:
> On Wed, Dec 6, 2017 at 12:32 PM, Łukasz Stelmach <l.stelmach@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 <krzk@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.stelmach@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).

Without cpu_relax() retry is decremented twice. So there are three
reads. It appears that a single call cpu_relax() gives the hardware
enough time to be ready when the first read occurs (retry is 99 after
the loop).

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

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

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 5899 bytes --]

It was <2017-12-05 wto 14:34>, when Krzysztof Kozlowski wrote:
> On Tue, Dec 5, 2017 at 1:35 PM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
>> Add support for PRNG in Exynos5250+ SoCs.
>>
>> Signed-off-by: Łukasz Stelmach <l.stelmach@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 <linux/err.h>
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>> +#include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>>
>>  #include <crypto/internal/rng.h>
>>
>>  #define EXYNOS_RNG_CONTROL             0x0
>>  #define EXYNOS_RNG_STATUS              0x10
>> +
>> +#define EXYNOS_RNG_SEED_CONF           0x14
>> +#define EXYNOS_RNG_GEN_PRNG            0x02
>
> 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_BASE            0x160
>> @@ -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.

>> +};
>> +
>>  /*
>>   * 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(&pdev->dev);
>> +       if (rng->type != EXYNOS_PRNG_TYPE4 &&
>> +           rng->type != EXYNOS_PRNG_TYPE5) {
>> +               dev_err(&pdev->dev, "Unsupported PRNG type: %d", rng->type);
>> +               return -ENOTSUPP;
>> +       }
>> +
>>         rng->dev = &pdev->dev;
>>         rng->clk = devm_clk_get(&pdev->dev, "secss");
>>         if (IS_ERR(rng->clk)) {
>> @@ -300,7 +323,10 @@ static int exynos_rng_probe(struct platform_device *pdev)
>>                 dev_err(&pdev->dev,
>>                         "Couldn't register rng crypto alg: %d\n", ret);
>>                 exynos_rng_dev = NULL;
>> -       }
>> +       } else
>
> Missing {} around else clause. Probably checkpatch should point it.

It doesn't. Fixed.

>> +               dev_info(&pdev->dev,
>> +                        "Exynos Pseudo Random Number Generator (type:%d)\n",
>
> dev_dbg, this is not that important information to affect the boot time.

Quite many devices report their presence during boot with such
messages. For example:

[    3.390247] exynos-ehci 12110000.usb: EHCI Host Controller
[    3.395493] exynos-ehci 12110000.usb: new USB bus registered, assigned bus number 1
[    3.403702] exynos-ehci 12110000.usb: irq 80, io mem 0x12110000
[    3.431793] exynos-ehci 12110000.usb: USB 2.0 started, EHCI 1.00

From my experience it isn't printk() itself that slows down boot but the
serial console.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

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

[Krzysztof Kozlowski]

On Wed, Dec 6, 2017 at 2:42 PM, Łukasz Stelmach <l.stelmach@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.stelmach@samsung.com> wrote:
>>> Add support for PRNG in Exynos5250+ SoCs.
>>>
>>> Signed-off-by: Łukasz Stelmach <l.stelmach@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 <linux/err.h>
>>>  #include <linux/io.h>
>>>  #include <linux/module.h>
>>> +#include <linux/of_device.h>
>>>  #include <linux/platform_device.h>
>>>
>>>  #include <crypto/internal/rng.h>
>>>
>>>  #define EXYNOS_RNG_CONTROL             0x0
>>>  #define EXYNOS_RNG_STATUS              0x10
>>> +
>>> +#define EXYNOS_RNG_SEED_CONF           0x14
>>> +#define EXYNOS_RNG_GEN_PRNG            0x02
>>
>> 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_BASE            0x160
>>> @@ -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) {
>>> +               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(&pdev->dev);
>>> +       if (rng->type != EXYNOS_PRNG_TYPE4 &&
>>> +           rng->type != EXYNOS_PRNG_TYPE5) {
>>> +               dev_err(&pdev->dev, "Unsupported PRNG type: %d", rng->type);
>>> +               return -ENOTSUPP;
>>> +       }
>>> +
>>>         rng->dev = &pdev->dev;
>>>         rng->clk = devm_clk_get(&pdev->dev, "secss");
>>>         if (IS_ERR(rng->clk)) {
>>> @@ -300,7 +323,10 @@ static int exynos_rng_probe(struct platform_device *pdev)
>>>                 dev_err(&pdev->dev,
>>>                         "Couldn't register rng crypto alg: %d\n", ret);
>>>                 exynos_rng_dev = NULL;
>>> -       }
>>> +       } else
>>
>> Missing {} around else clause. Probably checkpatch should point it.
>
> It doesn't. Fixed.
>
>>> +               dev_info(&pdev->dev,
>>> +                        "Exynos Pseudo Random Number Generator (type:%d)\n",
>>
>> dev_dbg, this is not that important information to affect the boot time.
>
> Quite many devices report their presence during boot with such
> messages. For example:
>
> [    3.390247] exynos-ehci 12110000.usb: EHCI Host Controller
> [    3.395493] exynos-ehci 12110000.usb: new USB bus registered, assigned bus number 1
> [    3.403702] exynos-ehci 12110000.usb: irq 80, io mem 0x12110000
> [    3.431793] exynos-ehci 12110000.usb: USB 2.0 started, EHCI 1.00
>
> From my experience it isn't printk() itself that slows down boot but the
> serial console.

True, the console is bottleneck (not necessarily serial) [1] but that
does not change the fact there is no need to print the type of RNG.
Before the device was not printing its presence and by adding support
for different flavor, you are adding the printk (not changing existing
printk). This driver is not that special to inform about flavor being
used especially that it is obvious from Exynos type (which comes from
board compatible). The example above prints resources so it brings
some information (not that useful but that is other point)... Really,
if every our driver started to inform how important he is, then the
info-level log would be polluted with a lot of useless printks. With
exynos-bus and exynos-nocp/event you can already find ridiculous
amount of useless messages.

[1] https://lwn.net/Articles/737822/

Best regards,
Krzysztof

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

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 5251 bytes --]

It was <2017-12-06 śro 15:05>, when Krzysztof Kozlowski wrote:
> On Wed, Dec 6, 2017 at 2:42 PM, Łukasz Stelmach <l.stelmach@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.stelmach@samsung.com> wrote:
>>>> Add support for PRNG in Exynos5250+ SoCs.
>>>>
>>>> Signed-off-by: Łukasz Stelmach <l.stelmach@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 <linux/err.h>
>>>>  #include <linux/io.h>
>>>>  #include <linux/module.h>
>>>> +#include <linux/of_device.h>
>>>>  #include <linux/platform_device.h>
>>>>
>>>>  #include <crypto/internal/rng.h>
>>>>
>>>>  #define EXYNOS_RNG_CONTROL             0x0
>>>>  #define EXYNOS_RNG_STATUS              0x10
>>>> +
>>>> +#define EXYNOS_RNG_SEED_CONF           0x14
>>>> +#define EXYNOS_RNG_GEN_PRNG            0x02
>>>
>>> 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_BASE            0x160
>>>> @@ -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 those
numbers would be OK.

> 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 :) ).

Fair enough.

>>>> +               dev_info(&pdev->dev,
>>>> +                        "Exynos Pseudo Random Number Generator (type:%d)\n",
>>>
>>> dev_dbg, this is not that important information to affect the boot time.
>>
>> Quite many devices report their presence during boot with such
>> messages. For example:
>>
>> [    3.390247] exynos-ehci 12110000.usb: EHCI Host Controller
>> [    3.395493] exynos-ehci 12110000.usb: new USB bus registered, assigned bus number 1
>> [    3.403702] exynos-ehci 12110000.usb: irq 80, io mem 0x12110000
>> [    3.431793] exynos-ehci 12110000.usb: USB 2.0 started, EHCI 1.00
>>
>> From my experience it isn't printk() itself that slows down boot but the
>> serial console.
>
> True, the console is bottleneck (not necessarily serial) [1] but that
> does not change the fact there is no need to print the type of RNG.

With values of the enum not being meaningful themselves printing the
type does not make much sense for me too. Is it ok just to print report
the device presence?

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

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

[Krzysztof Kozlowski]

On Wed, Dec 6, 2017 at 3:53 PM, Łukasz Stelmach <l.stelmach@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.stelmach@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.stelmach@samsung.com> wrote:
>>>>> Add support for PRNG in Exynos5250+ SoCs.
>>>>>
>>>>> Signed-off-by: Łukasz Stelmach <l.stelmach@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 <linux/err.h>
>>>>>  #include <linux/io.h>
>>>>>  #include <linux/module.h>
>>>>> +#include <linux/of_device.h>
>>>>>  #include <linux/platform_device.h>
>>>>>
>>>>>  #include <crypto/internal/rng.h>
>>>>>
>>>>>  #define EXYNOS_RNG_CONTROL             0x0
>>>>>  #define EXYNOS_RNG_STATUS              0x10
>>>>> +
>>>>> +#define EXYNOS_RNG_SEED_CONF           0x14
>>>>> +#define EXYNOS_RNG_GEN_PRNG            0x02
>>>>
>>>> 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_BASE            0x160
>>>>> @@ -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 those
> numbers would be OK.

It does not scale because we do not know what chips could be added...
and we might for example discover minor differences between 4210 and
4412 so you would have:
EXYNOS4 = 4
EXYNOS4412 = 5
EXYNOS5 = 6
or some other combinations confusing the reader in the future. Instead
just do not open-code the numbers because you do not need them (I mean
you do not need the exact values).

>
>> 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 :) ).
>
> Fair enough.

Great!

>
>>>>> +               dev_info(&pdev->dev,
>>>>> +                        "Exynos Pseudo Random Number Generator (type:%d)\n",
>>>>
>>>> dev_dbg, this is not that important information to affect the boot time.
>>>
>>> Quite many devices report their presence during boot with such
>>> messages. For example:
>>>
>>> [    3.390247] exynos-ehci 12110000.usb: EHCI Host Controller
>>> [    3.395493] exynos-ehci 12110000.usb: new USB bus registered, assigned bus number 1
>>> [    3.403702] exynos-ehci 12110000.usb: irq 80, io mem 0x12110000
>>> [    3.431793] exynos-ehci 12110000.usb: USB 2.0 started, EHCI 1.00
>>>
>>> From my experience it isn't printk() itself that slows down boot but the
>>> serial console.
>>
>> True, the console is bottleneck (not necessarily serial) [1] but that
>> does not change the fact there is no need to print the type of RNG.
>
> With values of the enum not being meaningful themselves printing the
> type does not make much sense for me too. Is it ok just to print report
> the device presence?

This does not change mine arguments - this printk does not bring any
information except that there is such device. Later you might want to
print this for Exynos TRNG. Later for other module, and other. Thus I
prefer not to add it. On the other hand that is not a reason to stop
this patch so for example you could split it into separate commit. It
is kind of unrelated change and keeping it separate will not block
everything if maintainer will not want it.

Besr regards,
Krzysztof

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

[Joe Perches]

On Wed, 2017-12-06 at 15:05 +0100, Krzysztof Kozlowski wrote:
> On Wed, Dec 6, 2017 at 2:42 PM, Łukasz Stelmach <l.stelmach@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.stelmach@samsung.com> wrote:
> > > > Add support for PRNG in Exynos5250+ SoCs.
[]
> > > > diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
[]
> > > > @@ -300,7 +323,10 @@ static int exynos_rng_probe(struct platform_device *pdev)
> > > >                 dev_err(&pdev->dev,
> > > >                         "Couldn't register rng crypto alg: %d\n", ret);
> > > >                 exynos_rng_dev = NULL;
> > > > -       }
> > > > +       } else
> > > 
> > > Missing {} around else clause. Probably checkpatch should point it.
> > 
> > It doesn't. Fixed.

checkpatch does report this if using --strict

$ ./scripts/checkpatch.pl --strict -
CHECK: Unbalanced braces around else statement
#119: FILE: drivers/crypto/exynos-rng.c:326:
+	} else

Arguably, this should always be reported.

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

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

It was <2017-12-06 śro 16:28>, when Krzysztof Kozlowski wrote:
> On Wed, Dec 6, 2017 at 3:53 PM, Łukasz Stelmach <l.stelmach@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.stelmach@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.stelmach@samsung.com> wrote:
>>>>>> Add support for PRNG in Exynos5250+ SoCs.
>>>>>>
>>>>>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>>>>>> ---

[...]

>>>>>> +               dev_info(&pdev->dev,
>>>>>> +                        "Exynos Pseudo Random Number Generator (type:%d)\n",
>>>>>
>>>>> dev_dbg, this is not that important information to affect the boot time.
>>>>
>>>> Quite many devices report their presence during boot with such
>>>> messages. For example:
>>>>
>>>> [    3.390247] exynos-ehci 12110000.usb: EHCI Host Controller
>>>> [    3.395493] exynos-ehci 12110000.usb: new USB bus registered, assigned bus number 1
>>>> [    3.403702] exynos-ehci 12110000.usb: irq 80, io mem 0x12110000
>>>> [    3.431793] exynos-ehci 12110000.usb: USB 2.0 started, EHCI 1.00
>>>>
>>>> From my experience it isn't printk() itself that slows down boot but the
>>>> serial console.
>>>
>>> True, the console is bottleneck (not necessarily serial) [1] but that
>>> does not change the fact there is no need to print the type of RNG.
>>
>> With values of the enum not being meaningful themselves printing the
>> type does not make much sense for me too. Is it ok just to print report
>> the device presence?
>
> This does not change mine arguments - this printk does not bring any
> information except that there is such device. Later you might want to
> print this for Exynos TRNG. Later for other module, and other. Thus I
> prefer not to add it. On the other hand that is not a reason to stop
> this patch so for example you could split it into separate commit.

Done.

> It is kind of unrelated change and keeping it separate will not block
> everything if maintainer will not want it.

I will. As a user a prefer when devices report their presence in the log
buffer and I will submit such patch.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

[PATCH v2 0/4] Assorted changes for Exynos PRNG driver

[Łukasz Stelmach]

Hello,

This is a series of patches for exynos-rng driver I've decided to
create after adding support for Exynos5250+ chips. They do not
strictly depend on each other, but I think it is better to send them
as a single patch-set.

The driver requires appropriate DT configuration introduced in

    https://patchwork.kernel.org/patch/10104445/

Patch #1 Add support for PRNG in Exynos5250+ SoCs

Patch #2 Improve output performance by using memcpy() rather than a
custom function to retrieve random bytes from registers. Rearrange
the loop for polling the hardware.

Patch #3 Reseed the PRNG after reading 2^16 bytes. Simmilar approach
is implemented in DRBG. (Thanks Stephan Mueller)

Patch #4 Introduce locking to prevent simultaneous access to the
hardware from more than one thread/process.

Changes since v1:

- Added Patch #4.
- Extended commit message for patch #3.
- Changed exynos_prng_type enum and a define according to Krzysztof Kozłowski's
  recommendations.
- Brought back cpu_relax() in a rearranged loop in
  exynos_rng_get_random().
- Moved an assignment of the read valuea away from an error path.
- Removed dev_info() reporting hardware presence from
  exynos_rng_probe().

Łukasz Stelmach (4):
  crypto: exynos - Support Exynos5250+ SoCs
  crypto: exynos - Improve performance of PRNG
  crypto: exynos - Reseed PRNG after generating 2^16 random bytes
  crypto: exynos - Introduce mutex to prevent concurrent access to
    hardware

 .../bindings/crypto/samsung,exynos-rng4.txt        |   4 +-
 drivers/crypto/exynos-rng.c                        | 104 +++++++++++++--------
 2 files changed, 70 insertions(+), 38 deletions(-)

-- 
2.11.0

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

[Łukasz Stelmach]

Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Add support for PRNG in Exynos5250+ SoCs.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 .../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 <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 
 #include <crypto/internal/rng.h>
 
 #define EXYNOS_RNG_CONTROL		0x0
 #define EXYNOS_RNG_STATUS		0x10
+
+#define EXYNOS_RNG_SEED_CONF		0x14
+#define EXYNOS_RNG_GEN_PRNG	        BIT(1)
+
 #define EXYNOS_RNG_SEED_BASE		0x140
 #define EXYNOS_RNG_SEED(n)		(EXYNOS_RNG_SEED_BASE + (n * 0x4))
 #define EXYNOS_RNG_OUT_BASE		0x160
@@ -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(&pdev->dev);
+	if (rng->type != EXYNOS_PRNG_EXYNOS4 &&
+	    rng->type != EXYNOS_PRNG_EXYNOS5) {
+		dev_err(&pdev->dev, "Unsupported PRNG type: %d", rng->type);
+		return -ENOTSUPP;
+	}
+
 	rng->dev = &pdev->dev;
 	rng->clk = devm_clk_get(&pdev->dev, "secss");
 	if (IS_ERR(rng->clk)) {
@@ -367,6 +391,10 @@ static SIMPLE_DEV_PM_OPS(exynos_rng_pm_ops, exynos_rng_suspend,
 static const struct of_device_id exynos_rng_dt_match[] = {
 	{
 		.compatible = "samsung,exynos4-rng",
+		.data = (const void *)EXYNOS_PRNG_EXYNOS4,
+	}, {
+		.compatible = "samsung,exynos5250-prng",
+		.data = (const void *)EXYNOS_PRNG_EXYNOS5,
 	},
 	{ },
 };
-- 
2.11.0

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

[Łukasz Stelmach]

Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

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). 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. On the other hand, performance in this
arrangement isn't much worse than in a loop without cpu_relax().

Signed-off-by: Łukasz Stelmach <l.stelmach@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 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

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

[Łukasz Stelmach]

Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

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

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.stelmach@samsung.com>
Reviewed-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/exynos-rng.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
index 7d8f658480d3..c72a838f1932 100644
--- a/drivers/crypto/exynos-rng.c
+++ b/drivers/crypto/exynos-rng.c
@@ -55,12 +55,14 @@ 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_BYTES		65536
+
 /*
  * In polling mode, do not wait infinitely for the engine to finish the work.
  */
@@ -82,6 +84,8 @@ struct exynos_rng_dev {
 	unsigned int			seed_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;
@@ -126,6 +130,7 @@ static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
 	}
 
 	rng->last_seeding = jiffies;
+	rng->bytes_seeding = 0;
 
 	return 0;
 }
@@ -165,6 +170,7 @@ static int exynos_rng_get_random(struct exynos_rng_dev *rng,
 			  EXYNOS_RNG_STATUS);
 	*read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
 	memcpy_fromio(dst, rng->mem + EXYNOS_RNG_OUT_BASE, *read);
+	rng->bytes_seeding += *read;
 
 	return 0;
 }
@@ -178,7 +184,8 @@ static void exynos_rng_reseed(struct exynos_rng_dev *rng)
 	unsigned int read = 0;
 	u8 seed[EXYNOS_RNG_SEED_SIZE];
 
-	if (time_before(now, next_seeding))
+	if (time_before(now, next_seeding) &&
+	    rng->bytes_seeding < EXYNOS_RNG_RESEED_BYTES)
 		return;
 
 	if (exynos_rng_get_random(rng, seed, sizeof(seed), &read))
-- 
2.11.0

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

[Łukasz Stelmach]

Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@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.stelmach@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 <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 
@@ -79,6 +80,7 @@ struct exynos_rng_dev {
 	enum exynos_prng_type		type;
 	void __iomem			*mem;
 	struct clk			*clk;
+	struct mutex 			lock;
 	/* Generated numbers stored for seeding during resume */
 	u8				seed_save[EXYNOS_RNG_SEED_SIZE];
 	unsigned int			seed_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(&rng->lock);
+	mutex_lock(&rng->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(&rng->lock);
 	do {
 		ret = exynos_rng_get_random(rng, dst, dlen, &read);
 		if (ret)
@@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
 
 		exynos_rng_reseed(rng);
 	} while (dlen > 0);
+	mutex_unlock(&rng->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(&rng->lock);
 	ret = exynos_rng_set_seed(ctx->rng, seed, slen);
+	mutex_unlock(&rng->lock);
 
 	clk_disable_unprepare(rng->clk);
 
@@ -284,6 +294,8 @@ static int exynos_rng_probe(struct platform_device *pdev)
 		return -ENOTSUPP;
 	}
 
+	mutex_init(&rng->lock);
+
 	rng->dev = &pdev->dev;
 	rng->clk = devm_clk_get(&pdev->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(&rng->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(&rng->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(&rng->lock);
+
 	ret = exynos_rng_set_seed(rng, rng->seed_save, rng->seed_save_len);
 
+	mutex_unlock(&rng->lock);
+
 	clk_disable_unprepare(rng->clk);
 
 	return ret;
-- 
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 <l.stelmach@samsung.com> wrote:
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>
> Add support for PRNG in Exynos5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  .../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 <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>
>  #include <crypto/internal/rng.h>
>
>  #define EXYNOS_RNG_CONTROL             0x0
>  #define EXYNOS_RNG_STATUS              0x10
> +
> +#define EXYNOS_RNG_SEED_CONF           0x14
> +#define EXYNOS_RNG_GEN_PRNG            BIT(1)
> +
>  #define EXYNOS_RNG_SEED_BASE           0x140
>  #define EXYNOS_RNG_SEED(n)             (EXYNOS_RNG_SEED_BASE + (n * 0x4))
>  #define EXYNOS_RNG_OUT_BASE            0x160
> @@ -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(&pdev->dev);
> +       if (rng->type != EXYNOS_PRNG_EXYNOS4 &&
> +           rng->type != EXYNOS_PRNG_EXYNOS5) {
> +               dev_err(&pdev->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 v2 2/4] crypto: exynos - Improve performance of PRNG

[Krzysztof Kozlowski]

On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

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 <l.stelmach@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 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 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 <l.stelmach@samsung.com> wrote:
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

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 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 <l.stelmach@samsung.com> wrote:
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@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.stelmach@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 <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>
> @@ -79,6 +80,7 @@ struct exynos_rng_dev {
>         enum exynos_prng_type           type;
>         void __iomem                    *mem;
>         struct clk                      *clk;
> +       struct mutex                    lock;
>         /* Generated numbers stored for seeding during resume */
>         u8                              seed_save[EXYNOS_RNG_SEED_SIZE];
>         unsigned int                    seed_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(&rng->lock);
> +       mutex_lock(&rng->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(&rng->lock);
>         do {
>                 ret = exynos_rng_get_random(rng, dst, dlen, &read);
>                 if (ret)
> @@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>
>                 exynos_rng_reseed(rng);
>         } while (dlen > 0);
> +       mutex_unlock(&rng->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(&rng->lock);
>         ret = exynos_rng_set_seed(ctx->rng, seed, slen);
> +       mutex_unlock(&rng->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(&rng->lock);
> +
>         rng->dev = &pdev->dev;
>         rng->clk = devm_clk_get(&pdev->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(&rng->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(&rng->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(&rng->lock);
> +
>         ret = exynos_rng_set_seed(rng, rng->seed_save, rng->seed_save_len);
>
> +       mutex_unlock(&rng->lock);
> +
>         clk_disable_unprepare(rng->clk);
>
>         return ret;
> --
> 2.11.0
>

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

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 5144 bytes --]

It was <2017-12-11 pon 16:03>, when Krzysztof Kozlowski wrote:
> On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@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.stelmach@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 <linux/err.h>
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>> +#include <linux/mutex.h>
>>  #include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>>
>> @@ -79,6 +80,7 @@ struct exynos_rng_dev {
>>         enum exynos_prng_type           type;
>>         void __iomem                    *mem;
>>         struct clk                      *clk;
>> +       struct mutex                    lock;
>>         /* Generated numbers stored for seeding during resume */
>>         u8                              seed_save[EXYNOS_RNG_SEED_SIZE];
>>         unsigned int                    seed_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(&rng->lock);
>> +       mutex_lock(&rng->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(&rng->lock);
>>         do {
>>                 ret = exynos_rng_get_random(rng, dst, dlen, &read);
>>                 if (ret)
>> @@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>>
>>                 exynos_rng_reseed(rng);
>>         } while (dlen > 0);
>> +       mutex_unlock(&rng->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(&rng->lock);
>>         ret = exynos_rng_set_seed(ctx->rng, seed, slen);
>> +       mutex_unlock(&rng->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.

>>         clk_disable_unprepare(rng->clk);
>>
>> @@ -284,6 +294,8 @@ static int exynos_rng_probe(struct platform_device *pdev)
>>                 return -ENOTSUPP;
>>         }
>>
>> +       mutex_init(&rng->lock);
>> +
>>         rng->dev = &pdev->dev;
>>         rng->clk = devm_clk_get(&pdev->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(&rng->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(&rng->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(&rng->lock);
>> +
>>         ret = exynos_rng_set_seed(rng, rng->seed_save, rng->seed_save_len);
>>
>> +       mutex_unlock(&rng->lock);
>> +
>>         clk_disable_unprepare(rng->clk);
>>
>>         return ret;
>> --
>> 2.11.0


-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

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.stelmach@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.stelmach@samsung.com> wrote:
>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@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.stelmach@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 <linux/err.h>
>>>  #include <linux/io.h>
>>>  #include <linux/module.h>
>>> +#include <linux/mutex.h>
>>>  #include <linux/of_device.h>
>>>  #include <linux/platform_device.h>
>>>
>>> @@ -79,6 +80,7 @@ struct exynos_rng_dev {
>>>         enum exynos_prng_type           type;
>>>         void __iomem                    *mem;
>>>         struct clk                      *clk;
>>> +       struct mutex                    lock;
>>>         /* Generated numbers stored for seeding during resume */
>>>         u8                              seed_save[EXYNOS_RNG_SEED_SIZE];
>>>         unsigned int                    seed_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(&rng->lock);
>>> +       mutex_lock(&rng->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(&rng->lock);
>>>         do {
>>>                 ret = exynos_rng_get_random(rng, dst, dlen, &read);
>>>                 if (ret)
>>> @@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>>>
>>>                 exynos_rng_reseed(rng);
>>>         } while (dlen > 0);
>>> +       mutex_unlock(&rng->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(&rng->lock);
>>>         ret = exynos_rng_set_seed(ctx->rng, seed, slen);
>>> +       mutex_unlock(&rng->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 <krzk@kernel.org>

Best regards,
Krzysztof

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

[Łukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

It was <2017-12-11 pon 15:54>, when Krzysztof Kozlowski wrote:
> On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej
>> Zolnierkiewicz <b.zolnierkie@samsung.com>
>
> This should not appear here.

A glitch in a scripted invocation of git-format-patch, fixed.

>> 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.

Apparently, after trhough tests, I must admit, the way the status
register is being polled is insignificant for the performance. I will
remove from the patch any changes in the loop.

It is the way, the random bytes are copied from the regiesteres, that
makes the difference (5.9 MB/s vs 7.1 MB/s)

Thank you very much for your assistance in reaching this conclusion.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

[PATCH v3 0/4] Assorted changes for Exynos PRNG driver

[Łukasz Stelmach]

Hello,

This is a series of patches for exynos-rng driver I've decided to
create after adding support for Exynos5250+ chips. They do not
strictly depend on each other, but I think it is better to send them
as a single patch-set.

The driver requires appropriate DT configuration introduced in

    https://patchwork.kernel.org/patch/10106797/

Patch #1 Add support for PRNG in Exynos5250+ SoCs

Patch #2 Improve output performance by using memcpy() rather than a
custom function to retrieve random bytes from registers. Rearrange
the loop for polling the hardware.

Patch #3 Reseed the PRNG after reading 2^16 bytes. Simmilar approach
is implemented in DRBG. (Thanks Stephan Mueller)

Patch #4 Introduce locking to prevent simultaneous access to the
hardware from more than one thread/process.

Changes since v2:

- Remove changes in the loop polling the status register in
  exynos_rng_get_random(). Thanks Krzysztof Kozłowski.
- Remove the check for valid values of rng->type from
  exynos_rng_probe(). The function wouldn't be called with values other
  than mentioned in exynos_rng_dt_match array.
- Applied changes to the commit message of patch #3 requested by Stephan
  Mueller.

Changes since v1:

- Added Patch #4.
- Extended commit message for patch #3.
- Changed exynos_prng_type enum and a define according to Krzysztof Kozłowski's
  recommendations.
- Brought back cpu_relax() in a rearranged loop in
  exynos_rng_get_random().
- Moved an assignment of the read valuea away from an error path.
- Removed dev_info() reporting hardware presence from
  exynos_rng_probe().

Łukasz Stelmach (4):
  crypto: exynos - Support Exynos5250+ SoCs
  crypto: exynos - Improve performance of PRNG
  crypto: exynos - Reseed PRNG after generating 2^16 random bytes
  crypto: exynos - Introduce mutex to prevent concurrent access to
    hardware

 .../bindings/crypto/samsung,exynos-rng4.txt        |  4 +-
 drivers/crypto/exynos-rng.c                        | 94 ++++++++++++++--------
 2 files changed, 62 insertions(+), 36 deletions(-)

-- 
2.11.0

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

[Łukasz Stelmach]

Add support for PRNG in Exynos5250+ SoCs.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 .../bindings/crypto/samsung,exynos-rng4.txt        |  4 +++-
 drivers/crypto/exynos-rng.c                        | 27 ++++++++++++++++++++--
 2 files changed, 28 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..825c09619eb8 100644
--- a/drivers/crypto/exynos-rng.c
+++ b/drivers/crypto/exynos-rng.c
@@ -22,12 +22,17 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 
 #include <crypto/internal/rng.h>
 
 #define EXYNOS_RNG_CONTROL		0x0
 #define EXYNOS_RNG_STATUS		0x10
+
+#define EXYNOS_RNG_SEED_CONF		0x14
+#define EXYNOS_RNG_GEN_PRNG	        BIT(1)
+
 #define EXYNOS_RNG_SEED_BASE		0x140
 #define EXYNOS_RNG_SEED(n)		(EXYNOS_RNG_SEED_BASE + (n * 0x4))
 #define EXYNOS_RNG_OUT_BASE		0x160
@@ -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,8 @@ static int exynos_rng_probe(struct platform_device *pdev)
 	if (!rng)
 		return -ENOMEM;
 
+	rng->type = (enum exynos_prng_type)of_device_get_match_data(&pdev->dev);
+
 	rng->dev = &pdev->dev;
 	rng->clk = devm_clk_get(&pdev->dev, "secss");
 	if (IS_ERR(rng->clk)) {
@@ -367,6 +386,10 @@ static SIMPLE_DEV_PM_OPS(exynos_rng_pm_ops, exynos_rng_suspend,
 static const struct of_device_id exynos_rng_dt_match[] = {
 	{
 		.compatible = "samsung,exynos4-rng",
+		.data = (const void *)EXYNOS_PRNG_EXYNOS4,
+	}, {
+		.compatible = "samsung,exynos5250-prng",
+		.data = (const void *)EXYNOS_PRNG_EXYNOS5,
 	},
 	{ },
 };
-- 
2.11.0

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

[Łukasz Stelmach]

Use memcpy_fromio() instead of custom exynos_rng_copy_random() function
to retrieve generated numbers from the registers of PRNG.

Signed-off-by: Łukasz Stelmach <l.stelmach@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

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

[Łukasz Stelmach]

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.stelmach@samsung.com>
Reviewed-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/exynos-rng.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
index dcdd444d0b3b..825ed7bfd881 100644
--- a/drivers/crypto/exynos-rng.c
+++ b/drivers/crypto/exynos-rng.c
@@ -55,12 +55,14 @@ 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_BYTES		65536
+
 /*
  * In polling mode, do not wait infinitely for the engine to finish the work.
  */
@@ -82,6 +84,8 @@ struct exynos_rng_dev {
 	unsigned int			seed_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;
@@ -126,6 +130,7 @@ static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
 	}
 
 	rng->last_seeding = jiffies;
+	rng->bytes_seeding = 0;
 
 	return 0;
 }
@@ -164,6 +169,7 @@ static int exynos_rng_get_random(struct exynos_rng_dev *rng,
 			  EXYNOS_RNG_STATUS);
 	*read = min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE);
 	memcpy_fromio(dst, rng->mem + EXYNOS_RNG_OUT_BASE, *read);
+	rng->bytes_seeding += *read;
 
 	return 0;
 }
@@ -177,7 +183,8 @@ static void exynos_rng_reseed(struct exynos_rng_dev *rng)
 	unsigned int read = 0;
 	u8 seed[EXYNOS_RNG_SEED_SIZE];
 
-	if (time_before(now, next_seeding))
+	if (time_before(now, next_seeding) &&
+	    rng->bytes_seeding < EXYNOS_RNG_RESEED_BYTES)
 		return;
 
 	if (exynos_rng_get_random(rng, seed, sizeof(seed), &read))
-- 
2.11.0

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

[Łukasz Stelmach]

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.stelmach@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 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 825ed7bfd881..4a06092074b9 100644
--- a/drivers/crypto/exynos-rng.c
+++ b/drivers/crypto/exynos-rng.c
@@ -22,6 +22,7 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 
@@ -79,6 +80,7 @@ struct exynos_rng_dev {
 	enum exynos_prng_type		type;
 	void __iomem			*mem;
 	struct clk			*clk;
+	struct mutex 			lock;
 	/* Generated numbers stored for seeding during resume */
 	u8				seed_save[EXYNOS_RNG_SEED_SIZE];
 	unsigned int			seed_save_len;
@@ -191,6 +193,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(&rng->lock);
+	mutex_lock(&rng->lock);
 }
 
 static int exynos_rng_generate(struct crypto_rng *tfm,
@@ -206,6 +212,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
 	if (ret)
 		return ret;
 
+	mutex_lock(&rng->lock);
 	do {
 		ret = exynos_rng_get_random(rng, dst, dlen, &read);
 		if (ret)
@@ -216,6 +223,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
 
 		exynos_rng_reseed(rng);
 	} while (dlen > 0);
+	mutex_unlock(&rng->lock);
 
 	clk_disable_unprepare(rng->clk);
 
@@ -233,7 +241,9 @@ static int exynos_rng_seed(struct crypto_rng *tfm, const u8 *seed,
 	if (ret)
 		return ret;
 
+	mutex_lock(&rng->lock);
 	ret = exynos_rng_set_seed(ctx->rng, seed, slen);
+	mutex_unlock(&rng->lock);
 
 	clk_disable_unprepare(rng->clk);
 
@@ -278,6 +288,8 @@ static int exynos_rng_probe(struct platform_device *pdev)
 
 	rng->type = (enum exynos_prng_type)of_device_get_match_data(&pdev->dev);
 
+	mutex_init(&rng->lock);
+
 	rng->dev = &pdev->dev;
 	rng->clk = devm_clk_get(&pdev->dev, "secss");
 	if (IS_ERR(rng->clk)) {
@@ -328,9 +340,14 @@ static int __maybe_unused exynos_rng_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	mutex_lock(&rng->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(&rng->lock);
+
 	dev_dbg(rng->dev, "Stored %u bytes for seeding on system resume\n",
 		rng->seed_save_len);
 
@@ -353,8 +370,12 @@ static int __maybe_unused exynos_rng_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	mutex_lock(&rng->lock);
+
 	ret = exynos_rng_set_seed(rng, rng->seed_save, rng->seed_save_len);
 
+	mutex_unlock(&rng->lock);
+
 	clk_disable_unprepare(rng->clk);
 
 	return ret;
-- 
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.stelmach@samsung.com> wrote:
> Add support for PRNG in Exynos5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelmach@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 <krzk@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.stelmach@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 <krzk@kernel.org>

Best regards,
Krzysztof

>
> Signed-off-by: Łukasz Stelmach <l.stelmach@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 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.stelmach@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.stelmach@samsung.com>
> Reviewed-by: Stephan Mueller <smueller@chronox.de>
> ---
>  drivers/crypto/exynos-rng.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>

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

Best regards,
Krzysztof

Re: [PATCH v3 0/4] Assorted changes for Exynos PRNG driver

[Herbert Xu]

On Tue, Dec 12, 2017 at 05:36:03PM +0100, Łukasz Stelmach wrote:
> Hello,
> 
> This is a series of patches for exynos-rng driver I've decided to
> create after adding support for Exynos5250+ chips. They do not
> strictly depend on each other, but I think it is better to send them
> as a single patch-set.
> 
> The driver requires appropriate DT configuration introduced in
> 
>     https://patchwork.kernel.org/patch/10106797/
> 
> Patch #1 Add support for PRNG in Exynos5250+ SoCs
> 
> Patch #2 Improve output performance by using memcpy() rather than a
> custom function to retrieve random bytes from registers. Rearrange
> the loop for polling the hardware.
> 
> Patch #3 Reseed the PRNG after reading 2^16 bytes. Simmilar approach
> is implemented in DRBG. (Thanks Stephan Mueller)
> 
> Patch #4 Introduce locking to prevent simultaneous access to the
> hardware from more than one thread/process.
> 
> Changes since v2:

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