Messages in this thread | | | From | Łukasz Stelmach <> | Subject | Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver | Date | Thu, 23 Nov 2017 19:46:24 +0100 |
| |
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@samsung.com> wrote: >> 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 >>
[...]
>> 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 [unhandled content-type:application/pgp-signature] | |