Hi Torsten,
On Sun, 20 Dec 2020 at 10:00, Torsten Duwe <duwe at lst.de> wrote:
>
> On Fri, 18 Dec 2020 19:29:12 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
> > > - int i;
> > > -
> > > - srand(get_ticks() + rand());
> > > + int i, ret;
> > > + struct udevice *devp;
> > > + u8 randv = 0;
> > > +
> > > +#if defined(CONFIG_DM_RNG)
> >
> > This seems a little backwards to me. The caller should request a RNG
> > device, getting either a hardware one or a software one, and then call
> > the uclass method to get the uuid.
>
> Strictly speaking, there's no such thing as a "software RNG". The term
> "DRBG" was coined for accurateness, "deterministic random bit
> generator". The oxymoron "deterministic random" pretty much nails it.
> Alternatively, it can be called "pseudo" RNG.
> rand() and srand() exactly implement such a mechanism already, with low
> coding overhead. U-Boot runs fine with them most of the time, but there
> are rare cases where real entropy would be needed. This is what these
> two patches are about. In case there's more, I already speculated about
> a centralised entity in my response to the v1 cover letter, but for now
> these two changes should do.
I am used to the term pseudo-random, but it doesn't much matter what
kind of random number is used. It is still covered by the RNG class.
You are currently burying device access in a utility function. That
really isn't the right way to do it. See my comment above. There is no
way to control which RNG device is used and no visibility that this is
happening at all, outside this function.
>
> > > + ret = uclass_get_device(UCLASS_RNG, 0, &devp);
> > > + if (ret) {
> > > + ret = dm_rng_read(devp, &randv, sizeof(randv));
> > > + if (ret < 0)
> > > + randv = 0;
> > > + }
> > > + if (randv)
> > > + srand(randv);
> > > + else
> > > +#endif
> > > + srand(get_ticks() + rand());
> > >
> > > /* Set all fields randomly */
> > > for (i = 0; i < 4; i++)
> [ rand() usage following here ^ ]
>
> The #ifdefs are already gone in v3.
OK good.
Regards,
Simon