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
- also displaying in sysfs whether the timeouts are 'original' or 'adjusted'
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 <[email protected]>
---
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 = {
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 <[email protected]>
>
> ---
> 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 = {
>
On 03/29/2011 10:34 AM, Rajiv Andrade wrote:
> 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.
Originally I sent a patch that corrected the evaluation of the return
code within the TPM's response and introduced the sysfs entry. I did not
scale the MEDIUM and LONG timeouts, which then caused the problems on
the Infineon TPM and the patch was removed. I then introduced the
scaling of the MEDIUM and LONG timeouts, which seems necessary as a
consequence to working with the TPM-reported timeouts. Then adding a
sysfs entry to it is an additional feature. I'd split this patch in 2
patches where the 2nd one introduces the sysfs entry. Is this ok with you?
Stefan