On 5/16/22 13:57, Jarkko Sakkinen wrote:
On Thu, May 12, 2022 at 08:32:55AM -0400, James Bottomley wrote:
On Thu, 2022-05-12 at 08:21 -0400, Mimi Zohar wrote:
On Wed, 2022-05-11 at 18:16 +0300, Jarkko Sakkinen wrote:
On Tue, May 10, 2022 at 01:29:03PM +0200, Johannes Holland wrote:
To comply with protocol requirements, minimum polling times must
often
be adhered to. Therefore, a macro like tpm_msleep() should sleep
at
least the given amount of time (not up to the given period). Have
tpm_msleep() sleep at least the given number of milliseconds.
Signed-off-by: Johannes Holland <johannes.holland@xxxxxxxxxxxx>
---
drivers/char/tpm/tpm.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2163c6ee0d36..0971b55fffe3 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -185,8 +185,8 @@ int tpm_pm_resume(struct device *dev);
static inline void tpm_msleep(unsigned int delay_msec)
{
- usleep_range((delay_msec * 1000) - TPM_TIMEOUT_RANGE_US,
- delay_msec * 1000);
+ usleep_range(delay_msec * 1000, (delay_msec * 1000)
+ + TPM_TIMEOUT_RANGE_US);
};
int tpm_chip_start(struct tpm_chip *chip);
--
2.34.1
For this I would really like to hear a 2nd opinion from Nayna and
Mimi.
This patch reverts commit 5ef924d9e2e8 ("tpm: use tpm_msleep() value
as max delay"). Are you experiencing TPM issues that require it?
I am:
https://lore.kernel.org/linux-integrity/1531328689.3260.8.camel@xxxxxxxxxxxxxxxxxxxxx/
I'm about 24h into a soak test of the patch with no TPM failure so far.
I think it probably needs to run another 24h just to be sure, but it
does seem the theory is sound (my TPM gets annoyed by being poked too
soon) so reverting 5ef924d9e2e8 looks to be the correct action. The
only other ways I've found to fix this are either revert the
usleep_range patch altogether or increase the timings:
https://lore.kernel.org/linux-integrity/1531329074.3260.9.camel@xxxxxxxxxxxxxxxxxxxxx/
Which obviously pushes the min past whatever issue my TPM is having
even with 5ef924d9e2e8 applied.
Given that even the commit message for 5ef924d9e2e8 admits it only
shaves about 12% off the TPM response time, that would appear to be an
optimization too far if it's going to cause some TPMs to fail.
James
What if TPM started with the timings as they are now and use the
"reverted" timings if coming up too early? The question here is
though, is such complexity worth of anything or should we just
revert and do nothing else.
TCG Specification(TCG PC Client Device Driver Design Principles, Section
10), says - General control timeouts, denoted as TIMEOUT_A (A),
TIMEOUT_B (B), TIMEOUT_C (C) and TIMEOUT_D (D), are the maximum waiting
time from a certain control operation from the DD until the TPM shows
the expected status change.
usleep_range(min, max) takes second parameter as max value which is same
as maximum delay in our case. Thus the code is in compliance with the
standard already. We still haven't heard back yet as to the reason for
this change, nor if the problem is pervasive.
Lastly, a 12% improvement makes a difference or not depends on usecase.
It did at least in our case.
Thanks & Regards,
- Nayna
