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

[Alexander Steffen]

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