g: usb_updater: clean up update protocol handling

In preparation of introducing new update protocol version this patch
cleans up the existing implementation.

The receive and transmit PDU headers are separated into their own
structures and are now shared between the server and client sides.

Some comments have been added to better explain different protocol
versions' details.

BRANCH=none
BUG=chrome-os-partner:55789
TEST=verified that it is still possible to update RW_A and RW_B on a
     Kevin CR50, works using both USB and SPI.

Change-Id: Ied65b2c2a7800bae045f1d2dd64e58dd5e793d27
Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/368989
Reviewed-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-by: Mary Ruthven <mruthven@chromium.org>

chip/g/upgrade_fw.c

@@ -13,6 +13,7 @@
 #include "memory.h"
 #include "uart.h"
 
+#include "upgrade_fw.h"
 #include "cryptoc/sha.h"
 
 #define CPRINTF(format, args...) cprintf(CC_EXTENSION, format, ## args)
@@ -28,20 +29,6 @@ enum return_value {
 	UPGRADE_GEN_ERROR = 6,
 };
 
-/*
- * The payload of the upgrade command. (Integer values in network byte order).
- *
- * block digest: the first four bytes of the sha1 digest of the rest of the
- *               structure.
- * block_base:  address where this block needs to be written to.
- * block_body:  variable size data to written at address 'block_base'.
- */
-struct upgrade_command {
-	uint32_t  block_digest;
-	uint32_t  block_base;
-	uint8_t   block_body[0];
-} __packed;
-
 /*
  * This array defines two possibe sections available for the firmare update.
  * The section whcih does not map the current execting code is picked as the
@@ -91,6 +78,7 @@ void fw_upgrade_command_handler(void *body,
 				size_t *response_size)
 {
 	struct upgrade_command *cmd_body = body;
+	void *upgrade_data;
 	uint8_t *rv = body;
 	uint8_t sha1_digest[SHA_DIGEST_SIZE];
 	size_t body_size;
@@ -102,12 +90,12 @@ void fw_upgrade_command_handler(void *body,
 	 */
 	*response_size = sizeof(*rv);
 
-	if (cmd_size < offsetof(struct upgrade_command, block_body)) {
+	if (cmd_size < sizeof(struct upgrade_command)) {
 		CPRINTF("%s:%d\n", __func__, __LINE__);
 		*rv = UPGRADE_GEN_ERROR;
 		return;
 	}
-	body_size = cmd_size - offsetof(struct upgrade_command, block_body);
+	body_size = cmd_size - sizeof(struct upgrade_command);
 
 	if (!cmd_body->block_base && !body_size) {
 		/*
@@ -171,8 +159,9 @@ void fw_upgrade_command_handler(void *body,
 
 	CPRINTF("%s: programming at address 0x%x\n", __func__,
 		block_offset + CONFIG_PROGRAM_MEMORY_BASE);
-	if (flash_physical_write(block_offset, body_size,
-				 cmd_body->block_body) != EC_SUCCESS) {
+	upgrade_data = cmd_body + 1;
+	if (flash_physical_write(block_offset, body_size, upgrade_data)
+	    != EC_SUCCESS) {
 		*rv = UPGRADE_WRITE_FAILURE;
 		CPRINTF("%s:%d upgrade write error\n",
 			__func__, __LINE__);
@@ -180,7 +169,7 @@ void fw_upgrade_command_handler(void *body,
 	}
 
 	/* Werify that data was written properly. */
-	if (memcmp(cmd_body->block_body, (void *)
+	if (memcmp(upgrade_data, (void *)
 		   (block_offset + CONFIG_PROGRAM_MEMORY_BASE),
 		   body_size)) {
 		*rv = UPGRADE_VERIFY_ERROR;

chip/g/upgrade_fw.h

@@ -8,6 +8,58 @@
 
 #include <stddef.h>
 
+#define UPGRADE_PROTOCOL_VERSION 2
+
+/* This is the format of the header the flash update function expects. */
+struct upgrade_command {
+	uint32_t  block_digest;  /* first 4 bytes of sha1 of the rest of the
+				  * block.
+				  */
+	uint32_t  block_base;    /* Offset of this block into the flash SPI. */
+	/* The actual payload goes here. */
+} __packed;
+
+/*
+ * This is the format of the header the host uses when sending update PDUs
+ * over USB. The PDUs are 1K bytes in size, they are fragmented into USB units
+ * of 64 bytes each and reassembled on the receive side before being passed to
+ * the flash update function.
+ *
+ * The flash update function receives the PDU body starting at the cmd field
+ * below, and puts its reply into the beginning of the same buffer.
+ */
+struct update_pdu_header {
+	uint32_t block_size;    /* Total size of the block, including this
+				 * field.
+				 */
+	struct upgrade_command cmd;
+};
+
+/*
+ * Response to the message initiating the update sequence.
+ *
+ * When responding to the very first packet of the upgrade sequence, the
+ * original USB update implementation was responding with a four byte value,
+ * just as to any other block of the transfer sequence.
+ *
+ * It became clear that there is a need to be able to enhance the upgrade
+ * protocol, while stayng backwards compatible.
+ *
+ * All newer protocol versions (satring with version 2) respond to the very
+ * first packet with an 8 byte or larger response, where the first 4 bytes are
+ * a version specific data, and the second 4 bytes - the protocol version
+ * number.
+ *
+ * This way the host receiving of a four byte value in response to the first
+ * packet is considered an indication of the target running the 'legacy'
+ * protocol, version 1. Receiving of an 8 byte or longer response would
+ * communicates the protocol version in the second 4 bytes.
+ */
+struct first_response_pdu {
+	uint32_t return_value;
+	uint32_t protocol_version;
+};
+
 /* TODO: Handle this in upgrade_fw.c, not usb_upgrade.c */
 #define UPGRADE_DONE          0xB007AB1E
 

chip/g/usb_upgrade.c

@@ -67,25 +67,6 @@ enum rx_state {
 	rx_awaiting_reset /* Waiting for reset confirmation. */
 };
 
-/* This is the format of the header the programmer expects. */
-struct upgrade_command {
-	uint32_t  block_digest;  /* first 4 bytes of sha1 of the rest of the
-				    block. */
-	uint32_t  block_base;    /* Offset of this block into the flash SPI. */
-};
-
-/* This is the format of the header the host uses. */
-struct update_pdu_header {
-	uint32_t block_size;    /* Total size of the block, including this
-				   field. */
-	union {
-		struct upgrade_command cmd;
-		uint32_t resp; /* The programmer puts response to the same
-				  buffer where the command was. */
-	};
-	/* The actual payload goes here. */
-};
-
 enum rx_state rx_state_ = rx_idle;
 static uint8_t *block_buffer;
 static uint32_t block_size;
@@ -131,8 +112,6 @@ static int valid_transfer_start(struct consumer const *consumer, size_t count,
  */
 static uint64_t prev_activity_timestamp;
 
-#define UPGRADE_PROTOCOL_VERSION 2
-
 /* Called to deal with data from the host */
 static void upgrade_out_handler(struct consumer const *consumer, size_t count)
 {
@@ -160,28 +139,7 @@ static void upgrade_out_handler(struct consumer const *consumer, size_t count)
 	}
 
 	if (rx_state_ == rx_idle) {
-		/*
-		 * When responding to the very first packet of the upgrade
-		 * sequence, the original implementation was responding with a
-		 * four byte value, just as to any other block of the transfer
-		 * sequence.
-		 *
-		 * It became clear that there is a need to be able to enhance
-		 * the upgrade protocol, while stayng backwards compatible. To
-		 * achieve that we respond to the very first packet with an 8
-		 * byte value, the first 4 bytes the same as before, the
-		 * second 4 bytes - the protocol version number.
-		 *
-		 * This way if on the host side receiving of a four byte value
-		 * in response to the first packet is an indication of the
-		 * 'legacy' protocol, version 0. Receiving of an 8 byte
-		 * response would communicate the protocol version in the
-		 * second 4 bytes.
-		 */
-		struct {
-			uint32_t value;
-			uint32_t version;
-		} startup_resp;
+		struct first_response_pdu *startup_resp;
 
 		if (!valid_transfer_start(consumer, count, &updu))
 			return;
@@ -193,20 +151,32 @@ static void upgrade_out_handler(struct consumer const *consumer, size_t count)
 						    cmd),
 					   &resp_size);
 
+		/*
+		 * The handler reuses receive buffer to return the result
+		 * value.
+		 */
+		startup_resp = (struct first_response_pdu *)&updu.cmd;
 		if (resp_size == 4) {
-			/* Already in network order. */
-			startup_resp.value = updu.resp;
+			/*
+			 * The handler is happy, returned a 4 byte base
+			 * offset, it is in startup_resp->return_value now in
+			 * the proper byte order.
+			 */
 			rx_state_ = rx_outside_block;
 		} else {
-			/* This must be a single byte error code. */
-			startup_resp.value = htobe32(*((uint8_t *)&updu.resp));
+			/*
+			 * This must be a single byte error code, convert it
+			 * into a 4 byte network order representation.
+			 */
+			startup_resp->return_value = htobe32
+				(*((uint8_t *)&startup_resp->return_value));
 		}
-
-		startup_resp.version = htobe32(UPGRADE_PROTOCOL_VERSION);
+		startup_resp->protocol_version =
+			htobe32(UPGRADE_PROTOCOL_VERSION);
 
 		/* Let the host know what upgrader had to say. */
-		QUEUE_ADD_UNITS(&upgrade_to_usb, &startup_resp,
-				sizeof(startup_resp));
+		QUEUE_ADD_UNITS(&upgrade_to_usb, startup_resp,
+				sizeof(*startup_resp));
 		return;
 	}
 

extra/usb_updater/usb_updater.c

@@ -403,11 +403,6 @@ static void usb_findit(uint16_t vid, uint16_t pid, struct usb_endpoint *uep)
 	printf("READY\n-------\n");
 }
 
-struct upgrade_command {
-	uint32_t  block_digest;
-	uint32_t  block_base;
-};
-
 struct update_pdu {
 	uint32_t block_size; /* Total block size, include this field's size. */
 	struct upgrade_command cmd;
@@ -415,26 +410,6 @@ struct update_pdu {
 };
 
 #define FLASH_BASE 0x40000
-/*
- * When responding to the very first packet of the upgrade sequence, the
- * original implementation was responding with a four byte value, just as to
- * any other block of the transfer sequence.
- *
- * It became clear that there is a need to be able to enhance the upgrade
- * protocol, while stayng backwards compatible. To achieve that a new startup
- * response option was introduced, 8 bytes in size. The first 4 bytes the same
- * as before, the second 4 bytes - the protocol version number.
- *
- * So, receiving of a four byte value in response to the startup packet is an
- * indication of the 'legacy' protocol, version 0. Receiving of an 8 byte
- * value communicates the protocol version in the second 4 bytes.
- */
-
-struct startup_resp {
-	uint32_t value;
-	uint32_t version;
-};
-
 static int transfer_block(struct usb_endpoint *uep, struct update_pdu *updu,
 			  uint8_t *transfer_data_ptr, size_t payload_size)
 {
@@ -535,7 +510,7 @@ static void transfer_section(struct transfer_endpoint *tep,
 				exit(1);
 			}
 		} else {
-			struct startup_resp resp;
+			struct first_response_pdu resp;
 			size_t rxed_size;
 
 			rxed_size = sizeof(resp);
@@ -553,7 +528,7 @@ static void transfer_section(struct transfer_endpoint *tep,
 			if ((rxed_size != 1) || *((uint8_t *)&resp)) {
 				fprintf(stderr,
 					"got response of size %zd, value %#x\n",
-					rxed_size, resp.value);
+					rxed_size, resp.return_value);
 
 				exit(1);
 			}
@@ -571,7 +546,7 @@ static void transfer_and_reboot(struct transfer_endpoint *tep,
 	uint32_t out;
 	uint32_t reply;
 	struct update_pdu updu;
-	struct startup_resp first_resp;
+	struct first_response_pdu first_resp;
 	size_t rxed_size;
 	struct usb_endpoint *uep = &tep->uep;
 
@@ -596,11 +571,11 @@ static void transfer_and_reboot(struct transfer_endpoint *tep,
 	if (rxed_size == sizeof(uint32_t))
 		protocol_version = 0;
 	else
-		protocol_version = be32toh(first_resp.version);
+		protocol_version = be32toh(first_resp.protocol_version);
 
 	printf("Target running protocol version %d\n", protocol_version);
 
-	reply = be32toh(first_resp.value);
+	reply = be32toh(first_resp.return_value);
 
 	if (reply < 256) {
 		fprintf(stderr, "Target reports error %d\n", reply);