tpm, tpm_tis: Fix timeout handling when waiting for TPM status
Subsystems (2)
PATCH 1/1 tpm, tpm_tis: Fix timeout handling when waiting for TPM status
From: Jonathan McDowell <[email protected]>
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.
--- drivers/char/tpm/tpm_tis_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/char/tpm/tpm_tis_core.c +2 -2
@@ -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 ; }
Signature · 1 line
2.48.1
Jarkko Sakkinen
maintainerQuoted 6 lines · Jonathan McDowell
From: Jonathan McDowell <[email protected]>
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...
Quoted 4 lines
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...
Quoted 31 lines
Cc [email protected] # v6.4+Fixes e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts")Signed-off-by Jonathan McDowell <[email protected]>Reviewed-by Michal Suchánek <[email protected]>--- drivers/char/tpm/tpm_tis_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/char/tpm/tpm_tis_core.c +2
-2@@ -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 ; }Signature · 1 line
2.48.1
The fix itself looks legit although you could just as well put it into a single line.
BR, Jarkko
Hi Jonathan,
Quoted 38 lines · Jonathan McDowell
From: Jonathan McDowell <[email protected]>
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 [email protected] # v6.4+Fixes e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts")Signed-off-by Jonathan McDowell <[email protected]>Reviewed-by Michal Suchánek <[email protected]>--- drivers/char/tpm/tpm_tis_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/char/tpm/tpm_tis_core.c +2
-3@@ -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 ; } --
Quoted 2 lines
2.48.1
FWIW:
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
PATCH v2 1/1 tpm, tpm_tis: Fix timeout handling when waiting for TPM status
In reply to Jonathan McDowellThe 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.
---
v2: Reword commit message
Don't needlessly wrap line
drivers/char/tpm/tpm_tis_core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/char/tpm/tpm_tis_core.c +1 -2
@@ -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 ; }
Signature · 1 line
2.48.1
Quoted 42 lines · Jonathan McDowell
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 [email protected] # v6.4+Fixes e87fcf0dc2b4 ("tpm, tpm_tis: Only handle supported interrupts")Signed-off-by Jonathan McDowell <[email protected]>Reviewed-by Michal Suchánek <[email protected]>Reviewed-by Lino Sanfilippo <[email protected]>---
v2: Reword commit message
Don't needlessly wrap linedrivers/char/tpm/tpm_tis_core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/char/tpm/tpm_tis_core.c +1
-2@@ -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 ; }Signature · 1 line
2.48.1
Probably apply late this and timeout fix late this week.
BR, Jarkko