Change in ...coreboot[master]: vboot: init dispaly when recovery request

[EricR Lai (Code Review)]

EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32906

Change subject: vboot: init dispaly when recovery request
......................................................................

vboot: init dispaly when recovery request

Dispaly is required by recovery mode. Do init when recovery
request by user.

BUG=b:133197727,b:133175864
TEST= enter recovery mode, checked the display shows up

Signed-off-by: Eric Lai 
Change-Id: Id6ac611f51241373bca3e2b394a94dcd52d3fde7
---
M src/security/vboot/vboot_logic.c
1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/32906/1

diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c
index 00347c3..68215fd 100644
--- a/src/security/vboot/vboot_logic.c
+++ b/src/security/vboot/vboot_logic.c
@@ -344,6 +344,10 @@
    printk(BIOS_INFO, "Phase 1\n");
    rv = vb2api_fw_phase1(&ctx);

+	/* If recovery mode, do initialize display */
+	if (ctx.flags & VB2_CONTEXT_RECOVERY_MODE)
+		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT;
+
    if (rv) {
    	/*
    	 * If vb2api_fw_phase1 fails, check for return value.

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: init dispaly when recovery request
......................................................................

Patch Set 1:

This issue is happened after this CL:https://review.coreboot.org/c/coreboot/+/32324 
I tried to fix it ,please review it.

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: init dispaly when recovery request
......................................................................

Patch Set 1:

(5 comments)

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG@7 
PS1, Line 7: when
on

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG@7 
PS1, Line 7: dispaly
display

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG@9 
PS1, Line 9: Dispaly
Display

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG@13 
PS1, Line 13:  
No space here

https://review.coreboot.org/#/c/32906/1/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/32906/1/src/security/vboot/vboot_logic.c@347 
PS1, Line 347: If recovery mode, do initialize display
On recovery mode, initialize display

Change in ...coreboot[master]: vboot: init display on recovery request

[EricR Lai (Code Review)]

Hello Aaron Durbin, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Joel Kitching, Furquan Shaikh,

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

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

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

Change subject: vboot: init display on recovery request
......................................................................

vboot: init display on recovery request

Display is required by recovery mode. Do init when recovery
request by user.

BUG=b:133197727,b:133175864
TEST=enter recovery mode, checked the display shows up

Signed-off-by: Eric Lai 
Change-Id: Id6ac611f51241373bca3e2b394a94dcd52d3fde7
---
M src/security/vboot/vboot_logic.c
1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/32906/2

Change in ...coreboot[master]: vboot: init display on recovery request

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: init display on recovery request
......................................................................

Patch Set 2:

(4 comments)

terrible typo

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG@7 
PS1, Line 7: when
> on

Done

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG@7 
PS1, Line 7: dispaly
> display

Done

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG@9 
PS1, Line 9: Dispaly
> Display

Done

https://review.coreboot.org/#/c/32906/1//COMMIT_MSG@13 
PS1, Line 13:
> No space here

Done

Change in ...coreboot[master]: vboot: init display on recovery request

[EricR Lai (Code Review)]

Hello Aaron Durbin, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Joel Kitching, Furquan Shaikh,

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

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

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

Change subject: vboot: init display on recovery request
......................................................................

vboot: init display on recovery request

Display is required by recovery mode. Do init when recovery
request by user.

BUG=b:133197727,b:133175864
TEST=enter recovery mode, checked the display shows up

Signed-off-by: Eric Lai 
Change-Id: Id6ac611f51241373bca3e2b394a94dcd52d3fde7
---
M src/security/vboot/vboot_logic.c
1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/32906/4

Change in ...coreboot[master]: vboot: init display on recovery request

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: init display on recovery request
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/32906/1/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/32906/1/src/security/vboot/vboot_logic.c@347 
PS1, Line 347: If recovery mode, do initialize display
> On recovery mode, initialize display

Done

Change in ...coreboot[master]: vboot: init display on recovery request

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: init display on recovery request
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/32906/4/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/32906/4/src/security/vboot/vboot_logic.c@371 
PS4, Line 371: 	/* Jot down some information from vboot which may be required later on
             : 	   in coreboot boot flow. */
             : 	if (ctx.flags & VB2_CONTEXT_DISPLAY_INIT)
             : 		/* Mainboard/SoC should initialize display. */
             : 		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT;
             : 	if (ctx.flags & VB2_CONTEXT_DEVELOPER_MODE)
             : 		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE;
Actually, I think we can fix this issue by just moving this block of code up above the if (rv) check.  No need to have a separate case for VB2_CONTEXT_RECOVERY_MODE.

Change in ...coreboot[master]: vboot: init display on recovery request

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: init display on recovery request
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/32906/4/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/32906/4/src/security/vboot/vboot_logic.c@371 
PS4, Line 371: 	/* Jot down some information from vboot which may be required later on
             : 	   in coreboot boot flow. */
             : 	if (ctx.flags & VB2_CONTEXT_DISPLAY_INIT)
             : 		/* Mainboard/SoC should initialize display. */
             : 		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT;
             : 	if (ctx.flags & VB2_CONTEXT_DEVELOPER_MODE)
             : 		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE;
> Actually, I think we can fix this issue by just moving this block of code up above the if (rv) check […]

Okay, I'll verified this tomorrow. I also guess that cause by recovery won't go to phase 2. Or you already verified that?

Change in ...coreboot[master]: vboot: init display on recovery request

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: init display on recovery request
......................................................................

Patch Set 4:

(1 comment)

Sorry for missing this.

https://review.coreboot.org/#/c/32906/4/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/32906/4/src/security/vboot/vboot_logic.c@371 
PS4, Line 371: 	/* Jot down some information from vboot which may be required later on
             : 	   in coreboot boot flow. */
             : 	if (ctx.flags & VB2_CONTEXT_DISPLAY_INIT)
             : 		/* Mainboard/SoC should initialize display. */
             : 		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT;
             : 	if (ctx.flags & VB2_CONTEXT_DEVELOPER_MODE)
             : 		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE;
> Okay, I'll verified this tomorrow. I also guess that cause by recovery won't go to phase 2. […]

Yes, I agree with Joel, the DISPLAY_INIT flag should already work correctly but the bug is that this block isn't executed in recovery mode. It needs to be moved before the return value check.

Change in ...coreboot[master]: vboot: determine display init before recovery check

[EricR Lai (Code Review)]

Hello Aaron Durbin, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Joel Kitching, Furquan Shaikh,

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

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

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

Change subject: vboot: determine display init before recovery check
......................................................................

vboot: determine display init before recovery check

Display is required by recovery mode. Determine display init
before recovery check.

BUG=b:133197727,b:133175864
TEST=enter recovery mode, checked the display shows up

Signed-off-by: Eric Lai 
Change-Id: Id6ac611f51241373bca3e2b394a94dcd52d3fde7
---
M src/security/vboot/vboot_logic.c
1 file changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/32906/5

Change in ...coreboot[master]: vboot: determine display init before recovery check

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: determine display init before recovery check
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/#/c/32906/4/src/security/vboot/vboot_logic.c 
File src/security/vboot/vboot_logic.c:

https://review.coreboot.org/#/c/32906/4/src/security/vboot/vboot_logic.c@371 
PS4, Line 371: 	/* Jot down some information from vboot which may be required later on
             : 	   in coreboot boot flow. */
             : 	if (ctx.flags & VB2_CONTEXT_DISPLAY_INIT)
             : 		/* Mainboard/SoC should initialize display. */
             : 		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT;
             : 	if (ctx.flags & VB2_CONTEXT_DEVELOPER_MODE)
             : 		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE;
> Yes, I agree with Joel, the DISPLAY_INIT flag should already work correctly but the bug is that this […]

It worked. Please review this.

Change in ...coreboot[master]: vboot: determine display init before recovery check

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: determine display init before recovery check
......................................................................

Patch Set 5: Code-Review+2

Change in ...coreboot[master]: vboot: determine display init before recovery check

[Duncan Laurie (Code Review)]

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

Change subject: vboot: determine display init before recovery check
......................................................................

Patch Set 5: Code-Review+2

Change in ...coreboot[master]: vboot: determine display init before recovery check

[Duncan Laurie (Code Review)]

Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32906 )

Change subject: vboot: determine display init before recovery check
......................................................................

vboot: determine display init before recovery check

Display is required by recovery mode. Determine display init
before recovery check.

BUG=b:133197727,b:133175864
TEST=enter recovery mode, checked the display shows up

Signed-off-by: Eric Lai 
Change-Id: Id6ac611f51241373bca3e2b394a94dcd52d3fde7
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32906
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Duncan Laurie 
---
M src/security/vboot/vboot_logic.c
1 file changed, 8 insertions(+), 8 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Duncan Laurie: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c
index 00347c3..d4ad327 100644
--- a/src/security/vboot/vboot_logic.c
+++ b/src/security/vboot/vboot_logic.c
@@ -344,6 +344,14 @@
    printk(BIOS_INFO, "Phase 1\n");
    rv = vb2api_fw_phase1(&ctx);

+	/* Jot down some information from vboot which may be required later on
+	   in coreboot boot flow. */
+	if (ctx.flags & VB2_CONTEXT_DISPLAY_INIT)
+		/* Mainboard/SoC should initialize display. */
+		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT;
+	if (ctx.flags & VB2_CONTEXT_DEVELOPER_MODE)
+		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE;
+
    if (rv) {
    	/*
    	 * If vb2api_fw_phase1 fails, check for return value.
@@ -364,14 +372,6 @@
    	vboot_reboot();
    }

-	/* Jot down some information from vboot which may be required later on
-	   in coreboot boot flow. */
-	if (ctx.flags & VB2_CONTEXT_DISPLAY_INIT)
-		/* Mainboard/SoC should initialize display. */
-		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT;
-	if (ctx.flags & VB2_CONTEXT_DEVELOPER_MODE)
-		vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE;
-
    /* Determine which firmware slot to boot (based on NVRAM) */
    printk(BIOS_INFO, "Phase 2\n");
    rv = vb2api_fw_phase2(&ctx);