Change in ...coreboot[master]: mainboard/google/sarien: skip tpm check when !verstage

[Jett Rink (Code Review)]

Jett Rink has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32030

Change subject: mainboard/google/sarien: skip tpm check when !verstage
......................................................................

mainboard/google/sarien: skip tpm check when !verstage

The TPM driver isn't loaded in other stages but verstage so when we try
to communicate with the TPM it fails. We don't need to communicate with
it anyway since the TPM won't continue to tell us that recovery was
requested, only the first query responds with the recovery request.

BRANCH=none
BUG=b:129150074
TEST=1)boot arcada without recovery and notice that the "tpm transaction
failed" log lines are no longer present. 2) boot into recovery using the
ESC refresh power key combination and verify that the recovery reason
was "recovery button pressed"

Change-Id: I13284483d069ed50b0d16b36d0120d006485f7f4
Signed-off-by: Jett Rink 
---
M src/mainboard/google/sarien/chromeos.c
1 file changed, 19 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/32030/1

diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c
index 1e363fd..2bef829 100644
--- a/src/mainboard/google/sarien/chromeos.c
+++ b/src/mainboard/google/sarien/chromeos.c
@@ -30,7 +30,6 @@
    REC_MODE_NOT_REQUESTED,
    REC_MODE_REQUESTED,
 };
-static enum rec_mode_state saved_rec_mode;

void fill_lb_gpios(struct lb_gpios *gpios)
 {
@@ -84,25 +83,31 @@

int get_recovery_mode_switch(void)
 {
-	enum rec_mode_state state = saved_rec_mode;
-	uint8_t recovery_button_state = 0;
+	static enum rec_mode_state saved_rec_mode = REC_MODE_UNINITIALIZED;
+	enum rec_mode_state state = REC_MODE_NOT_REQUESTED;
+	uint8_t cr50_state = 0;

-	/* Check the global variable first. */
-	if (state == REC_MODE_NOT_REQUESTED)
-		return 0;
-	else if (state == REC_MODE_REQUESTED)
-		return 1;
+	/* Check cached state, since TPM will only tell us the first time */
+	if (saved_rec_mode != REC_MODE_UNINITIALIZED)
+		return saved_rec_mode == REC_MODE_REQUESTED;

-	state = REC_MODE_NOT_REQUESTED;
+	/*
+	 * Read one-time recovery request from cr50 in verstage only since
+	 * the TPM driver won't be set up in time for other stages like romstage
+	 * and the value from the TPM would be wrong anyway since the verstage
+	 * read would have cleared the value on the TPM.
+	 *
+	 * The TPM recovery request is passed between stages through the
+	 * vboot_get_shared_data or cbmem depending on stage.
+	 */
+	if (ENV_VERSTAGE &&
+	    tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS &&
+	    cr50_state)
+		state = REC_MODE_REQUESTED;

/* Read state from the GPIO controlled by servo. */
    if (cros_get_gpio_value(CROS_GPIO_REC))
    	state = REC_MODE_REQUESTED;
-	/* Read one-time recovery request from cr50. */
-	else if (tlcl_cr50_get_recovery_button(&recovery_button_state)
-		 == TPM_SUCCESS)
-		state = recovery_button_state ?
-			REC_MODE_REQUESTED : REC_MODE_NOT_REQUESTED;

/* Store the state in case this is called again in verstage. */
    saved_rec_mode = state;

[Jett Rink (Code Review)]

Hello Aaron Durbin, Duncan Laurie, Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32030

to look at the new patch set (#2).

Change subject: mainboard/google/sarien: skip tpm check when !verstage
......................................................................

mainboard/google/sarien: skip tpm check when !verstage

The TPM driver isn't loaded in other stages but verstage so when we try
to communicate with the TPM it fails. We don't need to communicate with
it anyway since the TPM won't continue to tell us that recovery was
requested, only the first query responds with the recovery request.

BRANCH=none
BUG=b:129150074,b:123360379
TEST=1)boot arcada without recovery and notice that the "tpm transaction
failed" log lines are no longer present. 2) boot into recovery using the
ESC refresh power key combination and verify that the recovery reason
was "recovery button pressed"

Change-Id: I13284483d069ed50b0d16b36d0120d006485f7f4
Signed-off-by: Jett Rink 
---
M src/mainboard/google/sarien/chromeos.c
1 file changed, 19 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/32030/2

[Keith Short (Code Review)]

Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32030 )

Change subject: mainboard/google/sarien: skip tpm check when !verstage
......................................................................

Patch Set 2:

I also created fix for this bug with this change stack:
https://review.coreboot.org/c/coreboot/+/31800/2

My change was to save the recovery state in VBNV during verstage and then use VBNV state for all later stages.  Originally I was going to save the recovery state to cbmem, but cbmem is not part of verstage.

[Keith Short (Code Review)]

Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32030 )

Change subject: mainboard/google/sarien: skip tpm check when !verstage
......................................................................

Patch Set 2:
> Patch Set 2:
>
> I also created fix for this bug with this change stack:
> https://review.coreboot.org/c/coreboot/+/31800/2
>
> My change was to save the recovery state in VBNV during verstage and then use VBNV state for all later stages.  Originally I was going to save the recovery state to cbmem, but cbmem is not part of verstage.

After talking with Jett,  I'll abandon my change stack.

[Jett Rink (Code Review)]

Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32030 )

Change subject: mainboard/google/sarien: skip tpm check when !verstage
......................................................................

Patch Set 2:
> Patch Set 2:
>
> I also created fix for this bug with this change stack:
> https://review.coreboot.org/c/coreboot/+/31800/2
>
> My change was to save the recovery state in VBNV during verstage and then use VBNV state for all later stages.  Originally I was going to save the recovery state to cbmem, but cbmem is not part of verstage.

Turns out that this is actually plumbed through the various stages through NV or cbmem, it is just a very winding path to trace, but this is the start of the trail: https://chromium.git.corp.google.com/chromiumos/third_party/coreboot/+/c8fa6...

[Keith Short (Code Review)]

Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32030 )

Change subject: mainboard/google/sarien: skip tpm check when !verstage
......................................................................

Patch Set 2: Code-Review+1

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32030 )

Change subject: mainboard/google/sarien: skip tpm check when !verstage
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32030 )

Change subject: mainboard/google/sarien: skip tpm check when !verstage
......................................................................

mainboard/google/sarien: skip tpm check when !verstage

The TPM driver isn't loaded in other stages but verstage so when we try
to communicate with the TPM it fails. We don't need to communicate with
it anyway since the TPM won't continue to tell us that recovery was
requested, only the first query responds with the recovery request.

BRANCH=none
BUG=b:129150074,b:123360379
TEST=1)boot arcada without recovery and notice that the "tpm transaction
failed" log lines are no longer present. 2) boot into recovery using the
ESC refresh power key combination and verify that the recovery reason
was "recovery button pressed"

Change-Id: I13284483d069ed50b0d16b36d0120d006485f7f4
Signed-off-by: Jett Rink 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32030
Tested-by: build bot (Jenkins) 
Reviewed-by: Keith Short 
Reviewed-by: Duncan Laurie 
---
M src/mainboard/google/sarien/chromeos.c
1 file changed, 19 insertions(+), 14 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Duncan Laurie: Looks good to me, approved
  Keith Short: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c
index 1e363fd..2bef829 100644
--- a/src/mainboard/google/sarien/chromeos.c
+++ b/src/mainboard/google/sarien/chromeos.c
@@ -30,7 +30,6 @@
    REC_MODE_NOT_REQUESTED,
    REC_MODE_REQUESTED,
 };
-static enum rec_mode_state saved_rec_mode;

void fill_lb_gpios(struct lb_gpios *gpios)
 {
@@ -84,25 +83,31 @@

int get_recovery_mode_switch(void)
 {
-	enum rec_mode_state state = saved_rec_mode;
-	uint8_t recovery_button_state = 0;
+	static enum rec_mode_state saved_rec_mode = REC_MODE_UNINITIALIZED;
+	enum rec_mode_state state = REC_MODE_NOT_REQUESTED;
+	uint8_t cr50_state = 0;

-	/* Check the global variable first. */
-	if (state == REC_MODE_NOT_REQUESTED)
-		return 0;
-	else if (state == REC_MODE_REQUESTED)
-		return 1;
+	/* Check cached state, since TPM will only tell us the first time */
+	if (saved_rec_mode != REC_MODE_UNINITIALIZED)
+		return saved_rec_mode == REC_MODE_REQUESTED;

-	state = REC_MODE_NOT_REQUESTED;
+	/*
+	 * Read one-time recovery request from cr50 in verstage only since
+	 * the TPM driver won't be set up in time for other stages like romstage
+	 * and the value from the TPM would be wrong anyway since the verstage
+	 * read would have cleared the value on the TPM.
+	 *
+	 * The TPM recovery request is passed between stages through the
+	 * vboot_get_shared_data or cbmem depending on stage.
+	 */
+	if (ENV_VERSTAGE &&
+	    tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS &&
+	    cr50_state)
+		state = REC_MODE_REQUESTED;

/* Read state from the GPIO controlled by servo. */
    if (cros_get_gpio_value(CROS_GPIO_REC))
    	state = REC_MODE_REQUESTED;
-	/* Read one-time recovery request from cr50. */
-	else if (tlcl_cr50_get_recovery_button(&recovery_button_state)
-		 == TPM_SUCCESS)
-		state = recovery_button_state ?
-			REC_MODE_REQUESTED : REC_MODE_NOT_REQUESTED;

/* Store the state in case this is called again in verstage. */
    saved_rec_mode = state;