Re: [PATCH v2 3/4] tpm_tis: Use responseRetry to recover from data transfer errors

[Jarkko Sakkinen]

On Mon Jun 5, 2023 at 8:59 PM EEST, Alexander Steffen wrote:
> TPM responses may become damaged during transmission, for example due to
> bit flips on the wire. Instead of aborting when detecting such issues, the
> responseRetry functionality can be used to make the TPM retransmit its
> response and receive it again without errors.
>
> Signed-off-by: Alexander Steffen <Alexander.Steffen@xxxxxxxxxxxx>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 40 ++++++++++++++++++++++++++-------
>  drivers/char/tpm/tpm_tis_core.h |  1 +
>  2 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 5ddaf24518be..a08768e55803 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -345,11 +345,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  	u32 expected;
>  	int rc;
>  
> -	if (count < TPM_HEADER_SIZE) {
> -		size = -EIO;
> -		goto out;
> -	}
> -
>  	size = recv_data(chip, buf, TPM_HEADER_SIZE);
>  	/* read first 10 bytes, including tag, paramsize, and result */
>  	if (size < TPM_HEADER_SIZE) {
> @@ -382,7 +377,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  		goto out;
>  	}
>  	status = tpm_tis_status(chip);
> -	if (status & TPM_STS_DATA_AVAIL) {	/* retry? */
> +	if (status & TPM_STS_DATA_AVAIL) {

Please remove (no-op).

>  		dev_err(&chip->dev, "Error left over data\n");
>  		size = -EIO;
>  		goto out;
> @@ -396,10 +391,39 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  	}
>  
>  out:
> -	tpm_tis_ready(chip);
>  	return size;
>  }
>  
> +static int tpm_tis_recv_with_retries(struct tpm_chip *chip, u8 *buf, size_t count)

This *substitutes* the curent tpm_tis_recv(), right?

So it *is* tpm_tis_recv(), i.e. no renames thank you :-)

> +{
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	unsigned int try;
> +	int rc = 0;
> +
> +	if (count < TPM_HEADER_SIZE) {
> +		rc = -EIO;
> +		goto out;
> +	}
> +
> +	for (try = 0; try < TPM_RETRY; try++) {
> +		rc = tpm_tis_recv(chip, buf, count);

I would rename single shot tpm_tis_recv() as tpm_tis_try_recv().

> +
> +		if (rc == -EIO) {
> +			/* Data transfer errors, indicated by EIO, can be
> +			 * recovered by rereading the response.
> +			 */
> +			tpm_tis_write8(priv, TPM_STS(priv->locality),
> +				       TPM_STS_RESPONSE_RETRY);
> +		} else {
> +			break;
> +		}

And if this should really be managed inside tpm_tis_try_recv(), and
then return zero (as the code block consumes the return value).

> +	}
> +
> +out:
> +	tpm_tis_ready(chip);

Empty line here (nit).

> +	return rc;
> +}
> +
>  /*
>   * If interrupts are used (signaled by an irq set in the vendor structure)
>   * tpm.c can skip polling for the data to be available as the interrupt is
> @@ -986,7 +1010,7 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value)
>  static const struct tpm_class_ops tpm_tis = {
>  	.flags = TPM_OPS_AUTO_STARTUP,
>  	.status = tpm_tis_status,
> -	.recv = tpm_tis_recv,
> +	.recv = tpm_tis_recv_with_retries,
>  	.send = tpm_tis_send,
>  	.cancel = tpm_tis_ready,
>  	.update_timeouts = tpm_tis_update_timeouts,
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index e978f457fd4d..8458cd4a84ec 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -34,6 +34,7 @@ enum tis_status {
>  	TPM_STS_GO = 0x20,
>  	TPM_STS_DATA_AVAIL = 0x10,
>  	TPM_STS_DATA_EXPECT = 0x08,
> +	TPM_STS_RESPONSE_RETRY = 0x02,
>  	TPM_STS_READ_ZERO = 0x23, /* bits that must be zero on read */
>  };
>  
> -- 
> 2.34.1

BR, Jarkko