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
