On Mon, 2018-03-26 at 19:41 +0530, Nayna Jain wrote:
>
> On 03/22/2018 10:01 PM, James Bottomley wrote:
> >
> > On Thu, 2018-03-22 at 21:43 +0530, Nayna Jain wrote:
[...]
> > > > +ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space
> > > > *space,
> > > > + u8 *buf, size_t bufsiz, unsigned int
> > > > flags)
> > > > +{
> > > > + struct tpm_output_header *header = (struct
> > > > tpm_output_header *)buf;
> > > > + /* space for header and handles */
> > > > + u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
> > > > + unsigned int delay_msec = TPM2_DURATION_SHORT;
> > > > + u32 rc = 0;
> > > > + ssize_t ret;
> > > > + const size_t save_size = min(space ? sizeof(save) :
> > > > TPM_HEADER_SIZE,
> > > > + bufsiz);
> > > > +
> > > > + /*
> > > > + * Subtlety here: if we have a space, the handles will
> > > > be
> > > > + * transformed, so when we restore the header we also
> > > > have
> > > > to
> > > > + * restore the handles.
> > > > + */
> > > > + memcpy(save, buf, save_size);
> > > > +
> > > > + for (;;) {
> > > > + ret = tpm_try_transmit(chip, space, buf,
> > > > bufsiz,
> > > > flags);
> > > > + if (ret < 0)
> > > > + break;
> > > > + rc = be32_to_cpu(header->return_code);
> > > > + if (rc != TPM2_RC_RETRY)
> > > > + break;
> > > > + delay_msec *= 2;
> > > Was wondering if this increment could be moved after
> > > tpm_msleep(delay_msec) ?
> > I thought about that when I saw the logic in the original but what
> > it would do is double the maximum delay (which is already nearly
> > double TPM2_DURATION_LONG). I figured doubling the initial timeout
> > was fine rather than trying to do complex logic to get the delay
> > exact (and then having to litter the file with comments explaining
> > why).
>
> Sorry James, I didn't understand exactly about what complex logic is
> involved. Can you please explain it more ?
> My point was to just move "delay_msec *= 2" after tpm_msleep() as
> shown below:
>
> if (delay_msec > TPM2_DURATION_LONG) {
> dev_err(&chip->dev, "TPM is in retry loop\n");
> break;
> }
> tpm_msleep(delay_msec)
> delay_msec *= 2;
Yes, I understand the suggestion; however, if you simply do that,
you'll end up with a useless sleep at the end of the wait period before
you check to see if you've waited too long, which is even more
suboptimal than the current situation. The cardinal rule is we should
only sleep if we know we're going to retry.
James
