Hi all
On Sun, 21 Jul 2024 at 13:08, Simon Glass <s...@chromium.org> wrote:
>
> Hi,
>
> On Mon, 15 Jul 2024 at 12:23, <lukas.funke-...@weidmueller.com> wrote:
> >
> > From: Lukas Funke <lukas.fu...@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.fu...@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
Thanks
/Ilias
>
> Regards,
> Simon
