Re: [PATCH v7 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Mon, Jun 20, 2016 at 06:54:02PM -0700, Ed Swierk wrote:
> The STMicro ST19NP18-TPM sometimes takes much longer to execute
> commands than it reports in its capabilities. For example, command 186
> (TPM_FlushSpecific) has been observed to take 14560 msec to complete,
> far longer than the 3000 msec limit for "short" commands reported by
> the chip. The behavior has also been seen with command 101
> (TPM_GetCapability).
> 
> Worse, when the tpm_tis driver attempts to cancel the current command
> (by writing commandReady = 1 to TPM_STS_x), the chip locks up
> completely, returning all-1s from all memory-mapped register
> reads. The lockup can be cleared only by resetting the system.
> 
> The occurrence of this excessive command duration depends on the
> sequence of commands preceding it. One sequence is creating at least 2
> new keys via TPM_CreateWrapKey, then letting the TPM idle for at least
> 30 seconds, then loading a key via TPM_LoadKey2. The next
> TPM_FlushSpecific occasionally takes tens of seconds to
> complete. Another sequence is creating many keys in a row without
> pause. The TPM_CreateWrapKey operation gets much slower after the
> first few iterations, as one would expect when the pool of precomputed
> keys is exhausted. Then after a 35-second pause, the same TPM_LoadKey2
> followed by TPM_FlushSpecific sequence triggers the behavior.
> 
> Our working theory is that this older TPM sometimes pauses to
> precompute keys, which modern chips implement as a background
> process. Without access to the chip's implementation details it's
> impossible to know whether any commands are immune to being blocked by
> this process. So it seems safest to ignore the chip's reported command
> durations, and use a value much higher than any observed duration,
> like 180 sec (which is the duration this chip reports for "long"
> commands).
> 
> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>

I think this fine but I would like to hear other opinions on this.

Stefan?

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index caf7278..8355b45 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -485,6 +485,12 @@ static void tpm_tis_update_timeouts(struct tpm_chip *chip)
>  		chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
>  		chip->vendor.timeout_adjusted = true;
>  		break;
> +	case 0x0000104a: /* STMicro ST19NP18-TPM */
> +		chip->vendor.duration[TPM_SHORT] = 180 * HZ;
> +		chip->vendor.duration[TPM_MEDIUM] = 180 * HZ;
> +		chip->vendor.duration[TPM_LONG] = 180 * HZ;
> +		chip->vendor.duration_adjusted = true;
> +		break;
>  	}
>  }
>  
> -- 
> 1.9.1
>