[PATCH v2 1/1] tpm: clear state post probing

[Simon Glass]

Hi,

(replying to both of you)

On Wed, 17 Nov 2021 at 01:18, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
>
> On Wed, 17 Nov 2021 at 04:48, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Heinrich,
> >
> > On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
> > > > Before we can start measuring the TPM must be cleared. Do this in the
> > > > post_probe() method of the uclass.
> > > >
> > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> > > > ---
> > > > v2:
> > > >       tpm_startup2() is not available on all boards.
> > > >       tpm_startup() takes care of translating the call.
> > > > ---
> > > >  drivers/tpm/tpm-uclass.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > index f67fe1019b..abd9ce35e8 100644
> > > > --- a/drivers/tpm/tpm-uclass.c
> > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > @@ -11,6 +11,7 @@
> > > >  #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"
> > > > @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > >       return 0;
> > > >  }
> > > >
> > > > +static int dm_tpm_post_probe(struct udevice *dev)
> >
> > Please drop the dm_
> >
> > > > +{
> > > > +     /*
> > > > +      * Clearing the TPM state is only possible once after a hard reset.
> > > > +      * As we do not know if the TPM has been cleared by a prior boot stage
> > > > +      * ignore the return value here.
> > > > +      */
> > > > +     tpm_startup(dev, TPM_ST_CLEAR);
> >
> > blank line before final return
> >
> > > > +     return 0;
> > > > +}
> >
> > This should only happen once and if the TPM is set up in SPL then this
> > seems to cause a failure if done again.
> >
>
> Not really. If you run the tpm_startup twice and the TPM is already
> initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you
> can easily check against and decide.
>
> > Is there another way we can deal with this? Could the TPM user decide
> > whether it needs to be set?
>
> Why? Part of the TPM init is making it usable.  We are trying to move
> away from having to add something in the command line to make the
> device usable.

For a start, we should not start up the TPM until we need it. That
goes against the U-Boot lazy-init approach. We already have a command
to do it, as well as a TPM-API thing, so let's use that.

>
> >
> > For the approach you have here, I think the best option might be to
> > add a property to the devicetree. That way the prior stage can control
> > it.
> >

w.r.t. Heinrich's comment:

> The idea of a prior stage adding something U-Boot specific sucks.

The problem is that we need to track the device state. If we have
multiple firmware phases involved, there needs to be some coordination
across stages.

As it happens I am very familiar with this problem as it comes up in
Chrome OS all the time. For Coral vboot I had to make sure that the
TPM was only started once.

Anyway, this patch is wrong due to the 'probe' reason I mentioned above, sorry.


> > > > +
> > > >  UCLASS_DRIVER(tpm) = {
> > > >       .id             = UCLASS_TPM,
> > > >       .name           = "tpm",
> > > > @@ -143,5 +155,6 @@ UCLASS_DRIVER(tpm) = {
> > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > >       .post_bind      = dm_scan_fdt_dev,
> > > >  #endif
> > > > +     .post_probe     = dm_tpm_post_probe,
> > > >       .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > >  };
> > > > --
> > > > 2.32.0
> > > >
> > >
> > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>


Regards,
Simon