Re: [PATCH v2] tpm: use a map for tpm2_calc_ordinal_duration()

["Serge E. Hallyn" <serge@hallyn.com>]

On Thu, Sep 18, 2025 at 10:30:18PM +0300, Jarkko Sakkinen wrote:
> The current shenanigans for duration calculation introduce too much
> complexity for a trivial problem, and further the code is hard to patch and
> maintain.
> 
> Address these issues with a flat look-up table, which is easy to understand
> and patch. If leaf driver specific patching is required in future, it is
> easy enough to make a copy of this table during driver initialization and
> add the chip parameter back.
> 
> 'chip->duration' is retained for TPM 1.x.
> 
> As the first entry for this new behavior address TCG spec update mentioned
> in this issue:
> 
> https://github.com/raspberrypi/linux/issues/7054
> 
> Therefore, for TPM_SelfTest the duration is set to 3000 ms.
> 
> This does not categorize a as bug, given that this is introduced to the
> spec after the feature was originally made.
> 
> Cc: Frédéric Jouen <fjouen@sealsq.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

fwiw (which shouldn't be much) looks good to me, but two questions,
one here and one below.

First, it looks like in the existing code it is possible for a tpm2
chip to set its own timeouts and then set the TPM_CHIP_FLAG_HAVE_TIMEOUTS
flag to avoid using the defaults, but I don't see anything using that
in-tree.  Is it possible that there are out of tree drivers that will be
sabotaged here?  Or am I misunderstanding that completely?

> ---
> v2:
> - Add the missing msec_to_jiffies() calls.
> - Drop redundant stuff.
> ---
>  drivers/char/tpm/tpm-interface.c |   2 +-
>  drivers/char/tpm/tpm.h           |   2 +-
>  drivers/char/tpm/tpm2-cmd.c      | 127 ++++++++-----------------------
>  include/linux/tpm.h              |   5 +-
>  4 files changed, 37 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index b71725827743..c9f173001d0e 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -52,7 +52,7 @@ MODULE_PARM_DESC(suspend_pcr,
>  unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
>  {
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> -		return tpm2_calc_ordinal_duration(chip, ordinal);
> +		return tpm2_calc_ordinal_duration(ordinal);
>  	else
>  		return tpm1_calc_ordinal_duration(chip, ordinal);
>  }
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 7bb87fa5f7a1..2726bd38e5ac 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -299,7 +299,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
>  ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
>  int tpm2_auto_startup(struct tpm_chip *chip);
>  void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
> -unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> +unsigned long tpm2_calc_ordinal_duration(u32 ordinal);
>  int tpm2_probe(struct tpm_chip *chip);
>  int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip);
>  int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 524d802ede26..7d77f6fbc152 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -28,120 +28,57 @@ static struct tpm2_hash tpm2_hash_map[] = {
>  
>  int tpm2_get_timeouts(struct tpm_chip *chip)
>  {
> -	/* Fixed timeouts for TPM2 */
>  	chip->timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A);
>  	chip->timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B);
>  	chip->timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C);
>  	chip->timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D);
> -
> -	/* PTP spec timeouts */
> -	chip->duration[TPM_SHORT] = msecs_to_jiffies(TPM2_DURATION_SHORT);
> -	chip->duration[TPM_MEDIUM] = msecs_to_jiffies(TPM2_DURATION_MEDIUM);
> -	chip->duration[TPM_LONG] = msecs_to_jiffies(TPM2_DURATION_LONG);
> -
> -	/* Key creation commands long timeouts */
> -	chip->duration[TPM_LONG_LONG] =
> -		msecs_to_jiffies(TPM2_DURATION_LONG_LONG);
> -
>  	chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
> -
>  	return 0;
>  }
>  
> -/**
> - * tpm2_ordinal_duration_index() - returns an index to the chip duration table
> - * @ordinal: TPM command ordinal.
> - *
> - * The function returns an index to the chip duration table
> - * (enum tpm_duration), that describes the maximum amount of
> - * time the chip could take to return the result for a  particular ordinal.
> - *
> - * The values of the MEDIUM, and LONG durations are taken
> - * from the PC Client Profile (PTP) specification (750, 2000 msec)
> - *
> - * LONG_LONG is for commands that generates keys which empirically takes
> - * a longer time on some systems.
> - *
> - * Return:
> - * * TPM_MEDIUM
> - * * TPM_LONG
> - * * TPM_LONG_LONG
> - * * TPM_UNDEFINED
> +/*
> + * Contains the maximum durations in milliseconds for TPM2 commands.
>   */
> -static u8 tpm2_ordinal_duration_index(u32 ordinal)
> -{
> -	switch (ordinal) {
> -	/* Startup */
> -	case TPM2_CC_STARTUP:                 /* 144 */
> -		return TPM_MEDIUM;
> -
> -	case TPM2_CC_SELF_TEST:               /* 143 */
> -		return TPM_LONG;
> -
> -	case TPM2_CC_GET_RANDOM:              /* 17B */
> -		return TPM_LONG;
> -
> -	case TPM2_CC_SEQUENCE_UPDATE:         /* 15C */
> -		return TPM_MEDIUM;
> -	case TPM2_CC_SEQUENCE_COMPLETE:       /* 13E */
> -		return TPM_MEDIUM;
> -	case TPM2_CC_EVENT_SEQUENCE_COMPLETE: /* 185 */
> -		return TPM_MEDIUM;
> -	case TPM2_CC_HASH_SEQUENCE_START:     /* 186 */
> -		return TPM_MEDIUM;
> -
> -	case TPM2_CC_VERIFY_SIGNATURE:        /* 177 */
> -		return TPM_LONG_LONG;
> -
> -	case TPM2_CC_PCR_EXTEND:              /* 182 */
> -		return TPM_MEDIUM;
> -
> -	case TPM2_CC_HIERARCHY_CONTROL:       /* 121 */
> -		return TPM_LONG;
> -	case TPM2_CC_HIERARCHY_CHANGE_AUTH:   /* 129 */
> -		return TPM_LONG;
> -
> -	case TPM2_CC_GET_CAPABILITY:          /* 17A */
> -		return TPM_MEDIUM;
> -
> -	case TPM2_CC_NV_READ:                 /* 14E */
> -		return TPM_LONG;
> -
> -	case TPM2_CC_CREATE_PRIMARY:          /* 131 */
> -		return TPM_LONG_LONG;
> -	case TPM2_CC_CREATE:                  /* 153 */
> -		return TPM_LONG_LONG;
> -	case TPM2_CC_CREATE_LOADED:           /* 191 */
> -		return TPM_LONG_LONG;
> -
> -	default:
> -		return TPM_UNDEFINED;
> -	}
> -}
> +static const struct {
> +	unsigned long ordinal;
> +	unsigned long duration;
> +} tpm2_ordinal_duration_map[] = {
> +	{TPM2_CC_STARTUP, 750},
> +	{TPM2_CC_SELF_TEST, 3000},

I assume you intended to increase TPM2_CC_SELF_TEST from 2000 to 3000
here?  But it's not mentioned in the commit, so making sure...

> +	{TPM2_CC_GET_RANDOM, 2000},
> +	{TPM2_CC_SEQUENCE_UPDATE, 750},
> +	{TPM2_CC_SEQUENCE_COMPLETE, 750},
> +	{TPM2_CC_EVENT_SEQUENCE_COMPLETE, 750},
> +	{TPM2_CC_HASH_SEQUENCE_START, 750},
> +	{TPM2_CC_VERIFY_SIGNATURE, 30000},
> +	{TPM2_CC_PCR_EXTEND, 750},
> +	{TPM2_CC_HIERARCHY_CONTROL, 2000},
> +	{TPM2_CC_HIERARCHY_CHANGE_AUTH, 2000},
> +	{TPM2_CC_GET_CAPABILITY, 750},
> +	{TPM2_CC_NV_READ, 2000},
> +	{TPM2_CC_CREATE_PRIMARY, 30000},
> +	{TPM2_CC_CREATE, 30000},
> +	{TPM2_CC_CREATE_LOADED, 30000},
> +};
>  
>  /**
> - * tpm2_calc_ordinal_duration() - calculate the maximum command duration
> - * @chip:    TPM chip to use.
> + * tpm2_calc_ordinal_duration() - Calculate the maximum command duration
>   * @ordinal: TPM command ordinal.
>   *
> - * The function returns the maximum amount of time the chip could take
> - * to return the result for a particular ordinal in jiffies.
> - *
> - * Return: A maximal duration time for an ordinal in jiffies.
> + * Returns the maximum amount of time the chip is expected by kernel to
> + * take in jiffies.
>   */
> -unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
> +unsigned long tpm2_calc_ordinal_duration(u32 ordinal)
>  {
> -	unsigned int index;
> +	int i;
>  
> -	index = tpm2_ordinal_duration_index(ordinal);
> +	for (i = 0; i < ARRAY_SIZE(tpm2_ordinal_duration_map); i++)
> +		if (ordinal == tpm2_ordinal_duration_map[i].ordinal)
> +			return msecs_to_jiffies(tpm2_ordinal_duration_map[i].duration);
>  
> -	if (index != TPM_UNDEFINED)
> -		return chip->duration[index];
> -	else
> -		return msecs_to_jiffies(TPM2_DURATION_DEFAULT);
> +	return msecs_to_jiffies(TPM2_DURATION_DEFAULT);
>  }
>  
> -
>  struct tpm2_pcr_read_out {
>  	__be32	update_cnt;
>  	__be32	pcr_selects_cnt;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index b0e9eb5ef022..dc0338a783f3 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -228,10 +228,11 @@ enum tpm2_timeouts {
>  	TPM2_TIMEOUT_B          =   4000,
>  	TPM2_TIMEOUT_C          =    200,
>  	TPM2_TIMEOUT_D          =     30,
> +};
> +
> +enum tpm2_durations {
>  	TPM2_DURATION_SHORT     =     20,
> -	TPM2_DURATION_MEDIUM    =    750,
>  	TPM2_DURATION_LONG      =   2000,
> -	TPM2_DURATION_LONG_LONG = 300000,
>  	TPM2_DURATION_DEFAULT   = 120000,
>  };
>  
> -- 
> 2.39.5