* [PATCH] tpm: sleep at least <...> ms in tpm_msleep() @ 2022-05-10 11:29 Johannes Holland 2022-05-11 15:16 ` Jarkko Sakkinen 2022-05-11 20:12 ` James Bottomley 0 siblings, 2 replies; 12+ messages in thread From: Johannes Holland @ 2022-05-10 11:29 UTC (permalink / raw) To: jarkko, linux-integrity, linux-kernel; +Cc: peterhuewe, jgg, Johannes Holland 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@infineon.com> --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-10 11:29 [PATCH] tpm: sleep at least <...> ms in tpm_msleep() Johannes Holland @ 2022-05-11 15:16 ` Jarkko Sakkinen 2022-05-12 12:21 ` Mimi Zohar 2022-05-11 20:12 ` James Bottomley 1 sibling, 1 reply; 12+ messages in thread From: Jarkko Sakkinen @ 2022-05-11 15:16 UTC (permalink / raw) To: Johannes Holland, Nayna, Mimi Zohar Cc: linux-integrity, linux-kernel, peterhuewe, jgg 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@infineon.com> > --- > 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. BR, Jarkko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-11 15:16 ` Jarkko Sakkinen @ 2022-05-12 12:21 ` Mimi Zohar 2022-05-12 12:32 ` James Bottomley 2022-05-16 17:54 ` Jarkko Sakkinen 0 siblings, 2 replies; 12+ messages in thread From: Mimi Zohar @ 2022-05-12 12:21 UTC (permalink / raw) To: Jarkko Sakkinen, Johannes Holland, Nayna Cc: linux-integrity, linux-kernel, peterhuewe, jgg 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@infineon.com> > > --- > > 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? thanks, Mimi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-12 12:21 ` Mimi Zohar @ 2022-05-12 12:32 ` James Bottomley 2022-05-12 16:52 ` Mimi Zohar 2022-05-16 17:57 ` Jarkko Sakkinen 2022-05-16 17:54 ` Jarkko Sakkinen 1 sibling, 2 replies; 12+ messages in thread From: James Bottomley @ 2022-05-12 12:32 UTC (permalink / raw) To: Mimi Zohar, Jarkko Sakkinen, Johannes Holland, Nayna Cc: linux-integrity, linux-kernel, peterhuewe, jgg 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@infineon.com> > > > --- > > > 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@HansenPartnership.com/ 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@HansenPartnership.com/ 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-12 12:32 ` James Bottomley @ 2022-05-12 16:52 ` Mimi Zohar 2022-05-16 17:57 ` Jarkko Sakkinen 1 sibling, 0 replies; 12+ messages in thread From: Mimi Zohar @ 2022-05-12 16:52 UTC (permalink / raw) To: James Bottomley, Jarkko Sakkinen, Johannes Holland, Nayna Cc: linux-integrity, linux-kernel, peterhuewe, jgg On Thu, 2022-05-12 at 08:32 -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@infineon.com> > > > > --- > > > > 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@HansenPartnership.com/ > > 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@HansenPartnership.com/ > > 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. I'd like to understand how pervasive the problem is and which problem Johannes Holland is trying to address. Wasn't there already a patch to limit TPM performance degradation to a single buggy TPM chip already upstreamed? thanks, Mimi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-12 12:32 ` James Bottomley 2022-05-12 16:52 ` Mimi Zohar @ 2022-05-16 17:57 ` Jarkko Sakkinen 2022-05-18 19:26 ` Nayna 1 sibling, 1 reply; 12+ messages in thread From: Jarkko Sakkinen @ 2022-05-16 17:57 UTC (permalink / raw) To: James Bottomley Cc: Mimi Zohar, Johannes Holland, Nayna, linux-integrity, linux-kernel, peterhuewe, jgg 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@infineon.com> > > > > --- > > > > 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@HansenPartnership.com/ > > 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@HansenPartnership.com/ > > 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. BR, Jarkko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-16 17:57 ` Jarkko Sakkinen @ 2022-05-18 19:26 ` Nayna 2022-05-18 20:21 ` James Bottomley 0 siblings, 1 reply; 12+ messages in thread From: Nayna @ 2022-05-18 19:26 UTC (permalink / raw) To: Jarkko Sakkinen, James Bottomley, Johannes Holland Cc: Mimi Zohar, linux-integrity, linux-kernel, peterhuewe, jgg 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@infineon.com> >>>>> --- >>>>> 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@HansenPartnership.com/ >> >> 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@HansenPartnership.com/ >> >> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-18 19:26 ` Nayna @ 2022-05-18 20:21 ` James Bottomley 2022-05-27 21:37 ` Ken Goldman 0 siblings, 1 reply; 12+ messages in thread From: James Bottomley @ 2022-05-18 20:21 UTC (permalink / raw) To: Nayna, Jarkko Sakkinen, Johannes Holland Cc: Mimi Zohar, linux-integrity, linux-kernel, peterhuewe, jgg On Wed, 2022-05-18 at 15:26 -0400, Nayna wrote: > 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: [...] > > > > 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@HansenPartnership.com/ > > > > > > 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@HansenPartnership.com/ > > > > > > 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. Actually, this is nothing to do with the TIMEOUTS_A-D: those are the maximum times before a command should complete. This is the minimum time we should wait between pokes of the TPM to see if it is ready. Usually the use case is: while (read device status gives not ready) tpm_msleep(something) The tpm_msleep gives up CPU control (to prevent huge amounts of busy waiting) but before 424eaf910c32 ("tpm: reduce polling time to usecs for even finer granularity") we would sleep for an entire tick (time taken to make the process runnable) before the next poll, and since most TPM commands don't return immediately, that was a gate on how fast you could do simple TPM operations (like PCR extend). As far as I know, no TCG spec gives any details of the minimum wait time between poll cycles, so this is really something the manufacturer has to tell us. Just for completeness, my soak test did run to completion, but my TPM has since failed and dropped off the bus, so simply reverting this patch (5ef924d9e2e8) isn't sufficient to fully fix my problem. James ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-18 20:21 ` James Bottomley @ 2022-05-27 21:37 ` Ken Goldman 0 siblings, 0 replies; 12+ messages in thread From: Ken Goldman @ 2022-05-27 21:37 UTC (permalink / raw) To: James Bottomley, Nayna, Jarkko Sakkinen, Johannes Holland Cc: Mimi Zohar, linux-integrity, linux-kernel, peterhuewe, jgg [-- Attachment #1: Type: text/plain, Size: 1494 bytes --] On 5/18/2022 4:21 PM, James Bottomley wrote: > Actually, this is nothing to do with the TIMEOUTS_A-D: those are the > maximum times before a command should complete. This is the minimum > time we should wait between pokes of the TPM to see if it is ready. > Usually the use case is: > > while (read device status gives not ready) > tpm_msleep(something) > > The tpm_msleep gives up CPU control (to prevent huge amounts of busy > waiting) but before 424eaf910c32 ("tpm: reduce polling time to usecs > for even finer granularity") we would sleep for an entire tick (time > taken to make the process runnable) before the next poll, and since > most TPM commands don't return immediately, that was a gate on how fast > you could do simple TPM operations (like PCR extend). > > As far as I know, no TCG spec gives any details of the minimum wait > time between poll cycles, so this is really something the manufacturer > has to tell us. > > Just for completeness, my soak test did run to completion, but my TPM > has since failed and dropped off the bus, so simply reverting this > patch (5ef924d9e2e8) isn't sufficient to fully fix my problem. [FYI] The TPM vendors explained that polling interrupts TPM command processing. This will make commands take longer. They recommend adaptive sleep time based on (at least) the command. E.g., getcapability, pcrread, ... are fast so a short sleep is appropriate. create, createprimary are slow so a longer sleep will have better performance. [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4490 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-12 12:21 ` Mimi Zohar 2022-05-12 12:32 ` James Bottomley @ 2022-05-16 17:54 ` Jarkko Sakkinen 2022-06-20 15:58 ` Stefan Mahnke-Hartmann 1 sibling, 1 reply; 12+ messages in thread From: Jarkko Sakkinen @ 2022-05-16 17:54 UTC (permalink / raw) To: Mimi Zohar Cc: Johannes Holland, Nayna, linux-integrity, linux-kernel, peterhuewe, jgg On Thu, May 12, 2022 at 08:21:17AM -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@infineon.com> > > > --- > > > 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? > > thanks, > > Mimi Yeah, there's no data to support making anything. Without a live system having issues with this, I guess this quite definitive NAK. BR, Jarkko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-16 17:54 ` Jarkko Sakkinen @ 2022-06-20 15:58 ` Stefan Mahnke-Hartmann 0 siblings, 0 replies; 12+ messages in thread From: Stefan Mahnke-Hartmann @ 2022-06-20 15:58 UTC (permalink / raw) To: Jarkko Sakkinen, Mimi Zohar Cc: Johannes Holland, Nayna, linux-integrity, linux-kernel, peterhuewe, jgg, stefan.mahnke-hartmann On 16.05.22 19:54, Jarkko Sakkinen wrote: > On Thu, May 12, 2022 at 08:21:17AM -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@infineon.com> >>>> --- >>>> 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? >> >> thanks, >> >> Mimi > Yeah, there's no data to support making anything. Without a live > system having issues with this, I guess this quite definitive NAK. > > BR, Jarkko > Either way, a static range is not perfect. So I would suggest,to have the values (TPM_TIMEOUT_RANGE_US, TPM_TIMEOUT_POLL) settable, to give the possibility to adjust the polling timing vendor specific. Furthermore, increasing the polling delay as a function of time would limit the number of attempts, especially for long-running commands. BR, Stefan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] tpm: sleep at least <...> ms in tpm_msleep() 2022-05-10 11:29 [PATCH] tpm: sleep at least <...> ms in tpm_msleep() Johannes Holland 2022-05-11 15:16 ` Jarkko Sakkinen @ 2022-05-11 20:12 ` James Bottomley 1 sibling, 0 replies; 12+ messages in thread From: James Bottomley @ 2022-05-11 20:12 UTC (permalink / raw) To: Johannes Holland, jarkko, linux-integrity, linux-kernel; +Cc: peterhuewe, jgg On Tue, 2022-05-10 at 13:29 +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@infineon.com> This seems to fix the problem I reported years ago with my Nuvoton laptop TPM. https://lore.kernel.org/linux-integrity/1531328689.3260.8.camel@HansenPartnership.com/ It probably needs to soak a bit longer to declare victory (this timeout failure is very intermittent), but it looks hopeful. Regards, James ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-06-20 15:57 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-10 11:29 [PATCH] tpm: sleep at least <...> ms in tpm_msleep() Johannes Holland 2022-05-11 15:16 ` Jarkko Sakkinen 2022-05-12 12:21 ` Mimi Zohar 2022-05-12 12:32 ` James Bottomley 2022-05-12 16:52 ` Mimi Zohar 2022-05-16 17:57 ` Jarkko Sakkinen 2022-05-18 19:26 ` Nayna 2022-05-18 20:21 ` James Bottomley 2022-05-27 21:37 ` Ken Goldman 2022-05-16 17:54 ` Jarkko Sakkinen 2022-06-20 15:58 ` Stefan Mahnke-Hartmann 2022-05-11 20:12 ` James Bottomley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).