On Fri, Jul 02, 2021 at 12:16:12PM -0700, Hao Wu wrote:

quoted

On Jul 2, 2021, at 4:57 AM, Jarkko Sakkinen [off-list ref] wrote:

On Fri, Jul 02, 2021 at 11:42:39AM +0300, Jarkko Sakkinen wrote:

quoted

On Fri, Jul 02, 2021 at 12:59:18AM -0700, Hao Wu wrote:

quoted

quoted

On Jul 2, 2021, at 12:45 AM, Jarkko Sakkinen [off-list ref] wrote:

On Fri, Jul 02, 2021 at 12:33:15AM -0700, Hao Wu wrote:

quoted

quoted

On Jul 1, 2021, at 11:35 PM, Jarkko Sakkinen [off-list ref] wrote:

On Tue, Jun 29, 2021 at 09:22:05PM -0700, Hao Wu wrote:

quoted

This is a fix for the ATMEL TPM crash bug reported in
https://patchwork.kernel.org/project/linux-integrity/patch/20200926223150.109645-1-hao.wu@rubrik.com/

According to the discussions in the original thread,
we don't want to revert the timeout of wait_for_tpm_stat
for non-ATMEL chips, which brings back the performance cost.
For investigation and analysis of why wait_for_tpm_stat
caused the issue, and how the regression was introduced,
please read the original thread above.

Thus the proposed fix here is to only revert the timeout
for ATMEL chips by checking the vendor ID.

Signed-off-by: Hao Wu <redacted>
Fixes: 9f3fc7bcddcb ("tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers")

Fixes tag should be before SOB.

quoted

---
Test Plan:
- Run fixed kernel with ATMEL TPM chips and see crash
has been fixed.
- Run fixed kernel with non-ATMEL TPM chips, and confirm
the timeout has not been changed.

drivers/char/tpm/tpm.h          |  9 ++++++++-
drivers/char/tpm/tpm_tis_core.c | 19 +++++++++++++++++--
include/linux/tpm.h             |  2 ++
3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 283f78211c3a..bc6aa7f9e119 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h

@@ -42,7 +42,9 @@ enum tpm_timeout {

	TPM_TIMEOUT_RANGE_US = 300,	/* usecs */
	TPM_TIMEOUT_POLL = 1,	/* msecs */
	TPM_TIMEOUT_USECS_MIN = 100,      /* usecs */
-	TPM_TIMEOUT_USECS_MAX = 500      /* usecs */
+	TPM_TIMEOUT_USECS_MAX = 500,	/* usecs */

What is this change?

Need to add the tailing comma

quoted

quoted

+	TPM_TIMEOUT_WAIT_STAT = 500,	/* usecs */
+	TPM_ATML_TIMEOUT_WAIT_STAT = 15000	/* usecs */
};

/* TPM addresses */

@@ -189,6 +191,11 @@ static inline void tpm_msleep(unsigned int delay_msec)

		     delay_msec * 1000);
};

+static inline void tpm_usleep(unsigned int delay_usec)
+{
+	usleep_range(delay_usec - TPM_TIMEOUT_RANGE_US, delay_usec);
+};

Please remove this, and open code.

Ok, will do

quoted

quoted

+
int tpm_chip_start(struct tpm_chip *chip);
void tpm_chip_stop(struct tpm_chip *chip);
struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 55b9d3965ae1..9ddd4edfe1c2 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c

@@ -80,8 +80,12 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,

		}
	} else {
		do {
-			usleep_range(TPM_TIMEOUT_USECS_MIN,
-				     TPM_TIMEOUT_USECS_MAX);
+			if (chip->timeout_wait_stat && 
+				chip->timeout_wait_stat >= TPM_TIMEOUT_WAIT_STAT) {
+				tpm_usleep((unsigned int)(chip->timeout_wait_stat));
+			} else {
+				tpm_usleep((unsigned int)(TPM_TIMEOUT_WAIT_STAT));
+			}

Invalid use of braces. Please read

https://www.kernel.org/doc/html/v5.13/process/coding-style.html

Why do you have to use this field conditionally anyway? Why doesn't
it always contain a legit value?

The field is legit now, but doesn’t hurt to do addition check for robustness 
to ensure no crash ? Just in case the value is updated below TPM_TIMEOUT_WAIT_STAT ? 

Can remove if we think it is not needed.

A simple question: why you use it conditionally? Can the field contain invalid value?

There are two checks
- chip->timeout_wait_stat >= TPM_TIMEOUT_WAIT_STAT
It could be invalid when future developer set it to some value less than `TPM_TIMEOUT_USECS_MIN`,
and crash the usleep 

I don't understand this. Why you don't set to appropriate value?

Ok, fair enough, I assume developers will test it anyway to ensure no crash. Will remove this check.

quoted

What you should do, is to define two fields:

- tpm_timeout_min
- tpm_timeout_max

And initialize these to TPM_TIMEOUT_USECS_MIN and TPM_TIMEOUT_USECS_MAX.

Then fixup those for Atmel (with a simple if-statement, switch-case is
overkill).

Switch was more for extensibility when other vendor has similar issue,
but we can refactor when needed in the future. I can use if-statement for now.

Make things more fancy *only* when you actually need more fancy.

quoted

The way you work out things right now is broken:

1. Before for non-Atmel: usleep_range(100, 500)
2. After for non-Atmel: usleep_range(200, 500)

I realized this in day-1, I think this range change does not matter much.

By saying that you are actually saying that *undocumented* semantic changes
to the kernel code are fine as long as they don't change things "too much"

Are you serious about this?

`TPM_TIMEOUT_RANGE_US=300` is already used in the codebase, I assume people define
such if for general use cases for usleep_range in TPM
But we can add two fields if that makes us more comfortable to strictly follow the current code
semantically.

This has absolutely nothing to do with "comfortable". It's black and white
wrong.

/Jarkko