[PATCH 1/1] tpm: call tpm_tis_wait_init() after tpm_tis_init()

[Ilias Apalodimas]

Hi Simon

On Tue, 23 Jul 2024 at 15:47, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias
>
> On Mon, 22 Jul 2024 at 08:35, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi all
> >
> > On Sun, 21 Jul 2024 at 13:08, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, 15 Jul 2024 at 12:23, <lukas.funke-oss at weidmueller.com> wrote:
> > > >
> > > > From: Lukas Funke <lukas.funke at weidmueller.com>
> > > >
> > > > tpm_tis_wait_init() is using the 'chip->timeout_b' field which is
> > > > initialized in tpm_tis_init(). However, the init-function is called
> > > > *after* tpm_tis_wait_init() introducing an uninitalized field access.
> > > >
> > > > This commit switches both routines.
> > > >
> > > > Signed-off-by: Lukas Funke <lukas.funke at weidmueller.com>
> > > > ---
> > > >
> > > >  drivers/tpm/tpm2_tis_spi.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > > > index b0fe97ab1d0..5a4dbfd3ccb 100644
> > > > --- a/drivers/tpm/tpm2_tis_spi.c
> > > > +++ b/drivers/tpm/tpm2_tis_spi.c
> > > > @@ -256,17 +256,17 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> > > >         /* Ensure a minimum amount of time elapsed since reset of the TPM */
> > > >         mdelay(drv_data->time_before_first_cmd_ms);
> > > >
> > > > +       tpm_tis_ops_register(dev, &phy_ops);
> > > > +       ret = tpm_tis_init(dev);
> > > > +       if (ret)
> > > > +               goto err;
> > > > +
> > > >         ret = tpm_tis_wait_init(dev, chip->locality);
> > > >         if (ret) {
> > > >                 log(LOGC_DM, LOGL_ERR, "%s: no device found\n", __func__);
> > > >                 return ret;
> > > >         }
> > > >
> > > > -       tpm_tis_ops_register(dev, &phy_ops);
> > > > -       ret = tpm_tis_init(dev);
> > > > -       if (ret)
> > > > -               goto err;
> > > > -
> > > >         priv->pcr_count = drv_data->pcr_count;
> > > >         priv->pcr_select_min = drv_data->pcr_select_min;
> > > >         priv->version = TPM_V2;
> > > > --
> > > > 2.30.2
> > > >
> > >
> > > This needs a Fixes tag for a5c30c26b28 (HEAD) tpm: Use the new API on
> > > tpm2 spi driver
> > >
> >
> > Yes please we need a fixes tag
> >
> > > The old code set up the timeouts first, then did the wait_init.
> > > Presumably the point of wait_init is to wait before doing the init, so
> > > we should try to keep that behaviour, unless it is actually wrong.
> > >
> > > So my thought would be to move the setup of the required timeout into
> > > tpm_tis_ops_register(), instead.
> >
> > tpm_tis_ops_register() is setting up the bus accesses and I'd prefer
> > to keep it that way.
> > Since this is a static function, we can fold it in tpm_tis_init(),
> > which makes more sense
>
> Since this is used by i2c and SPI we need to make sure that both work.

The tpm_tis_wait_init function is SPI-specific right now. It was SPI
specifric before a5c30c26b28c6 as well. The i2c driver was after the
refactoring and it doesn't call tpm_tis_wait_init(). However, I think
we should call it on all drivers.

The only bus-specific bit is how we read back from the TPM. So I think
the right thing to do here is
1. Teach tpm_tis_wait_init() to call the proper bus function rather
than the hardcoded tpm_tis_spi_read()
2. call it from tpm_tis_init()

That way we'll call it regardless of the bus the TPM happens to sit on.

Regards
/Ilias

>
> I believe the way it used to be, some initial values were set for the
> timeouts. The naming is somewhat confusing I suppose.
>
> Regards,
> Simon