Patch tpm-correct-tpm-timeouts-to-jiffies-conversion reveals a bug in the
Broadcom BCM0102 TPM chipset used in the Dell Latitude D820 - although most of
the timeouts are returned in usecs as per the spec, one is apparently returned
in msecs, which results in a timeout when the code treats it as usecs. To
prevent a regression, we check for the known too-short value and adjust it to a
value that makes things work.
Signed-off-by: Valdis Kletnieks <[email protected]>
cc: Marcin Obara <[email protected]>
--- linux-2.6.26-mmotm-0724/drivers/char/tpm/tpm.c.broadcom 2008-07-25 22:13:47.000000000 -0400
+++ linux-2.6.26-mmotm-0724/drivers/char/tpm/tpm.c 2008-07-25 23:30:38.000000000 -0400
@@ -557,6 +557,13 @@ duration:
usecs_to_jiffies(be32_to_cpu
(*((__be32 *) (data +
TPM_GET_CAP_RET_UINT32_1_IDX))));
+ /* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
+ * value wrong and apparently reports msecs rather than usecs. Test
+ * for the specific buggy value and adjust it to prevent a regression.
+ */
+ if (chip->vendor.duration[TPM_SHORT] == 1)
+ chip->vendor.duration[TPM_SHORT] *= 1000;
+
chip->vendor.duration[TPM_MEDIUM] =
usecs_to_jiffies(be32_to_cpu
(*((__be32 *) (data +
2008/7/26 <[email protected]>:
> + if (chip->vendor.duration[TPM_SHORT] == 1)
> + chip->vendor.duration[TPM_SHORT] *= 1000;
> +
> chip->vendor.duration[TPM_MEDIUM] =
> usecs_to_jiffies(be32_to_cpu
> (*((__be32 *) (data +
Value in chip->vendor.duration[TPM_SHORT] is in jiffies not in milliseconds.
(As I know it's not the same. Jiffy is in range 1-10 ms.)
I know the result may be the same, but it is unclear.
Maybe... value should be compared (to 1000) before conversion?
or...
If after conversion, there should be something like this:
if (chip->vendor.duration[TPM_SHORT] < (HZ/100)) /* less
than 10ms ? */
chip->vendor.duration[TPM_SHORT] = HZ;
What do you think?
Regards
Marcin
On Sat, 26 Jul 2008 20:27:33 +0100
"Marcin Obara" <[email protected]> wrote:
> 2008/7/26 <[email protected]>:
> > + if (chip->vendor.duration[TPM_SHORT] == 1)
> > + chip->vendor.duration[TPM_SHORT] *= 1000;
> > +
> > chip->vendor.duration[TPM_MEDIUM] =
> > usecs_to_jiffies(be32_to_cpu
> > (*((__be32 *) (data +
>
>
> Value in chip->vendor.duration[TPM_SHORT] is in jiffies not in milliseconds.
> (As I know it's not the same. Jiffy is in range 1-10 ms.)
> I know the result may be the same, but it is unclear.
>
> Maybe... value should be compared (to 1000) before conversion?
> or...
> If after conversion, there should be something like this:
> if (chip->vendor.duration[TPM_SHORT] < (HZ/100)) /* less
> than 10ms ? */
> chip->vendor.duration[TPM_SHORT] = HZ;
>
> What do you think?
>
This all seemed to die off without a conclusion.
Here's the current
tpm-correct-tpm-timeouts-to-jiffies-conversion.patch. Does it need
updating?
Thanks.
From: Marcin Obara <[email protected]>
This patch fixes timeouts conversion to jiffies, by replacing
msecs_to_jiffies() calls with usecs_to_jiffies(). According to TCG TPM
Specification Version 1.2 Revision 103 (pages 166, 167) TPM timeouts and
durations are returned in microseconds (usec) not in miliseconds (msec).
Signed-off-by: Marcin Obara <[email protected]>
Cc: Marcel Selhorst <[email protected]>
Cc: Kylene Jo Hall <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
drivers/char/tpm/tpm.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff -puN drivers/char/tpm/tpm.c~tpm-correct-tpm-timeouts-to-jiffies-conversion drivers/char/tpm/tpm.c
--- a/drivers/char/tpm/tpm.c~tpm-correct-tpm-timeouts-to-jiffies-conversion
+++ a/drivers/char/tpm/tpm.c
@@ -525,19 +525,19 @@ void tpm_get_timeouts(struct tpm_chip *c
timeout =
be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_UINT32_1_IDX)));
if (timeout)
- chip->vendor.timeout_a = msecs_to_jiffies(timeout);
+ chip->vendor.timeout_a = usecs_to_jiffies(timeout);
timeout =
be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_UINT32_2_IDX)));
if (timeout)
- chip->vendor.timeout_b = msecs_to_jiffies(timeout);
+ chip->vendor.timeout_b = usecs_to_jiffies(timeout);
timeout =
be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_UINT32_3_IDX)));
if (timeout)
- chip->vendor.timeout_c = msecs_to_jiffies(timeout);
+ chip->vendor.timeout_c = usecs_to_jiffies(timeout);
timeout =
be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_UINT32_4_IDX)));
if (timeout)
- chip->vendor.timeout_d = msecs_to_jiffies(timeout);
+ chip->vendor.timeout_d = usecs_to_jiffies(timeout);
duration:
memcpy(data, tpm_cap, sizeof(tpm_cap));
@@ -554,15 +554,15 @@ duration:
return;
chip->vendor.duration[TPM_SHORT] =
- msecs_to_jiffies(be32_to_cpu
+ usecs_to_jiffies(be32_to_cpu
(*((__be32 *) (data +
TPM_GET_CAP_RET_UINT32_1_IDX))));
chip->vendor.duration[TPM_MEDIUM] =
- msecs_to_jiffies(be32_to_cpu
+ usecs_to_jiffies(be32_to_cpu
(*((__be32 *) (data +
TPM_GET_CAP_RET_UINT32_2_IDX))));
chip->vendor.duration[TPM_LONG] =
- msecs_to_jiffies(be32_to_cpu
+ usecs_to_jiffies(be32_to_cpu
(*((__be32 *) (data +
TPM_GET_CAP_RET_UINT32_3_IDX))));
}
_
On Sat, 26 Jul 2008 20:27:33 BST, Marcin Obara said:
> Value in chip->vendor.duration[TPM_SHORT] is in jiffies not in milliseconds.
> (As I know it's not the same. Jiffy is in range 1-10 ms.)
> I know the result may be the same, but it is unclear.
I suppose I could have worded the comment block better - the intent was to
point out what the Broadcom chip returns, but by that point in the code
we're dealing with jiffies...
> Maybe... value should be compared (to 1000) before conversion?
Actually, that's probably a better idea, because my kernel is built with
HZ=1000 - usecs_to_jiffies will do something different than ==1 for HZ=100
or HZ=250 or other odd values.
> If after conversion, there should be something like this:
> if (chip->vendor.duration[TPM_SHORT] < (HZ/100)) /* less
> than 10ms ? */
> chip->vendor.duration[TPM_SHORT] = HZ;
That's another option as well, that does the right thing for various HZ values.
> What do you think?
Let me go cook up and test another iteration of the patch, will probably be
a few hours...