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

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

[Orlov, Ivan]

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;
 
 		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;
-- 
2.43.0

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

[Orlov, Ivan]

On 20/06/2025 19:08, Orlov, Ivan wrote:
> The current implementation of timeout detection works in the following
> way:

Ah, I forgot to remove "& use ktime" from the commit title. If there are 
any comments regarding the patch content, I'll fix it in V3. Otherwise, 
I'd kindly ask to fix it when applying. Thanks!

--
Kind Regards,
Ivan Orlov

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

[Jonathan McDowell]

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

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

[Jarkko Sakkinen]

On Sun, Jun 22, 2025 at 09:52:58PM +0100, Jonathan McDowell wrote:
> 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.

It would be easy to to prevent at least double reads after completion
e.g., in tpm_chip_status():

/*
 * Read the chip status bitmask. After completion, the returned will mask will
 * return value cached at the point of completion up until the next transmit.
 */
static u8 tpm_chip_status(struct tpm_chip *chip)
{
	u8 status_masked = chip->status & chip->ops_req_complete_mask;

	if (status_masked == chip->ops->req_complete_val)
		return chip->status;

	chip->status = tpm_chip_status(chip);

	return chip->status;
}

I think tpm_chip_status() should be the gatekeeper for such event, or
like the correct layer of abstraction here ...

Then just reset chip->status to zero at the beginning of tpm_try_transmit().

BR, Jarkko

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

[Jarkko Sakkinen]

On Wed, Jun 25, 2025 at 07:43:11PM +0300, Jarkko Sakkinen wrote:
> On Sun, Jun 22, 2025 at 09:52:58PM +0100, Jonathan McDowell wrote:
> > 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.
> 
> It would be easy to to prevent at least double reads after completion
> e.g., in tpm_chip_status():
> 
> /*
>  * Read the chip status bitmask. After completion, the returned will mask will
>  * return value cached at the point of completion up until the next transmit.
>  */
> static u8 tpm_chip_status(struct tpm_chip *chip)
> {
> 	u8 status_masked = chip->status & chip->ops_req_complete_mask;
> 
> 	if (status_masked == chip->ops->req_complete_val)
> 		return chip->status;
> 
> 	chip->status = tpm_chip_status(chip);

OOPS:

	chip->status = chip->ops->status(chip);

Couple of additional constraints:

1. tpm_chip_alloc() should initialize chip->status to chip->ops->req_complete_mask.
2. tpm_try_transmit() should reset the value to chip->ops->req_complete_mask on
   failure paths.

With these constraints there is framework where chip->ops->status() is
only invoked when it actually makes sense i.e., only during the course
of tpm_try_trasmit().

BR, Jarkko

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

[Jonathan McDowell]

On Wed, Jun 25, 2025 at 07:43:07PM +0300, Jarkko Sakkinen wrote:
>On Sun, Jun 22, 2025 at 09:52:58PM +0100, Jonathan McDowell wrote:
>> 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.
>
>It would be easy to to prevent at least double reads after completion
>e.g., in tpm_chip_status():

Or just take the simple approach and make the check after the while 
loop:

	if ((tpm_chip_status(chip) & chip->ops->req_complete_mask) ==
	    chip->ops->req_complete_val)
		goto out_recv;

There might be potential for a longer term cleanup using chip->status to 
cache things, but I'm little concerned that's going to open paths where 
we might not correctly update it, so I think it should be a separate 
piece.

(I'm motivated by the fact we've started to see the "Operation Canceled" 
error and I'd like us to close on the best way to fix it. :) )

J.

-- 
I am afraid of the dark.

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

[Jarkko Sakkinen]

On Fri, Jul 04, 2025 at 10:02:33AM +0100, Jonathan McDowell wrote:
> On Wed, Jun 25, 2025 at 07:43:07PM +0300, Jarkko Sakkinen wrote:
> > On Sun, Jun 22, 2025 at 09:52:58PM +0100, Jonathan McDowell wrote:
> > > 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.
> > 
> > It would be easy to to prevent at least double reads after completion
> > e.g., in tpm_chip_status():
> 
> Or just take the simple approach and make the check after the while loop:
> 
> 	if ((tpm_chip_status(chip) & chip->ops->req_complete_mask) ==
> 	    chip->ops->req_complete_val)
> 		goto out_recv;
> 
> There might be potential for a longer term cleanup using chip->status to
> cache things, but I'm little concerned that's going to open paths where we
> might not correctly update it, so I think it should be a separate piece.
> 
> (I'm motivated by the fact we've started to see the "Operation Canceled"
> error and I'd like us to close on the best way to fix it. :) )

This would work for me too!

Please send a new version if you feel like it but next week I won't be
reviewing that as I'm on holiday.

> 
> J.
> -- 
> I am afraid of the dark.

BR, Jarkko

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

[Orlov, Ivan]

On 04/07/2025 16:16, Jarkko Sakkinen wrote:
> On Fri, Jul 04, 2025 at 10:02:33AM +0100, Jonathan McDowell wrote:
>> On Wed, Jun 25, 2025 at 07:43:07PM +0300, Jarkko Sakkinen wrote:
>>> On Sun, Jun 22, 2025 at 09:52:58PM +0100, Jonathan McDowell wrote:
>>>> 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.
>>>
>>> It would be easy to to prevent at least double reads after completion
>>> e.g., in tpm_chip_status():
>>
>> Or just take the simple approach and make the check after the while loop:
>>
>> 	if ((tpm_chip_status(chip) & chip->ops->req_complete_mask) ==
>> 	    chip->ops->req_complete_val)
>> 		goto out_recv;
>>
>> There might be potential for a longer term cleanup using chip->status to
>> cache things, but I'm little concerned that's going to open paths where we
>> might not correctly update it, so I think it should be a separate piece.
>>
>> (I'm motivated by the fact we've started to see the "Operation Canceled"
>> error and I'd like us to close on the best way to fix it. :) )
> 
> This would work for me too!
> 

Hi, and sorry for the late reply :(

I believe this option would work for us as well. Please let me know 
whether you'd like me to send V3 or you feel free to send it yourself if 
you want.

--
Kind regards,
Ivan Orlov

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

[Jonathan McDowell]

On Fri, Jul 04, 2025 at 04:39:20PM +0100, Orlov, Ivan wrote:
>On 04/07/2025 16:16, Jarkko Sakkinen wrote:
>>On Fri, Jul 04, 2025 at 10:02:33AM +0100, Jonathan McDowell wrote:
>>>On Wed, Jun 25, 2025 at 07:43:07PM +0300, Jarkko Sakkinen wrote:
>>>>On Sun, Jun 22, 2025 at 09:52:58PM +0100, Jonathan McDowell wrote:
>>>>>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.
>>>>
>>>>It would be easy to to prevent at least double reads after completion
>>>>e.g., in tpm_chip_status():
>>>
>>>Or just take the simple approach and make the check after the while loop:
>>>
>>>	if ((tpm_chip_status(chip) & chip->ops->req_complete_mask) ==
>>>	    chip->ops->req_complete_val)
>>>		goto out_recv;
>>>
>>>There might be potential for a longer term cleanup using chip->status to
>>>cache things, but I'm little concerned that's going to open paths where we
>>>might not correctly update it, so I think it should be a separate piece.
>>>
>>>(I'm motivated by the fact we've started to see the "Operation Canceled"
>>>error and I'd like us to close on the best way to fix it. :) )
>>
>>This would work for me too!
>>
>
>Hi, and sorry for the late reply :(
>
>I believe this option would work for us as well. Please let me know 
>whether you'd like me to send V3 or you feel free to send it yourself 
>if you want.

As far as I'm concerned it's something you found, and your patch. You 
send a new version, I'll test + review it. :)

J.

-- 
  What have you got in your pocket? |  .''`.  Debian GNU/Linux Developer
                                    | : :' :  Happy to accept PGP signed
                                    | `. `'   or encrypted mail - RSA
                                    |   `-    key on the keyservers.

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

[Jarkko Sakkinen]

On Fri, Jul 04, 2025 at 04:39:20PM +0100, Orlov, Ivan wrote:
> On 04/07/2025 16:16, Jarkko Sakkinen wrote:
> > On Fri, Jul 04, 2025 at 10:02:33AM +0100, Jonathan McDowell wrote:
> > > On Wed, Jun 25, 2025 at 07:43:07PM +0300, Jarkko Sakkinen wrote:
> > > > On Sun, Jun 22, 2025 at 09:52:58PM +0100, Jonathan McDowell wrote:
> > > > > 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.
> > > > 
> > > > It would be easy to to prevent at least double reads after completion
> > > > e.g., in tpm_chip_status():
> > > 
> > > Or just take the simple approach and make the check after the while loop:
> > > 
> > > 	if ((tpm_chip_status(chip) & chip->ops->req_complete_mask) ==
> > > 	    chip->ops->req_complete_val)
> > > 		goto out_recv;
> > > 
> > > There might be potential for a longer term cleanup using chip->status to
> > > cache things, but I'm little concerned that's going to open paths where we
> > > might not correctly update it, so I think it should be a separate piece.
> > > 
> > > (I'm motivated by the fact we've started to see the "Operation Canceled"
> > > error and I'd like us to close on the best way to fix it. :) )
> > 
> > This would work for me too!
> > 
> 
> Hi, and sorry for the late reply :(
> 
> I believe this option would work for us as well. Please let me know whether
> you'd like me to send V3 or you feel free to send it yourself if you want.

Please send one more patch. I just came from holiday and now have
hands full backtracking last two weeks.

> 
> --
> Kind regards,
> Ivan Orlov

BR, Jarkko

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

[Orlov, Ivan]

On 19/07/2025 12:37, Jarkko Sakkinen wrote:
>> I believe this option would work for us as well. Please let me know whether
>> you'd like me to send V3 or you feel free to send it yourself if you want.
> 
> Please send one more patch. I just came from holiday and now have
> hands full backtracking last two weeks.

Hi,

I've just sent a V3. Sorry for the delay, these were busy 2 weeks :(

--
Kind regards,
Ivan Orlov