Hello Brian Nemec,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/43550
to review the following change.
Change subject: raiden_debug_spi.c: Adds USB context states and helper functions ......................................................................
raiden_debug_spi.c: Adds USB context states and helper functions
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 bnemec@chromium.com Change-Id: Id7b598b39923b4b8c1b6905e5d5c5a2be4078f96 --- M raiden_debug_spi.c 1 file changed, 202 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/50/43550/1
diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index cb7f809..1711ce6 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -155,6 +155,26 @@ 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, + /* The number of bytes written did not match expected. */ + USB_SPI_HOST_TX_WRITE_FAILURE = 0x10002, + + + /* We did not receive the expected USB packet. */ + USB_SPI_HOST_RX_UNEXPECTED_PACKET = 0x11001, + /* We received a continue packet with an invalid data index. */ + USB_SPI_HOST_RX_BAD_DATA_INDEX = 0x11002, + /* We too much data. */ + USB_SPI_HOST_RX_DATA_OVERFLOW = 0x11003, + /* The number of bytes read did not match expected. */ + USB_SPI_HOST_RX_READ_FAILURE = 0x11004, + + /* We were unable to configure the device. */ + USB_SPI_HOST_INIT_FAILURE = 0x12001, +}; + enum usb_spi_error { USB_SPI_SUCCESS = 0x0000, USB_SPI_TIMEOUT = 0x0001, @@ -213,6 +233,27 @@ uint8_t data[PAYLOAD_SIZE_V1]; } __attribute__((packed));
+union usb_spi_packet_v1 { + struct usb_spi_command_v1 command; + struct usb_spi_response_v1 response; +} __attribute__((packed)); + +struct usb_spi_packet_ctx { + union { + uint8_t bytes[USB_MAX_PACKET_SIZE]; + union usb_spi_packet_v1 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. + */ + size_t header_size; + /* Number of bytes in the packet */ + size_t packet_size; +}; + struct usb_spi_transmit_ctx { /* Buffer we are reading data from. */ const uint8_t *buffer; @@ -265,9 +306,131 @@ }
/* - * Version 1 Protocol: Responsible for constructing the packet to start - * a USB SPI transfer. Write and read counts and payloads to write from - * the write_buffer are transmitted to the device. + * Read data into the receive buffer. + * + * @param dst Destination receive context we are writing data to. + * @param src Source packet context we are reading data from. + * + * @returns status code 0 on success. + * USB_SPI_HOST_RX_DATA_OVERFLOW if the source packet is too + * large to fit in read buffer. + */ +static int read_usb_packet(struct usb_spi_receive_ctx *dst, + const struct usb_spi_packet_ctx *src) +{ + size_t max_read_length = dst->receive_size - dst->receive_index; + size_t 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 = %zu\n" + " max_read_length = %zu\n" + " receive_index = %zu\n" + " receive_size = %zu\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; +} + +/* + * Fill the USB packet with data from the transmit buffer. + * + * @param dst Destination packet context we are writing data to. + * @param src Source transmit context we are reading data from. + */ +static void fill_usb_packet(struct usb_spi_packet_ctx *dst, + struct usb_spi_transmit_ctx *src) +{ + size_t transmit_size = src->transmit_size - src->transmit_index; + size_t max_buffer_size = USB_MAX_PACKET_SIZE - dst->header_size; + uint8_t *packet_buffer = dst->bytes + dst->header_size; + + if (transmit_size > max_buffer_size) + transmit_size = max_buffer_size; + + memcpy(packet_buffer, src->buffer + src->transmit_index, transmit_size); + + dst->packet_size = dst->header_size + transmit_size; + src->transmit_index += transmit_size; +} + +/* + * Receive the data from the device USB endpoint and store in the packet. + * + * @param ctx_data Raiden SPI config. + * @param packet Destination packet used to store the endpoint data. + * + * @returns Returns status code with 0 on success. + */ +static int receive_packet(const struct flashctx *flash, + struct usb_spi_packet_ctx *packet) +{ + int received; + const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash); + int status = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->in_ep, + packet->bytes, + USB_MAX_PACKET_SIZE, + &received, + TRANSFER_TIMEOUT_MS)); + packet->packet_size = received; + if (status) { + msg_perr("Raiden: IN transfer failed\n" + " received = %d\n" + " status = 0x%05x\n", + received, status); + } + return status; +} + +/* + * Transmit data from the packet to the device's USB endpoint. + * + * @param flash Flash context storing SPI capabilities and USB device + * information. + * @param packet Source packet we will write to the endpoint data. + * + * @returns Returns status code with 0 on success. + */ +static int transmit_packet(const struct flashctx *flash, + struct usb_spi_packet_ctx *packet) +{ + int transferred; + const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash); + int status = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->out_ep, + packet->bytes, + packet->packet_size, + &transferred, + TRANSFER_TIMEOUT_MS)); + if (status || (size_t)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 = %zu\n" + " status = 0x%05x\n", + transferred, packet->packet_size, status); + + } + return status; +} + +/* + * Version 1 protocol command to start a USB SPI transfer and write the payload. * * @param flash Flash context storing SPI capabilities and USB device * information. @@ -280,38 +443,17 @@ struct usb_spi_transmit_ctx *write, struct usb_spi_receive_ctx *read) { + struct usb_spi_packet_ctx command = { + .header_size = offsetof(struct usb_spi_command_v1, data), + .packet_v1.command.write_count = write->transmit_size, + .packet_v1.command.read_count = read->receive_size + };
- int transferred; - int ret; - struct usb_spi_command_v1 command_packet; - const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash); + /* Reset the write context to the start. */ + write->transmit_index = 0;
- command_packet.write_count = write->transmit_size; - command_packet.read_count = read->receive_size; - - memcpy(command_packet.data, write->buffer, write->transmit_size); - - ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, - ctx_data->out_ep, - (void*)&command_packet, - write->transmit_size + PACKET_HEADER_SIZE, - &transferred, - TRANSFER_TIMEOUT_MS)); - if (ret != 0) { - msg_perr("Raiden: OUT transfer failed\n" - " write_count = %zu\n" - " read_count = %zu\n", - write->transmit_size, read->receive_size); - return ret; - } - - if ((unsigned) transferred != write->transmit_size + PACKET_HEADER_SIZE) { - msg_perr("Raiden: Write failure (wrote %d, expected %zu)\n", - transferred, write->transmit_size + PACKET_HEADER_SIZE); - return 0x10001; - } - - return 0; + fill_usb_packet(&command, write); + return transmit_packet(flash, &command); }
/* @@ -330,34 +472,24 @@ struct usb_spi_transmit_ctx *write, struct usb_spi_receive_ctx *read) { - int transferred; - int ret; - struct usb_spi_response_v1 response_packet; - const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash); + int status; + struct usb_spi_packet_ctx response;
- ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, - ctx_data->in_ep, - (void*)&response_packet, - read->receive_size + PACKET_HEADER_SIZE, - &transferred, - TRANSFER_TIMEOUT_MS)); - if (ret != 0) { - msg_perr("Raiden: IN transfer failed\n" - " write_count = %zu\n" - " read_count = %zu\n", - write->transmit_size, read->receive_size); - return ret; + /* Reset the read context to the start. */ + read->receive_index = 0; + + status = receive_packet(flash, &response); + if (status) { + /* Return the transfer error since the status_code is unreliable */ + return status; } - - if ((unsigned) transferred != read->receive_size + PACKET_HEADER_SIZE) { - msg_perr("Raiden: Read failure (read %d, expected %zu)\n", - transferred, read->receive_size + PACKET_HEADER_SIZE); - return 0x10002; + if (response.packet_v1.response.status_code) { + return response.packet_v1.response.status_code; } + response.header_size = offsetof(struct usb_spi_response_v1, data);
- memcpy(read->buffer, response_packet.data, read->receive_size); - - return response_packet.status_code; + status = read_usb_packet(read, &response); + return status; }
/* @@ -413,6 +545,12 @@
status = write_command_v1(flash, &write_ctx, &read_ctx);
+ if (!status && + (write_ctx.transmit_index != write_ctx.transmit_size)) { + /* No errors were reported, but write is incomplete. */ + status = USB_SPI_HOST_TX_WRITE_FAILURE; + } + if (status) { /* Write operation failed. */ msg_perr("Raiden: Write command failed\n" @@ -437,8 +575,13 @@ status = read_response_v1(flash, &write_ctx, &read_ctx);
if (!status) { - /* Successful transfer. */ - return status; + if (read_ctx.receive_size == read_ctx.receive_index) { + /* Successful transfer. */ + return status; + } else { + /* Report the error from the failed read. */ + status = USB_SPI_HOST_RX_READ_FAILURE; + } }
if (status) {