[PATCH] 2.6.26-mmotm tpm-correct-tpm-timeouts-to-jiffies-conversion-d820-fix.patch

[PATCH] 2.6.26-mmotm tpm-correct-tpm-timeouts-to-jiffies-conversion-d820-fix.patch

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

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 <valdis.kletnieks@vt.edu>
cc: Marcin Obara <marcin_obara@users.sourceforge.net>

--- 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 +


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] 2.6.26-mmotm tpm-correct-tpm-timeouts-to-jiffies-conversion-d820-fix.patch

[Marcin Obara]

2008/7/26  <Valdis.Kletnieks@vt.edu>:
> +       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

Re: [PATCH] 2.6.26-mmotm tpm-correct-tpm-timeouts-to-jiffies-conversion-d820-fix.patch

[Andrew Morton]

On Sat, 26 Jul 2008 20:27:33 +0100
"Marcin Obara" <marcin_obara@users.sourceforge.net> wrote:

> 2008/7/26  <Valdis.Kletnieks@vt.edu>:
> > +       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 <marcin_obara@users.sourceforge.net>

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 <marcin_obara@users.sourceforge.net>
Cc: Marcel Selhorst <tpm@selhorst.net>
Cc: Kylene Jo Hall <kjhall@us.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 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))));
 }
_

Re: [PATCH] 2.6.26-mmotm tpm-correct-tpm-timeouts-to-jiffies-conversion-d820-fix.patch

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

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...


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]