[PATCH v5 4/9] tpm: Add the RNG child device

[Sughosh Ganu]

hi Simon,

On Mon, 14 Mar 2022 at 23:54, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 14 Mar 2022 at 05:43, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Mon, 14 Mar 2022 at 03:53, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > The TPM device comes with the random number generator(RNG)
> > > > functionality which is built into the TPM device. Add logic to add the
> > > > RNG child device in the TPM uclass post probe callback.
> > > >
> > > > The RNG device can then be used to pass a set of random bytes to the
> > > > linux kernel, need for address space randomisation through the
> > > > EFI_RNG_PROTOCOL interface.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > ---
> > > >
> > > > Changes since V4:
> > > >
> > > > * Put a check for CONFIG_TPM_RNG for binding the RNG device with it's
> > > >   driver in the post_probe callback instead of putting
> > > >   CONFIG_{SPL,TPL}_BUILD guards
> > > >
> > > >  drivers/tpm/tpm-uclass.c | 29 +++++++++++++++++++++++++----
> > > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > >
> > >
> > > This looks a lot better, please see below.
> > >
> > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > index f67fe1019b..e1c61d26f0 100644
> > > > --- a/drivers/tpm/tpm-uclass.c
> > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > @@ -11,10 +11,15 @@
> > > >  #include <log.h>
> > > >  #include <linux/delay.h>
> > > >  #include <linux/unaligned/be_byteshift.h>
> > > > +#include <tpm_api.h>
> > > >  #include <tpm-v1.h>
> > > >  #include <tpm-v2.h>
> > > >  #include "tpm_internal.h"
> > > >
> > > > +#include <dm/lists.h>
> > > > +
> > > > +#define TPM_RNG_DRV_NAME       "tpm-rng"
> > > > +
> > > >  int tpm_open(struct udevice *dev)
> > > >  {
> > > >         struct tpm_ops *ops = tpm_get_ops(dev);
> > > > @@ -136,12 +141,28 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > >         return 0;
> > > >  }
> > > >
> > > > +static int tpm_uclass_post_probe(struct udevice *dev)
> > > > +{
> > > > +       int ret;
> > > > +       const char *drv = TPM_RNG_DRV_NAME;
> > > > +       struct udevice *child;
> > > > +
> > > > +       if (CONFIG_IS_ENABLED(TPM_RNG)) {
> > > > +               ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> > > > +               if (ret)
> > > > +                       return log_msg_ret("bind", ret);
> > > > +       }
> > >
> > > This really should be in the device tree so what you are doing here is
> > > quite strange.
> >
> > Like I had mentioned in my earlier emails, the TPM device has a
> > builtin RNG functionality, which is non-optional. So I don't
> > understand why we need to use a device tree subnode here. Whether the
> > device is being bound to the parent is being controlled by the TPM_RNG
> > config that you asked me to put in my previous version, which I am
> > doing.
>
> See how PMICs work, for example. We have GPIOs, regulators and
> suchlike in the PMIC and we add subnodes for them in the DT. It is
> just how it is done.
>
> Driver model is designed to automatically bind devices based on the
> device tree. There are cases where it is necessary to manually bind
> things, but we mustn't prevent people from doing it 'properly'.
>
> Finally, I know you keep saying that random numbers are only needed in
> U-Boot proper, but if I want a random number in SPL, it may not work,
> since device_bind() is often not available, for code-size reasons.
>
> So that is why I say that what you are doing is quite strange. Perhaps
> you are coming from a different project, which does things
> differently.

Well, FWIW I actually found usage of this kind of device binding in
this very project. There are quite a few drivers which are using the
API in the same way that is being done in this patch. And I have
already mentioned the reason that I am using this method as against a
device tree. Thanks.

-sughosh

>
> >
> >  If you want to manually bind it, please call
> > > device_find_first_child_by_uclass() first to make sure it isn't
> > > already there.
> >
> > Okay
> >
> > >
> > > Also you should bind it in the bind() method, not in probe().
> >
> > Okay
> >
> > >
> > > This is the code used for the same thing in the bootstd series:
> > >
> > > struct udevice *bdev;
> > > int ret;
> > >
> > > ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev);
> > > if (ret) {
> > >    if (ret != -ENODEV) {
> > >    log_debug("Cannot access bootdev device\n");
> > >    return ret;
> > >    }
> > >
> > >    ret = bootdev_bind(parent, drv_name, "bootdev", &bdev);
> > >    if (ret) {
> > >       log_debug("Cannot create bootdev device\n");
> > >       return ret;
> > >    }
> > > }
> > >
> > >
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > >  UCLASS_DRIVER(tpm) = {
> > > > -       .id             = UCLASS_TPM,
> > > > -       .name           = "tpm",
> > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > +       .id                     = UCLASS_TPM,
> > > > +       .name                   = "tpm",
> > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > +       .post_bind              = dm_scan_fdt_dev,
> > > >  #endif
> > > > +       .post_probe             = tpm_uclass_post_probe,
> > >
> > > Should be post_bind.
> >
> > Okay
>
> [..]
>
> Regards,
> Simon