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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, Jun 11, 2025 at 04:25:24PM +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).

Got it.

> 
> Instead, perform the check in the following way:
> 
> 1. Read the current timestamp
> 2. Read the completion status. If completed, return the result
> 3. Sleep
> 4. Check if the timestamp read at step 1 exceeds the timeout. Return
>    an error if it does
> 5. Goto 1
> 
> Also, use ktime instead of jiffes as a more reliable and precise timing
> source.

"also", i.e. a logically separate change which should be split up to
a separate patch.

> 
> Signed-off-by: Ivan Orlov <iorlov@amazon.com>
> ---
>  drivers/char/tpm/tpm-interface.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 8d7e4da6ed53..959330212a16 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -88,7 +88,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>  	int rc;
>  	ssize_t len = 0;
>  	u32 count, ordinal;
> -	unsigned long stop;
> +	ktime_t timeout, curr_time;
> +	unsigned int ord_dur_us;
>  
>  	if (bufsiz < TPM_HEADER_SIZE)
>  		return -EINVAL;
> @@ -126,8 +127,16 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>  	if (chip->flags & TPM_CHIP_FLAG_IRQ)
>  		goto out_recv;
>  
> -	stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
> +	ord_dur_us = jiffies_to_usecs(tpm_calc_ordinal_duration(chip, ordinal));
> +	timeout = ktime_add_us(ktime_get(), ord_dur_us);
>  	do {
> +		/*
> +		 * Save the time of the completion check. This way even if CPU
> +		 * goes to sleep indefinitely on tpm_sleep, the driver will
> +		 * check for completion one more time instead of timing out
> +		 * instantly after waking up.
> +		 */
> +		curr_time = ktime_get();
>  		u8 status = tpm_chip_status(chip);
>  		if ((status & chip->ops->req_complete_mask) ==
>  		    chip->ops->req_complete_val)
> @@ -140,7 +149,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>  
>  		tpm_msleep(TPM_TIMEOUT_POLL);
>  		rmb();
> -	} while (time_before(jiffies, stop));
> +	} while (ktime_before(curr_time, timeout));


Wouldn't it be simpler fix to just check completion after dropping out
of the loop?

	} while (time_before(jiffies, stop));

	if (tpm_transmit_completed(chip))
		goto out_recv;

And declare this before tpm_try_transmit():

static bool tpm_transmit_completed(struct tpm_chip *chip)
{
	u8 status = tpm_chip_status(chip);
	
	return (status & chip->ops->req_complete_mask) == chip->ops->req_complete_val;
}

>  
>  	tpm_chip_cancel(chip);
>  	dev_err(&chip->dev, "Operation Timed out\n");
> -- 
> 2.43.0
> 

BR, Jarkko