Change in flashrom[master]: raiden_debug_spi.c: Adds context states and helper functions

[Brian Nemec (Code Review)]

Hello Brian Nemec,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/flashrom/+/41608

to review the following change.

Change subject: raiden_debug_spi.c: Adds context states and helper functions
......................................................................

raiden_debug_spi.c: Adds context states and helper functions

Adds context states to handle the read and write buffers as transmit
and receive states. These are used to keep track of the number of
bytes transmitted and received allowing future support of multi-packet
messages in the v2 protocol.

Adds context states to handle the USB packets, these allow us to
simplify the process of loading data from the transmit buffer
into a USB packets' data section and from a USB packet to it's
receive buffers. These will also keep track of the size of the USB
packet allowing a simpler interface to transmit them.

Helper functions have been added to help with copying data between
the transmit and receive context states to and from the USB packets.

BUG=b:139058552
BRANCH=none
TEST=Manual testing of ServoMicro and Flashrom when performing
    reads, writes, and verification of the EC firmware on Nami.
TEST=Builds

Signed-off-by: Brian J. Nemec 
Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c
---
M raiden_debug_spi.c
1 file changed, 241 insertions(+), 94 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/1

diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c
index b18d580..4de8038 100644
--- a/raiden_debug_spi.c
+++ b/raiden_debug_spi.c
@@ -141,7 +141,21 @@

enum {
    GOOGLE_RAIDEN_SPI_PROTOCOL_V1 = 0x01,
-	GOOGLE_RAIDEN_SPI_PROTOCOL_V2 = 0x02,
+};
+
+enum {
+	/* The host failed to transfer the data with no libusb error. */
+	USB_SPI_HOST_TX_BAD_TRANSFER      = 0x10001,
+	/* We did not receive the expected USB packet. */
+	USB_SPI_HOST_RX_UNEXPECTED_PACKET = 0x18002,
+	/* We received a continue packet with an invalid data index.*/
+	USB_SPI_HOST_RX_BAD_DATA_INDEX    = 0x18003,
+	/* We too much data. */
+	USB_SPI_HOST_RX_DATA_OVERFLOW     = 0x18004,
+	/* We too little data. */
+	USB_SPI_HOST_RX_READ_FAILURE      = 0x18005,
+	/* We were unable to configure the device. */
+	USB_SPI_HOST_INIT_FAILURE         = 0x18006,
 };

enum usb_spi_error {
@@ -162,8 +176,8 @@
 };

#define PACKET_HEADER_SIZE      (2)
-#define MAX_PACKET_SIZE         (64)
-#define PAYLOAD_SIZE_V1         (MAX_PACKET_SIZE - PACKET_HEADER_SIZE)
+#define MAX_USB_PACKET_SIZE     (64)
+#define PAYLOAD_SIZE_V1         (MAX_USB_PACKET_SIZE - PACKET_HEADER_SIZE)

/*
  * Servo Micro has an error where it is capable of acknowledging USB packets
@@ -180,11 +194,10 @@
  */
 #define TRANSFER_TIMEOUT_MS     (200 + 800)

-struct raiden_debug_spi_data {
-	struct usb_device *dev;
-	uint8_t in_ep;
-	uint8_t out_ep;
-};
+
+/*
+ * Version 1 protocol specific attributes
+ */

typedef struct {
    int8_t write_count;
@@ -198,6 +211,51 @@
    uint8_t data[PAYLOAD_SIZE_V1];
 } __attribute__((packed)) usb_spi_response_v1_t;

+typedef union {
+	usb_spi_command_v1_t command;
+	usb_spi_response_v1_t response;
+} __attribute__((packed)) usb_spi_packet_v1_t;
+
+typedef struct {
+	union {
+		uint8_t bytes[MAX_USB_PACKET_SIZE];
+		usb_spi_packet_v1_t packet_v1;
+	};
+	/*
+	 * By storing the number of bytes in the header and knowing that the
+	 * USB data packets are all 64B long, we are able to use the header
+	 * size to store the offset of the buffer and it's size without
+	 * duplicating variables that can go out of sync.
+	 */
+	int header_size;
+	/* Number of bytes in the packet */
+	int packet_size;
+} usb_spi_packet_ctx_t;
+
+typedef struct {
+	/* Buffer we are reading data from. */
+	const uint8_t *buffer;
+	/* Total size of the transmit transfer. */
+	int transmit_size;
+	/* Number of bytes transmitted. */
+	int transmit_index;
+} usb_spi_transmit_ctx_t;
+
+typedef struct {
+	/* Buffer we are writing data into. */
+	uint8_t *buffer;
+	/* Total size of the receive transfer. */
+	int receive_size;
+	/* Number of bytes received. */
+	int receive_index;
+} usb_spi_receive_ctx_t;
+
+struct raiden_debug_spi_data {
+	struct usb_device *dev;
+	uint8_t in_ep;
+	uint8_t out_ep;
+};
+
 /*
  * This function will return true when an error code can potentially recover
  * if we attempt to write SPI data to the device or read from it. We know
@@ -213,7 +271,7 @@
    	 * during transfer errors to the device and can be recovered.
    	 */
    	if (USB_SPI_READ_COUNT_INVALID <= error_code &&
-		    error_code <= USB_SPI_DISABLED) {
+				error_code <= USB_SPI_DISABLED) {
    		return false;
    	}
    } else if (usb_device_is_libusb_error(error_code)) {
@@ -231,90 +289,139 @@
    return (const struct raiden_debug_spi_data *)flash->mst->spi.data;
 }

-static int write_command(const struct flashctx *flash,
-		unsigned int write_count,
-		unsigned int read_count,
-		const unsigned char *write_buffer,
-		unsigned char *read_buffer)
+/*
+ * Fill the data section in the USB packets.
+ */
+static int fill_usb_packet(usb_spi_packet_ctx_t* dst,
+		usb_spi_transmit_ctx_t* src)
 {
+	int transfer_size = src->transmit_size - src->transmit_index;
+	int max_buffer_size = MAX_USB_PACKET_SIZE - dst->header_size;
+	uint8_t *packet_buffer = dst->bytes + dst->header_size;

-	int transferred;
-	int ret;
-	usb_spi_command_v1_t command_packet;
-	const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash);
-
-	if (write_count > PAYLOAD_SIZE_V1) {
-		msg_perr("Raiden: Invalid write_count of %d\n", write_count);
-		return SPI_INVALID_LENGTH;
+	if (transfer_size > max_buffer_size) {
+		transfer_size = max_buffer_size;
    }
+	memcpy(packet_buffer, src->buffer + src->transmit_index, transfer_size);

-	if (read_count > PAYLOAD_SIZE_V1) {
-		msg_perr("Raiden: Invalid read_count of %d\n", read_count);
-		return SPI_INVALID_LENGTH;
-	}
-
-	command_packet.write_count = write_count;
-	command_packet.read_count = read_count;
-
-	memcpy(command_packet.data, write_buffer, write_count);
-
-	ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle,
-				ctx_data->out_ep,
-				(void*)&command_packet,
-				write_count + PACKET_HEADER_SIZE,
-				&transferred,
-				TRANSFER_TIMEOUT_MS));
-	if (ret != 0) {
-		msg_perr("Raiden: OUT transfer failed\n"
-		         "    write_count = %d\n"
-		         "    read_count  = %d\n",
-		         write_count, read_count);
-		return ret;
-	}
-
-	if ((unsigned) transferred != write_count + PACKET_HEADER_SIZE) {
-		msg_perr("Raiden: Write failure (wrote %d, expected %d)\n",
-			 transferred, write_count + PACKET_HEADER_SIZE);
-		return 0x10001;
-	}
-
+	dst->packet_size = dst->header_size + transfer_size;
+	src->transmit_index += transfer_size;
    return 0;
 }

-static int read_response(const struct flashctx *flash,
-		unsigned int write_count,
-		unsigned int read_count,
-		const unsigned char *write_buffer,
-		unsigned char *read_buffer)
+static int read_usb_packet(usb_spi_receive_ctx_t* dst,
+		const usb_spi_packet_ctx_t* src)
+{
+	int max_read_length = dst->receive_size - dst->receive_index;
+	int bytes_in_buffer = src->packet_size - src->header_size;
+	const uint8_t *packet_buffer = src->bytes + src->header_size;
+
+	if (bytes_in_buffer > max_read_length) {
+		/*
+		 * An error occurred, we should not receive more data than
+		 * the buffer can support.
+		 */
+		msg_perr("Raiden: Receive packet overflowed\n"
+				"    bytes_in_buffer = %d\n"
+				"    max_read_length = %d\n"
+				"    receive_index   = %d\n"
+				"    receive_size    = %d\n",
+				bytes_in_buffer, max_read_length,
+				dst->receive_size, dst->receive_index);
+		return USB_SPI_HOST_RX_DATA_OVERFLOW;
+	}
+	memcpy(dst->buffer + dst->receive_index, packet_buffer, bytes_in_buffer);
+
+	dst->receive_index += bytes_in_buffer;
+	return 0;
+}
+
+static int transmit_packet(const struct raiden_debug_spi_data * ctx_data,
+		usb_spi_packet_ctx_t* packet)
 {
    int transferred;
-	int ret;
-	usb_spi_response_v1_t response_packet;
-	const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash);
-
-	ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle,
-				ctx_data->in_ep,
-				(void*)&response_packet,
-				read_count + PACKET_HEADER_SIZE,
+	int status = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle,
+				ctx_data->out_ep,
+				packet->bytes,
+				packet->packet_size,
    			&transferred,
    			TRANSFER_TIMEOUT_MS));
-	if (ret != 0) {
+	if (status || transferred != packet->packet_size) {
+		if (!status) {
+			/* No error was reported, but we didn't transmit the data expected. */
+			status = USB_SPI_HOST_TX_BAD_TRANSFER;
+		}
+		msg_perr("Raiden: OUT transfer failed\n"
+				"    transferred = %d\n"
+				"    packet_size = %d\n"
+				"    status      = %d\n",
+				transferred, packet->packet_size, status);
+
+	}
+	return status;
+}
+
+static int receive_packet(const struct raiden_debug_spi_data * ctx_data,
+		usb_spi_packet_ctx_t* packet)
+{
+	int received;
+	int status = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle,
+				ctx_data->in_ep,
+				packet->bytes,
+				MAX_USB_PACKET_SIZE,
+				&received,
+				TRANSFER_TIMEOUT_MS));
+	packet->packet_size = received;
+	if (status) {
    	msg_perr("Raiden: IN transfer failed\n"
-		         "    write_count = %d\n"
-		         "    read_count  = %d\n",
-		         write_count, read_count);
-		return ret;
+				"    received = %d\n"
+				"    status   = %d\n",
+				received, status);
    }
+	return status;
+}

-	if ((unsigned) transferred != read_count + PACKET_HEADER_SIZE) {
-		msg_perr("Raiden: Read failure (read %d, expected %d)\n",
-				transferred, read_count + PACKET_HEADER_SIZE);
-		return 0x10002;
+/*
+ * Version 1 Protocol
+ */
+
+static int write_command_v1(const struct raiden_debug_spi_data * ctx_data,
+		usb_spi_transmit_ctx_t* write, usb_spi_receive_ctx_t* read)
+{
+	usb_spi_packet_ctx_t command = {
+		.header_size = offsetof(usb_spi_command_v1_t, data),
+		.packet_v1.command.write_count = write->transmit_size,
+		.packet_v1.command.read_count = read->receive_size
+	};
+
+	/* Reset the write context to the start. */
+	write->transmit_index = 0;
+
+	fill_usb_packet(&command, write);
+	return transmit_packet(ctx_data, &command);
+}
+
+static int read_response_v1(const struct raiden_debug_spi_data * ctx_data,
+		usb_spi_transmit_ctx_t* write, usb_spi_receive_ctx_t* read)
+{
+	int status;
+	usb_spi_packet_ctx_t response;
+
+	/* Reset the read context to the start. */
+	read->receive_index = 0;
+
+	status = receive_packet(ctx_data, &response);
+	if (status) {
+		/* Return the transfer error since the status_code is unreliable */
+		return status;
    }
+	if (response.packet_v1.response.status_code) {
+		return response.packet_v1.response.status_code;
+	}
+	response.header_size = offsetof(usb_spi_response_v1_t, data);

-	memcpy(read_buffer, response_packet.data, read_count);
-
-	return response_packet.status_code;
+	status = read_usb_packet(read, &response);
+	return status;
 }

static int send_command(const struct flashctx *flash,
@@ -323,20 +430,50 @@
    	const unsigned char *write_buffer,
    	unsigned char *read_buffer)
 {
+	const struct raiden_debug_spi_data * ctx_data =
+		get_raiden_data_from_context(flash);
    int status = -1;

-	for (int write_attempt = 0; write_attempt < WRITE_RETRY_ATTEMPTS;
-	         write_attempt++) {
+	usb_spi_transmit_ctx_t write_ctx = {
+		.buffer = write_buffer,
+		.transmit_size = write_count
+	};
+	usb_spi_receive_ctx_t read_ctx = {
+		.buffer = read_buffer,
+		.receive_size = read_count
+	};

-		status = write_command(flash, write_count, read_count,
-		                       write_buffer, read_buffer);
+	if (write_count > PAYLOAD_SIZE_V1) {
+		msg_perr("Raiden: Invalid read count\n"
+				"    write count = %d\n"
+				"    max write   = %d\n",
+				read_count, PAYLOAD_SIZE_V1);
+		return SPI_INVALID_LENGTH;
+	}
+
+	if (read_count > PAYLOAD_SIZE_V1) {
+		msg_perr("Raiden: Invalid read count\n"
+				"    read count = %d\n"
+				"    max read   = %d\n",
+				read_count, PAYLOAD_SIZE_V1);
+		return SPI_INVALID_LENGTH;
+	}
+
+	for (int write_attempt = 0; write_attempt < WRITE_RETRY_ATTEMPTS;
+			write_attempt++) {
+
+		status = write_command_v1(ctx_data, &write_ctx, &read_ctx);

if (status) {
    		/* Write operation failed. */
    		msg_perr("Raiden: Write command failed\n"
-				"Write attempt = %d\n"
-				"status = %d\n",
-				write_attempt + 1, status);
+					"    write count       = %d\n"
+					"    read count        = %d\n"
+					"    transmitted bytes = %d\n"
+					"    write attempt     = %d\n"
+					"    status            = %d\n",
+					write_count, read_count, write_ctx.transmit_index,
+					write_attempt + 1, status);
    		if (!retry_recovery(status)) {
    			/* Reattempting will not result in a recovery. */
    			return status;
@@ -344,26 +481,36 @@
    		programmer_delay(RETRY_INTERVAL_US);
    		continue;
    	}
-		for (int read_attempt = 0; read_attempt < READ_RETRY_ATTEMPTS; read_attempt++) {
+		for (int read_attempt = 0; read_attempt < READ_RETRY_ATTEMPTS;
+				read_attempt++) {

-			status = read_response(flash, write_count, read_count,
-					write_buffer, read_buffer);
+			status = read_response_v1(ctx_data, &write_ctx, &read_ctx);
+
+			if (!status) {
+				if (read_ctx.receive_size == read_ctx.receive_index) {
+					/* No errors and read is complete. */
+					return status;
+				} else {
+					status = USB_SPI_HOST_RX_READ_FAILURE;
+				}
+			}

if (status) {
    			/* Read operation failed. */
    			msg_perr("Raiden: Read response failed\n"
-					"Write attempt = %d\n"
-					"Read attempt = %d\n"
-					"status = %d\n",
-					write_attempt + 1, read_attempt + 1, status);
+						"    write count    = %d\n"
+						"    read count     = %d\n"
+						"    received bytes = %d\n"
+						"    write attempt  = %d\n"
+						"    read attempt   = %d\n"
+						"    status         = %d\n",
+						write_count, read_count, read_ctx.receive_index,
+						write_attempt + 1, read_attempt + 1, status);
    			if (!retry_recovery(status)) {
    				/* Reattempting will not result in a recovery. */
    				return status;
    			}
    			programmer_delay(RETRY_INTERVAL_US);
-			} else {
-				/* We were successful at performing the SPI transfer. */
-				return status;
    		}
    	}
    }