Change in em100[master]: Make firmware update hardware version aware

[Stefan Reinauer (Code Review)]

Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/36836 )

Change subject: Make firmware update hardware version aware
......................................................................

Make firmware update hardware version aware

Up to now, you can accidently write an EM100Pro firmware version on
an EM100Pro-G2 device. This will brick your device and can not easily
be recovered with a SPI flasher, so be careful.
Hence, the firmware update code should make sure that HW version 4 is
used to commence a firmware update.

Right now, release binaries do not contain EM100Pro-G2 firmware images,
so this version of the code bails out on anything but HW version 4.

Signed-off-by: Stefan Reinauer 
Change-Id: Ia284adca4cb4d341a8d229374b6ea0b5d2a5d5c0
---
M firmware.c
1 file changed, 25 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/em100 refs/changes/36/36836/1

diff --git a/firmware.c b/firmware.c
index f5130f8..bbb0ca4 100644
--- a/firmware.c
+++ b/firmware.c
@@ -118,12 +118,22 @@
    }

if (firmware_is_dpfw) {
-		int fpga_size = 0, mcu_size = 0;
+		int fpga_size = 0, mcu_size = 0, hdrversion = 0;
    	char all_ff[256];
    	char mcu_version[8];
    	char fpga_version[8];
    	unsigned char header[0x100];

+		switch (em100->hwversion) {
+		case 4:
+			hdrversion=1;
+			break;
+		default:
+			printf("Dumping DPFW firmware on hardware version %u is "
+					"not yet supported.\n", em100->hwversion);
+			exit(1);
+		}
+
    	memset(all_ff, 255, sizeof(all_ff));
    	for (i = 0; i < 0x100000; i+=0x100) {
    		if (memcmp(data+i, all_ff, 256) == 0)
@@ -153,7 +163,8 @@
    			em100->fpga >> 8 & 0x7f, em100->fpga & 0xff);

memset(header, 0, 0x100);
-		memcpy(header, "em100pro", 8);
+		if (hdrversion == 1)
+			memcpy(header, "em100pro", 8);
    	memcpy(header + 0x28, "WFPD", 4);
    	memcpy(header + 0x14, mcu_version, 4);
    	memcpy(header + 0x1e, fpga_version, 4);
@@ -188,6 +199,16 @@
    char fpga_version[MAX_VERSION_LENGTH + 1],
         mcu_version[MAX_VERSION_LENGTH + 1];

+	switch (em100->hwversion) {
+	case 4:
+		printf("Detected EM100Pro-G1.\n");
+		break;
+	default:
+		printf("Updating EM100Pro firmware on hardware version %u is "
+				"not yet supported.\n", em100->hwversion);
+		exit(1);
+	}
+
    printf("\nAttempting firmware update with file %s\n", filename);

f = fopen(filename, "rb");
@@ -219,8 +240,8 @@
    }
    fclose(f);

-	if (memcmp(fw, "em100pro", 8) != 0 ||
-			memcmp(fw + 0x28, "WFPD", 4) != 0) {
+	if (em100->hwversion == 4 && (memcmp(fw, "em100pro", 8) != 0 ||
+			memcmp(fw + 0x28, "WFPD", 4) != 0)) {
    	printf("ERROR: Not an EM100Pro firmware file.\n");
    	free(fw);
    	return 0;

[Patrick Georgi (Code Review)]

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

Change subject: Make firmware update hardware version aware
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/em100/+/36836/1/firmware.c 
File firmware.c:

https://review.coreboot.org/c/em100/+/36836/1/firmware.c@203 
PS1, Line 203: 4
should we name them? or only once there are three of them?

https://review.coreboot.org/c/em100/+/36836/1/firmware.c@243 
PS1, Line 243: em100->hwversion == 4 
It's still potentially an EM100Pro firmware file, so the error message would be confusing, no?
I'd also put the hwversion test separately and before the content snooping to prepare for future changes when there are updates.

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/36836 )

Change subject: Make firmware update hardware version aware
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/em100/+/36836/1/firmware.c 
File firmware.c:

https://review.coreboot.org/c/em100/+/36836/1/firmware.c@203 
PS1, Line 203: 4
> should we name them? or only once there are three of them?

HW_VERSION_4? I considered that, but I didn't feel that was going to be helpful. Particularly since I don't know at this point if there are non-4 EM100Pro or non-6 EM100Pro-G2s out there (and I never saw an EM100 non PRO)

https://review.coreboot.org/c/em100/+/36836/1/firmware.c@243 
PS1, Line 243: em100->hwversion == 4
> It's still potentially an EM100Pro firmware file, so the error message would be confusing, no? […]

I have a followon patch in the queue. I think I'll change the message to EM100Pro (non-G2) or EM100Pro (G1).

The other files will be referred to as EM100Pro-G2 firmware files.

[Patrick Georgi (Code Review)]

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

Change subject: Make firmware update hardware version aware
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/em100/+/36836/1/firmware.c 
File firmware.c:

https://review.coreboot.org/c/em100/+/36836/1/firmware.c@203 
PS1, Line 203: 4
> HW_VERSION_4? I considered that, but I didn't feel that was going to be helpful. […]

How about:
#define HWVER_GEN1 4 // or HWVER_ORIGINAL
#define HWVER_GEN2 6

That way it's clearer that "4" is the old stuff, and that there's a 6, and that any other number is something we definitely don't support yet (suppose somebody skips Pro-G2 and sees versions 4 and 8, assuming that all non-4 code here is for "8")

https://review.coreboot.org/c/em100/+/36836/1/firmware.c@243 
PS1, Line 243: em100->hwversion == 4
> I have a followon patch in the queue. […]

Even then this is saying "this is an EM100Pro (non-G2) file if and only if there's also an EM100Pro (non-G2) attached)"

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/36836 )

Change subject: Make firmware update hardware version aware
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/em100/+/36836/1/firmware.c 
File firmware.c:

https://review.coreboot.org/c/em100/+/36836/1/firmware.c@203 
PS1, Line 203: 4
> How about: […]

What if then a HWVER_5 shows up?

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/36836 )

Change subject: Make firmware update hardware version aware
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/em100/+/36836/1/firmware.c 
File firmware.c:

https://review.coreboot.org/c/em100/+/36836/1/firmware.c@243 
PS1, Line 243: em100->hwversion == 4
> Even then this is saying "this is an EM100Pro (non-G2) file if and only if there's also an EM100Pro  […]

It says "This is NOT an EM100Pro file", not "This is an EM100Pro file.

At this point you will not get to this code if hwversion is anything but 4. The reason for this additional check is that on G2 (when the G2 code is added), the check and message will not make sense (and there is a second check for G2 specifically)

[Patrick Georgi (Code Review)]

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

Change subject: Make firmware update hardware version aware
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

Let's do things in follow up commits. This one is about preventing bricks.

https://review.coreboot.org/c/em100/+/36836/1/firmware.c 
File firmware.c:

https://review.coreboot.org/c/em100/+/36836/1/firmware.c@203 
PS1, Line 203: 4
> What if then a HWVER_5 shows up?

In that case I'd like to know even more if it's a GEN1 or a GEN2 (HWVER_GEN[12]a, maybe?) or, in case it's neither, whether it's a HWVER_GEN1_5 or a EM250 Dancing Chicken (HWVER_EM250DC). All of that provides more insight than a simple number IMHO.

But I'm fine with leaving the number around right now until there's a real need to give more context.

[Stefan Reinauer (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Martin Roth,

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

https://review.coreboot.org/c/em100/+/36836

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

Change subject: Make firmware update hardware version aware
......................................................................

Make firmware update hardware version aware

Up to now, you can accidently write an EM100Pro firmware version on
an EM100Pro-G2 device. This will brick your device and can not easily
be recovered with a SPI flasher, so be careful.
Hence, the firmware update code should make sure that HW version 4 is
used to commence a firmware update.

Right now, release binaries do not contain EM100Pro-G2 firmware images,
so this version of the code bails out on anything but HW version 4.

Signed-off-by: Stefan Reinauer 
Change-Id: Ia284adca4cb4d341a8d229374b6ea0b5d2a5d5c0
---
M em100.h
M firmware.c
2 files changed, 29 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/em100 refs/changes/36/36836/2

[Patrick Georgi (Code Review)]

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

Change subject: Make firmware update hardware version aware
......................................................................

Patch Set 2: Code-Review+2

[Stefan Reinauer (Code Review)]

Stefan Reinauer has submitted this change. ( https://review.coreboot.org/c/em100/+/36836 )

Change subject: Make firmware update hardware version aware
......................................................................

Make firmware update hardware version aware

Up to now, you can accidently write an EM100Pro firmware version on
an EM100Pro-G2 device. This will brick your device and can not easily
be recovered with a SPI flasher, so be careful.
Hence, the firmware update code should make sure that HW version 4 is
used to commence a firmware update.

Right now, release binaries do not contain EM100Pro-G2 firmware images,
so this version of the code bails out on anything but HW version 4.

Signed-off-by: Stefan Reinauer 
Change-Id: Ia284adca4cb4d341a8d229374b6ea0b5d2a5d5c0
Reviewed-on: https://review.coreboot.org/c/em100/+/36836
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
---
M em100.h
M firmware.c
2 files changed, 29 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/em100.h b/em100.h
index 2159fba..9ebcb13 100644
--- a/em100.h
+++ b/em100.h
@@ -29,6 +29,10 @@
    uint8_t hwversion;
 };

+/* Hardware versions */
+#define HWVERSION_EM100PRO    4
+#define HWVERSION_EM100PRO_G2 6
+
 #define BULK_SEND_TIMEOUT	5000	/* sentinel value */

/* usb.c */
diff --git a/firmware.c b/firmware.c
index c88011c..6e2ff36 100644
--- a/firmware.c
+++ b/firmware.c
@@ -119,12 +119,22 @@
    }

if (firmware_is_dpfw) {
-		int fpga_size = 0, mcu_size = 0;
+		int fpga_size = 0, mcu_size = 0, hdrversion = 0;
    	char all_ff[256];
    	char mcu_version[8];
    	char fpga_version[8];
    	unsigned char header[0x100];

+		switch (em100->hwversion) {
+		case HWVERSION_EM100PRO:
+			hdrversion=1;
+			break;
+		default:
+			printf("Dumping DPFW firmware on hardware version %u is "
+					"not yet supported.\n", em100->hwversion);
+			exit(1);
+		}
+
    	memset(all_ff, 255, sizeof(all_ff));
    	for (i = 0; i < 0x100000; i+=0x100) {
    		if (memcmp(data+i, all_ff, 256) == 0)
@@ -156,7 +166,8 @@
    			em100->fpga >> 8 & 0x7f, em100->fpga & 0xff);

memset(header, 0, 0x100);
-		memcpy(header, "em100pro", 8);
+		if (hdrversion == 1)
+			memcpy(header, "em100pro", 8);
    	memcpy(header + 0x28, "WFPD", 4);
    	memcpy(header + 0x14, mcu_version, 4);
    	memcpy(header + 0x1e, fpga_version, 4);
@@ -192,6 +203,16 @@
    char fpga_version[MAX_VERSION_LENGTH + 1],
         mcu_version[MAX_VERSION_LENGTH + 1];

+	switch (em100->hwversion) {
+	case HWVERSION_EM100PRO:
+		printf("Detected EM100Pro-G1.\n");
+		break;
+	default:
+		printf("Updating EM100Pro firmware on hardware version %u is "
+				"not yet supported.\n", em100->hwversion);
+		exit(1);
+	}
+
    printf("\nAttempting firmware update with file %s\n", filename);

f = fopen(filename, "rb");
@@ -223,8 +244,8 @@
    }
    fclose(f);

-	if (memcmp(fw, "em100pro", 8) != 0 ||
-			memcmp(fw + 0x28, "WFPD", 4) != 0) {
+	if (em100->hwversion == HWVERSION_EM100PRO && (memcmp(fw, "em100pro", 8) != 0 ||
+			memcmp(fw + 0x28, "WFPD", 4) != 0)) {
    	printf("ERROR: Not an EM100Pro firmware file.\n");
    	free(fw);
    	return 0;