> On Jul 2, 2021, at 12:33 AM, Hao Wu <hao.wu@xxxxxxxxxx> wrote: > > > >> 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 Ah, sorry, didn’t notice added the duplicated line by mistake. Will remove it. > >> >>> + 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 Hao
