diff mbox series

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

Message ID 20230605175959.2131-4-Alexander.Steffen@infineon.com
State New
Headers
Series Recovery from data transfer errors for tpm_tis |

Commit Message

Alexander Steffen June 5, 2023, 5:59 p.m. UTC 
  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@infineon.com>
---
 drivers/char/tpm/tpm_tis_core.c | 40 ++++++++++++++++++++++++++-------
 drivers/char/tpm/tpm_tis_core.h |  1 +
 2 files changed, 33 insertions(+), 8 deletions(-)

Comments

Jarkko Sakkinen June 6, 2023, 9:17 p.m. UTC | #1 
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@infineon.com>
> ---
>  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
Jarkko Sakkinen June 6, 2023, 9:18 p.m. UTC | #2 
On Mon Jun 5, 2023 at 8:59 PM EEST, Alexander Steffen wrote:
> +static int tpm_tis_recv_with_retries(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> +	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;
> +	}

	if (count < TPM_HEADER_SIZE)
		return -EIO;

BR, Jarkko
Alexander Steffen June 7, 2023, 5:14 p.m. UTC | #3 
On 06.06.23 23:17, Jarkko Sakkinen wrote:
> 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@infineon.com>
>> ---
>>   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).

You mean I shouldn't change the line and leave the comment in? To me it 
looked like a very brief TODO comment "should we retry here?", and since 
with this change it now actually does retry, I removed it.

>>                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).

What exactly should be done in tpm_tis_try_recv()? It could set 
TPM_STS_RESPONSE_RETRY, but then it would still need to return an error 
code, so that this loop knows whether to call it again or not.

>> +     }
>> +
>> +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
Jarkko Sakkinen June 8, 2023, 2 p.m. UTC | #4 
On Wed Jun 7, 2023 at 8:14 PM EEST, Alexander Steffen wrote:
> >> -     if (status & TPM_STS_DATA_AVAIL) {      /* retry? */
> >> +     if (status & TPM_STS_DATA_AVAIL) {
> > 
> > Please remove (no-op).
>
> You mean I shouldn't change the line and leave the comment in? To me it 
> looked like a very brief TODO comment "should we retry here?", and since 
> with this change it now actually does retry, I removed it.

Right, ok, point taken, you can keep it.

> >>                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).
>
> What exactly should be done in tpm_tis_try_recv()? It could set 
> TPM_STS_RESPONSE_RETRY, but then it would still need to return an error 
> code, so that this loop knows whether to call it again or not.

So my thinking was to:

- Rename tpm_tis_recv() as tpm_tis_try_recv()
- Rename this new function as tpm_tis_recv().

BR, Jarkko
Alexander Steffen June 13, 2023, 5:37 p.m. UTC | #5 
On 08.06.23 16:00, Jarkko Sakkinen wrote:
> On Wed Jun 7, 2023 at 8:14 PM EEST, Alexander Steffen wrote:
>>>> -     if (status & TPM_STS_DATA_AVAIL) {      /* retry? */
>>>> +     if (status & TPM_STS_DATA_AVAIL) {
>>>
>>> Please remove (no-op).
>>
>> You mean I shouldn't change the line and leave the comment in? To me it
>> looked like a very brief TODO comment "should we retry here?", and since
>> with this change it now actually does retry, I removed it.
> 
> Right, ok, point taken, you can keep it.
> 
>>>>                 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).
>>
>> What exactly should be done in tpm_tis_try_recv()? It could set
>> TPM_STS_RESPONSE_RETRY, but then it would still need to return an error
>> code, so that this loop knows whether to call it again or not.
> 
> So my thinking was to:
> 
> - Rename tpm_tis_recv() as tpm_tis_try_recv()
> - Rename this new function as tpm_tis_recv().

Sounds good, thanks. Will be done in v3.

> BR, Jarkko
diff mbox series

Patch

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) {
 		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)
+{
+	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);
+
+		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;
+		}
+	}
+
+out:
+	tpm_tis_ready(chip);
+	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 */
 };