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

[Åukasz Stelmach]

It was <2017-11-23 czw 17:31>, when Krzysztof Kozlowski wrote:
> On Thu, Nov 23, 2017 at 4:09 PM, Åukasz Stelmach <l.stelmach@xxxxxxxxxxx> wrote:
>> Add support for True Random Number Generator found in Samsung Exynos
>> 5250+ SoCs.
>>
>> Signed-off-by: Åukasz Stelmach <l.stelmach@xxxxxxxxxxx>
>> ---
>>  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
>>

[...]

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

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

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

Attachment: signature.asc
Description: PGP signature