> On Jul 4, 2021, at 10:19 PM, Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote: > > On Fri, Jul 02, 2021 at 12:16:12PM -0700, Hao Wu wrote: >> >>> On Jul 2, 2021, at 4:57 AM, Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote: >>> >>> On Fri, Jul 02, 2021 at 11:42:39AM +0300, Jarkko Sakkinen wrote: >>>> On Fri, Jul 02, 2021 at 12:59:18AM -0700, Hao Wu wrote: >>>>>> On Jul 2, 2021, at 12:45 AM, Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote: >>>>>> >>>>>> On Fri, Jul 02, 2021 at 12:33:15AM -0700, Hao Wu 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 >>>>>>> >>>>>>>> >>>>>>>>> + 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. >>>>>> >>>>>> 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. >> >>> 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. > >>> 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? Fair enough, I agree that keeping things as it avoid potential issues. Thanks for pointing this out! > >> `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 I believe the comments are addressed in https://patchwork.kernel.org/project/linux-integrity/patch/20210704000754.1384-1-hao.wu@xxxxxxxxxx/ Have tested it with ATMEL 1.2 chip. Thanks Hao
