- TPM save state before suspending to ram

PATCH In review Marcel Selhorst 6 participants 9 messages Jan 3, 2008 – Jan 14, 2008 TPM device driver

lore

Share & tools

lore

Marcel Selhorst

author

2008-01-03 20:51 UTC #0 lore raw

PATCH 1/1 - TPM save state before suspending to ram

Dear list,

this patch fixes a bug, that prevents the TPM chip to resume correctly from a suspended state.

Signed-off-by Marcel Selhorst <[email protected]>

---

drivers/char/tpm/tpm.c +4 -2

@@ -1041,7 +1041,7 @@ void tpm_remove_hardware(struct device *
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
-static u8 savestate[] = {
+static const u8 savestate[] = {
 	0, 193,			/* TPM_TAG_RQU_COMMAND */
 	0, 0, 0, 10,		/* blob length (in bytes) */
 	0, 0, 0, 152		/* TPM_ORD_SaveState */

@@ -1053,11 +1053,13 @@ static u8 savestate[] = {
  */
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
+	u8 data[max_t(int, max(ARRAY_SIZE(savestate), ARRAY_SIZE(savestate)), 10)];
 	struct tpm_chip *chip = dev_get_drvdata(dev);
 	if (chip == NULL)
 		return -ENODEV;
 
-	tpm_transmit(chip, savestate, sizeof(savestate));
+	memcpy(data, savestate, sizeof(savestate));
+	tpm_transmit(chip, data, sizeof(data));
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_pm_suspend);

Pavel Machek

2008-01-04 16:39 UTC #1 lore raw

Quoted 23 lines · Marcel Selhorst

Dear list,

this patch fixes a bug, that prevents the TPM chip to resume correctly from a suspended state.

Signed-off-by Marcel Selhorst <[email protected]>

---

drivers/char/tpm/tpm.c +2 -1

@@ -1041,7 +1041,7 @@ void tpm_remove_hardware(struct device *
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
-static u8 savestate[] = {
+static const u8 savestate[] = {
 	0, 193,			/* TPM_TAG_RQU_COMMAND */
 	0, 0, 0, 10,		/* blob length (in bytes) */
 	0, 0, 0, 152		/* TPM_ORD_SaveState */

@@ -1053,11 +1053,13 @@ static u8 savestate[] = {
  */
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
+	u8 data[max_t(int, max(ARRAY_SIZE(savestate), ARRAY_SIZE(savestate)), 10)];

drivers/char/tpm/tpm.c static u8 savestate[] = { · line 1065

Quoted 10 lines

drivers/char/tpm/tpm.c +2 -1

 	struct tpm_chip *chip = dev_get_drvdata(dev);
 	if (chip == NULL)
 		return -ENODEV;
 
-	tpm_transmit(chip, savestate, sizeof(savestate));
+	memcpy(data, savestate, sizeof(savestate));
+	tpm_transmit(chip, data, sizeof(data));
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_pm_suspend);

I'm not sure if we want to use variable-size array on stack. What hacks are you doing with max_t/max?
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Kent Yoder

2008-01-04 20:09 UTC #2 lore raw

Quoted 8 lines · Pavel Machek

Quoted 6 lines · Marcel Selhorst
Dear list,
this patch fixes a bug, that prevents the TPM chip to resume correctly from a suspended state.
Signed-off-by Marcel Selhorst <[email protected]>

[cut]

Quoted 4 lines

I'm not sure if we want to use variable-size array on stack. What hacks are you doing with max_t/max?
Pavel

Hi Pavel,

Here's an alternate solution from David, signoffs pending --

commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b

Author: David Smith <[email protected]>
Date:   Fri Jan 4 03:33:11 2008 +0900

Fix for TPM suspend/resume failure

drivers/char/tpm/tpm.c +7 -7

index 9bb5429..79d2fd5 100644
@@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
-static u8 savestate[] = {
-	0, 193,			/* TPM_TAG_RQU_COMMAND */
-	0, 0, 0, 10,		/* blob length (in bytes) */
-	0, 0, 0, 152		/* TPM_ORD_SaveState */
-};
-
 /*
  * We are about to suspend. Save the TPM state
  * so that it can be restored.

@@ -1059,8 +1053,14 @@ static u8 savestate[] = {
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
+        u8 savestate[] = {
+          0, 193,		/* TPM_TAG_RQU_COMMAND */
+          0, 0, 0, 10,		/* blob length (in bytes) */
+          0, 0, 0, 152		/* TPM_ORD_SaveState */
+        };
+
 	if (chip == NULL)
-		return -ENODEV;
+          return -ENODEV;
 
 	tpm_transmit(chip, savestate, sizeof(savestate));
 	return 0;

Kent

Signature · 2 lines

Kent Yoder
IBM LTC Security Dev.

Kent is right, the solution from David is way better. I wanted to be consistent with the other functions in tpm.c, but it definitely makes more sense in Davids way. So please consider my patch as reverted and his patch as

Signed-off-by Marcel Selhorst <[email protected]>

Thanks,
Marcel

Quoted 56 lines · Kent Yoder

Quoted 7 lines · Pavel Machek

Quoted 6 lines · Marcel Selhorst
Dear list,
this patch fixes a bug, that prevents the TPM chip to resume correctly from a suspended state.
Signed-off-by Marcel Selhorst <[email protected]>

[cut]

Quoted 3 lines

I'm not sure if we want to use variable-size array on stack. What hacks are you doing with max_t/max?
Pavel

Hi Pavel,

Here's an alternate solution from David, signoffs pending --

commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b

Author: David Smith <[email protected]>
Date:   Fri Jan 4 03:33:11 2008 +0900

Fix for TPM suspend/resume failure

drivers/char/tpm/tpm.c +7 -7

index 9bb5429..79d2fd5 100644
@@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
-static u8 savestate[] = {
-	0, 193,			/* TPM_TAG_RQU_COMMAND */
-	0, 0, 0, 10,		/* blob length (in bytes) */
-	0, 0, 0, 152		/* TPM_ORD_SaveState */
-};
-
 /*
  * We are about to suspend. Save the TPM state
  * so that it can be restored.

@@ -1059,8 +1053,14 @@ static u8 savestate[] = {
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
+        u8 savestate[] = {
+          0, 193,		/* TPM_TAG_RQU_COMMAND */
+          0, 0, 0, 10,		/* blob length (in bytes) */
+          0, 0, 0, 152		/* TPM_ORD_SaveState */
+        };
+
 	if (chip == NULL)
-		return -ENODEV;
+          return -ENODEV;
 
 	tpm_transmit(chip, savestate, sizeof(savestate));
 	return 0;

Pavel Machek

2008-01-04 23:44 UTC #4 lore raw

In reply to Kent Yoder

Quoted 48 lines · Kent Yoder

Quoted 8 lines · Pavel Machek
Quoted 6 lines · Marcel Selhorst
Dear list,
this patch fixes a bug, that prevents the TPM chip to resume correctly from a suspended state.
Signed-off-by Marcel Selhorst <[email protected]>
[cut]
Quoted 4 lines
I'm not sure if we want to use variable-size array on stack. What hacks are you doing with max_t/max?
Pavel
Hi Pavel,
Here's an alternate solution from David, signoffs pending --
commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b
Author: David Smith <[email protected]>
Date:   Fri Jan 4 03:33:11 2008 +0900
Fix for TPM suspend/resume failure
drivers/char/tpm/tpm.c +2 -6
index 9bb5429..79d2fd5 100644
@@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
-static u8 savestate[] = {
-	0, 193,			/* TPM_TAG_RQU_COMMAND */
-	0, 0, 0, 10,		/* blob length (in bytes) */
-	0, 0, 0, 152		/* TPM_ORD_SaveState */
-};
-
 /*
  * We are about to suspend. Save the TPM state
  * so that it can be restored.
@@ -1059,8 +1053,14 @@ static u8 savestate[] = {
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
+        u8 savestate[] = {
+          0, 193,		/* TPM_TAG_RQU_COMMAND */

drivers/char/tpm/tpm.c static u8 savestate[] = { · line 1066

Quoted 10 lines

drivers/char/tpm/tpm.c +5 -1

+          0, 0, 0, 10,		/* blob length (in bytes) */
+          0, 0, 0, 152		/* TPM_ORD_SaveState */
+        };
+
 	if (chip == NULL)
-		return -ENODEV;
+          return -ENODEV;
 
 	tpm_transmit(chip, savestate, sizeof(savestate));
 	return 0;

Yep, but please fix the whitespace.
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

David Smith

2008-01-05 04:55 UTC #5 lore raw

Quoted 58 lines · Kent Yoder

Quoted 57 lines · Pavel Machek
Quoted 7 lines · Marcel Selhorst
Quoted 6 lines
Dear list,
this patch fixes a bug, that prevents the TPM chip to resume correctly from a suspended state.
Signed-off-by Marcel Selhorst <[email protected]>
[cut]
Quoted 4 lines
I'm not sure if we want to use variable-size array on stack. What hacks are you doing with max_t/max?
Pavel
Hi Pavel,
Here's an alternate solution from David, signoffs pending --
commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b
Author: David Smith <[email protected]>
Date:   Fri Jan 4 03:33:11 2008 +0900
Fix for TPM suspend/resume failure
drivers/char/tpm/tpm.c +7 -7
index 9bb5429..79d2fd5 100644
@@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
-static u8 savestate[] = {
-	0, 193,			/* TPM_TAG_RQU_COMMAND */
-	0, 0, 0, 10,		/* blob length (in bytes) */
-	0, 0, 0, 152		/* TPM_ORD_SaveState */
-};
-
 /*
  * We are about to suspend. Save the TPM state
  * so that it can be restored.
@@ -1059,8 +1053,14 @@ static u8 savestate[] = {
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
+        u8 savestate[] = {
+          0, 193,		/* TPM_TAG_RQU_COMMAND */
+          0, 0, 0, 10,		/* blob length (in bytes) */
+          0, 0, 0, 152		/* TPM_ORD_SaveState */
+        };
+
 	if (chip == NULL)
-		return -ENODEV;
+          return -ENODEV;
 

Quoted 5 lines

drivers/char/tpm/tpm.c static u8 savestate[] = { · line 1066
Quoted 2 lines
drivers/char/tpm/tpm.c +0 -0
 	tpm_transmit(chip, savestate, sizeof(savestate));
 	return 0;
Yep, but please fix the whitespace.
Pavel

OK, attached.

Signature · 1 line

man perl | tail -6 | head -2

commit a17f28b93a6e11097ea806d576eb317263aa8768

Author: David Smith <[email protected]>
Date:   Fri Jan 4 16:12:58 2008 +0900

Fix TPM suspend and resume failure.

The savestate command structure was being overwritten by the result of running the TPM_SaveState command after one run, so make it a local variable to the function instead of a global variable that gets overwritten.

drivers/char/tpm/tpm.c +6 -6

index 9bb5429..4f18ddf 100644
@@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
-static u8 savestate[] = {
-	0, 193,			/* TPM_TAG_RQU_COMMAND */
-	0, 0, 0, 10,		/* blob length (in bytes) */
-	0, 0, 0, 152		/* TPM_ORD_SaveState */
-};
-
 /*
  * We are about to suspend. Save the TPM state
  * so that it can be restored.

@@ -1059,6 +1053,12 @@ static u8 savestate[] = {
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
+	u8 savestate[] = {
+		0, 193,		/* TPM_TAG_RQU_COMMAND */
+		0, 0, 0, 10,	/* blob length (in bytes) */
+		0, 0, 0, 152	/* TPM_ORD_SaveState */
+	};
+
 	if (chip == NULL)
 		return -ENODEV;

Rafael J. Wysocki

2008-01-14 21:38 UTC #6 lore raw

Quoted 58 lines · David Smith

Quoted 58 lines · Kent Yoder
Quoted 57 lines · Pavel Machek
Quoted 7 lines · Marcel Selhorst
Quoted 6 lines
Dear list,
this patch fixes a bug, that prevents the TPM chip to resume correctly from a suspended state.
Signed-off-by Marcel Selhorst <[email protected]>
[cut]
Quoted 4 lines
I'm not sure if we want to use variable-size array on stack. What hacks are you doing with max_t/max?
Pavel
Hi Pavel,
Here's an alternate solution from David, signoffs pending --
commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b
Author: David Smith <[email protected]>
Date:   Fri Jan 4 03:33:11 2008 +0900
Fix for TPM suspend/resume failure
drivers/char/tpm/tpm.c +7 -7
index 9bb5429..79d2fd5 100644
@@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
-static u8 savestate[] = {
-	0, 193,			/* TPM_TAG_RQU_COMMAND */
-	0, 0, 0, 10,		/* blob length (in bytes) */
-	0, 0, 0, 152		/* TPM_ORD_SaveState */
-};
-
 /*
  * We are about to suspend. Save the TPM state
  * so that it can be restored.
@@ -1059,8 +1053,14 @@ static u8 savestate[] = {
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
+        u8 savestate[] = {
+          0, 193,		/* TPM_TAG_RQU_COMMAND */
+          0, 0, 0, 10,		/* blob length (in bytes) */
+          0, 0, 0, 152		/* TPM_ORD_SaveState */
+        };
+
 	if (chip == NULL)
-		return -ENODEV;
+          return -ENODEV;
 

Quoted 7 lines

Quoted 5 lines
drivers/char/tpm/tpm.c static u8 savestate[] = { · line 1066
Quoted 2 lines
drivers/char/tpm/tpm.c +0 -0
 	tpm_transmit(chip, savestate, sizeof(savestate));
 	return 0;
Yep, but please fix the whitespace.
Pavel
OK, attached.

Is anyone taking care of this patch or should I do that?

Rafael

Signature · 1 line

"Premature optimization is the root of all evil." - Donald Knuth

Andrew Morton

2008-01-14 21:54 UTC #7 lore raw

Quoted 7 lines · On Mon, 14 Jan 2008 22:40:58 +0100 "Rafael J. Wysocki" <[email protected]> wrote:

Quoted 5 lines
Quoted 3 lines
Yep, but please fix the whitespace.
Pavel
OK, attached.
Is anyone taking care of this patch or should I do that?

Linus just merged it as 2490c681ea3d7f5ac3fb876f14567bf1a9e0aa87. That's if we're talking about the same thing (there were a few iterations).

Rafael J. Wysocki

2008-01-14 22:07 UTC #8 lore raw

Quoted 7 lines · Andrew Morton

Quoted 4 lines · On Mon, 14 Jan 2008 22:40:58 +0100 "Rafael J. Wysocki" <[email protected]> wrote:
Quoted 4 lines
Quoted 3 lines
Yep, but please fix the whitespace.
Pavel

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
OK, attached.
Is anyone taking care of this patch or should I do that?
Linus just merged it as 2490c681ea3d7f5ac3fb876f14567bf1a9e0aa87. That's if we're talking about the same thing (there were a few iterations).

Yes, that's the same thing, thanks.