Change in ...coreboot[master]: lib/edid.c: Dump EDID breakdown after null check

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32055

Change subject: lib/edid.c: Dump EDID breakdown after null check
......................................................................

lib/edid.c: Dump EDID breakdown after null check

The edid variable was being dereferenced before the null check. Split
off the null check to before dumping and update the error message.

Fixes CID 1370576 (REVERSE_INULL)

Signed-off-by: Jacob Garber 
Change-Id: I8fe3d911df3a11a873056d3a5c05c5a3cbcfe2c0
---
M src/lib/edid.c
1 file changed, 8 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/32055/1

diff --git a/src/lib/edid.c b/src/lib/edid.c
index 553b0a2..5925759 100644
--- a/src/lib/edid.c
+++ b/src/lib/edid.c
@@ -1138,11 +1138,16 @@
        .conformant = EDID_CONFORMANT,
    };

-	dump_breakdown(edid);
-
    memset(out, 0, sizeof(*out));

-	if (!edid || memcmp(edid, "\x00\xFF\xFF\xFF\xFF\xFF\xFF\x00", 8)) {
+	if (!edid) {
+		printk(BIOS_SPEW, "No EDID found\n");
+		return EDID_ABSENT;
+	}
+
+	dump_breakdown(edid);
+
+	if (memcmp(edid, "\x00\xFF\xFF\xFF\xFF\xFF\xFF\x00", 8)) {
    	printk(BIOS_SPEW, "No header found\n");
    	return EDID_ABSENT;
    }

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32055 )

Change subject: lib/edid.c: Dump EDID breakdown after null check
......................................................................

Patch Set 1: Code-Review+1

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32055 )

Change subject: lib/edid.c: Dump EDID breakdown after null check
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32055 )

Change subject: lib/edid.c: Dump EDID breakdown after null check
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32055/1/src/lib/edid.c 
File src/lib/edid.c:

https://review.coreboot.org/#/c/32055/1/src/lib/edid.c@1144 
PS1, Line 1144: 		printk(BIOS_SPEW, "No EDID found\n");
Shouldn’t this be level error, warning or info instead? Maybe error?

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32055 )

Change subject: lib/edid.c: Dump EDID breakdown after null check
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32055/1/src/lib/edid.c 
File src/lib/edid.c:

https://review.coreboot.org/#/c/32055/1/src/lib/edid.c@1144 
PS1, Line 1144: 		printk(BIOS_SPEW, "No EDID found\n");
> Shouldn’t this be level error, warning or info instead? Maybe error?

That makes sense. We should also probably change the incorrect header message to an error too then. Should I put those into a separate commit?

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32055 )

Change subject: lib/edid.c: Dump EDID breakdown after null check
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32055/1/src/lib/edid.c 
File src/lib/edid.c:

https://review.coreboot.org/#/c/32055/1/src/lib/edid.c@1144 
PS1, Line 1144: 		printk(BIOS_SPEW, "No EDID found\n");
> That makes sense. We should also probably change the incorrect header message to an error too then. […]

Please update both in a follow-up commit.

[Patrick Georgi (Code Review)]

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

Change subject: lib/edid.c: Dump EDID breakdown after null check
......................................................................

lib/edid.c: Dump EDID breakdown after null check

The edid variable was being dereferenced before the null check. Split
off the null check to before dumping and update the error message.

Fixes CID 1370576 (REVERSE_INULL)

Signed-off-by: Jacob Garber 
Change-Id: I8fe3d911df3a11a873056d3a5c05c5a3cbcfe2c0
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32055
Tested-by: build bot (Jenkins) 
Reviewed-by: Edward O'Callaghan 
---
M src/lib/edid.c
1 file changed, 8 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Edward O'Callaghan: Looks good to me, approved

diff --git a/src/lib/edid.c b/src/lib/edid.c
index 553b0a2..5925759 100644
--- a/src/lib/edid.c
+++ b/src/lib/edid.c
@@ -1138,11 +1138,16 @@
        .conformant = EDID_CONFORMANT,
    };

-	dump_breakdown(edid);
-
    memset(out, 0, sizeof(*out));

-	if (!edid || memcmp(edid, "\x00\xFF\xFF\xFF\xFF\xFF\xFF\x00", 8)) {
+	if (!edid) {
+		printk(BIOS_SPEW, "No EDID found\n");
+		return EDID_ABSENT;
+	}
+
+	dump_breakdown(edid);
+
+	if (memcmp(edid, "\x00\xFF\xFF\xFF\xFF\xFF\xFF\x00", 8)) {
    	printk(BIOS_SPEW, "No header found\n");
    	return EDID_ABSENT;
    }