> On Jul 1, 2021, at 11:35 PM, Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote: > > On Tue, Jun 29, 2021 at 09:22:05PM -0700, Hao Wu wrote: >> 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@xxxxxxxxxx/ >> >> 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 <hao.wu@xxxxxxxxxx> >> Fixes: 9f3fc7bcddcb ("tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers") > > Fixes tag should be before SOB. > >> --- >> 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 > >> + 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 >> + >> 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. > >> status = chip->ops->status(chip); >> if ((status & mask) == mask) >> return 0; >> @@ -934,6 +938,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >> chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX); >> chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX); >> chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX); >> + /* init timeout for wait_for_tpm_stat */ >> + chip->timeout_wait_stat = TPM_TIMEOUT_WAIT_STAT; >> priv->phy_ops = phy_ops; >> dev_set_drvdata(&chip->dev, priv); >> >> @@ -983,6 +989,15 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >> >> priv->manufacturer_id = vendor; >> >> + switch (priv->manufacturer_id) { >> + case TPM_VID_ATML: >> + /* ATMEL chip needs longer timeout to avoid crash */ Will fix the indentation Also according to Kenneth we only want to do so for TPM 1.2, I will try checking chip->flags against TPM_CHIP_FLAG_TPM2 here Let me know if there are concerns. >> + chip->timeout_wait_stat = TPM_ATML_TIMEOUT_WAIT_STAT; >> + break; >> + default: >> + chip->timeout_wait_stat = TPM_TIMEOUT_WAIT_STAT; >> + } >> + >> rc = tpm_tis_read8(priv, TPM_RID(0), &rid); >> if (rc < 0) >> goto out_err; >> diff --git a/include/linux/tpm.h b/include/linux/tpm.h >> index aa11fe323c56..35f2a0260d76 100644 >> --- a/include/linux/tpm.h >> +++ b/include/linux/tpm.h >> @@ -150,6 +150,7 @@ struct tpm_chip { >> bool timeout_adjusted; >> unsigned long duration[TPM_NUM_DURATIONS]; /* jiffies */ >> bool duration_adjusted; >> + unsigned long timeout_wait_stat; /* usecs */ >> >> struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; >> >> @@ -269,6 +270,7 @@ enum tpm2_cc_attrs { >> #define TPM_VID_INTEL 0x8086 >> #define TPM_VID_WINBOND 0x1050 >> #define TPM_VID_STM 0x104A >> +#define TPM_VID_ATML 0x1114 >> >> enum tpm_chip_flags { >> TPM_CHIP_FLAG_TPM2 = BIT(1), >> -- >> 2.29.0.vfs.0.0 >> >> > > /Jarkko
