Re: [Bug Report] Kernel 4.14+ TPM Driver Bug for Atmel TPM Chip

[hamza@hpe.com]

Hello everyone,

Dear Hao,

The first aim of the patch you mentioned is not to shorten TPM timings, 
as no
timing is changed at all on this patch, but to count time more 
accurately.

If your TPM needs more time to perform a specific operation, it has 
nothing to
do with this patch, but rather with its specified timings in the 
kernel. The
timings for your Atmel TPM were certainly wrong before 4.14 to begin 
with, they
were not counted accurately and your TPM working was potentially a 
"positive"
side effect of the imprecise time counting (ie non-specified delays 
were added
every time msleep was used instead of usleep_range).
TPM polling should not be affected because we use a different way to 
count
timings in the kernel.

If you see issues with your Atmel TPM, I'd suggest to make the changes 
in the
tpm_atmel code only (tpm_infineon would be a suitable example to look 
at) and
discuss the timing values with Atmel TPMs experts, but reverting this 
patch and
affecting all the other TPMs seems like a misunderstanding.

Thanks,
Hamza ATTAK.

On Sat, Sep 12, 2020 at 10:14, Paul Menzel <pmenzel@molgen.mpg.de> 
wrote:
> Dear Hao,
> 
> 
> Thank you for the reply.
> 
> Am 12.09.20 um 10:10 schrieb Hao Wu:
> 
>> Thanks for quick responses over this report.
> 
> Thank you for the quick follow-up.
> 
>>> Hao, I wouldn’t expect a longer timeout causing the TPM to be
>>> queried less frequently, but I do not know the code well.
>> From our understanding, the TPM queries might be retried due to some
>> reason, increase timeout 3x would lower the query frequency to 1/3.
>> The explanation might be wrong, but the fact looks like the timeout
>> matters.  Unfortunately, engineers from Rubrik are not experts over
>> the TPM driver code neither :(
>> 
>>>> Be careful about making this a global change.  It could reduce
>>>> the TPM performance by 3x. We don't want to affect all TPMs to
>>>> fix a bug in an old TPM 1.2 chip from one vendor.
>>> 
>>> Linux has a no regression policy, so the performance penalty
>>> wouldn’t matter. Unfortunately, the regression was only noticed
>>> several years after being introduced in Linux v4.14-rc2.
>> 
>> So does that mean we are good to apply the global change ? Or we need
>> to discuss about the actual fix further?
> To get a fix into the stable series, it first needs to be applied to 
> the master branch. I guess you tested also with Linux master, right?
> 
> Please add the explanation from your email to Greg into the git 
> commit message, format the patch with `git format-patch -1 -o 
> outgoing` and send it with `git send-email outgoing/*` to the 
> addresses listed for the subsystem in `MAINTAINERS` and the people 
> listed in the commit introducing the regression.
> 
> Then it can be properly reviewed and discussed.
> 
> 
> Kind regards,
> 
> Paul