Re: [PATCH v2] tpm, tpm_tis: Workaround failed command reception on Infineon devices

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Jonathan,


Some nits, should you resend. Feel free to ignore. The verb *work 
around* is spelled with a space.

Am 10.03.25 um 13:19 schrieb Jonathan McDowell:
> From: Jonathan McDowell <noodles@meta.com>
> 
> Some Infineon devices have a issue where the status register will get
> stuck with a quick REQUEST_USE / COMMAND_READY sequence. This is not
> simply a matter of requiring a longer timeout; the work around is to

The noun without. ;-)

> retry the command submission. Add appropriate logic to do this in the
> send path.

Does the workaround have downsides?

> This is fixed in later firmware revisions, but those are not always
> available, and cannot generally be easily updated from outside a
> firmware environment.

Please mention the affected revisions. Is there an errata for tis.

> Testing has been performed with a simple repeated loop of doing a
> TPM2_CC_GET_CAPABILITY for TPM_CAP_PROP_MANUFACTURER using the Go code
> at:
> 
>   https://the.earth.li/~noodles/tpm-stuff/timeout-reproducer-simple.go

Awesome. Thank you for sharing.

> It can take several hours to reproduce, and several million operations.
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> ---
> v2: Rename flag to TPM_TIS_STATUS_VALID_RETRY
> 
>   drivers/char/tpm/tpm_tis_core.c | 17 ++++++++++++++---
>   drivers/char/tpm/tpm_tis_core.h |  1 +
>   include/linux/tpm.h             |  1 +
>   3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/ 
> tpm_tis_core.c
> index c969a1793184..4ab69c3e103c 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -463,7 +463,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, 
> const u8 *buf, size_t len)
> 
>           if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>                       &priv->int_queue, false) < 0) {
> -            rc = -ETIME;
> +            if (test_bit(TPM_TIS_STATUS_VALID_RETRY, &priv->flags))
> +                rc = -EAGAIN;
> +            else
> +                rc = -ETIME;

I’d use a ternary operator as the same variable is assigned to.

>               goto out_err;
>           }
>           status = tpm_tis_status(chip);
> @@ -480,7 +483,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, 
> const u8 *buf, size_t len)
> 
>       if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>                   &priv->int_queue, false) < 0) {
> -        rc = -ETIME;
> +        if (test_bit(TPM_TIS_STATUS_VALID_RETRY, &priv->flags))
> +            rc = -EAGAIN;
> +        else
> +            rc = -ETIME;
>           goto out_err;
>       }
>       status = tpm_tis_status(chip);
> @@ -545,9 +551,11 @@ static int tpm_tis_send_main(struct tpm_chip *chip, 
> const u8 *buf, size_t len)
>           if (rc >= 0)
>               /* Data transfer done successfully */
>               break;
> -        else if (rc != -EIO)
> +        else if (rc != EAGAIN && rc != -EIO)
>               /* Data transfer failed, not recoverable */
>               return rc;
> +
> +        usleep_range(priv->timeout_min, priv->timeout_max);
>       }
> 
>       /* go and do it */
> @@ -1143,6 +1151,9 @@ int tpm_tis_core_init(struct device *dev, struct 
> tpm_tis_data *priv, int irq,
>           priv->timeout_max = TIS_TIMEOUT_MAX_ATML;
>       }
> 
> +    if (priv->manufacturer_id == TPM_VID_IFX)
> +        set_bit(TPM_TIS_STATUS_VALID_RETRY, &priv->flags);
> +
>       if (is_bsw()) {
>           priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
>                       ILB_REMAP_SIZE);
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/ 
> tpm_tis_core.h
> index 690ad8e9b731..970d02c337c7 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -89,6 +89,7 @@ enum tpm_tis_flags {
>       TPM_TIS_INVALID_STATUS        = 1,
>       TPM_TIS_DEFAULT_CANCELLATION    = 2,
>       TPM_TIS_IRQ_TESTED        = 3,
> +    TPM_TIS_STATUS_VALID_RETRY    = 4,
>   };
> 
>   struct tpm_tis_data {
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 20a40ade8030..6c3125300c00 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -335,6 +335,7 @@ enum tpm2_cc_attrs {
>   #define TPM_VID_WINBOND  0x1050
>   #define TPM_VID_STM      0x104A
>   #define TPM_VID_ATML     0x1114
> +#define TPM_VID_IFX      0x15D1
> 
>   enum tpm_chip_flags {
>       TPM_CHIP_FLAG_BOOTSTRAPPED        = BIT(0),