[v1] tpm, tpm_tis: Fix timeout handling when waiting for TPM status

[PATCH] tpm, tpm_tis: Fix timeout handling when waiting for TPM status

Posted by Jonathan McDowell 11 months, 1 week ago

From: Jonathan McDowell <noodles@meta.com>

The change to only use interrupts to handle supported status changes,
then switch to polling for the rest, inverted the status test and sleep
such that we can end up sleeping beyond our timeout and not actually
checking the status. This can result in spurious TPM timeouts,
especially on a more loaded system. Fix by switching the order back so
we sleep *then* check. We've done a up front check when we enter the
function so this won't cause an additional delay when the status is
already what we're looking for.

Cc: stable@vger.kernel.org # v6.4+
Fixes: e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts")
Signed-off-by: Jonathan McDowell <noodles@meta.com>
Reviewed-by: Michal Suchánek <msuchanek@suse.de>
---
 drivers/char/tpm/tpm_tis_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fdef214b9f6b..167d71747666 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -114,11 +114,11 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 		return 0;
 	/* process status changes without irq support */
 	do {
+		usleep_range(priv->timeout_min,
+			     priv->timeout_max);
 		status = chip->ops->status(chip);
 		if ((status & mask) == mask)
 			return 0;
-		usleep_range(priv->timeout_min,
-			     priv->timeout_max);
 	} while (time_before(jiffies, stop));
 	return -ETIME;
 }
-- 
2.48.1

Re: [PATCH] tpm, tpm_tis: Fix timeout handling when waiting for TPM status

Posted by Lino Sanfilippo 11 months, 1 week ago

Hi Jonathan,

On 05.03.25 10:45, Jonathan McDowell wrote:

> From: Jonathan McDowell <noodles@meta.com>
> 
> The change to only use interrupts to handle supported status changes,
> then switch to polling for the rest, inverted the status test and sleep
> such that we can end up sleeping beyond our timeout and not actually
> checking the status. This can result in spurious TPM timeouts,
> especially on a more loaded system. Fix by switching the order back so
> we sleep *then* check. We've done a up front check when we enter the
> function so this won't cause an additional delay when the status is
> already what we're looking for.
> 
> Cc: stable@vger.kernel.org # v6.4+
> Fixes: e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts")
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> Reviewed-by: Michal Suchánek <msuchanek@suse.de>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index fdef214b9f6b..167d71747666 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -114,11 +114,11 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
>                 return 0;
>         /* process status changes without irq support */
>         do {
> +               usleep_range(priv->timeout_min,
> +                            priv->timeout_max);
>                 status = chip->ops->status(chip);
>                 if ((status & mask) == mask)
>                         return 0;
> -               usleep_range(priv->timeout_min,
> -                            priv->timeout_max);
>         } while (time_before(jiffies, stop));
>         return -ETIME;
>  }
> --
> 2.48.1
> 

FWIW:
Reviewed-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>

I cannot remember any more but I think the change of the logic in 
the while loop must have been a leftover from some tests I did
at this time. However it should not have been part of the patch, so
good that you found it and thanks for the fix!

BR,
Lino

Re: [PATCH] tpm, tpm_tis: Fix timeout handling when waiting for TPM status

Posted by Jarkko Sakkinen 11 months, 1 week ago

On Wed, Mar 05, 2025 at 09:45:15AM +0000, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> The change to only use interrupts to handle supported status changes,
> then switch to polling for the rest, inverted the status test and sleep
> such that we can end up sleeping beyond our timeout and not actually
> checking the status. This can result in spurious TPM timeouts,

I *really* have hard time understanding what I'm reading the first
sentence *but* I do understand the code change. Maybe you could try
to be a bit more punctual there...

> especially on a more loaded system. Fix by switching the order back so
> we sleep *then* check. We've done a up front check when we enter the
> function so this won't cause an additional delay when the status is
> already what we're looking for.

Remove the use of we-pronoun: it is best for science papers. Also, e.g.
why not just say "switch" instead of "fix by switching" ? :-)

I'd rewrite this tail part like:

"Switch the order back where it was i.e., sleep before check."

Instead of "upfront check" it'd nice to be a bit more specific...

> 
> Cc: stable@vger.kernel.org # v6.4+
> Fixes: e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts")
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> Reviewed-by: Michal Suchánek <msuchanek@suse.de>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index fdef214b9f6b..167d71747666 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -114,11 +114,11 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
>  		return 0;
>  	/* process status changes without irq support */
>  	do {
> +		usleep_range(priv->timeout_min,
> +			     priv->timeout_max);
>  		status = chip->ops->status(chip);
>  		if ((status & mask) == mask)
>  			return 0;
> -		usleep_range(priv->timeout_min,
> -			     priv->timeout_max);
>  	} while (time_before(jiffies, stop));
>  	return -ETIME;
>  }
> -- 
> 2.48.1
> 
> 

The fix itself looks legit although you could just as well put it into a
single line.

BR, Jarkko

[PATCH v2] tpm, tpm_tis: Fix timeout handling when waiting for TPM status

Posted by Jonathan McDowell 11 months, 1 week ago

The change to only use interrupts to handle supported status changes
introduced an issue when it is necessary to poll for the status. Rather
than checking for the status after sleeping the code now sleeps after
the check. This means a correct, but slower, status change on the part
of the TPM can be missed, resulting in a spurious timeout error,
especially on a more loaded system. Switch back to sleeping *then*
checking. An up front check of the status has been done at the start of
the function, so this does not cause an additional delay when the status
is already what we're looking for.

Cc: stable@vger.kernel.org # v6.4+
Fixes: e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts")
Signed-off-by: Jonathan McDowell <noodles@meta.com>
Reviewed-by: Michal Suchánek <msuchanek@suse.de>
Reviewed-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
v2: Reword commit message
     Don't needlessly wrap line

  drivers/char/tpm/tpm_tis_core.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fdef214b9f6b..c969a1793184 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -114,11 +114,10 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
  		return 0;
  	/* process status changes without irq support */
  	do {
+		usleep_range(priv->timeout_min, priv->timeout_max);
  		status = chip->ops->status(chip);
  		if ((status & mask) == mask)
  			return 0;
-		usleep_range(priv->timeout_min,
-			     priv->timeout_max);
  	} while (time_before(jiffies, stop));
  	return -ETIME;
  }
-- 
2.48.1

Re: [PATCH v2] tpm, tpm_tis: Fix timeout handling when waiting for TPM status

Posted by Jarkko Sakkinen 11 months ago

On Mon, Mar 10, 2025 at 12:22:08PM +0000, Jonathan McDowell wrote:
> The change to only use interrupts to handle supported status changes
> introduced an issue when it is necessary to poll for the status. Rather
> than checking for the status after sleeping the code now sleeps after
> the check. This means a correct, but slower, status change on the part
> of the TPM can be missed, resulting in a spurious timeout error,
> especially on a more loaded system. Switch back to sleeping *then*
> checking. An up front check of the status has been done at the start of
> the function, so this does not cause an additional delay when the status
> is already what we're looking for.
> 
> Cc: stable@vger.kernel.org # v6.4+
> Fixes: e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts")
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> Reviewed-by: Michal Suchánek <msuchanek@suse.de>
> Reviewed-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
> v2: Reword commit message
>     Don't needlessly wrap line
> 
>  drivers/char/tpm/tpm_tis_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index fdef214b9f6b..c969a1793184 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -114,11 +114,10 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
>  		return 0;
>  	/* process status changes without irq support */
>  	do {
> +		usleep_range(priv->timeout_min, priv->timeout_max);
>  		status = chip->ops->status(chip);
>  		if ((status & mask) == mask)
>  			return 0;
> -		usleep_range(priv->timeout_min,
> -			     priv->timeout_max);
>  	} while (time_before(jiffies, stop));
>  	return -ETIME;
>  }
> -- 
> 2.48.1
> 

Probably apply late this and timeout fix late this week.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko