g: introduce versioning and backwards compatibility of usb_upgrade

The original version of usb_upgrade does not provide for any error
recovery and also does not support any protocol version notion.

Real life experience has shown that error recovery is essential, it
needs to be introduced as a protocol enhancement. We want to stay
backwards compatible though, so there is a need for protocol
versioning.

In the original version of the protocol target response is always the
same: a 4 byte number which is the error code (and zero means no
error). This patch modifies response to the very first packet from the
host, the startup packet. The startup response is 8 bytes long. The
first 4 bytes is still the same as before, the second 4 bytes carry
the protocol version supported by the target, an integer in network
byte order.

Thus, receiving a 4 byte reply to the startup message tells the host
that the target is running protocol version zero, 8 byte reply carries
the actual version number in the last 4 bytes.

The USB transfer function on the host is enhanced to accept responses
shorter then expected, when allowed.

BRANCH=none
BUG=chrome-os-partner:52856
TEST=usb_updater can successfully update both old and new cr50 images,
     properly reporting protocol version as 0 or 1 respectively.

Change-Id: I9920d2708b21f29615282161fc0eb027018f9378
Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/341617
Reviewed-by: Mary Ruthven <mruthven@chromium.org>

2 files changed, 92 insertions, 10 deletions

diff --git a/chip/g/usb_upgrade.c b/chip/g/usb_upgrade.c
index df3d34ca11..634556cffb 100644
--- a/chip/g/usb_upgrade.c
+++ b/chip/g/usb_upgrade.c
@@ -97,6 +97,8 @@ static uint32_t block_index;
  */
 static uint64_t prev_activity_timestamp;
 
+#define UPGRADE_PROTOCOL_VERSION 1
+
 /* Called to deal with data from the host */
 static void upgrade_out_handler(struct consumer const *consumer, size_t count)
 {
@@ -124,6 +126,29 @@ 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;
+
 		/* This better be the first block, of zero size. */
 		if (count != sizeof(struct update_pdu_header)) {
 			CPRINTS("FW update: wrong first block size %d",
@@ -140,15 +165,19 @@ static void upgrade_out_handler(struct consumer const *consumer, size_t count)
 					   &resp_size);
 
 		if (resp_size == 4) {
-			resp_value = updu.resp; /* Already in network order. */
+			/* Already in network order. */
+			startup_resp.value = updu.resp;
 			rx_state_ = rx_outside_block;
 		} else {
 			/* This must be a single byte error code. */
-			resp_value = htobe32(*((uint8_t *)&updu.resp));
+			startup_resp.value = htobe32(*((uint8_t *)&updu.resp));
 		}
+
+		startup_resp.version = htobe32(UPGRADE_PROTOCOL_VERSION);
+
 		/* Let the host know what upgrader had to say. */
-		QUEUE_ADD_UNITS(&upgrade_to_usb, &resp_value,
-				sizeof(resp_value));
+		QUEUE_ADD_UNITS(&upgrade_to_usb, &startup_resp,
+				sizeof(startup_resp));
 		return;
 	}
 
diff --git a/extra/usb_updater/usb_updater.c b/extra/usb_updater/usb_updater.c
index 588a176a27..808c042e87 100644
--- a/extra/usb_updater/usb_updater.c
+++ b/extra/usb_updater/usb_updater.c
@@ -113,8 +113,15 @@ static uint8_t *get_file_or_die(const char *filename, uint32_t *len_ptr)
 	fprintf(stderr, "%s:%d, %s returned %d (%s)\n", __FILE__, __LINE__, \
 		m, r, libusb_strerror(r))
 
-static void xfer(struct usb_endpoint *uep, void *outbuf,
-		 int outlen, void *inbuf, int inlen)
+/*
+ * Actual USB transfer function, the 'allow_less' flag indicates that the
+ * valid response could be shortef than allotted memory, the 'rxed_count'
+ * pointer, if provided along with 'allow_less' lets the caller know how mavy
+ * bytes were received.
+ */
+static void do_xfer(struct usb_endpoint *uep, void *outbuf, int outlen,
+		    void *inbuf, int inlen, int allow_less,
+		    int *rxed_count)
 {
 
 	int r, actual;
@@ -147,14 +154,22 @@ static void xfer(struct usb_endpoint *uep, void *outbuf,
 			USB_ERROR("libusb_bulk_transfer", r);
 			exit(1);
 		}
-		if (actual != inlen) {
+		if ((actual != inlen) && !allow_less) {
 			fprintf(stderr, "%s:%d, only received %d/%d bytes\n",
 				__FILE__, __LINE__, actual, inlen);
 			shut_down(uep);
 		}
+
+		if (rxed_count)
+			*rxed_count = actual;
 	}
 }
 
+static void xfer(struct usb_endpoint *uep, void *outbuf,
+		 int outlen, void *inbuf, int inlen)
+{
+	do_xfer(uep, outbuf, outlen, inbuf, inlen, 0, NULL);
+}
 
 /* Return 0 on error, since it's never gonna be EP 0 */
 static int find_endpoint(const struct libusb_interface_descriptor *iface,
@@ -288,6 +303,25 @@ 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 void transfer_and_reboot(struct usb_endpoint *uep,
 				uint8_t *data, uint32_t data_len)
@@ -297,15 +331,34 @@ static void transfer_and_reboot(struct usb_endpoint *uep,
 	uint8_t *data_ptr;
 	uint32_t next_offset;
 	struct update_pdu updu;
+	struct startup_resp first_resp;
+	int rxed_size;
+	uint32_t protocol_version;
 
 	/* Send start/erase request */
 	printf("erase\n");
 
 	memset(&updu, 0, sizeof(updu));
 	updu.block_size = htobe32(sizeof(updu));
-	xfer(uep, &updu, sizeof(updu), &reply, sizeof(reply));
-/* check the offset here. */
-	next_offset = be32toh(reply) - FLASH_BASE;
+
+	do_xfer(uep, &updu, sizeof(updu), &first_resp, sizeof(first_resp),
+		1, &rxed_size);
+
+	if (rxed_size == sizeof(uint32_t))
+		protocol_version = 0;
+	else
+		protocol_version = be32toh(first_resp.version);
+
+	printf("Target running protocol version %d\n", protocol_version);
+
+	reply = be32toh(first_resp.value);
+
+	if (reply < 256) {
+		fprintf(stderr, "Target reports error %d\n", reply);
+		shut_down(uep);
+	}
+
+	next_offset = reply - FLASH_BASE;
 	printf("Updating at offset 0x%08x\n", next_offset);
 
 	data_ptr = data + next_offset;