[PATCH v4 1/8] dm: rng: Add random number generator(rng) uclass

[Sughosh Ganu]

On Wed, 25 Dec 2019 at 06:41, Heinrich Schuchardt <xypron.glpk at gmx.de>
wrote:

> On 12/17/19 12:51 PM, Sughosh Ganu wrote:
> > Add a uclass for reading a random number seed from a random number
> > generator device.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > Reviewed-by: Patrice Chotard <patrice.chotard at st.com>
> > ---
> > Changes since V3:
> >   Handle review comments from Patrick Delaunay.
> >
> >   drivers/Kconfig          |  2 ++
> >   drivers/Makefile         |  1 +
> >   drivers/rng/Kconfig      |  7 +++++++
> >   drivers/rng/Makefile     |  6 ++++++
> >   drivers/rng/rng-uclass.c | 23 +++++++++++++++++++++++
> >   include/dm/uclass-id.h   |  1 +
> >   include/rng.h            | 32 ++++++++++++++++++++++++++++++++
> >   7 files changed, 72 insertions(+)
> >   create mode 100644 drivers/rng/Kconfig
> >   create mode 100644 drivers/rng/Makefile
> >   create mode 100644 drivers/rng/rng-uclass.c
> >   create mode 100644 include/rng.h
> >
> > diff --git a/drivers/Kconfig b/drivers/Kconfig
> > index 9d99ce0..e34a227 100644
> > --- a/drivers/Kconfig
> > +++ b/drivers/Kconfig
> > @@ -90,6 +90,8 @@ source "drivers/remoteproc/Kconfig"
> >
> >   source "drivers/reset/Kconfig"
> >
> > +source "drivers/rng/Kconfig"
> > +
> >   source "drivers/rtc/Kconfig"
> >
> >   source "drivers/scsi/Kconfig"
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index e977f19..6c619b1 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -115,4 +115,5 @@ obj-$(CONFIG_W1_EEPROM) += w1-eeprom/
> >
> >   obj-$(CONFIG_MACH_PIC32) += ddr/microchip/
> >   obj-$(CONFIG_DM_HWSPINLOCK) += hwspinlock/
> > +obj-$(CONFIG_DM_RNG) += rng/
> >   endif
> > diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> > new file mode 100644
> > index 0000000..dd44cc0
> > --- /dev/null
> > +++ b/drivers/rng/Kconfig
> > @@ -0,0 +1,7 @@
> > +config DM_RNG
> > +     bool "Driver support for Random Number Generator devices"
> > +     depends on DM
> > +     help
> > +       Enable driver model for random number generator(rng) devices.
> > +       This interface is used to initialise the rng device and to
> > +       read the random seed from the device.
> > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > new file mode 100644
> > index 0000000..311705b
> > --- /dev/null
> > +++ b/drivers/rng/Makefile
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +# Copyright (c) 2019, Linaro Limited
> > +#
> > +
> > +obj-$(CONFIG_DM_RNG) += rng-uclass.o
> > diff --git a/drivers/rng/rng-uclass.c b/drivers/rng/rng-uclass.c
> > new file mode 100644
> > index 0000000..b6af3b8
> > --- /dev/null
> > +++ b/drivers/rng/rng-uclass.c
> > @@ -0,0 +1,23 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2019, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <rng.h>
> > +
> > +int dm_rng_read(struct udevice *dev, void *buffer, size_t size)
> > +{
> > +     const struct dm_rng_ops *ops = device_get_ops(dev);
> > +
> > +     if (!ops->read)
> > +             return -ENOSYS;
> > +
> > +     return ops->read(dev, buffer, size);
> > +}
> > +
> > +UCLASS_DRIVER(rng) = {
> > +     .name = "rng",
> > +     .id = UCLASS_RNG,
> > +};
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index 0c563d8..192202d 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -86,6 +86,7 @@ enum uclass_id {
> >       UCLASS_REGULATOR,       /* Regulator device */
> >       UCLASS_REMOTEPROC,      /* Remote Processor device */
> >       UCLASS_RESET,           /* Reset controller device */
> > +     UCLASS_RNG,             /* Random Number Generator */
> >       UCLASS_RTC,             /* Real time clock device */
> >       UCLASS_SCSI,            /* SCSI device */
> >       UCLASS_SERIAL,          /* Serial UART */
> > diff --git a/include/rng.h b/include/rng.h
> > new file mode 100644
> > index 0000000..80891fc
> > --- /dev/null
> > +++ b/include/rng.h
> > @@ -0,0 +1,32 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2019, Linaro Limited
> > + */
> > +
> > +#if !defined _RNG_H_
> > +#define _RNG_H_
> > +
> > +#include <dm.h>
> > +
> > +/**
> > + * dm_rng_read() - read a random number seed from the rng device
> > + * @buffer:  input buffer to put the read random seed into
> > + * @size:    number of bytes of random seed read
> > + * @return 0 if OK, -ve on error
>
> Please, stick to the Spinx syntax (Return:). See
>
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation


Ok.


>
>
> The Linux kernel code uses the return value to return the number of
> bytes retrieved.
>
> Please, provide as description here of what will happen if the RNG
> device blocks.
>

I guess that would be device specific behavior. For example, in the
stm32mp1 rng driver, readl_poll_timeout is used to check for random data
availability, failing which, we return an error.


> > + *
> > + */
> > +int dm_rng_read(struct udevice *dev, void *buffer, size_t size);
>
> We have different places where we retrieve the first RNG device and read
> from it. Shouldn't we provide a single function in the uclass that
> returns random bytes without a udevice parameter?
>

Even in scenarios where there is no hardware rng device, i think we can
have a dummy driver, with it's read function using srand kind on function.


> Linux uses an entropy pool where the hardware RNG is one of multiple
> sources of randomness and the entropy runs through hash functions
> (chacha20, sha1) before being consumed.
>
> Shouldn't we call a hashing function in this uclass?
>

Should these functions be  part of the uclass, or should these be called
separately by the caller, in case the caller needs to run the random data
through an additional hashing function. I think that would be better than
mixing rng uclass with hashing.


> Shouldn't we further provide a function that can later be used to add
> additional sources of entropy? E.g. a seed file, the MAC address, timing
> of network accesses.
>

I am not sure if we need to add this functionality in the bootloader. In
case you think this is needed, I think this can be taken up as a separate
task.

-sughosh