Re: [patch 1/8] tpm_tis: Use timeouts returned from TPM

[Rajiv Andrade <srajiv@linux.vnet.ibm.com>]

Hi Stefan,

Some comments:

On 03/15/2011 08:13 AM, Stefan Berger wrote:
> v3:
> - sysfs entry now called 'durations' to resemble TPM-speak (previously
>   was called 'timeouts')
> 
> v2:
> - adjusting all timeouts for TPM devices reporting timeouts in msec rather
>   than usec

This is a bugfix that's different than the one you originally sent, can you
submit this as a separated patch? This is mainly for easier debug in the
future, so each feature/bugfix/commit can be tested separately.

> - also displaying in sysfs whether the timeouts are 'original' or 'adjusted'
> 

Minor comment, it's easier to read the log when the changelog comes after the main 
patch description.

> The current TPM TIS driver in git discards the timeout values returned
> from the TPM. The check of the response packet needs to consider that
> the return_code field is 0 on success and the size of the expected
> packet is equivalent to the header size + u32 length indicator for the
> TPM_GetCapability() result + 3 timeout indicators of type u32.
> 
> Since some TPMs seem to return timeouts in msec rather than usec,
> I am now adjusting all the timeouts rather than just the one for short
> durations.
> 
> I am also adding a sysfs entry 'durations' showing the timeouts that are
> being used.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> 
> 
> ---
>  drivers/char/tpm/tpm.c     |   38 ++++++++++++++++++++++++++++++--------
>  drivers/char/tpm/tpm.h     |    3 +++
>  drivers/char/tpm/tpm_tis.c |    4 +++-
>  3 files changed, 36 insertions(+), 9 deletions(-)
> 
> Index: linux-2.6/drivers/char/tpm/tpm.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/tpm/tpm.c
> +++ linux-2.6/drivers/char/tpm/tpm.c
> @@ -575,23 +575,31 @@ duration:
>  	if (rc)
>  		return;
> 
> -	if (be32_to_cpu(tpm_cmd.header.out.return_code)
> -	    != 3 * sizeof(u32))
> +	if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
> +	    be32_to_cpu(tpm_cmd.header.out.length)
> +	    != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 * sizeof(u32))
>  		return;
> +
>  	duration_cap = &tpm_cmd.params.getcap_out.cap.duration;
>  	chip->vendor.duration[TPM_SHORT] =
>  	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));
> +	chip->vendor.duration[TPM_MEDIUM] =
> +	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_medium));
> +	chip->vendor.duration[TPM_LONG] =
> +	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_long));
> +
>  	/* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
>  	 * value wrong and apparently reports msecs rather than usecs. So we
>  	 * fix up the resulting too-small TPM_SHORT value to make things work.
> +	 * We also scale the TPM_MEDIUM and -_LONG values by 1000.
>  	 */
> -	if (chip->vendor.duration[TPM_SHORT] < (HZ/100))
> +	if (chip->vendor.duration[TPM_SHORT] < (HZ / 100)) {
>  		chip->vendor.duration[TPM_SHORT] = HZ;
> -
> -	chip->vendor.duration[TPM_MEDIUM] =
> -	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_medium));
> -	chip->vendor.duration[TPM_LONG] =
> -	    usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_long));
> +		chip->vendor.duration[TPM_MEDIUM] *= 1000;
> +		chip->vendor.duration[TPM_LONG] *= 1000;
> +		chip->vendor.duration_adjusted = true;
> +		dev_info(chip->dev, "Adjusting TPM timeout parameters.");
> +	}
>  }
>  EXPORT_SYMBOL_GPL(tpm_get_timeouts);
> 
> @@ -937,6 +945,20 @@ ssize_t tpm_show_caps_1_2(struct device 
>  }
>  EXPORT_SYMBOL_GPL(tpm_show_caps_1_2);
> 
> +ssize_t tpm_show_durations(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d %d %d [%s]\n",
> +		       jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
> +		       jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
> +		       jiffies_to_usecs(chip->vendor.duration[TPM_LONG]),
> +		       chip->vendor.duration_adjusted
> +		       ? "adjusted" : "original");
> +}
> +EXPORT_SYMBOL_GPL(tpm_show_durations);
> +
>  ssize_t tpm_store_cancel(struct device *dev, struct device_attribute *attr,
>  			const char *buf, size_t count)
>  {
> Index: linux-2.6/drivers/char/tpm/tpm.h
> ===================================================================
> --- linux-2.6.orig/drivers/char/tpm/tpm.h
> +++ linux-2.6/drivers/char/tpm/tpm.h
> @@ -56,6 +56,8 @@ extern ssize_t tpm_show_owned(struct dev
>  				char *);
>  extern ssize_t tpm_show_temp_deactivated(struct device *,
>  					 struct device_attribute *attr, char *);
> +extern ssize_t tpm_show_durations(struct device *,
> +				  struct device_attribute *attr, char *);
> 
>  struct tpm_chip;
> 
> @@ -82,6 +84,7 @@ struct tpm_vendor_specific {
>  	int locality;
>  	unsigned long timeout_a, timeout_b, timeout_c, timeout_d; /* jiffies */
>  	unsigned long duration[3]; /* jiffies */
> +	bool duration_adjusted;
> 
>  	wait_queue_head_t read_queue;
>  	wait_queue_head_t int_queue;
> Index: linux-2.6/drivers/char/tpm/tpm_tis.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/tpm/tpm_tis.c
> +++ linux-2.6/drivers/char/tpm/tpm_tis.c
> @@ -376,6 +376,7 @@ static DEVICE_ATTR(temp_deactivated, S_I
>  		   NULL);
>  static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
>  static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
> +static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
> 
>  static struct attribute *tis_attrs[] = {
>  	&dev_attr_pubek.attr,
> @@ -385,7 +386,8 @@ static struct attribute *tis_attrs[] = {
>  	&dev_attr_owned.attr,
>  	&dev_attr_temp_deactivated.attr,
>  	&dev_attr_caps.attr,
> -	&dev_attr_cancel.attr, NULL,
> +	&dev_attr_cancel.attr,
> +	&dev_attr_durations.attr, NULL,
>  };
> 
>  static struct attribute_group tis_attr_grp = {
>