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

It was <2017-11-23 czw 17:31>, when Krzysztof Kozlowski wrote:

On Thu, Nov 23, 2017 at 4:09 PM, Łukasz Stelmach [off-list ref] wrote:

quoted

Add support for True Random Number Generator found in Samsung Exynos
5250+ SoCs.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 MAINTAINERS                          |   7 +
 drivers/char/hw_random/Kconfig       |  12 ++
 drivers/char/hw_random/Makefile      |   1 +
 drivers/char/hw_random/exynos-trng.c | 256 +++++++++++++++++++++++++++++++++++
 4 files changed, 276 insertions(+)
 create mode 100644 drivers/char/hw_random/exynos-trng.c

[...]

quoted

 endif # HW_RANDOM

Thanks for working on TRNG.

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

As far as I can tell, these are two different devices. However, PRNG
shares hardware with the hash engine. Indeed there is a hardware to
connect TRNG and PRNG, but, IMHO, it might be hard to model that
dependency in kernel. To me it seems easier to read TRNG (or
/dev/random) and and write the result to PRNG manually (in software).

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

I guess so. I expect no problems as long as TRNG_SEED_START is not set
in the HASH_SEED_CONF register.

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

quoted

+
+       trng->rng.init = exynos_trng_init;
+       trng->rng.read = exynos_trng_do_read;
+       trng->rng.priv = (unsigned long) trng;
+
+       platform_set_drvdata(pdev, trng);
+       trng->dev = &pdev->dev;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       trng->mem = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(trng->mem)) {
+               dev_err(&pdev->dev, "Couldn't map IO resources.\n");
+               ret = PTR_ERR(trng->mem);
+               goto err_ioremap;
+       }
+
+       pm_runtime_enable(&pdev->dev);
+       ret = pm_runtime_get_sync(&pdev->dev);
+       if (ret < 0) {
+               dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d.\n", ret);
+               goto err_ioremap;

Missing pm_runtime_disable?

Added a separate label down below.

quoted

+               dev_err(&pdev->dev, "Couldn't get clock.\n");
+               ret = PTR_ERR(trng->clk);
+               goto err_clock;
+       }
+
+       ret = clk_prepare_enable(trng->clk);
+       if (ret) {
+               dev_err(&pdev->dev, "Unable to enable the clk: %d.\n", ret);
+               goto err_clock;
+       }
+
+       ret = hwrng_register(&trng->rng);
+       if (ret) {
+               dev_err(&pdev->dev, "Couldn't register hwrng device.\n");
+               goto err_register;
+       }
+
+       dev_info(&pdev->dev, "Exynos True Random Number Generator.\n");
+
+       return 0;
+
+err_register:
+       clk_disable_unprepare(trng->clk);
+
+err_clock:
+       trng->mem = NULL;

Why this has to be null-ified?

I found this pattern in omap-rng.c.

Removed.

I'll wait a little bit more before posting v2.

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