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

[Jonathan McDowell <noodles@earth.li>]

Jarkko, I've realised I've somehow introduced a typo in the patch below 
that means it doesn't fire correctly; I'm not sure how this happened as 
my local copy I was testing on is definitely correct. Would you like a 
one line fix up patch, or can you manually fix it up in your tree?

This hunk:

>@@ -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 */

should be "rc != -EAGAIN" - the "-" sign has somehow been lost.

Apologies for this, let me know what's easiest for you in terms of 
resolving it.

On Mon, Mar 10, 2025 at 12:19:55PM +0000, Jonathan McDowell wrote:
>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
>retry the command submission. Add appropriate logic to do this in the
>send path.
>
>This is fixed in later firmware revisions, but those are not always
>available, and cannot generally be easily updated from outside a
>firmware environment.
>
>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
>
>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;
> 			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),
>-- 
>2.48.1
>
>

J.

-- 
101 things you can't have too much of : 7 - Memory.