On Sat, Jul 19, 2025 at 08:13:39PM +0000, Ivan Orlov 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 <ivan.orlov0...@gmail.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
> V2 -> V3:
> - Avoid reading the chip status twice in the inner loop by passing
> status into tpm_transmit_completed
>
> 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..8d18b33aa62d 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(u8 status, struct tpm_chip *chip)
> +{
> + u8 status_masked = status & 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(status, chip))
> goto out_recv;
>
> 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(tpm_chip_status(chip), chip))
> + goto out_recv;
> +
> tpm_chip_cancel(chip);
> dev_err(&chip->dev, "Operation Timed out\n");
> return -ETIME;
> --
> 2.43.0
>
I guess this is completed too by now ...
Reviewed-by: Jarkko Sakkinen <jar...@kernel.org>
BR, Jarkko
