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

[Ilias Apalodimas]

Thanks Lukas,

On Wed, 24 Jul 2024 at 10:35, <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>
> Acked-by: Miquel Raynal <miquel.raynal at bootlin.com>
> ---
>
> Changes in v2:
> - Call tpm_tis_wait_init() from tpm_tis_init()
> - Use phy_ops for bus access in tpm_tis_wait_init()
>
>  drivers/tpm/tpm2_tis_core.c | 28 ++++++++++++++++++++++++++++
>  drivers/tpm/tpm2_tis_spi.c  | 29 -----------------------------
>  2 files changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/tpm/tpm2_tis_core.c b/drivers/tpm/tpm2_tis_core.c
> index 680a6409433..1fdf8cfa319 100644
> --- a/drivers/tpm/tpm2_tis_core.c
> +++ b/drivers/tpm/tpm2_tis_core.c
> @@ -419,6 +419,28 @@ static bool tis_check_ops(struct tpm_tis_phy_ops *phy_ops)
>         return true;
>  }
>
> +static int tpm_tis_wait_init(struct udevice *dev, int loc)
> +{
> +       struct tpm_chip *chip = dev_get_priv(dev);
> +       unsigned long start, stop;
> +       u8 status;
> +       int ret;
> +
> +       start = get_timer(0);
> +       stop = chip->timeout_b;
> +       do {
> +               mdelay(TPM_TIMEOUT_MS);
> +               ret = chip->phy_ops->read_bytes(dev, TPM_ACCESS(loc), 1, &status);
> +               if (ret)
> +                       break;
> +
> +               if (status & TPM_ACCESS_VALID)
> +                       return 0;
> +       } while (get_timer(start) < stop);
> +
> +       return -EIO;
> +}
> +
>  int tpm_tis_init(struct udevice *dev)
>  {
>         struct tpm_chip *chip = dev_get_priv(dev);
> @@ -436,6 +458,12 @@ int tpm_tis_init(struct udevice *dev)
>         chip->timeout_c = TIS_SHORT_TIMEOUT_MS;
>         chip->timeout_d = TIS_SHORT_TIMEOUT_MS;
>
> +       ret = tpm_tis_wait_init(dev, chip->locality);
> +       if (ret) {
> +               log(LOGC_DM, LOGL_ERR, "%s: no device found\n", __func__);
> +               return ret;
> +       }
> +
>         ret = tpm_tis_request_locality(dev, 0);
>         if (ret)
>                 return ret;
> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> index b0fe97ab1d0..7909a147c2d 100644
> --- a/drivers/tpm/tpm2_tis_spi.c
> +++ b/drivers/tpm/tpm2_tis_spi.c
> @@ -188,29 +188,6 @@ static int tpm_tis_spi_write32(struct udevice *dev, u32 addr, u32 value)
>         return tpm_tis_spi_write(dev, addr, sizeof(value), (u8 *)&value_le);
>  }
>
> -static int tpm_tis_wait_init(struct udevice *dev, int loc)
> -{
> -       struct tpm_chip *chip = dev_get_priv(dev);
> -       unsigned long start, stop;
> -       u8 status;
> -       int ret;
> -
> -       start = get_timer(0);
> -       stop = chip->timeout_b;
> -       do {
> -               mdelay(TPM_TIMEOUT_MS);
> -
> -               ret = tpm_tis_spi_read(dev, TPM_ACCESS(loc), 1, &status);
> -               if (ret)
> -                       break;
> -
> -               if (status & TPM_ACCESS_VALID)
> -                       return 0;
> -       } while (get_timer(start) < stop);
> -
> -       return -EIO;
> -}
> -
>  static struct tpm_tis_phy_ops phy_ops = {
>         .read_bytes = tpm_tis_spi_read,
>         .write_bytes = tpm_tis_spi_write,
> @@ -256,12 +233,6 @@ 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);
>
> -       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)
> --
> 2.30.2
>

I'll manually add the fixes tag on merge. Other than that
Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>