Re: [PATCH v2] tpm: Fix the timeout & use ktime

[Jonathan McDowell <noodles@earth.li>]

On Fri, Jun 20, 2025 at 06:08:31PM +0000, Orlov, Ivan wrote:
>The current implementation of timeout detection works in the following
>way:
>
>1. Read completion status. If completed, return the data
>2. Sleep for some time (usleep_range)
>3. Check for timeout using current jiffies value. Return an error if
>   timed out
>4. Goto 1
>
>usleep_range doesn't guarantee it's always going to wake up strictly in
>(min, max) range, so such a situation is possible:
>
>1. Driver reads completion status. No completion yet
>2. Process sleeps indefinitely. In the meantime, TPM responds
>3. We check for timeout without checking for the completion again.
>   Result is lost.
>
>Such a situation also happens for the guest VMs: if vCPU goes to sleep
>and doesn't get scheduled for some time, the guest TPM driver will
>timeout instantly after waking up without checking for the completion
>(which may already be in place).
>
>Perform the completion check once again after exiting the busy loop in
>order to give the device the last chance to send us some data.
>
>Since now we check for completion in two places, extract this check into
>a separate function.
>
>Signed-off-by: Ivan Orlov <iorlov@amazon.com>
>---
>V1 -> V2:
>- Exclude the jiffies -> ktime change from the patch
>- Instead of recording the time before checking for completion, check
>  for completion once again after leaving the loop
>
> drivers/char/tpm/tpm-interface.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>index 8d7e4da6ed53..6960ee2798e1 100644
>--- a/drivers/char/tpm/tpm-interface.c
>+++ b/drivers/char/tpm/tpm-interface.c
>@@ -82,6 +82,13 @@ static bool tpm_chip_req_canceled(struct tpm_chip *chip, u8 status)
> 	return chip->ops->req_canceled(chip, status);
> }
>
>+static bool tpm_transmit_completed(struct tpm_chip *chip)
>+{
>+	u8 status_masked = tpm_chip_status(chip) & chip->ops->req_complete_mask;
>+
>+	return status_masked == chip->ops->req_complete_val;
>+}
>+
> static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> {
> 	struct tpm_header *header = buf;
>@@ -129,8 +136,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> 	stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
> 	do {
> 		u8 status = tpm_chip_status(chip);
>-		if ((status & chip->ops->req_complete_mask) ==
>-		    chip->ops->req_complete_val)
>+		if (tpm_transmit_completed(chip))
> 			goto out_recv;

The only thing I'd point out here is we end up doing a double status 
read one after the other (once here, once in tpm_transmit_completed), 
and I'm pretty sure I've seen instances where that caused a problem.

> 		if (tpm_chip_req_canceled(chip, status)) {
>@@ -142,6 +148,13 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> 		rmb();
> 	} while (time_before(jiffies, stop));
>
>+	/*
>+	 * Check for completion one more time, just in case the device reported
>+	 * it while the driver was sleeping in the busy loop above.
>+	 */
>+	if (tpm_transmit_completed(chip))
>+		goto out_recv;
>+
> 	tpm_chip_cancel(chip);
> 	dev_err(&chip->dev, "Operation Timed out\n");
> 	return -ETIME;

J.

-- 
She's the one for me. She's all I really need, oh yeah.
This .sig brought to you by the letter K and the number  3
Product of the Republic of HuggieTag