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

[Alexander Steffen]

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