Re: [PATCH v5 4/9] tpm: Add the RNG child device
Hi Simon, A bit late on this, but at least I found it... On Wed, Mar 16, 2022 at 10:15:34AM +1300, Simon Glass wrote: > Hi Ilias, > > On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas > wrote: > > > > Hi Simon, > > > > On Mon, 14 Mar 2022 at 20:24, Simon Glass wrote: > > > > > > > [...] > > > > > > > > + } > > > > > > > > > > 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'. > > > > There's a small difference here though. The RNG is not a device. The > > TPM is the device and an encoded command to that device returns a > > random number. There's no binding initiating etc. > > A device is just something with a struct udevice, so I don't see the > random number generator as anything different from another device. We > might have a white-noise generator which produces random numbers. Just > because the feature is packaged inside a single chip doesn't make it > any less a device. Just like the PMIC. > > > > > > > > > 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. > > > > And the entire tpm framework will fit? > > Yes. For verified boot it has to, since you cannot init RAM until you > have selected your A/B/recovery image. If so I still don't see why this needs a DT node. If, as you say, the TPM framework fits in then you just call tpm_get_random(). Getting the RNG is a series of commands to the TPM. Sou you'll need the TPM lib in SPL regardless. > > > > > > > > > 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. > > > > I don't personally find it strange. The device is already described > > in the DTS and I don't see a strong reason to deviate for the upstream > > version again. > > Linux tends to rely a lot more on manually adding devices. It can have > a pretty dramatic bloating effect on code size in U-Boot. > > Anyway, so long as we can detect an existing device, as I explained > below, it is fine to manually add it when it is missing. > You can detect a TPM. Once you add that tpm then you *know* you can get an RNG [...] Regards /Ilias
Re: [PATCH v5 4/9] tpm: Add the RNG child device
Hi Sughosh, On Mon, 21 Mar 2022 at 00:01, Sughosh Ganu wrote: > > hi Simon, > > On Wed, 16 Mar 2022 at 02:45, Simon Glass wrote: > > > > Hi Ilias, > > > > On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas > > wrote: > > > > > > Hi Simon, > > > > > > On Mon, 14 Mar 2022 at 20:24, Simon Glass wrote: > > > > > > > > > > [...] > > > > > > > > > > + } > > > > > > > > > > > > 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'. > > > > > > There's a small difference here though. The RNG is not a device. The > > > TPM is the device and an encoded command to that device returns a > > > random number. There's no binding initiating etc. > > > > A device is just something with a struct udevice, so I don't see the > > random number generator as anything different from another device. We > > might have a white-noise generator which produces random numbers. Just > > because the feature is packaged inside a single chip doesn't make it > > any less a device. Just like the PMIC. > > > > > > > > > > > > > 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. > > > > > > And the entire tpm framework will fit? > > > > Yes. For verified boot it has to, since you cannot init RAM until you > > have selected your A/B/recovery image. > > > > > > > > > > > > > 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. > > > > > > I don't personally find it strange. The device is already described > > > in the DTS and I don't see a strong reason to deviate for the upstream > > > version again. > > > > Linux tends to rely a lot more on manually adding devices. It can have > > a pretty dramatic bloating effect on code size in U-Boot. > > > > Anyway, so long as we can detect an existing device, as I explained > > below, it is fine to manually add it when it is missing. > > Just so that I understand what you are saying, do you want support for > both approaches. Meaning, using device tree when the rng node is > described in the device tree, and otherwise using the manual device > binding when the device tree node is absent. Do I understand this > right? Yes that's what we normally do in U-Boot. Regards, Simon
Re: [PATCH v5 4/9] tpm: Add the RNG child device
hi Simon,
On Wed, 16 Mar 2022 at 02:45, Simon Glass wrote:
>
> Hi Ilias,
>
> On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas
> wrote:
> >
> > Hi Simon,
> >
> > On Mon, 14 Mar 2022 at 20:24, Simon Glass wrote:
> > >
> >
> > [...]
> >
> > > > > > + }
> > > > >
> > > > > 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'.
> >
> > There's a small difference here though. The RNG is not a device. The
> > TPM is the device and an encoded command to that device returns a
> > random number. There's no binding initiating etc.
>
> A device is just something with a struct udevice, so I don't see the
> random number generator as anything different from another device. We
> might have a white-noise generator which produces random numbers. Just
> because the feature is packaged inside a single chip doesn't make it
> any less a device. Just like the PMIC.
>
> >
> > >
> > > 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.
> >
> > And the entire tpm framework will fit?
>
> Yes. For verified boot it has to, since you cannot init RAM until you
> have selected your A/B/recovery image.
>
> >
> > >
> > > 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.
> >
> > I don't personally find it strange. The device is already described
> > in the DTS and I don't see a strong reason to deviate for the upstream
> > version again.
>
> Linux tends to rely a lot more on manually adding devices. It can have
> a pretty dramatic bloating effect on code size in U-Boot.
>
> Anyway, so long as we can detect an existing device, as I explained
> below, it is fine to manually add it when it is missing.
Just so that I understand what you are saying, do you want support for
both approaches. Meaning, using device tree when the rng node is
described in the device tree, and otherwise using the manual device
binding when the device tree node is absent. Do I understand this
right?
-sughosh
>
>
> >
> > Regards
> > /Ilias
> > >
> > > >
> > > > 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,
> > > > > );
> > > > > if (ret) {
> > > > >if (ret != -ENODEV) {
> > > > >log_debug("Cannot access bootdev device\n");
> > > > >return ret;
> > > > >}
> > > > >
> > > > >ret = bootdev_bind(parent, drv_name, "bootdev", );
> > > > >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
Re: [PATCH v5 4/9] tpm: Add the RNG child device
Hi Ilias,
On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas
wrote:
>
> Hi Simon,
>
> On Mon, 14 Mar 2022 at 20:24, Simon Glass wrote:
> >
>
> [...]
>
> > > > > + }
> > > >
> > > > 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'.
>
> There's a small difference here though. The RNG is not a device. The
> TPM is the device and an encoded command to that device returns a
> random number. There's no binding initiating etc.
A device is just something with a struct udevice, so I don't see the
random number generator as anything different from another device. We
might have a white-noise generator which produces random numbers. Just
because the feature is packaged inside a single chip doesn't make it
any less a device. Just like the PMIC.
>
> >
> > 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.
>
> And the entire tpm framework will fit?
Yes. For verified boot it has to, since you cannot init RAM until you
have selected your A/B/recovery image.
>
> >
> > 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.
>
> I don't personally find it strange. The device is already described
> in the DTS and I don't see a strong reason to deviate for the upstream
> version again.
Linux tends to rely a lot more on manually adding devices. It can have
a pretty dramatic bloating effect on code size in U-Boot.
Anyway, so long as we can detect an existing device, as I explained
below, it is fine to manually add it when it is missing.
>
> Regards
> /Ilias
> >
> > >
> > > 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, );
> > > > if (ret) {
> > > >if (ret != -ENODEV) {
> > > >log_debug("Cannot access bootdev device\n");
> > > >return ret;
> > > >}
> > > >
> > > >ret = bootdev_bind(parent, drv_name, "bootdev", );
> > > >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
Re: [PATCH v5 4/9] tpm: Add the RNG child device
hi Simon,
On Mon, 14 Mar 2022 at 23:54, Simon Glass wrote:
>
> Hi Sughosh,
>
> On Mon, 14 Mar 2022 at 05:43, Sughosh Ganu wrote:
> >
> > hi Simon,
> >
> > On Mon, 14 Mar 2022 at 03:53, Simon Glass wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu
> > > 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
> > > > ---
> > > >
> > > > 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
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include "tpm_internal.h"
> > > >
> > > > +#include
> > > > +
> > > > +#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", );
> > > > + 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, );
> > > if (ret) {
> > >if (ret != -ENODEV) {
> > >log_debug("Cannot access bootdev device\n");
> > >return ret;
> > >}
> > >
> > >ret = bootdev_bind(parent, drv_name, "bootdev", );
> > >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
Re: [PATCH v5 4/9] tpm: Add the RNG child device
Hi Simon,
On Mon, 14 Mar 2022 at 20:24, Simon Glass wrote:
>
[...]
> > > > + }
> > >
> > > 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'.
There's a small difference here though. The RNG is not a device. The
TPM is the device and an encoded command to that device returns a
random number. There's no binding initiating etc.
>
> 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.
And the entire tpm framework will fit?
>
> 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.
I don't personally find it strange. The device is already described
in the DTS and I don't see a strong reason to deviate for the upstream
version again.
Regards
/Ilias
>
> >
> > 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, );
> > > if (ret) {
> > >if (ret != -ENODEV) {
> > >log_debug("Cannot access bootdev device\n");
> > >return ret;
> > >}
> > >
> > >ret = bootdev_bind(parent, drv_name, "bootdev", );
> > >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
Re: [PATCH v5 4/9] tpm: Add the RNG child device
Hi Sughosh,
On Mon, 14 Mar 2022 at 05:43, Sughosh Ganu wrote:
>
> hi Simon,
>
> On Mon, 14 Mar 2022 at 03:53, Simon Glass wrote:
> >
> > Hi Sughosh,
> >
> > On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu 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
> > > ---
> > >
> > > 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
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include "tpm_internal.h"
> > >
> > > +#include
> > > +
> > > +#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", );
> > > + 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.
>
> 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, );
> > if (ret) {
> >if (ret != -ENODEV) {
> >log_debug("Cannot access bootdev device\n");
> >return ret;
> >}
> >
> >ret = bootdev_bind(parent, drv_name, "bootdev", );
> >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
Re: [PATCH v5 4/9] tpm: Add the RNG child device
hi Simon,
On Mon, 14 Mar 2022 at 03:53, Simon Glass wrote:
>
> Hi Sughosh,
>
> On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu 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
> > ---
> >
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include "tpm_internal.h"
> >
> > +#include
> > +
> > +#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", );
> > + 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.
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, );
> if (ret) {
>if (ret != -ENODEV) {
>log_debug("Cannot access bootdev device\n");
>return ret;
>}
>
>ret = bootdev_bind(parent, drv_name, "bootdev", );
>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
-sughosh
>
> > .per_device_auto= sizeof(struct tpm_chip_priv),
> > };
> > --
> > 2.25.1
> >
>
> Regards,
> Simon
Re: [PATCH v5 4/9] tpm: Add the RNG child device
Hi Sughosh,
On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu 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
> ---
>
> 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
> #include
> #include
> +#include
> #include
> #include
> #include "tpm_internal.h"
>
> +#include
> +
> +#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", );
> + 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. If you want to manually bind it, please call
device_find_first_child_by_uclass() first to make sure it isn't
already there.
Also you should bind it in the bind() method, not in probe().
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, );
if (ret) {
if (ret != -ENODEV) {
log_debug("Cannot access bootdev device\n");
return ret;
}
ret = bootdev_bind(parent, drv_name, "bootdev", );
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.
> .per_device_auto= sizeof(struct tpm_chip_priv),
> };
> --
> 2.25.1
>
Regards,
Simon
