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